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.

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 first row flickering with autoHeight enabled #14235

Merged
merged 8 commits into from
Aug 30, 2024
4 changes: 4 additions & 0 deletions docs/data/data-grid/virtualization/virtualization.md
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,10 @@ Row virtualization is the insertion and removal of rows as the data grid scrolls
The grid renders some additional rows above and below the visible rows. You can use `rowBufferPx` prop to hint to the Data Grid the area to render, but this value may not be respected in certain situations, for example during high-speed scrolling.
Row virtualization is limited to 100 rows in the `DataGrid` component.

:::warning
Row virtualization does not work with the `autoHeight` prop enabled.
:::

## Column virtualization

Column virtualization is the insertion and removal of columns as the data grid scrolls horizontally.
Expand Down
10 changes: 9 additions & 1 deletion docs/pages/x/api/data-grid/selectors.json
Original file line number Diff line number Diff line change
Expand Up @@ -518,16 +518,24 @@
"name": "gridVirtualizationColumnEnabledSelector",
"returnType": "boolean",
"category": "Virtualization",
"description": "Get the enabled state for virtualization",
"description": "Get the enabled state for column virtualization",
"supportsApiRef": true
},
{
"name": "gridVirtualizationEnabledSelector",
"returnType": "boolean",
"category": "Virtualization",
"deprecated": "Use `gridVirtualizationColumnEnabledSelector` and `gridVirtualizationRowEnabledSelector`",
"description": "Get the enabled state for virtualization",
"supportsApiRef": true
},
{
"name": "gridVirtualizationRowEnabledSelector",
"returnType": "boolean",
"category": "Virtualization",
"description": "Get the enabled state for row virtualization",
"supportsApiRef": true
},
{
"name": "gridVirtualizationSelector",
"returnType": "GridVirtualizationState",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -168,7 +168,7 @@ export const useGridColumnHeaders = (props: UseGridColumnHeadersProps) => {
maxLastColumn = visibleColumns.length,
} = params || {};

const firstColumnToRender = !hasVirtualization ? 0 : currentContext.firstColumnIndex;
const firstColumnToRender = currentContext.firstColumnIndex;
Copy link
Contributor Author

@KenanYusuf KenanYusuf Aug 28, 2024

Choose a reason for hiding this comment

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

There was a bug here. hasVirtualization is using gridVirtualizationColumnEnabledSelector to get the column virtualization state. This was previously always true, even when the disableVirtualization prop was passed to DataGrid. The 0 here meant that pinned column headers were duplicated were not being removed from the middle section of columns.

const lastColumnToRender = !hasVirtualization ? maxLastColumn : currentContext.lastColumnIndex;
const renderedColumns = visibleColumns.slice(firstColumnToRender, lastColumnToRender);

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11,21 +11,31 @@ export const gridVirtualizationSelector = (state: GridStateCommunity) => state.v
/**
* Get the enabled state for virtualization
* @category Virtualization
* @deprecated Use `gridVirtualizationColumnEnabledSelector` and `gridVirtualizationRowEnabledSelector`
*/
export const gridVirtualizationEnabledSelector = createSelector(
gridVirtualizationSelector,
(state) => state.enabled,
);

/**
* Get the enabled state for virtualization
* Get the enabled state for column virtualization
* @category Virtualization
*/
export const gridVirtualizationColumnEnabledSelector = createSelector(
gridVirtualizationSelector,
(state) => state.enabledForColumns,
);

/**
* Get the enabled state for row virtualization
* @category Virtualization
*/
export const gridVirtualizationRowEnabledSelector = createSelector(
gridVirtualizationSelector,
(state) => state.enabledForRows,
);

/**
* Get the render context
* @category Virtualization
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ import { GridRowProps } from '../../../components/GridRow';
import { GridInfiniteLoaderPrivateApi } from '../../../models/api/gridInfiniteLoaderApi';
import {
gridRenderContextSelector,
gridVirtualizationEnabledSelector,
gridVirtualizationRowEnabledSelector,
gridVirtualizationColumnEnabledSelector,
} from './gridVirtualizationSelectors';
import { EMPTY_RENDER_CONTEXT } from './useGridVirtualization';
Expand Down Expand Up @@ -99,7 +99,7 @@ export const useGridVirtualScroller = () => {
const apiRef = useGridPrivateApiContext() as React.MutableRefObject<PrivateApiWithInfiniteLoader>;
const rootProps = useGridRootProps();
const visibleColumns = useGridSelector(apiRef, gridVisibleColumnDefinitionsSelector);
const enabled = useGridSelector(apiRef, gridVirtualizationEnabledSelector) && !isJSDOM;
const enabledForRows = useGridSelector(apiRef, gridVirtualizationRowEnabledSelector) && !isJSDOM;
const enabledForColumns =
useGridSelector(apiRef, gridVirtualizationColumnEnabledSelector) && !isJSDOM;
const dimensions = useGridSelector(apiRef, gridDimensionsSelector);
Expand Down Expand Up @@ -262,7 +262,7 @@ export const useGridVirtualScroller = () => {
MINIMUM_COLUMN_WIDTH * 6,
);

const inputs = inputsSelector(apiRef, rootProps, enabled, enabledForColumns);
const inputs = inputsSelector(apiRef, rootProps, enabledForRows, enabledForColumns);
const nextRenderContext = computeRenderContext(inputs, scrollPosition.current, scrollCache);

// Prevents batching render context changes
Expand All @@ -276,7 +276,7 @@ export const useGridVirtualScroller = () => {
};

const forceUpdateRenderContext = () => {
const inputs = inputsSelector(apiRef, rootProps, enabled, enabledForColumns);
const inputs = inputsSelector(apiRef, rootProps, enabledForRows, enabledForColumns);
const nextRenderContext = computeRenderContext(inputs, scrollPosition.current, scrollCache);
// Reset the frozen context when the render context changes, see the illustration in https://github.com/mui/mui-x/pull/12353
frozenContext.current = undefined;
Expand Down Expand Up @@ -551,15 +551,17 @@ export const useGridVirtualScroller = () => {
}, [apiRef, rowsMeta.currentPageTotalHeight]);

useEnhancedEffect(() => {
if (enabled) {
// TODO a scroll reset should not be necessary
// TODO a scroll reset should not be necessary
if (enabledForColumns) {
scrollerRef.current!.scrollLeft = 0;
}
if (enabledForRows) {
scrollerRef.current!.scrollTop = 0;
}
}, [enabled, gridRootRef, scrollerRef]);
}, [enabledForColumns, enabledForRows, gridRootRef, scrollerRef]);

useRunOnce(outerSize.width !== 0, () => {
const inputs = inputsSelector(apiRef, rootProps, enabled, enabledForColumns);
const inputs = inputsSelector(apiRef, rootProps, enabledForRows, enabledForColumns);

const initialRenderContext = computeRenderContext(inputs, scrollPosition.current, scrollCache);
updateRenderContext(initialRenderContext);
Expand Down Expand Up @@ -608,7 +610,7 @@ export const useGridVirtualScroller = () => {
};

type RenderContextInputs = {
enabled: boolean;
enabledForRows: boolean;
enabledForColumns: boolean;
apiRef: React.MutableRefObject<GridPrivateApiCommunity>;
autoHeight: boolean;
Expand All @@ -631,7 +633,7 @@ type RenderContextInputs = {
function inputsSelector(
apiRef: React.MutableRefObject<GridPrivateApiCommunity>,
rootProps: ReturnType<typeof useGridRootProps>,
enabled: boolean,
enabledForRows: boolean,
enabledForColumns: boolean,
): RenderContextInputs {
const dimensions = gridDimensionsSelector(apiRef.current.state);
Expand All @@ -640,7 +642,7 @@ function inputsSelector(
const lastRowId = apiRef.current.state.rows.dataRowIds.at(-1);
const lastColumn = visibleColumns.at(-1);
return {
enabled,
enabledForRows,
enabledForColumns,
apiRef,
autoHeight: rootProps.autoHeight,
Expand All @@ -666,19 +668,17 @@ function computeRenderContext(
scrollPosition: ScrollPosition,
scrollCache: ScrollCache,
) {
let renderContext: GridRenderContext;

if (!inputs.enabled) {
renderContext = {
firstRowIndex: 0,
lastRowIndex: inputs.rows.length,
firstColumnIndex: 0,
lastColumnIndex: inputs.visibleColumns.length,
};
} else {
const { top, left } = scrollPosition;
const realLeft = Math.abs(left) + inputs.leftPinnedWidth;
const renderContext: GridRenderContext = {
firstRowIndex: 0,
lastRowIndex: inputs.rows.length,
firstColumnIndex: 0,
lastColumnIndex: inputs.visibleColumns.length,
};

const { top, left } = scrollPosition;
const realLeft = Math.abs(left) + inputs.leftPinnedWidth;

if (inputs.enabledForRows) {
// Clamp the value because the search may return an index out of bounds.
// In the last index, this is not needed because Array.slice doesn't include it.
const firstRowIndex = Math.min(
Expand All @@ -694,46 +694,42 @@ function computeRenderContext(
? firstRowIndex + inputs.rows.length
: getNearestIndexToRender(inputs, top + inputs.viewportInnerHeight);

renderContext.firstRowIndex = firstRowIndex;
renderContext.lastRowIndex = lastRowIndex;
}

if (inputs.enabledForColumns) {
let firstColumnIndex = 0;
let lastColumnIndex = inputs.columnPositions.length;

if (inputs.enabledForColumns) {
let hasRowWithAutoHeight = false;

const [firstRowToRender, lastRowToRender] = getIndexesToRender({
firstIndex: firstRowIndex,
lastIndex: lastRowIndex,
minFirstIndex: 0,
maxLastIndex: inputs.rows.length,
bufferBefore: scrollCache.buffer.rowBefore,
bufferAfter: scrollCache.buffer.rowAfter,
positions: inputs.rowsMeta.positions,
lastSize: inputs.lastRowHeight,
});
let hasRowWithAutoHeight = false;

const [firstRowToRender, lastRowToRender] = getIndexesToRender({
firstIndex: renderContext.firstRowIndex,
lastIndex: renderContext.lastRowIndex,
minFirstIndex: 0,
maxLastIndex: inputs.rows.length,
bufferBefore: scrollCache.buffer.rowBefore,
bufferAfter: scrollCache.buffer.rowAfter,
positions: inputs.rowsMeta.positions,
lastSize: inputs.lastRowHeight,
});

for (let i = firstRowToRender; i < lastRowToRender && !hasRowWithAutoHeight; i += 1) {
const row = inputs.rows[i];
hasRowWithAutoHeight = inputs.apiRef.current.rowHasAutoHeight(row.id);
}
for (let i = firstRowToRender; i < lastRowToRender && !hasRowWithAutoHeight; i += 1) {
const row = inputs.rows[i];
hasRowWithAutoHeight = inputs.apiRef.current.rowHasAutoHeight(row.id);
}

if (!hasRowWithAutoHeight) {
firstColumnIndex = binarySearch(realLeft, inputs.columnPositions, {
atStart: true,
lastPosition: inputs.columnsTotalWidth,
});
lastColumnIndex = binarySearch(
realLeft + inputs.viewportInnerWidth,
inputs.columnPositions,
);
}
if (!hasRowWithAutoHeight) {
firstColumnIndex = binarySearch(realLeft, inputs.columnPositions, {
atStart: true,
lastPosition: inputs.columnsTotalWidth,
});
lastColumnIndex = binarySearch(realLeft + inputs.viewportInnerWidth, inputs.columnPositions);
}

renderContext = {
firstRowIndex,
lastRowIndex,
firstColumnIndex,
lastColumnIndex,
};
renderContext.firstColumnIndex = firstColumnIndex;
renderContext.lastColumnIndex = lastColumnIndex;
}

const actualRenderContext = deriveRenderContext(inputs, renderContext, scrollCache);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,11 +5,12 @@ import { DataGridProcessedProps } from '../../../models/props/DataGridProps';
import { useGridApiMethod } from '../../utils/useGridApiMethod';
import { GridStateInitializer } from '../../utils/useGridInitializeState';

type RootProps = Pick<DataGridProcessedProps, 'disableVirtualization'>;
type RootProps = Pick<DataGridProcessedProps, 'disableVirtualization' | 'autoHeight'>;

export type GridVirtualizationState = {
enabled: boolean;
enabledForColumns: boolean;
enabledForRows: boolean;
renderContext: GridRenderContext;
};

Expand All @@ -21,9 +22,12 @@ export const EMPTY_RENDER_CONTEXT = {
};

export const virtualizationStateInitializer: GridStateInitializer<RootProps> = (state, props) => {
const { disableVirtualization, autoHeight } = props;

const virtualization = {
enabled: !props.disableVirtualization,
enabledForColumns: true,
enabled: !disableVirtualization,
enabledForColumns: !disableVirtualization,
enabledForRows: !disableVirtualization && !autoHeight,
renderContext: EMPTY_RENDER_CONTEXT,
};

Expand All @@ -47,6 +51,8 @@ export function useGridVirtualization(
virtualization: {
...state.virtualization,
enabled,
enabledForColumns: enabled,
enabledForRows: enabled && !props.autoHeight,
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure how to feel about putting the !prop.autoheight logic in the state setter. I would have maybe gone for keeping the state setter as simple as possible. But I also don't find a satisfying place to put it, so I don't know.

Copy link
Contributor

Choose a reason for hiding this comment

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

Could we move the !autoheight somewhere it doesn't need to be repeated?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Any suggestions where/how @romgrk?

Copy link
Contributor

Choose a reason for hiding this comment

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

No, feel free to ignore if there's no other good place.

},
}));
};
Expand Down Expand Up @@ -75,6 +81,6 @@ export function useGridVirtualization(
/* eslint-disable react-hooks/exhaustive-deps */
React.useEffect(() => {
setVirtualization(!props.disableVirtualization);
}, [props.disableVirtualization]);
}, [props.disableVirtualization, props.autoHeight]);
/* eslint-enable react-hooks/exhaustive-deps */
}
1 change: 1 addition & 0 deletions scripts/x-data-grid-premium.exports.json
Original file line number Diff line number Diff line change
Expand Up @@ -622,6 +622,7 @@
{ "name": "GridVirtualizationApi", "kind": "Interface" },
{ "name": "gridVirtualizationColumnEnabledSelector", "kind": "Variable" },
{ "name": "gridVirtualizationEnabledSelector", "kind": "Variable" },
{ "name": "gridVirtualizationRowEnabledSelector", "kind": "Variable" },
{ "name": "gridVirtualizationSelector", "kind": "Variable" },
{ "name": "GridVirtualizationState", "kind": "TypeAlias" },
{ "name": "GridVisibilityOffIcon", "kind": "Variable" },
Expand Down
1 change: 1 addition & 0 deletions scripts/x-data-grid-pro.exports.json
Original file line number Diff line number Diff line change
Expand Up @@ -575,6 +575,7 @@
{ "name": "GridVirtualizationApi", "kind": "Interface" },
{ "name": "gridVirtualizationColumnEnabledSelector", "kind": "Variable" },
{ "name": "gridVirtualizationEnabledSelector", "kind": "Variable" },
{ "name": "gridVirtualizationRowEnabledSelector", "kind": "Variable" },
{ "name": "gridVirtualizationSelector", "kind": "Variable" },
{ "name": "GridVirtualizationState", "kind": "TypeAlias" },
{ "name": "GridVisibilityOffIcon", "kind": "Variable" },
Expand Down
1 change: 1 addition & 0 deletions scripts/x-data-grid.exports.json
Original file line number Diff line number Diff line change
Expand Up @@ -524,6 +524,7 @@
{ "name": "GridVirtualizationApi", "kind": "Interface" },
{ "name": "gridVirtualizationColumnEnabledSelector", "kind": "Variable" },
{ "name": "gridVirtualizationEnabledSelector", "kind": "Variable" },
{ "name": "gridVirtualizationRowEnabledSelector", "kind": "Variable" },
{ "name": "gridVirtualizationSelector", "kind": "Variable" },
{ "name": "GridVirtualizationState", "kind": "TypeAlias" },
{ "name": "GridVisibilityOffIcon", "kind": "Variable" },
Expand Down
Loading