-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[DataGrid] Fix the accessibility of the column menu #900
Conversation
event.preventDefault(); | ||
hideMenu(); | ||
} | ||
}, | ||
[hideMenu], | ||
); | ||
return ( | ||
<MenuList id="menu-list-grow" onKeyDown={handleListKeyDown}> | ||
<MenuList aria-labelledby={`menu-button-${currentColumn.field}`} autoFocus autoFocusItem id="menu-list-grow" onKeyDown={handleListKeyDown}> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not sure about the menu-button
hardcoded prefix
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dtassone thanks! any good suggestions?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is the id
here necessary, maybe I'm not seeing something?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
https://www.w3.org/TR/wai-aria-practices/examples/menu-button/menu-button-links.html
This document clarifies that aria-labelledby
must be an ID reference, so i added a id
to the column header filter button, and then used this ID as the aria-labelledby
attribute of the column header MenuList
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have a useId()
helper that can be imported to solve this problem, it's meant as a bridge until reactjs/rfcs#32 is resolved.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have some doubts about how I can share this id
between these two components if I use useId()
? should i store id
in ColumnMenuState
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In theory, I would expect each button and menu to be rendered in the same component (duplicate for each column), so sharing the id should be possible because in the same scope.
It appears that we share the same menu instance between different columns. This is likely creating an issue with the animation open/close too.
This comment has been minimized.
This comment has been minimized.
Any update on this effort? What's the blocker? |
This comment has been minimized.
This comment has been minimized.
This reverts commit b36b7d2.
Closes #882