-
-
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 role=presentation a11y issue #13891
Conversation
Deploy preview: https://deploy-preview-13891--material-ui-x.netlify.app/ |
@@ -583,7 +583,6 @@ export const useGridVirtualScroller = () => { | |||
}), | |||
getScrollerProps: () => ({ | |||
ref: scrollerRef, | |||
tabIndex: -1, |
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.
Without the explicit tabIndex here, Firefox focuses on the virtual scroller element:
Chromium:
Screen.Recording.2024-07-18.at.21.59.30.mov
Firefox:
Screen.Recording.2024-07-18.at.21.59.46.mov
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.
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've added aria-hidden=true
for the scrollbar, same as ag-grid does, it's probably the best aria tag for that case.
@cherniavskii For the Firefox issue, wouldn't that be a Firefox bug? Can you test on Safari to see what's the behavior? If Chrome & Safari agree and Firefox disagrees, we could add the tabindex only as a workaround for FF and file an issue with them.
Looks vaguely similar to this: https://bugzilla.mozilla.org/show_bug.cgi?id=1069739
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 checked Chrome, Safari, and Firefox, and the issue is only reproducible in Firefox.
we could add the tabindex only as a workaround for FF and file an issue with them
Yeah, that makes sense 👍🏻
Looks vaguely similar to this: bugzilla.mozilla.org/show_bug.cgi?id=1069739
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.
Pretty common lifespan for a firefox bug, they've been around for longer and have way less resources :|
Also I can't reproduce the issue in Firefox on linux/wayland.
Co-authored-by: Andrew Cherniavskyi <andrew@mui.com>
Closes #12837
The
tabIndex=-1
on this element is probably a leftover from a refactor as the element doesn't need to be keyboard-focusable.