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

[datagrid] Toolbar panels don't restore focus after closing with Escape #10241

Closed
Isaac1606 opened this issue Sep 5, 2023 · 20 comments · Fixed by #10412
Closed

[datagrid] Toolbar panels don't restore focus after closing with Escape #10241

Isaac1606 opened this issue Sep 5, 2023 · 20 comments · Fixed by #10412
Assignees
Labels
accessibility a11y component: data grid This is the name of the generic UI component, not the React module!

Comments

@Isaac1606
Copy link

Isaac1606 commented Sep 5, 2023

Steps to reproduce 🕹

  • For Density selector:
    Link to live example: https://mui.com/x/react-data-grid/accessibility/#density

    Steps:

    1. Go to the live example link.
    2. Navigate with keyboard or click with the mouse the density button in the DataGrid.
    3. Select any option in the menu or press esc.
  • For GridActionsCellItem with showInMenu enabled:
    Link to live example: https://mui.com/x/react-data-grid/accessibility/#density

    Steps:

    1. Go to the live example link.
    2. Reach the DataGrid example.
    3. Go to the last column.
    4. Press/select the icon with the three dots.
    5. Select any option in the menu or press esc.

Current behavior 😯

The menu with the set of options disappears (as expected) after one of the options is selected or if the esc button is pressed, but the cursor ceases to exist and there is no focused element whatsoever.

Expected behavior 🤔

Focus should return to the button that caused the menu to appear.

Context 🔦

Give users the capability to navigate throughout the entire DataGrid and options using only Keyboard.

Your environment 🌎

npx @mui/envinfo
  Used browser: Google Chrome.
   System:
    OS: macOS 13.4
  Binaries:
    Node: 18.17.0 - /usr/local/bin/node
    Yarn: 1.22.19 - /usr/local/bin/yarn
    npm: 9.8.1 - /usr/local/bin/npm
  Browsers:
    Chrome: 114.0.5735.106
    Edge: Not Found
    Safari: 16.5
  npmPackages:
    @emotion/react: ^11.11.1 => 11.11.1 
    @emotion/styled: ^11.11.0 => 11.11.0 
    @mui/base:  5.0.0-beta.14 
    @mui/core-downloads-tracker:  5.14.8 
    @mui/icons-material: ^5.14.1 => 5.14.8 
    @mui/material: ^5.14.2 => 5.14.8 
    @mui/private-theming:  5.14.8 
    @mui/styled-engine:  5.14.8 
    @mui/system:  5.14.8 
    @mui/types:  7.2.4 
    @mui/utils:  5.14.8 
    @mui/x-data-grid: ^6.10.2 => 6.12.1 
    @types/react: 18.2.17 => 18.2.17 
    react: 18.2.0 => 18.2.0 
    react-dom: 18.2.0 => 18.2.0 
    typescript: 5.1.6 => 5.1.6 

Order ID or Support key 💳 (optional)

No response

@Isaac1606 Isaac1606 added the status: waiting for maintainer These issues haven't been looked at yet by a maintainer label Sep 5, 2023
@MBilalShafi MBilalShafi added accessibility a11y component: data grid This is the name of the generic UI component, not the React module! labels Sep 5, 2023
@MBilalShafi
Copy link
Member

Thanks for raising the issue.

Seems like a similar problem exists with all the menus that open up by clicking on the Toolbar menu buttons and actions menu. Honestly I am not sure what should be the behavior when clicking on the button and pressing escape key or closing the menu.

Focus shifting to the button could certainly be one direction to proceed to but that way the user will still not be able to maneuver through the cells by pressing arrow keys as that happens only when a Grid cell is focused. So pressing tab and moving the focus to the Grid cells will be required before the keyboard navigation starts working with the arrow keys.

To me, it would make more sense in conjunction with #4658 so the button will also be tabbable and the navigation will feel more natural.

@mui/xgrid What do you think?

@MBilalShafi MBilalShafi added status: waiting for author Issue with insufficient information and removed status: waiting for maintainer These issues haven't been looked at yet by a maintainer labels Sep 5, 2023
@michelengelen
Copy link
Member

Hey @MBilalShafi

Considering this section:

Focus shifting to the button could certainly be one direction to proceed to but that way the user will still not be able to maneuver through the cells by pressing arrow keys as that happens only when a Grid cell is focused. So pressing tab and moving the focus to the Grid cells will be required before the keyboard navigation starts working with the arrow keys.

There is actually a pretty straight forward definition in the W3C standards to be found here, so the proposed solution from @Isaac1606 is correct. If the menu gets closed (and is not navigating away from the page through an action) the button should receive focus back from the menu.

I'd like some more opinions on it though.

@github-actions
Copy link

The issue has been inactive for 7 days and has been automatically closed. If you think that it has been incorrectly closed, please reopen it and provide missing information (if any) or continue the last discussion.

@romgrk romgrk reopened this Sep 13, 2023
@romgrk
Copy link
Contributor

romgrk commented Sep 13, 2023

Yes this is an accessibility violation, focus management is part of the ARIA spec.

@github-project-automation github-project-automation bot moved this to 🆕 Needs refinement in MUI X Data Grid Sep 13, 2023
@romgrk romgrk changed the title [DataGrid] Density selector and GridActionsCellItem with showInMenu enabled won't return focus after selecting an option or pressing esc. [datagrid] Toolbar panels don't restore focus after closing with Escape Sep 13, 2023
@github-actions
Copy link

The issue has been inactive for 7 days and has been automatically closed. If you think that it has been incorrectly closed, please reopen it and provide missing information (if any) or continue the last discussion.

@michelengelen michelengelen reopened this Sep 13, 2023
@github-project-automation github-project-automation bot moved this from 🆕 Needs refinement to 📋 Backlog in MUI X Data Grid Sep 13, 2023
@michelengelen
Copy link
Member

@romgrk if the bot closes this again we should probably move this? Don't know why that got reopened 🤷🏼

@romgrk romgrk removed the status: waiting for author Issue with insufficient information label Sep 13, 2023
@DanailH
Copy link
Member

DanailH commented Sep 14, 2023

@michelengelen we forgot to remove the status: waiting for follow-up label when the issue was reopened, which brings me to - there is an opportunity for improvement of the no-response.yml action to also remove the status: waiting for follow-up label when it closes the issue.

@romgrk
Copy link
Contributor

romgrk commented Sep 14, 2023

No I think the label should stay, to mark that it's the author's turn to respond. Anyway it's a rare case when we keep it open even if the author loses interest.

@MBilalShafi
Copy link
Member

Yes, keeping the status: waiting for follow-up label intact should reopen the issue if ticket author replies after auto closing is done.

@Isaac1606
Copy link
Author

Hey guys any updates on this? or a temporary workaround? Like a wrote in the context section I need to give the users the capability to navigate throughout the entire DataGrid and options using Keyboard only. This a blocker for me sine I can not release the project without this feature. I would truly appreciate any updates, thanks in advance.

@romgrk romgrk self-assigned this Sep 19, 2023
@romgrk
Copy link
Contributor

romgrk commented Sep 19, 2023

I'll take a look this week.

@romgrk
Copy link
Contributor

romgrk commented Sep 20, 2023

So it looks like we're using MUI Core's MenuList, which has an autoFocus prop. The problem is that it doesn't restore focus, and it also doesn't trap it inside itself. I'm not sure if it's intentionally missing or not. We'll need to add the logic either to that component, or to our internal component.

@mui/core Any thoughts on the above? Is there value in adding those capacities to the component?

@oliviertassinari
Copy link
Member

This is intentional as far as I know. The root problem as far as I remember is the existence of a GridMenu component, this one shouldn't exist in the first place. It's related to mui/material-ui#38756 and mui/base-ui#39. We should likely have a brand new Popover primitive to replace GridMenu.

@romgrk
Copy link
Contributor

romgrk commented Sep 21, 2023

Yeah I figured there's a missing primitive. Is that something I should work on to solve this or is the hacky solution in mui/material-ui#10412 fine for now? Not sure how often you release new components in the core.

@romgrk
Copy link
Contributor

romgrk commented Sep 21, 2023

We've integrated mui/material-ui#10412 because that workaround works for now and solves the a11y issue. We should definitely replace GridMenu once the core has a primitive for that.

@Isaac1606
Copy link
Author

Isaac1606 commented Oct 10, 2023

Hey guys, it is me again I am sorry to inform that #10412 workaround does not work when using anchor tag, see #9913. The behavior with the current code is everything works fine when clicking on it with the mouse, but if you press enter nothing happens and the menuItem remains on the screen
image

@romgrk
Copy link
Contributor

romgrk commented Oct 11, 2023

Do you have a live example for that?

@romgrk romgrk reopened this Oct 11, 2023
@romgrk romgrk added the status: waiting for author Issue with insufficient information label Oct 11, 2023
@Isaac1606
Copy link
Author

No, I do not have a live example, but it is pretty straightforward in a DataGrid define a GridActionsCellItem as it follows:
<GridActionsCellItem showInMenu {...{component: "a", href: elem.href, target: "_blank"}} />
And if you click on it works fine but if you use keyboard only the cursor goes back to the action's button and the menu keeps floating like in the Image that I previously attached.

@Isaac1606 Isaac1606 reopened this Oct 12, 2023
@github-actions github-actions bot added status: waiting for maintainer These issues haven't been looked at yet by a maintainer and removed status: waiting for author Issue with insufficient information labels Oct 12, 2023
@romgrk romgrk closed this as completed Oct 14, 2023
@romgrk romgrk removed the status: waiting for maintainer These issues haven't been looked at yet by a maintainer label Oct 14, 2023
@romgrk
Copy link
Contributor

romgrk commented Oct 14, 2023

Let's track this in mui/material-ui#10675, I think we need to discuss other keyboard interactions for GridActionsCellItem. You can subscribe to that issue to get notifications.

@HolyNoodle
Copy link

I have been struggling with a similar problem. I want the datagrid column menu to close when the user clicks on a menu item. This click triggers a focus in an input elsewhere in the page.

Menu keeps stealing the focus. Even with event propagation stopped on multiple key board and mouse events. Couldn't pin the events I should bypass.

So I went for a different way: waiting for the event to finish bubbling by adding the code I want to trigger at the end of the call stack

Promise.resolve().then(() => trigger focus action);

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.

7 participants