Skip to content
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

[question] DataGridToolbar button violating accessibility standards #9307

Closed
2 tasks done
CalebJamesStevens opened this issue Jun 12, 2023 · 8 comments
Closed
2 tasks done
Labels
accessibility a11y component: data grid This is the name of the generic UI component, not the React module! support: commercial Support request from paid users

Comments

@CalebJamesStevens
Copy link

CalebJamesStevens commented Jun 12, 2023

Order ID or Support key 💳 (optional)

37b83db94a4536050ab7b3cc2e79a6a5Tz01NjU3NixFPTE3MDMzNDc4NzI4MjAsUz1wcm8sTE09cGVycGV0dWFsLEtWPTI

Duplicates

  • I have searched the existing issues

Latest version

  • I have tested the latest version

The problem in depth 🔍

image
You can see the same error on the mui site
Screenshot 2023-06-12 at 2 22 45 PM

DataGridPro's GridToolbar violates accessibility requirements.

https://dequeuniversity.com/rules/axe/4.6/aria-required-children

Your environment 🌎

`npx @mui/envinfo`
System:
    OS: macOS 13.0.1
  Binaries:
    Node: 18.16.0 - /usr/local/bin/node
    Yarn: 1.22.19 - /opt/homebrew/bin/yarn
    npm: 9.5.1 - /usr/local/bin/npm
  Browsers:
    Chrome: 113.0.5672.92
    Edge: 113.0.1774.57
    Firefox: Not Found
    Safari: 16.1
  npmPackages:
    @emotion/react: ^11.10.4 => 11.11.1 
    @emotion/styled: ^11.10.4 => 11.11.0 
    @mui/base:  5.0.0-beta.4 
    @mui/core-downloads-tracker:  5.13.4 
    @mui/icons-material: ^5.8.4 => 5.11.16 
    @mui/lab: ^5.0.0-alpha.95 => 5.0.0-alpha.134 
    @mui/material: ^5.10.1 => 5.13.5 
    @mui/private-theming:  5.13.1 
    @mui/styled-engine:  5.13.2 
    @mui/styles: ^5.9.3 => 5.13.2 
    @mui/system:  5.13.5 
    @mui/types:  7.2.4 
    @mui/utils:  5.13.1 
    @mui/x-data-grid:  6.7.0 
    @mui/x-data-grid-pro: ^6.0.4 => 6.7.0 
    @mui/x-date-pickers:  5.0.20 
    @mui/x-date-pickers-pro: ^5.0.0-beta.7 => 5.0.20 
    @mui/x-license-pro:  5.17.12 
    @types/react: ^18.0.28 => 18.2.12 
    react: ^18.2.0 => 18.2.0 
    react-dom: ^18.2.0 => 18.2.0 
    typescript: ^4.9.5 => 4.9.5 
@CalebJamesStevens CalebJamesStevens added status: waiting for maintainer These issues haven't been looked at yet by a maintainer support: commercial Support request from paid users labels Jun 12, 2023
@romgrk
Copy link
Contributor

romgrk commented Jun 12, 2023

Probably linked to #8862.

Not sure I understand correctly, the lighthouse report highlights some issues but doesn't explain what it thinks are the issues. IIUC, it's not happy because those buttons have aria-haspopup=menu but we don't add the aria-controls=id until the moment where the button is clicked. We don't add it because those menus aren't mounted until the moment when they are needed, for performance reasons. On a page with a lot of components, mounting all the popups from the start would slow things down.

I'm not sure if the ARIA spec has a way to describe that there will be a popup mounting when the button is clicked, the spec is quite dated in some aspects. If we want to comply with the lighthouse report, we'd need to trade aria-conformance for performance, which is not ideal. We should probably test the behavior on a screen reader to check how accessible we are in reality, regardless of what lighthouse says.

@romgrk romgrk added component: data grid This is the name of the generic UI component, not the React module! accessibility a11y and removed status: waiting for maintainer These issues haven't been looked at yet by a maintainer labels Jun 12, 2023
@CalebJamesStevens
Copy link
Author

@romgrk

https://w3c.github.io/aria/#aria-haspopup
https://w3c.github.io/aria/#aria-controls

We're performing a VPAT at the request of a customer so it's important that we value the accessibility of these components.

Reading the w3c standards I'm not seeing anything about the children needing to be active in the dom are you? I'm wondering where lighthouse is getting this from.

@romgrk
Copy link
Contributor

romgrk commented Jun 13, 2023

We're performing a VPAT at the request of a customer so it's important that we value the accessibility of these components.

Sure, what I'm saying is we should prioritize accessibility testing in real accessibility tools over what a report says.

Reading the w3c standards I'm not seeing anything about the children needing to be active in the dom are you? I'm wondering where lighthouse is getting this from.

If you look at the linked issue in my first message, we made that change specifically because some a11y testing tool was complaining about the DOM element not being mounted. Maybe that tool's reports were wrong and lighthouse's are better, we should test which one works best for screen readers before doing more changes.

Link to the original tool: https://accessibilityinsights.io/docs/web/overview/

@romgrk
Copy link
Contributor

romgrk commented Jun 13, 2023

It would be nice if we could first confirm what lighthouse thinks the ARIA issues are. Maybe you could open an issue with them first? The reported issues don't have a clear explanation of what the problem is, they should probably adjust that on their side.

@romgrk
Copy link
Contributor

romgrk commented Jun 15, 2023

I've reviewed the specs but couldn't find a clear answer. I've opened an issue with the ARIA group, maybe they'll be able to help: w3c/aria#1956

@CalebJamesStevens
Copy link
Author

I've reviewed the specs but couldn't find a clear answer. I've opened an issue with the ARIA group, maybe they'll be able to help: w3c/aria#1956

Aside form technically clearing the aria required has your team tried actually using datagrid with a screen reader? It's impossible to navigate to the toolbar

https://imgur.com/gallery/zcU0kVS Here is a video showing

@romgrk
Copy link
Contributor

romgrk commented Jun 17, 2023

Can you open a separate issue for the keyboard navigation?

@romgrk
Copy link
Contributor

romgrk commented Jun 20, 2023

Regarding the message from lighthouse, I think it's describing #8525 which we can only fix at the next major release due to the breaking change to the DOM structure. IMHO we can close this as duplicate of that issue, unless you have more feedback on what lighthouse is pointing out.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
accessibility a11y component: data grid This is the name of the generic UI component, not the React module! support: commercial Support request from paid users
Projects
None yet
Development

No branches or pull requests

2 participants