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 resizing right pinned column #12979

Closed
wants to merge 7 commits into from
Closed

[DataGrid] Fix resizing right pinned column #12979

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
73f4201
fix: resize logic
romgrk May 2, 2024
56fc930
lint
romgrk May 2, 2024
60d4d13
fix: edge cases
romgrk May 30, 2024
306a73d
Merge branch 'master' into fix-resize-right-pinned-column
romgrk May 30, 2024
a6d2a3b
lint
romgrk May 30, 2024
0f4363c
lint
romgrk May 30, 2024
3669e05
Merge branch 'master' into fix-resize-right-pinned-column
romgrk Jul 18, 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
8 changes: 7 additions & 1 deletion packages/x-data-grid/src/components/GridRow.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -524,7 +524,13 @@ const GridRow = React.forwardRef<HTMLDivElement, GridRowProps>(function GridRow(
/>
{cells}
{emptyCellWidth > 0 && <EmptyCell width={emptyCellWidth} />}
{rightCells.length > 0 && <div role="presentation" className={gridClasses.filler} />}
{rightCells.length > 0 && (
<div
role="presentation"
className={clsx(gridClasses.filler, gridClasses['filler--borderTop'])}
Copy link
Member

@MBilalShafi MBilalShafi May 4, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Is this space being filled by the filler:

width-minor.mp4

Probably need to adjust it during the resize.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Could you give me more details about how to reproduce that? It's working fine on my end.

Copy link
Member

Choose a reason for hiding this comment

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

I can observe this small glitch while resizing the right pinned column on every browser (Chrome, Firefox, Edge, Safari) in Mac OS.
The trick to reproduce is to first scroll to the right-most pixel of the scrollable area (for non-pinned columns) and then try to resize the right-pinned column without stopping the mouse down. It gets fixed on mouse up though. Let me know if you can reproduce it in Linux or guess the possible reason. I'll take it otherwise.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If you can take a look that would be very nice, I can't reproduce on linux:

Screencast.from.2024-05-15.23-54-04.webm

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 have finished handling this case. It feels like the column resize code is becoming more ugly every time I touch it.

Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure if it's the same issue, but I can still reproduce some weird behavior in https://codesandbox.io/p/sandbox/flamboyant-wing-wdkzzl (it uses the latest package builds from this branch):

Screen.Recording.2024-05-31.at.14.41.10.mov

Copy link
Member

@MBilalShafi MBilalShafi May 31, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Yes, it still causes issues.

A dumb way to solve it during my debugging was to call apiRef.current.setColumnWidth in updateWidth behind a debounce (which actually updates the state and causes recomputation)

This partially fixed the issue with visible flickering. I am not sure if it could be done without hurting the UX (performance-wise) but one idea could be to involve some state updation for the specific column the user is resizing for proper re-computation to happen (still partial enough not to trigger the whole tree recomputation).

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 been thinking about doing state updates as well. The complexity of the resizing code is getting very high with this PR, and I feel like I'm playing whack-a-mole with edge cases.

Copy link
Member

Choose a reason for hiding this comment

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

I cannot reproduce the issue I posted in this thread with disableVirtualization={true}, so it seems like the virtualization still kicks in, even after #12979 (comment)

/>
)}

{rightCells}
{scrollbarWidth !== 0 && <ScrollbarFiller pinnedRight={pinnedColumns.right.length > 0} />}
</div>
Expand Down
2 changes: 1 addition & 1 deletion packages/x-data-grid/src/components/containers/GridRootStyles.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -649,7 +649,7 @@ export const GridRootStyles = styled('div', {
flex: 1,
},
[`& .${c['filler--borderTop']}`]: {
borderTop: '1px solid var(--DataGrid-rowBorderColor)',
borderTop: '1px solid var(--rowBorderColor)',
},
};

Expand Down
26 changes: 22 additions & 4 deletions packages/x-data-grid/src/hooks/features/columnResize/useGridColumnResize.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -273,6 +273,7 @@ function createResizeRefs() {
initialColWidth: 0,
initialTotalWidth: 0,
previousMouseClickEvent: undefined as undefined | MouseEvent,
scrollerContentElement: undefined as undefined | HTMLDivElement,
columnHeaderElement: undefined as undefined | HTMLDivElement,
headerFilterElement: undefined as undefined | HTMLDivElement,
groupHeaderElements: [] as Element[],
Expand Down Expand Up @@ -315,13 +316,20 @@ export const useGridColumnResize = (
const stopResizeEventTimeout = useTimeout();
const touchId = React.useRef<number>();

const updateWidth = (newWidth: number) => {
const updateWidth = (requestWidth: number) => {
const newWidth = clamp(requestWidth, refs.colDef!.minWidth ?? 50, refs.colDef!.maxWidth ?? Infinity);

logger.debug(`Updating width to ${newWidth} for col ${refs.colDef!.field}`);

const dimensions = apiRef.current.getRootDimensions();

const prevWidth = refs.columnHeaderElement!.offsetWidth;
const widthDiff = newWidth - prevWidth;
const columnWidthDiff = newWidth - refs.initialColWidth;
const newTotalWidth = refs.initialTotalWidth + columnWidthDiff;
const newTotalWidth = Math.max(
refs.initialTotalWidth + columnWidthDiff,
dimensions.viewportOuterSize.width,
);

apiRef.current.rootElementRef?.current?.style.setProperty(
'--DataGrid-rowWidth',
Expand All @@ -332,6 +340,9 @@ export const useGridColumnResize = (
refs.colDef!.width = newWidth;
refs.colDef!.flex = 0;

refs.scrollerContentElement!.style.width =
`${parseInt(refs.scrollerContentElement!.style.width) + widthDiff}px`;

refs.columnHeaderElement!.style.width = `${newWidth}px`;
refs.columnHeaderElement!.style.minWidth = `${newWidth}px`;
refs.columnHeaderElement!.style.maxWidth = `${newWidth}px`;
Expand Down Expand Up @@ -360,6 +371,7 @@ export const useGridColumnResize = (
div.style.maxWidth = finalWidth;
});

refs.cellElements = findGridCellElementsFromCol(refs.columnHeaderElement!, apiRef.current);
refs.cellElements!.forEach((element) => {
const div = element as HTMLDivElement;
let finalWidth: `${number}px`;
Expand Down Expand Up @@ -408,6 +420,8 @@ export const useGridColumnResize = (
// eslint-disable-next-line @typescript-eslint/no-use-before-define
stopListening();

apiRef.current.setVirtualScrollerLock(false);

// Prevent double-clicks from being interpreted as two separate clicks
if (refs.previousMouseClickEvent) {
const prevEvent = refs.previousMouseClickEvent;
Expand Down Expand Up @@ -462,6 +476,8 @@ export const useGridColumnResize = (

refs.colDef = colDef as GridStateColDef;

refs.scrollerContentElement = root.querySelector(`.${gridClasses.virtualScrollerContent}`) as HTMLDivElement;

refs.columnHeaderElement = findHeaderElementFromField(
apiRef.current.columnHeadersContainerRef!.current!,
colDef.field,
Expand Down Expand Up @@ -533,7 +549,6 @@ export const useGridColumnResize = (
resizeDirection.current!,
);

newWidth = clamp(newWidth, refs.colDef!.minWidth!, refs.colDef!.maxWidth!);
updateWidth(newWidth);

const params: GridColumnResizeParams = {
Expand Down Expand Up @@ -573,7 +588,6 @@ export const useGridColumnResize = (
resizeDirection.current!,
);

newWidth = clamp(newWidth, refs.colDef!.minWidth!, refs.colDef!.maxWidth!);
updateWidth(newWidth);

const params: GridColumnResizeParams = {
Expand Down Expand Up @@ -619,6 +633,8 @@ export const useGridColumnResize = (
const doc = ownerDocument(event.currentTarget as HTMLElement);
doc.addEventListener('touchmove', handleTouchMove);
doc.addEventListener('touchend', handleTouchEnd);

apiRef.current.setVirtualScrollerLock(true);
});

const stopListening = React.useCallback(() => {
Expand Down Expand Up @@ -688,6 +704,8 @@ export const useGridColumnResize = (
// Prevent the click event if we have resized the column.
// Fixes /~https://github.com/mui/mui-x/issues/4777
doc.addEventListener('click', preventClick, true);

apiRef.current.setVirtualScrollerLock(true);
});

const handleColumnSeparatorDoubleClick: GridEventListener<'columnSeparatorDoubleClick'> =
Expand Down
12 changes: 12 additions & 0 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 @@ -100,6 +100,7 @@ export const useGridVirtualScroller = () => {
const pinnedColumns = useGridSelector(apiRef, gridVisiblePinnedColumnDefinitionsSelector);
const hasBottomPinnedRows = pinnedRows.bottom.length > 0;
const [panels, setPanels] = React.useState(EMPTY_DETAIL_PANELS);
const [lock, setLock] = React.useState(false);

const theme = useTheme();
const cellFocus = useGridSelector(apiRef, gridFocusCellSelector);
Expand Down Expand Up @@ -274,6 +275,10 @@ export const useGridVirtualScroller = () => {
};

const handleScroll = useEventCallback((event: React.UIEvent) => {
if (lock) {
return;
}

const { scrollTop, scrollLeft } = event.currentTarget;

// On iOS and macOS, negative offsets are possible when swiping past the start
Expand Down Expand Up @@ -301,10 +306,16 @@ export const useGridVirtualScroller = () => {
});

const handleWheel = useEventCallback((event: React.WheelEvent) => {
if (lock) {
return;
}
apiRef.current.publishEvent('virtualScrollerWheel', {}, event);
});

const handleTouchMove = useEventCallback((event: React.TouchEvent) => {
if (lock) {
return;
}
apiRef.current.publishEvent('virtualScrollerTouchMove', {}, event);
});

Expand Down Expand Up @@ -566,6 +577,7 @@ export const useGridVirtualScroller = () => {

apiRef.current.register('private', {
updateRenderContext: forceUpdateRenderContext,
setVirtualScrollerLock: setLock,
});

useGridApiEventHandler(apiRef, 'columnsChange', forceUpdateRenderContext);
Expand Down
4 changes: 4 additions & 0 deletions packages/x-data-grid/src/models/api/gridVirtualizationApi.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,4 +17,8 @@ export interface GridVirtualizationPrivateApi {
* @returns {GridRenderContext} The `GridRenderContext`.
*/
updateRenderContext?: () => void;
/**
* Lock/unlock the virtual scroller events.
*/
setVirtualScrollerLock: (locked: boolean) => void;
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 virtualization logic was running during resize in one edge case, it was too complicated to solve it otherwise so I've just prevented it from running. This is terrible but I think it's the best solution.

}