Skip to content

[DataGrid] Prevent column header scroll #4280

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

Merged
merged 5 commits into from
Apr 11, 2022
Merged

[DataGrid] Prevent column header scroll #4280

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
65cf41c
[DataGrid] Prevent column header scroll
m4theushw Mar 25, 2022
1e2e95d
Revert changes
m4theushw Mar 28, 2022
ec9ed8b
Use field as key
m4theushw Mar 28, 2022
8e76c0a
Merge branch 'master' into prevent-column-header-scroll
m4theushw Apr 11, 2022
d420045
rerun ci
m4theushw Apr 11, 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
20 changes: 20 additions & 0 deletions packages/grid/x-data-grid-pro/src/tests/columnHeaders.DataGridPro.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,26 @@ describe('<DataGridPro /> - Column Headers', () => {
],
};

it('should not scroll the column headers when a column is focused', function test() {
if (isJSDOM) {
this.skip(); // JSDOM version of .focus() doesn't scroll
}
render(
<div style={{ width: 102, height: 500 }}>
<DataGridPro
{...baselineProps}
columns={[{ field: 'brand' }, { field: 'foundationYear' }]}
/>
</div>,
);
const columnHeaders = document.querySelector('.MuiDataGrid-columnHeaders')!;
expect(columnHeaders.scrollLeft).to.equal(0);
const columnCell = getColumnHeaderCell(0);
columnCell.focus();
fireEvent.keyDown(columnCell, { key: 'End' });
expect(columnHeaders.scrollLeft).to.equal(0);
});

describe('GridColumnHeaderMenu', () => {
it('should close the menu when the window is scrolled', () => {
render(
Expand Down
15 changes: 1 addition & 14 deletions packages/grid/x-data-grid/src/components/cell/GridCell.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ import { useGridApiContext } from '../../hooks/utils/useGridApiContext';
import { useGridRootProps } from '../../hooks/utils/useGridRootProps';
import { gridFocusCellSelector } from '../../hooks/features/focus/gridFocusStateSelector';
import { DataGridProcessedProps } from '../../models/props/DataGridProps';
import { doesSupportPreventScroll } from '../../utils/utils';

export interface GridCellProps {
align: GridAlignment;
Expand Down Expand Up @@ -45,20 +46,6 @@ export interface GridCellProps {
[x: string]: any; // TODO it should not accept unspecified props
}

// Based on https://stackoverflow.com/a/59518678
let cachedSupportsPreventScroll: boolean;
function doesSupportPreventScroll(): boolean {
if (cachedSupportsPreventScroll === undefined) {
document.createElement('div').focus({
get preventScroll() {
cachedSupportsPreventScroll = true;
return false;
},
});
}
return cachedSupportsPreventScroll;
}

type OwnerState = Pick<GridCellProps, 'align' | 'showRightBorder' | 'isEditable'> & {
classes?: DataGridProcessedProps['classes'];
};
Expand Down
13 changes: 8 additions & 5 deletions packages/grid/x-data-grid/src/components/columnHeaders/GridColumnHeaderItem.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ import { GridColumnHeaderMenu } from '../menu/columnMenu/GridColumnHeaderMenu';
import { getDataGridUtilityClass } from '../../constants/gridClasses';
import { useGridRootProps } from '../../hooks/utils/useGridRootProps';
import { DataGridProcessedProps } from '../../models/props/DataGridProps';
import { doesSupportPreventScroll } from '../../utils/utils';

interface GridColumnHeaderItemProps {
colIndex: number;
Expand Down Expand Up @@ -201,13 +202,15 @@ function GridColumnHeaderItem(props: GridColumnHeaderItemProps) {
const columnMenuState = apiRef.current.state.columnMenu;
if (hasFocus && !columnMenuState.open) {
const focusableElement = headerCellRef.current!.querySelector<HTMLElement>('[tabindex="0"]');
if (focusableElement) {
focusableElement!.focus();
} else {
headerCellRef.current!.focus();
const elementToFocus = focusableElement || headerCellRef.current;
elementToFocus?.focus({ preventScroll: true });
Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Collaborator Author

@m4theushw m4theushw Mar 25, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

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

msedge_0wKPMSGImu

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.

Copy link
Member

@oliviertassinari oliviertassinari Mar 26, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

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

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

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

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

  1. focus a header cell
  2. use the arrow left or arrow right to move the focus
  3. 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 😁

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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)


if (!doesSupportPreventScroll()) {
// A ponyfill for .focus({ preventScroll: true })
apiRef.current.columnHeadersContainerElementRef!.current!.scrollLeft = 0;
}
}
});
}, [apiRef, hasFocus]);

const headerClassName =
typeof column.headerClassName === 'function'
Expand Down
6 changes: 2 additions & 4 deletions packages/grid/x-data-grid/src/components/columnHeaders/GridColumnHeaders.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,13 +44,11 @@ const GridColumnHeadersRoot = styled('div', {
};
});

interface GridColumnHeadersProps extends React.HTMLAttributes<HTMLDivElement> {
innerRef?: React.Ref<HTMLDivElement>;
}
interface GridColumnHeadersProps extends React.HTMLAttributes<HTMLDivElement> {}

export const GridColumnHeaders = React.forwardRef<HTMLDivElement, GridColumnHeadersProps>(
function GridColumnHeaders(props, ref) {
const { innerRef, className, ...other } = props;
const { className, ...other } = props;
const rootProps = useGridRootProps();

const ownerState = { classes: rootProps.classes };
Expand Down
14 changes: 14 additions & 0 deletions packages/grid/x-data-grid/src/utils/utils.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,20 @@ export function escapeRegExp(value: string): string {
export const clamp = (value: number, min: number, max: number) =>
Math.max(min, Math.min(max, value));

// Based on https://stackoverflow.com/a/59518678
let cachedSupportsPreventScroll: boolean;
export function doesSupportPreventScroll(): boolean {
if (cachedSupportsPreventScroll === undefined) {
document.createElement('div').focus({
get preventScroll() {
cachedSupportsPreventScroll = true;
return false;
},
});
}
return cachedSupportsPreventScroll;
}

/**
* Based on `fast-deep-equal`
*
Expand Down