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

[data grid] a11y the toolbar should be outside of the role='grid' element #8525

Closed
tj36 opened this issue Apr 6, 2023 · 16 comments · Fixed by #9496
Closed

[data grid] a11y the toolbar should be outside of the role='grid' element #8525

tj36 opened this issue Apr 6, 2023 · 16 comments · Fixed by #9496
Labels
accessibility a11y component: data grid This is the name of the generic UI component, not the React module!

Comments

@tj36
Copy link

tj36 commented Apr 6, 2023

Hello, I am having an accessibility issue and I'm having trouble debugging this.

Please let me know if there is a resolution for this. Thanks.

@tj36 tj36 changed the title Support: Certain ARIA roles must contain particular children Question: Certain ARIA roles must contain particular children Apr 6, 2023
@zannager zannager added component: data grid This is the name of the generic UI component, not the React module! status: waiting for maintainer These issues haven't been looked at yet by a maintainer labels Apr 6, 2023
@alexfauquette
Copy link
Member

Where can we see your data grid, and what is the tool you're using for this report?

Because with only the generic message in your screenshot, we will not be able to investigate

@tj36
Copy link
Author

tj36 commented Apr 6, 2023

Hi @alexfauquette ,
Below is the code I have.

<DataGrid columns={columns} rows={notifications.data?.data?.data?.result?.entries ?? []} rowCount={rowCountState} loading={notifications.isLoading} page={pageNumber} pageSize={pageSize} paginationMode="server" onPageChange={newPageNumber => setPageNumber(newPageNumber)} autoHeight />

Below is the grid I see on the UI
image

I am using storybook to test the code.
Please let me know if you need some more details.

@m4theushw m4theushw removed the status: waiting for maintainer These issues haven't been looked at yet by a maintainer label Apr 10, 2023
@m4theushw
Copy link
Member

Could you create a CodeSandbox reproducing the error? Without more details there's no much we can do.

@m4theushw m4theushw added the status: waiting for author Issue with insufficient information label Apr 10, 2023
@github-actions
Copy link

Since the issue is missing key information and has been inactive for 7 days, it has been automatically closed. If you wish to see the issue reopened, please provide the missing information.

@eljo
Copy link

eljo commented Apr 19, 2023

Stumbled across the same. Steps to reproduce:

Steps:

  1. Install the axe DevTools extension (for chrome: https://chrome.google.com/webstore/detail/axe-devtools-web-accessib/lhdoppojpmngadmnindnejefpokejbdd/related)
  2. Open https://mui.com/x/react-data-grid/demo/
  3. Open the axe DevTools tab and run a page scan
  4. Open resulting "Certain ARIA roles must contain particular children" result.

@alexfauquette
Copy link
Member

From what I understand, the problem comes from the toolbar. The role="grid" should not includes the toolbar. It only expect row and row headers (no button or input)

@alexfauquette alexfauquette reopened this Apr 19, 2023
@github-actions
Copy link

Since the issue is missing key information and has been inactive for 7 days, it has been automatically closed. If you wish to see the issue reopened, please provide the missing information.

@alexfauquette alexfauquette reopened this Apr 20, 2023
@alexfauquette alexfauquette removed the status: waiting for author Issue with insufficient information label Apr 20, 2023
@RPiAwesomeness
Copy link

Just another data point, but I've run into this using @mui/x-data-grid-pro version 5.17.20

@emilefleming
Copy link

@alexfauquette were you able to find a workaround?

@alexfauquette alexfauquette changed the title Question: Certain ARIA roles must contain particular children [data grid] a11y the toolbar should be outside of the role='grid' element Jun 14, 2023
@alexfauquette
Copy link
Member

I'm not sure there is a workaround. It would require to move the role attributes but I'm wondering if it's doable without a breaking change.

@romgrk
Copy link
Contributor

romgrk commented Jun 14, 2023

We should add to #7902 then.

@cherniavskii
Copy link
Member

While changing aria attributes would be a breaking change, we can do this under the experimental ariaV7 flag, so that we can unblock people and prepare these changes in advance.
@romgrk What do you think?
I looked into it while investigating #9403 and I can submit a draft PR.

@romgrk
Copy link
Contributor

romgrk commented Jun 27, 2023

Yeah, fixing this would probably be a breaking change. I was thinking we could fix this and do the DOM layout change required for point 1 of #9171 at the same time.

I'm not sure how you see this working but if you have it figured out feel free to open a PR right away.

@cherniavskii
Copy link
Member

@romgrk I have opened #9496, let's discuss it there

@oliviertassinari
Copy link
Member

oliviertassinari commented Jun 29, 2023

I have seen this issue too in the past. For example, it's reported by Lighthouse: https://pagespeed.web.dev/analysis/https-mui-com-x/2irxt3dly6?hl=en&form_factor=desktop

Screenshot 2023-06-29 at 02 33 11

While changing aria attributes would be a breaking change

I'm not sure, it could be sold as a bug fix.

@cherniavskii
Copy link
Member

I'm not sure, it could be sold as a bug fix.

Perhaps, but people might use the role in their tests as we do:

const firstHeight = getByRole('grid').clientHeight;

This might break things for them.

@github-project-automation github-project-automation bot moved this to 🆕 Needs refinement in MUI X Data Grid Jul 11, 2023
@LukasTy LukasTy moved this from 🆕 Needs refinement to 🏗 In progress in MUI X Data Grid Jul 11, 2023
@joserodolfofreitas joserodolfofreitas moved this from 🏗 In progress to 🆕 Needs refinement in MUI X Data Grid Jul 11, 2023
@cherniavskii cherniavskii moved this from 🆕 Needs refinement to ✅ Done in MUI X Data Grid Aug 1, 2023
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!
Projects
Status: Done
Development

Successfully merging a pull request may close this issue.

10 participants