[DataGrid] Fix `GridOverlays` bypassing pointer events #5674

philjones88 · 2022-08-01T10:03:47Z

mui-bot · 2022-08-01T10:12:46Z

These are the results for the performance tests:

Test case	Unit	Min	Max	Median	Mean	σ
Filter 100k rows	ms	255.1	671.9	401.1	407.02	153.508
Sort 100k rows	ms	506.7	1,006.1	859.8	832.94	171.505
Select 100k rows	ms	220.2	270.6	236.9	240	16.651
Deselect 100k rows	ms	145.5	486	211.8	258.14	122.371

Generated by 🚫 dangerJS against 3e8530c

cherniavskii · 2022-08-01T10:33:26Z

For the context: pointer-events: none was added to avoid this regression #4405 after we changed zIndex of overlay container in #5558

philjones88 · 2022-08-02T12:21:55Z

@cherniavskii I'm not sure what to suggest then, the change to add pointerEvents caused other regressions, this one being one for us.

cherniavskii · 2022-08-02T18:19:27Z

What we can do to solve both issues is to:

revert [DataGrid] Fix grid loading overlays position #5558
apply zIndex and pointerEvents styles only to default error overlay

This diff should do the job:

diff --git a/packages/grid/x-data-grid/src/components/GridLoadingOverlay.tsx b/packages/grid/x-data-grid/src/components/GridLoadingOverlay.tsx
index 8e7e4ea35..55ef3921f 100644
--- a/packages/grid/x-data-grid/src/components/GridLoadingOverlay.tsx
+++ b/packages/grid/x-data-grid/src/components/GridLoadingOverlay.tsx
@@ -1,14 +1,23 @@
 import * as React from 'react';
 import PropTypes from 'prop-types';
 import CircularProgress from '@mui/material/CircularProgress';
+import { styled } from '@mui/material/styles';
 import { GridOverlay, GridOverlayProps } from './containers/GridOverlay';
 
+const GridLoadingOverlayRoot = styled(GridOverlay)({
+  position: 'absolute',
+  top: 0,
+  zIndex: 4, // should be above pinned columns, pinned rows and detail panel
+  width: '100%',
+  pointerEvents: 'none',
+});
+
 const GridLoadingOverlay = React.forwardRef<HTMLDivElement, GridOverlayProps>(
   function GridLoadingOverlay(props, ref) {
     return (
-      <GridOverlay ref={ref} {...props}>
+      <GridLoadingOverlayRoot ref={ref} {...props}>
         <CircularProgress />
-      </GridOverlay>
+      </GridLoadingOverlayRoot>
     );
   },
 );
diff --git a/packages/grid/x-data-grid/src/components/base/GridOverlays.tsx b/packages/grid/x-data-grid/src/components/base/GridOverlays.tsx
index c4f1fec86..ce3a618e5 100644
--- a/packages/grid/x-data-grid/src/components/base/GridOverlays.tsx
+++ b/packages/grid/x-data-grid/src/components/base/GridOverlays.tsx
@@ -44,8 +44,6 @@ function GridOverlayWrapper(props: React.PropsWithChildren<{}>) {
         position: 'absolute',
         top: headerHeight,
         bottom: height === 'auto' ? 0 : undefined,
-        zIndex: 4, // should be above pinned columns, pinned rows and detail panel
-        pointerEvents: 'none',
       }}
       {...props}
     />

m4theushw · 2022-08-03T23:59:53Z

packages/grid/x-data-grid/src/components/base/GridOverlays.tsx

@@ -44,8 +44,6 @@ function GridOverlayWrapper(props: React.PropsWithChildren<{}>) {
        position: 'absolute',
        top: headerHeight,
        bottom: height === 'auto' ? 0 : undefined,
-        zIndex: 4, // should be above pinned columns, pinned rows and detail panel


@cherniavskii shouldn't we keep zIndex: 4 here? It was added in #4863. Without it, other overlays, e.g. no filter results, don't take the full height.

I added a background on purpose to one overlay. Here it's how it looks like:

I could add it back, I thought moving it in the git patch proposed would be ok but the naming suggests that zIndex only applies to loading overlays...

How can we test this locally? Is thee a way to produce a custom NPM package? do a file:/ reference in my project's package.json...

How can we test this locally?

@philjones88 Please refer to the contribution guide for options on how to test an unreleased package.

@m4theushw nice catch!
I think we should not keep zIndex on the overlay wrapper, because there's no way to customize it.

Instead, we can move those styles to GridOverlay instead:

diff --git a/packages/grid/x-data-grid/src/components/GridLoadingOverlay.tsx b/packages/grid/x-data-grid/src/components/GridLoadingOverlay.tsx index 55ef3921f..8e7e4ea35 100644 --- a/packages/grid/x-data-grid/src/components/GridLoadingOverlay.tsx +++ b/packages/grid/x-data-grid/src/components/GridLoadingOverlay.tsx @@ -1,23 +1,14 @@ import * as React from 'react'; import PropTypes from 'prop-types'; import CircularProgress from '@mui/material/CircularProgress'; -import { styled } from '@mui/material/styles'; import { GridOverlay, GridOverlayProps } from './containers/GridOverlay'; -const GridLoadingOverlayRoot = styled(GridOverlay)({ - position: 'absolute', - top: 0, - zIndex: 4, // should be above pinned columns, pinned rows and detail panel - width: '100%', - pointerEvents: 'none', -}); - const GridLoadingOverlay = React.forwardRef<HTMLDivElement, GridOverlayProps>( function GridLoadingOverlay(props, ref) { return ( - <GridLoadingOverlayRoot ref={ref} {...props}> + <GridOverlay ref={ref} {...props}> <CircularProgress /> - </GridLoadingOverlayRoot> + </GridOverlay> ); }, ); diff --git a/packages/grid/x-data-grid/src/components/containers/GridOverlay.tsx b/packages/grid/x-data-grid/src/components/containers/GridOverlay.tsx index 789704435..77146b237 100644 --- a/packages/grid/x-data-grid/src/components/containers/GridOverlay.tsx +++ b/packages/grid/x-data-grid/src/components/containers/GridOverlay.tsx @@ -29,8 +29,13 @@ const GridOverlayRoot = styled('div', { slot: 'Overlay', overridesResolver: (props, styles) => styles.overlay, })(({ theme }) => ({ - display: 'flex', + position: 'absolute', + top: 0, + zIndex: 4, // should be above pinned columns, pinned rows and detail panel + width: '100%', height: '100%', + pointerEvents: 'none', + display: 'flex', alignSelf: 'center', alignItems: 'center', justifyContent: 'center',

This would make default overlays taking full height while allowing to scroll horizontally (for example to access column used for filtering that is not in the viewport).
But most importantly, it would allow to implement custom overlays with interactive elements while avoiding pointerEvents: none (so it won't cause another issue like #5590).

I've tried to push the proposed changes to this branch to make sure there's no visual regressions, but turned out I don't have the permission to push to https://github.com/VQComms/mui-x/tree/remove-grid-overlays-pointer-events-style

@philjones88 could you push the changes so we trigger the CI? Merging latest master to this branch would be good as well

I've applied the git diff above, updated my fork and rebased this branch onto master latest.

@philjones88
Thanks! Can you rebase again to restart the CI? The failure in Argos CI looks like a false negative

@cherniavskii I pushed an empty commit to trigger the CI. The branch was already up to date with master. Seems to have all passed!

m4theushw · 2022-08-11T12:13:42Z

packages/grid/x-data-grid/src/components/base/GridOverlays.tsx

@@ -44,8 +44,6 @@ function GridOverlayWrapper(props: React.PropsWithChildren<{}>) {
        position: 'absolute',


We removed pointerEvents: none but this was not sufficient to fix #5725. Since we didn't add zIndex #5674 (comment), the overlay stays behind the virtual scroller because it has position: relative, while the overlay has position: absoulte. We need to put the overlay on top of everything to fix #5725 too

Suggested change

position: 'absolute',

position: 'relative',

In https://codesandbox.io/s/mui-x-norowsoverlay-bug-forked-g6npfv?file=/src/App.tsx I still can't click the button.

This PR only fixes the regression described in #5590
#5725 is a bit different, since it was reproducible in v5.13.0 as well (before #5590 regression introduced in 5.14.0) - see https://codesandbox.io/s/mui-x-norowsoverlay-bug-forked-tzjfhj?file=/src/App.tsx

the overlay stays behind the virtual scroller because it has position: relative, while the overlay has position: absoulte

We tried to fix it with #5558, but this introduced the regression above.
If we add zIndex to GridOverlays - then https://github.com/mui/mui-x/blob/master/test/e2e/index.test.ts#L330 would fail.
This is the reason I've added pointerEvents: 'none', so that scroll gets bypassed to virtualScroller.

So it's kind of a vicious circle with the regressions here :D

For the #5725, after we merge this PR users can do this for custom overlays:

const NoRowsOverlay = () => { return <div style={{ position: 'relative', zIndex: 1 }}></div> }

That's the best trade-off I see right now, but I'm open to other solutions

One solution that would keep https://github.com/mui/mui-x/blob/master/test/e2e/index.test.ts#L330 passing is to render the overlays inside the virtualization component.

Maybe it will be possible to do without breaking changes - I'll take a look

m4theushw

Looks good. It's what we can do given the constraints we have. In v6 we can rearrange the overlay to render correctly without additional styling.

cherniavskii · 2022-08-12T10:27:21Z

Thanks @philjones88

cherniavskii changed the title ~~Remove DataGrid GridOverlays pointerEvents style~~ [DataGrid] Fix GridOverlays bypassing pointer events Aug 3, 2022

cherniavskii approved these changes Aug 3, 2022

View reviewed changes

m4theushw reviewed Aug 3, 2022

View reviewed changes

m4theushw added component: data grid This is the name of the generic UI component, not the React module! feature: Rendering layout Related to the data grid Rendering engine labels Aug 4, 2022

m4theushw mentioned this pull request Aug 9, 2022

[data grid] Virtual scroller covers NoRowsOverlay #5725

Closed

2 tasks

philjones88 added 3 commits August 10, 2022 08:34

Remove DataGrid GridOverlays pointerEvents style

c76996b

patch applied

84586e9

apply patch

15346e2

philjones88 force-pushed the remove-grid-overlays-pointer-events-style branch from 1cdf936 to 15346e2 Compare August 10, 2022 07:34

trigger ci

3e8530c

cherniavskii added the regression A bug, but worse label Aug 10, 2022

cherniavskii requested a review from m4theushw August 10, 2022 12:52

cherniavskii approved these changes Aug 10, 2022

View reviewed changes

m4theushw reviewed Aug 11, 2022

View reviewed changes

m4theushw approved these changes Aug 12, 2022

View reviewed changes

cherniavskii merged commit 2b10cdd into mui:master Aug 12, 2022

alexfauquette pushed a commit to alexfauquette/mui-x that referenced this pull request Aug 26, 2022

[DataGrid] Fix GridOverlays bypassing pointer events (mui#5674)

a85164c

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[DataGrid] Fix `GridOverlays` bypassing pointer events #5674

[DataGrid] Fix `GridOverlays` bypassing pointer events #5674

philjones88 commented Aug 1, 2022

mui-bot commented Aug 1, 2022 •

edited

Loading

cherniavskii commented Aug 1, 2022

philjones88 commented Aug 2, 2022 •

edited

Loading

cherniavskii commented Aug 2, 2022

m4theushw Aug 3, 2022

philjones88 Aug 6, 2022

LukasTy Aug 8, 2022

cherniavskii Aug 9, 2022

cherniavskii Aug 9, 2022 •

edited

Loading

philjones88 Aug 10, 2022

cherniavskii Aug 10, 2022

philjones88 Aug 10, 2022

m4theushw Aug 11, 2022 •

edited

Loading

cherniavskii Aug 11, 2022

m4theushw Aug 12, 2022

cherniavskii Aug 12, 2022

m4theushw left a comment

cherniavskii commented Aug 12, 2022

		@@ -44,8 +44,6 @@ function GridOverlayWrapper(props: React.PropsWithChildren<{}>) {
		position: 'absolute',

[DataGrid] Fix GridOverlays bypassing pointer events #5674

[DataGrid] Fix GridOverlays bypassing pointer events #5674

Conversation

philjones88 commented Aug 1, 2022

mui-bot commented Aug 1, 2022 • edited Loading

cherniavskii commented Aug 1, 2022

philjones88 commented Aug 2, 2022 • edited Loading

cherniavskii commented Aug 2, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cherniavskii Aug 9, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

m4theushw Aug 11, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

m4theushw left a comment

Choose a reason for hiding this comment

cherniavskii commented Aug 12, 2022

[DataGrid] Fix `GridOverlays` bypassing pointer events #5674

[DataGrid] Fix `GridOverlays` bypassing pointer events #5674

mui-bot commented Aug 1, 2022 •

edited

Loading

philjones88 commented Aug 2, 2022 •

edited

Loading

cherniavskii Aug 9, 2022 •

edited

Loading

m4theushw Aug 11, 2022 •

edited

Loading