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.

Sign up for GitHub

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

Jump to bottom

[DataGrid] Fix GridOverlays bypassing pointer events #5674

Merged
merged 4 commits into from
Aug 12, 2022
Merged

[DataGrid] Fix GridOverlays bypassing pointer events #5674

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
c76996b
Remove DataGrid GridOverlays pointerEvents style
philjones88 Aug 1, 2022
84586e9
patch applied
philjones88 Aug 3, 2022
15346e2
apply patch
philjones88 Aug 10, 2022
3e8530c
trigger ci
philjones88 Aug 10, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 0 additions & 2 deletions packages/grid/x-data-grid/src/components/base/GridOverlays.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,8 +44,6 @@ function GridOverlayWrapper(props: React.PropsWithChildren<{}>) {
position: 'absolute',
Copy link
Member

@m4theushw m4theushw Aug 11, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

top: headerHeight,
bottom: height === 'auto' ? 0 : undefined,
zIndex: 4, // should be above pinned columns, pinned rows and detail panel
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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:

image

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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...

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How can we test this locally?

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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).

Copy link
Member

@cherniavskii cherniavskii Aug 9, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

pointerEvents: 'none',
}}
{...props}
/>
Expand Down
7 changes: 6 additions & 1 deletion packages/grid/x-data-grid/src/components/containers/GridOverlay.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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',
Expand Down