[DataGrid] Prevent column header scroll #4280

m4theushw · 2022-03-25T21:18:24Z

Same solution from #3718 and #2558

Before: https://codesandbox.io/s/intelligent-pasteur-5etmhd?file=/src/App.js
After: https://codesandbox.io/s/immutable-star-qb78zz?file=/src/App.js

mui-bot · 2022-03-25T21:23:22Z

These are the results for the performance tests:

Test case	Unit	Min	Max	Median	Mean	σ
Filter 100k rows	ms	241.3	398.8	301.5	317.02	69.329
Sort 100k rows	ms	409.3	836.3	800.1	701.34	162.455
Select 100k rows	ms	107.3	198.9	155.4	154.54	31.693
Deselect 100k rows	ms	90.4	291.2	291.2	149.26	73.74

Generated by 🚫 dangerJS against d420045

oliviertassinari · 2022-03-25T23:19:47Z

packages/grid/x-data-grid/src/components/columnHeaders/GridColumnHeaderItem.tsx

-      } else {
-        headerCellRef.current!.focus();
+      const elementToFocus = focusableElement || headerCellRef.current;
+      elementToFocus?.focus({ preventScroll: true });


AFAIK this preventScroll is not viable because it doesn't work on Safari. What I would propose instead is to look into why the container is scrollable in the first place. We use translate 3d, so why is it possible for scrollLeft to not always be 0 in the first place?

Even adding overflow: hidden I still can scroll a div. It's expected: https://stackoverflow.com/questions/33703204/is-scrolltop-and-scrollleft-for-overflow-hidden-elements-reliable

In the past, we even did a small hack by listening to scroll and calling scrollLeft=0 to prevent scrolling the container - which had overflow: hidden - when using a touchpad device.

mui-x/packages/grid/_modules_/grid/hooks/features/virtualization/useGridVirtualRows.ts

Lines 487 to 492 in b81daef

const preventScroll = React.useCallback((event: any) => {

event.target.scrollLeft = 0;

event.target.scrollTop = 0;

}, []);

useNativeEventListener(apiRef, windowRef, 'scroll', handleScroll, { passive: true });

AFAIK this preventScroll is not viable because it doesn't work on Safari.

That's why I call scrollLeft=0 down below if preventScroll is not supported.

Right ok, great points, all my previous points have been answered.

I had a closer look: did you consider this option?

Only do this:

mui-x/packages/grid/x-data-grid/src/components/columnHeaders/GridColumnHeaderItem.tsx

Line 210 in 65cf41c

apiRef.current.columnHeadersContainerElementRef!.current!.scrollLeft = 0;

Remove (seems dead code)

mui-x/packages/grid/x-data-grid/src/hooks/features/columnHeaders/useGridColumnHeaders.tsx

Lines 52 to 54 in 6643404

React.useEffect(() => {

apiRef.current.columnHeadersContainerElementRef!.current!.scrollLeft = 0;

}, [apiRef]);

I'm asking because using preventScroll here might introduce the same problems as #4282 (vertical scrolling) but for horizontal scrolling.

Actually, speaking of horizontal scrolling, I have found a bug when rows={[]} https://codesandbox.io/s/datagridprodemo-material-demo-forked-8e4l8w:

focus a header cell

use the arrow left or arrow right to move the focus

the body doesn't scroll horizontally

Now, maybe we don't need to care. Who would try to scroll/use the arrow key? There are no data loaded 😁

The "no row" bug can be fixed. It's an early return to modify in scroll hook

https://github.com/mui/mui-x/blob/master/packages/grid/x-data-grid/src/hooks/features/scroll/useGridScroll.ts#L56:L58

Is it necessary to prevent scroll? I thought adding the hasFocus un the useLayoutEffect dependencies was enough to prevent scroll on rendering

The only bug I can think of with the scroll on rendering is when the element is unmounted because it goes out of the virtualization engine and comes back. But that's a larger problem.

For example, I set focus on a cell, I can use keyboard navigation as long as the cell is in the virtualisation buffer. As soon as it exits the buffer, the focus moves to <body> and the gird does not respond anymore to keyboard interactions
https://github.com/mui/mui-x/blob/master/packages/grid/x-data-grid/src/hooks/features/scroll/useGridScroll.ts#L56:L58

I removed preventScroll and kept only scrollLeft=0. I didn't realize the side effect of using it.

I thought adding the hasFocus un the useLayoutEffect dependencies was enough to prevent scroll on rendering

It's not sufficient. If I remove scrollLeft=0 and scroll very fast to left and right the bug appears at some point. This happens because the column header component may be mounted during scroll.

The "no row" bug can be fixed. It's an early return to modify in scroll hook

Interesting, I won't fix it here. Feel free to submit a PR.

Now, maybe we don't need to care. Who would try to scroll/use the arrow key? There are no data loaded 😁

If what caused "no results" was applying a filter in a column that's outside the visible area, then the user may use the arrow keys to navigate to them. We had a issue about that when we blocked the scrollbar if rows=[]: #3795 (comment)

m4theushw · 2022-03-28T20:36:39Z

packages/grid/x-data-grid/src/hooks/features/columnHeaders/useGridColumnHeaders.tsx

@@ -171,7 +171,7 @@ export const useGridColumnHeaders = (props: UseGridColumnHeadersProps) => {

      columns.push(
        <GridColumnHeaderItem
-          key={i}
+          key={column.field}


Prevents the effect triggering twice.

[DataGrid] Prevent column header scroll

65cf41c

m4theushw mentioned this pull request Mar 25, 2022

[data-grid] Clicking header and scrolling causes headers to skew #4275

Closed

2 tasks

m4theushw requested a review from alexfauquette March 25, 2022 21:41

m4theushw added bug 🐛 Something doesn't work component: data grid This is the name of the generic UI component, not the React module! labels Mar 25, 2022

oliviertassinari reviewed Mar 25, 2022

View reviewed changes

This was referenced Mar 26, 2022

[data grid] Keyboard arrow doesn't scroll #4282

Open

[core] Remove dead code #4283

Merged

m4theushw added 2 commits March 28, 2022 16:36

Revert changes

1e2e95d

Use field as key

ec9ed8b

m4theushw self-assigned this Mar 28, 2022

m4theushw commented Mar 28, 2022

View reviewed changes

alexfauquette mentioned this pull request Mar 29, 2022

[DataGrid] Allows to use keyboard navigation even with no rows #4302

Merged

alexfauquette approved these changes Apr 4, 2022

View reviewed changes

This was referenced Apr 6, 2022

[data grid] Scroll glitches after column reordering when a column is pinned right #4300

Closed

Pinned columns shift when clicking column underneath #4382

Closed

m4theushw added 2 commits April 11, 2022 12:00

Merge branch 'master' into prevent-column-header-scroll

8e76c0a

rerun ci

d420045

m4theushw merged commit 33db58d into mui:master Apr 11, 2022

m4theushw deleted the prevent-column-header-scroll branch April 11, 2022 15:42

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

[DataGrid] Prevent column header scroll (mui#4280)

b7d9c00

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[DataGrid] Prevent column header scroll #4280

[DataGrid] Prevent column header scroll #4280

m4theushw commented Mar 25, 2022 •

edited

Loading

mui-bot commented Mar 25, 2022 •

edited

Loading

oliviertassinari Mar 25, 2022

m4theushw Mar 25, 2022 •

edited

Loading

oliviertassinari Mar 26, 2022 •

edited

Loading

alexfauquette Mar 28, 2022

m4theushw Mar 28, 2022

m4theushw Mar 28, 2022

	const preventScroll = React.useCallback((event: any) => {
	event.target.scrollLeft = 0;
	event.target.scrollTop = 0;
	}, []);

	useNativeEventListener(apiRef, windowRef, 'scroll', handleScroll, { passive: true });

	React.useEffect(() => {
	apiRef.current.columnHeadersContainerElementRef!.current!.scrollLeft = 0;
	}, [apiRef]);

[DataGrid] Prevent column header scroll #4280

[DataGrid] Prevent column header scroll #4280

Conversation

m4theushw commented Mar 25, 2022 • edited Loading

mui-bot commented Mar 25, 2022 • edited Loading

oliviertassinari Mar 25, 2022

Choose a reason for hiding this comment

m4theushw Mar 25, 2022 • edited Loading

Choose a reason for hiding this comment

oliviertassinari Mar 26, 2022 • edited Loading

Choose a reason for hiding this comment

alexfauquette Mar 28, 2022

Choose a reason for hiding this comment

m4theushw Mar 28, 2022

Choose a reason for hiding this comment

m4theushw Mar 28, 2022

Choose a reason for hiding this comment

m4theushw commented Mar 25, 2022 •

edited

Loading

mui-bot commented Mar 25, 2022 •

edited

Loading

m4theushw Mar 25, 2022 •

edited

Loading

oliviertassinari Mar 26, 2022 •

edited

Loading