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

[DataGrid] Fix scrollbar position not being updated after scrollToIndexes #14888

Merged
merged 14 commits into from
Oct 10, 2024
Merged

[DataGrid] Fix scrollbar position not being updated after scrollToIndexes #14888

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
d552ec5
Revert changes made in #14877
arminmeh Oct 9, 2024
4b31474
Update scrollbar position through ref
arminmeh Oct 9, 2024
ec40d0b
Adjust test
arminmeh Oct 9, 2024
005e2cf
Revert test update
arminmeh Oct 9, 2024
a07586b
Karma output with debug logs
arminmeh Oct 9, 2024
ed8524b
Revert config change
arminmeh Oct 9, 2024
a82a340
Use different scrollTop numbers
arminmeh Oct 9, 2024
5d6ff4e
Revert test change
arminmeh Oct 9, 2024
f51a99f
Do not update vertical scrollbar
arminmeh Oct 9, 2024
84af820
Fix lint
arminmeh Oct 9, 2024
11ec9e5
Align with master
arminmeh Oct 9, 2024
a1d2b2b
Revert code changes
arminmeh Oct 9, 2024
d112bfd
Wrap events in
arminmeh Oct 9, 2024
4724c6f
Missing revert
arminmeh Oct 9, 2024
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
41 changes: 27 additions & 14 deletions packages/x-data-grid-pro/src/tests/infiniteLoader.DataGridPro.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import * as React from 'react';
import { createRenderer, waitFor } from '@mui/internal-test-utils';
import { act, createRenderer, waitFor } from '@mui/internal-test-utils';
import { expect } from 'chai';
import { DataGridPro } from '@mui/x-data-grid-pro';
import { spy, restore } from 'sinon';
Expand Down Expand Up @@ -40,24 +40,37 @@ describe('<DataGridPro /> - Infnite loader', () => {
}
const { container, setProps } = render(<TestCase rows={baseRows} />);
const virtualScroller = container.querySelector('.MuiDataGrid-virtualScroller')!;
// arbitrary number to make sure that the bottom of the grid window is reached.
virtualScroller.scrollTop = 12345;
virtualScroller.dispatchEvent(new Event('scroll'));

await act(async () => {
// arbitrary number to make sure that the bottom of the grid window is reached.
virtualScroller.scrollTop = 12345;
virtualScroller.dispatchEvent(new Event('scroll'));
});

await waitFor(() => {
expect(handleRowsScrollEnd.callCount).to.equal(1);
});
setProps({
rows: baseRows.concat(
{ id: 6, brand: 'Gucci' },
{ id: 7, brand: "Levi's" },
{ id: 8, brand: 'Ray-Ban' },
),

await act(async () => {
setProps({
rows: baseRows.concat(
{ id: 6, brand: 'Gucci' },
{ id: 7, brand: "Levi's" },
{ id: 8, brand: 'Ray-Ban' },
),
});

// Trigger a scroll again to notify the grid that we're not in the bottom area anymore
virtualScroller.dispatchEvent(new Event('scroll'));
});
// Trigger a scroll again to notify the grid that we're not in the bottom area anymore
virtualScroller.dispatchEvent(new Event('scroll'));

expect(handleRowsScrollEnd.callCount).to.equal(1);
virtualScroller.scrollTop = 12345;
virtualScroller.dispatchEvent(new Event('scroll'));

await act(async () => {
virtualScroller.scrollTop = 12345;
virtualScroller.dispatchEvent(new Event('scroll'));
});

await waitFor(() => {
expect(handleRowsScrollEnd.callCount).to.equal(2);
});
Expand Down
20 changes: 13 additions & 7 deletions packages/x-data-grid/src/components/virtualization/GridVirtualScrollbar.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,8 +70,8 @@ const GridVirtualScrollbar = React.forwardRef<HTMLDivElement, GridVirtualScrollb
function GridVirtualScrollbar(props, ref) {
const apiRef = useGridPrivateApiContext();
const rootProps = useGridRootProps();
const lastPositionScroller = React.useRef(0);
const lastPositionScrollbar = React.useRef(0);
const isLocked = React.useRef(false);
const lastPosition = React.useRef(0);
const scrollbarRef = React.useRef<HTMLDivElement>(null);
const contentRef = React.useRef<HTMLDivElement>(null);
const classes = useUtilityClasses(rootProps, props.position);
Expand All @@ -96,28 +96,34 @@ const GridVirtualScrollbar = React.forwardRef<HTMLDivElement, GridVirtualScrollb
const scroller = apiRef.current.virtualScrollerRef.current!;
const scrollbar = scrollbarRef.current!;

if (scroller[propertyScroll] === lastPositionScroller.current) {
if (scroller[propertyScroll] === lastPosition.current) {
return;
}

if (isLocked.current) {
isLocked.current = false;
return;
}
isLocked.current = true;

const value = scroller[propertyScroll] / contentSize;
scrollbar[propertyScroll] = value * scrollbarInnerSize;

lastPositionScrollbar.current = scrollbar[propertyScroll];
lastPosition.current = scroller[propertyScroll];
});

const onScrollbarScroll = useEventCallback(() => {
const scroller = apiRef.current.virtualScrollerRef.current!;
const scrollbar = scrollbarRef.current!;

if (scrollbar[propertyScroll] === lastPositionScrollbar.current) {
if (isLocked.current) {
isLocked.current = false;
return;
}
isLocked.current = true;

const value = scrollbar[propertyScroll] / scrollbarInnerSize;
scroller[propertyScroll] = value * contentSize;

lastPositionScroller.current = scroller[propertyScroll];
});

useOnMount(() => {
Expand Down
4 changes: 4 additions & 0 deletions packages/x-data-grid/src/hooks/core/useGridRefs.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,8 @@ export const useGridRefs = <PrivateApi extends GridPrivateApiCommon>(
const rootElementRef = React.useRef<HTMLDivElement>(null);
const mainElementRef = React.useRef<HTMLDivElement>(null);
const virtualScrollerRef = React.useRef<HTMLDivElement>(null);
const virtualScrollbarVerticalRef = React.useRef<HTMLDivElement>(null);
const virtualScrollbarHorizontalRef = React.useRef<HTMLDivElement>(null);
const columnHeadersContainerRef = React.useRef<HTMLDivElement>(null);

apiRef.current.register('public', {
Expand All @@ -16,6 +18,8 @@ export const useGridRefs = <PrivateApi extends GridPrivateApiCommon>(
apiRef.current.register('private', {
mainElementRef,
virtualScrollerRef,
virtualScrollbarVerticalRef,
virtualScrollbarHorizontalRef,
columnHeadersContainerRef,
});
};
26 changes: 23 additions & 3 deletions packages/x-data-grid/src/hooks/features/scroll/useGridScroll.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,8 @@ export const useGridScroll = (
const logger = useGridLogger(apiRef, 'useGridScroll');
const colRef = apiRef.current.columnHeadersContainerRef;
const virtualScrollerRef = apiRef.current.virtualScrollerRef!;
const virtualScrollbarHorizontalRef = apiRef.current.virtualScrollbarHorizontalRef!;
const virtualScrollbarVerticalRef = apiRef.current.virtualScrollbarVerticalRef!;
const visibleSortedRows = useGridSelector(apiRef, gridExpandedSortedRowEntriesSelector);

const scrollToIndexes = React.useCallback<GridScrollApi['scrollToIndexes']>(
Expand Down Expand Up @@ -144,19 +146,37 @@ export const useGridScroll = (

const scroll = React.useCallback<GridScrollApi['scroll']>(
(params: Partial<GridScrollParams>) => {
if (virtualScrollerRef.current && params.left !== undefined && colRef.current) {
if (
virtualScrollerRef.current &&
virtualScrollbarHorizontalRef.current &&
params.left !== undefined &&
colRef.current
) {
const direction = isRtl ? -1 : 1;
colRef.current.scrollLeft = params.left;
virtualScrollerRef.current.scrollLeft = direction * params.left;
virtualScrollbarHorizontalRef.current.scrollLeft = direction * params.left;
logger.debug(`Scrolling left: ${params.left}`);
}
if (virtualScrollerRef.current && params.top !== undefined) {
if (
virtualScrollerRef.current &&
virtualScrollbarVerticalRef.current &&
params.top !== undefined
) {
virtualScrollerRef.current.scrollTop = params.top;
virtualScrollbarVerticalRef.current.scrollTop = params.top;
logger.debug(`Scrolling top: ${params.top}`);
}
logger.debug(`Scrolling, updating container, and viewport`);
},
[virtualScrollerRef, isRtl, colRef, logger],
[
virtualScrollerRef,
virtualScrollbarHorizontalRef,
virtualScrollbarVerticalRef,
isRtl,
colRef,
logger,
],
);

const getScrollPosition = React.useCallback<GridScrollApi['getScrollPosition']>(() => {
Expand Down
4 changes: 2 additions & 2 deletions packages/x-data-grid/src/hooks/features/virtualization/useGridVirtualScroller.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -119,8 +119,8 @@ export const useGridVirtualScroller = () => {
const gridRootRef = apiRef.current.rootElementRef;
const mainRef = apiRef.current.mainElementRef;
const scrollerRef = apiRef.current.virtualScrollerRef;
const scrollbarVerticalRef = React.useRef<HTMLDivElement>(null);
const scrollbarHorizontalRef = React.useRef<HTMLDivElement>(null);
const scrollbarVerticalRef = apiRef.current.virtualScrollbarVerticalRef;
const scrollbarHorizontalRef = apiRef.current.virtualScrollbarHorizontalRef;
const contentHeight = dimensions.contentSize.height;
const columnsTotalWidth = dimensions.columnsTotalWidth;
const hasColSpan = useGridSelector(apiRef, gridHasColSpanSelector);
Expand Down
10 changes: 9 additions & 1 deletion packages/x-data-grid/src/models/api/gridCoreApi.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,9 +69,17 @@ export interface GridCorePrivateApi<
*/
mainElementRef: React.RefObject<HTMLDivElement>;
/**
* The React ref of the grid virtual scroller container element.
* The React ref of the grid's virtual scroller container element.
*/
virtualScrollerRef: React.RefObject<HTMLDivElement>;
/**
* The React ref of the grid's vertical virtual scrollbar container element.
*/
virtualScrollbarVerticalRef: React.RefObject<HTMLDivElement>;
/**
* The React ref of the grid's horizontal virtual scrollbar container element.
*/
virtualScrollbarHorizontalRef: React.RefObject<HTMLDivElement>;
Comment on lines +75 to +82
Copy link
Contributor

Choose a reason for hiding this comment

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

Do these need to be public? I'd rather avoid exposing stuff until it needs to be exposed, once it's public it can't be refactored/removed.

Copy link
Contributor

@romgrk romgrk Oct 9, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I also wanted to refactor the names to remove the "virtual" prefix from everywhere (but I couldn't because it would be a breaking change), it would make for more readable names and shorter lines. It could be interesting to name those just scrollbarXxxxRef.

Copy link
Member

Choose a reason for hiding this comment

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

They're defined in the GridCorePrivateApi interface, so not public 👍🏻

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The only public thing around this are few classes
https://mui.com/x/api/data-grid/data-grid/#data-grid-classes-MuiDataGrid-virtualScroller

we can make the change in v8 if you want to have a cleanup

Copy link
Contributor Author

Choose a reason for hiding this comment

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

by the way, merging this to fix the regression for the release
renames can be handled separately

/**
* The React ref of the grid column container virtualized div element.
*/
Expand Down