Skip to content

[DataGrid] Fix column menu scroll close #18065

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
arminmeh merged 4 commits into from
Jun 16, 2025
Merged

[DataGrid] Fix column menu scroll close #18065

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
cac7395
keep column menu open when scrolling within the menu
alisasanib May 29, 2025
f94fc3a
add tests for column menu functionality
alisasanib May 29, 2025
ca9da73
refactor: move handleMenuScrollCapture outside GridColumnMenuContainer
alisasanib Jun 12, 2025
4562756
Merge branch 'master' into fix-column-menu-scroll-close
alisasanib Jun 12, 2025
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
10 changes: 10 additions & 0 deletions packages/x-data-grid/src/components/menu/columnMenu/GridColumnMenuContainer.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,14 @@ const StyledMenuList = styled(NotRendered<GridSlotProps['baseMenuList']>)(() =>
minWidth: 248,
}));

function handleMenuScrollCapture(event: React.WheelEvent | React.TouchEvent) {
if (!event.currentTarget.contains(event.target as Node)) {
return;
}

event.stopPropagation();
}

const GridColumnMenuContainer = forwardRef<HTMLUListElement, GridColumnMenuContainerProps>(
function GridColumnMenuContainer(props, ref) {
const { hideMenu, colDef, id, labelledby, className, children, open, ...other } = props;
Expand All @@ -39,6 +47,8 @@ const GridColumnMenuContainer = forwardRef<HTMLUListElement, GridColumnMenuConta
className={clsx(gridClasses.menuList, className)}
aria-labelledby={labelledby}
onKeyDown={handleListKeyDown}
onWheel={handleMenuScrollCapture}
onTouchMove={handleMenuScrollCapture}
autoFocus={open}
{...other}
ref={ref}
Expand Down
39 changes: 38 additions & 1 deletion packages/x-data-grid/src/tests/columnHeaders.DataGrid.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, screen, waitFor, within } from '@mui/internal-test-utils';
import { createRenderer, fireEvent, screen, waitFor, within } from '@mui/internal-test-utils';
import { expect } from 'chai';
import { DataGrid } from '@mui/x-data-grid';
import { getColumnHeaderCell, getColumnHeadersTextContent } from 'test/utils/helperFn';
Expand Down Expand Up @@ -132,6 +132,43 @@ describe('<DataGrid /> - Column headers', () => {
expect(screen.queryByRole('menu')).to.equal(null);
});
});

it('should prevent wheel scroll event from closing the menu when scrolling within the menu', async () => {
const { user } = render(
<div style={{ width: 300, height: 500 }}>
<DataGrid {...baselineProps} columns={[{ field: 'brand' }]} />
</div>,
);

await user.click(within(getColumnHeaderCell(0)).getByLabelText('brand column menu'));
const menu = screen.getByRole('menu');

fireEvent.wheel(menu);

await waitFor(() => {
expect(screen.queryByRole('menu')).not.to.equal(null);
});
});

it('should close the column menu when the grid is scrolled', async () => {
Copy link
Copy Markdown
Member

@kenanyusuf kenanyusuf Jun 9, 2025

Choose a reason for hiding this comment

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

IMO, it shouldn't close the menu at all on scroll, seems unconventional to me.

@mui/xgrid keen to get others thoughts on this behavior.

Copy link
Copy Markdown
Contributor

@arminmeh arminmeh Jun 9, 2025

Choose a reason for hiding this comment

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

I have tried to get the reason why it was added in the first place
closest I got is this
#4139 (comment)

The column menu should be closed if the user scrolls the grid, that's the rule.

still don't have the answer why exactly

AG Grid does not close the menu on scroll
https://www.ag-grid.com/react-data-grid/filter-advanced/#enabling-advanced-filter

Copy link
Copy Markdown
Member

@cherniavskii cherniavskii Jun 9, 2025

Choose a reason for hiding this comment

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

I think it's not a great UX if we don't close the menu on horizontal scroll – it first leaves the data grid viewport, and then disappears anyway due to virtualization.
I don't see a reason for closing the menu on vertical scroll, though. Should we keep closing the menu for horizontal scroll only?

Copy link
Copy Markdown
Contributor

@arminmeh arminmeh Jun 10, 2025

Choose a reason for hiding this comment

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

On another look, I think that we can leave it as is (only including changes made in this PR)
If you use premium grid with all major features enabled, the menu can get pretty tall and in most cases it will cover all the content from the related column. For me, in that situation, I would prefer to have the current behavior

Screenshot 2025-06-10 at 08 34 44

const { user } = render(
<div style={{ height: 300, width: 300 }}>
<DataGrid
rows={Array.from({ length: 50 }, (_, id) => ({ id, name: id }))}
columns={[{ field: 'brand' }]}
/>
</div>,
);

await user.click(within(getColumnHeaderCell(0)).getByLabelText('brand column menu'));

const scroller = document.querySelector('.MuiDataGrid-virtualScroller')!;
fireEvent.wheel(scroller, { deltaY: 120 });

await waitFor(() => {
expect(screen.queryByRole('menu')).to.equal(null);
});
});
});

it("should use 'headerName' as the aria-label for the menu icon button", async () => {
Expand Down