mui / mui-x

MUI X: Build complex and data-rich applications using a growing list of advanced React components, like the Data Grid, Date and Time Pickers, Charts, and more!
https://mui.com/x/
4.12k stars 1.28k forks source link

[data grid] The filter panel in custom toolbar steal the focus from an input #7044

Open sasa5195 opened 1 year ago

sasa5195 commented 1 year ago

Steps to reproduce πŸ•Ή

Link to live example: https://codesandbox.io/s/tender-shannon-2o3hv7

https://www.loom.com/share/eb4bd147cdb043139c30a055f0f9f842

Browser - Chrome, Firefox

Current behavior 😯

Opening the date range picker after the filter panel or column panel closes the date range picker automatically for the first time.

But this doesn't happens when density menu appears and date range picker is being opened

Expected behavior πŸ€”

Opening the date range picker after the filter panel or column panel should not close the date range picker automatically. It should behave the same way as when we toggle between different panels like filter, column, density, etc.

Context πŸ”¦

I need to add a date range filter which is not related to the columns of the data grid. So inorder to achieve that i am using the date range picker component and placing it in the data grid toolbar.

Your environment 🌎

npx @mui/envinfo ``` System: OS: macOS 12.6.1 Binaries: Node: 18.10.0 - ~/.nvm/versions/node/v18.10.0/bin/node Yarn: 1.22.19 - /opt/homebrew/bin/yarn npm: 8.19.2 - ~/.nvm/versions/node/v18.10.0/bin/npm Browsers: Chrome: 106.0.5249.119 Edge: Not Found Firefox: 106.0.5 Safari: 16.1 npmPackages: @emotion/react: ^11.10.4 => 11.10.5 @emotion/styled: ^11.10.4 => 11.10.5 @mui/base: 5.0.0-alpha.103 @mui/core-downloads-tracker: 5.10.11 @mui/icons-material: ^5.10.6 => 5.10.9 @mui/material: ^5.10.6 => 5.10.11 @mui/private-theming: 5.10.9 @mui/styled-engine: 5.10.8 @mui/system: 5.10.10 @mui/types: 7.2.0 @mui/utils: 5.10.9 @mui/x-data-grid: 5.17.8 @mui/x-data-grid-pro: ^5.17.4 => 5.17.8 @mui/x-date-pickers: 5.0.9 @mui/x-date-pickers-pro: ^5.0.9 => 5.0.9 @mui/x-license-pro: 5.17.0 @types/react: ^18.0.20 => 18.0.24 react: ^18.2.0 => 18.2.0 react-dom: ^18.2.0 => 18.2.0 typescript: ^4.8.3 => 4.8.4 ```

Order ID πŸ’³ (optional)

No response

m4theushw commented 1 year ago

The filter panel is based on the FocusTrap, previously named TrapFocus. When you click the filter button to open the panel, this component stores which element is focused (the button), then once the panel is closed it tries to restore the focus to the element it had saved. What is happening here is that when the date range picker input is focused, the filter panel understands that a click outside occurred and it should be closed. When it closes, FocusTrap restores the focus to the button, which for the date range picker means that the input lost focus and the calendar should be closed. A solution would be to use the disableRestoreFocus prop to disable the logic that restores the focus. The downside is that accessibility will be affected, as the active element once the panel is closed will be document.body and, e.g. pressing Tab, will focus a complete random element.

If this is acceptable, then we need to give the possibility of passing props to the FocusTrap used by the panel. This can be done with:

diff --git a/packages/grid/x-data-grid/src/components/panel/GridPanelWrapper.tsx b/packages/grid/x-data-grid/src/components/panel/GridPanelWrapper.tsx
index f738196ce..4701aa1bf 100644
--- a/packages/grid/x-data-grid/src/components/panel/GridPanelWrapper.tsx
+++ b/packages/grid/x-data-grid/src/components/panel/GridPanelWrapper.tsx
@@ -1,6 +1,6 @@
 import * as React from 'react';
 import clsx from 'clsx';
-import TrapFocus from '@mui/material/Unstable_TrapFocus';
+import TrapFocus, { TrapFocusProps } from '@mui/material/Unstable_TrapFocus';
 import { styled, Theme } from '@mui/material/styles';
 import { MUIStyledCommonProps } from '@mui/system';
 import { unstable_composeClasses as composeClasses } from '@mui/utils';
@@ -36,18 +36,20 @@ const GridPanelWrapperRoot = styled('div', {
 const isEnabled = () => true;

 export interface GridPanelWrapperProps
-  extends React.PropsWithChildren<React.HTMLAttributes<HTMLDivElement>>,
+  extends React.PropsWithChildren<
+      React.HTMLAttributes<HTMLDivElement> & { TrapFocusProps: Omit<TrapFocusProps, 'open'> }
+    >,
     MUIStyledCommonProps<Theme> {}

 const GridPanelWrapper = React.forwardRef<HTMLDivElement, GridPanelWrapperProps>(
   function GridPanelWrapper(props, ref) {
-    const { className, ...other } = props;
+    const { className, TrapFocusProps: TrapFocusPropsProps, ...other } = props;
     const rootProps = useGridRootProps();
     const ownerState = { classes: rootProps.classes };
     const classes = useUtilityClasses(ownerState);

     return (
-      <TrapFocus open disableEnforceFocus isEnabled={isEnabled}>
+      <TrapFocus open disableEnforceFocus isEnabled={isEnabled} {...TrapFocusPropsProps}>
         <GridPanelWrapperRoot
           ref={ref}
           tabIndex={-1}

Then, disableRestoreFocus will be able to be used with:

<DataGrid
  componentsProps={{ filterPanel: { TrapFocusProps: { disableRestoreFocus: true } } }}
/>

Do you want to submit a PR with the diff above?

Vivek-Prajapatii commented 1 year ago

Hi @sasa5195 @m4theushw i would like to pick this issue, can you help me out with it?

m4theushw commented 1 year ago

@Vivek-Prajapatii Yes, you can find more information in https://github.com/mui/material-ui/blob/HEAD/CONTRIBUTING.md. When opening the PR make sure to set the target branch to next.

ayushcs commented 1 year ago

@m4theushw @Vivek-Prajapatii @sasa5195 Is this solution is available in the MUI or still needs to be updated ? Do we have a workaround solution to fix this?

Vivek-Prajapatii commented 1 year ago

@m4theushw if i missed something please let me know

oliviertassinari commented 1 year ago

I have simplified the problem a bit more in https://codesandbox.io/s/date-range-picker-issue-in-data-grid-inside-cutom-toolbar-forked-smx7j4?file=/demo.js so that we can consider the issue without the date pickers, a basic <input> is enough:

https://user-images.githubusercontent.com/3165635/215286084-8ad7325c-674f-4b3d-8e01-1fd0dfa6d0de.mov


Overall, I think that it's more a FocusTrap bug than a data grid one. For example in https://www.w3.org/WAI/ARIA/apg/example-index/menu-button/menu-button-links.html the focus is only restored to the button if the modality that triggered the close of the popup is the keyboard, nothing happens if its the mouse. So overall, we have a few options:

  1. Use Popover to block all clicks interactions on the page (what's done by Notion & GitHub)
  2. Extend the FocusTrap so that the focus is only restored for keyboard, e.g. clickOutsideDeactivates https://focus-trap.github.io/focus-trap/
  3. Replace the FocusTrap to implement the restore focus on keyboard action on our own (least preferred option)

I have noticed the same bug on this demo: https://mui.com/material-ui/react-menu/#menulist-composition, 2. could be its solution)

m4theushw commented 1 year ago

Overall, I think that it's more a FocusTrap bug than a data grid one.

@oliviertassinari I also agree but we can't wait for the Core to properly fix this bug (if there's a proper fix available). My proposal here is to at least allow a workaround.

Extend the FocusTrap so that the focus is only restored for keyboard, e.g. clickOutsideDeactivates https://focus-trap.github.io/focus-trap/

I've proposed a more customizable solution in https://github.com/mui/material-ui/issues/35307

m4theushw commented 1 year ago

7733 is merged and it will be included in the next release. For now, the following workaround can be used:

<DataGrid
  componentsProps={{
    filterPanel: {
      slotProps: {
        TrapFocus: {
          disableRestoreFocus: true,
        },
      },
    },
  }}
/>
oliviertassinari commented 1 year ago

@m4theushw I'm reopening as the UX is broken by default; It's great that we have a workaround πŸ‘

ayushcs commented 1 year ago

@m4theushw @oliviertassinari On which version we can use this workaround I have tried 5.17.23 and even 6.0.0-beta.3 https://github.com/mui/mui-x/issues/7696 issue is still happening

Codesandbox https://codesandbox.io/s/customfilterpanelposition-demo-mui-x-forked-gw96fs?file=/demo.tsx:600-615

m4theushw commented 1 year ago

@ayushcs It will be available in the version we're going to release this week, probably on Thursday.