Dropdown a11y updates #3586
Dropdown a11y updates #3586
Conversation
|
Deploy preview for carbon-components-react ready! Built with commit 42d2d73 https://deploy-preview-3586--carbon-components-react.netlify.com |
|
Deploy preview for the-carbon-components ready! Built with commit 42d2d73 https://deploy-preview-3586--the-carbon-components.netlify.com |
|
Deploy preview for carbon-elements ready! Built with commit 42d2d73 |
|
@carbon-design-system/developers is there a reason dropdown has the |
ghost
requested review from
|
@asudoh can you answer this question: #3586 (comment)? It won't be part of this PR but it impacts the stuff that I'm working on now for Dropdown menu |
|
Is this focus on the whole drawer browser determined when it comes to the styling of it? Because it looks different from our regular focus @elizabethsjudd |
the fix for that is going to come in the next PR. The whole menu technically gets focus but then we'll use the |
|
@asudoh @laurenmrice The updates to the JavaScript wasn't that much so I combined my two branches in to this single PR. |
|
Quick answer on your floating menu question (sorry @elizabethsjudd for missing this!) - The attribute seems to have been added by mistake - Dropdown is not a floating menu, and my quick look tells me that dropdown code/style (still) does not refer to that attribute. |
|
@asudoh I've updated the code based on your comments. |
There was a problem hiding this comment.
Just one last comment in code. Would you be able to add test cases that represents the enhancement you did? Thanks @elizabethsjudd!
| if (this.element.querySelector(this.options.selectorTrigger)) { | ||
| const listNode = this.element.querySelector(this.options.selectorMenu); | ||
| const focusedId = listNode.getAttribute('aria-activedescendant'); | ||
| focusedNode = focusedId ? listNode.querySelector(`#${focusedId}`) : null; |
There was a problem hiding this comment.
@asudoh getElementById only works from the document object which is why I opted for querySelector here. I do believe that I could use this.element.ownerDocument.getElementById here though if you'd prefer that.
There was a problem hiding this comment.
OK thank you for clarifying @elizabethsjudd! - Makes sense as-is.
There was a problem hiding this comment.
LGTM 👍 - Thanks @elizabethsjudd for all the updates and tests!
Closes: #1147
Update to Dropdown for a11y.
Changelog
Changed
aria-activedescendantTesting / Reviewing