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

Jump to bottom

[DataGrid] Keep focused cell in the DOM #5911

Closed
wants to merge 1 commit into from
Closed

[DataGrid] Keep focused cell in the DOM #5911

wants to merge 1 commit into from

Conversation

m4theushw
Copy link
Member

@m4theushw m4theushw commented Aug 26, 2022 •
edited
Loading

Preview: https://deploy-preview-5911--material-ui-x.netlify.app/x/react-data-grid/#commercial-version

Fixes #5750

The idea to fix this issue was to check if the row belonging to the focused cell is visible and, if not, render the entire row only to keep the same focused element. This prevent the focused element being changed to document.body that happens if the document.activeElement is removed from the DOM.

@m4theushw m4theushw added the component: data grid This is the name of the generic UI component, not the React module! label Aug 26, 2022
@mui-bot
Copy link

mui-bot commented Aug 26, 2022 •
edited
Loading

These are the results for the performance tests:

Test case Unit Min Max Median Mean σ
Filter 100k rows ms 537.9 1,084.9 540 745.24 214.762
Sort 100k rows ms 546.2 1,125 546.2 858.62 215.097
Select 100k rows ms 203.4 269.1 225.4 227.28 22.494
Deselect 100k rows ms 135.7 266.4 198.9 199.52 41.357

Generated by 🚫 dangerJS against cf78d03

@m4theushw m4theushw mentioned this pull request Aug 30, 2022
Merged
@m4theushw m4theushw force-pushed the keep-focused-cell-in-dom branch from 8184c8c to 07fa30b Compare August 30, 2022 22:50
@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Sep 1, 2022
@github-actions
Copy link

github-actions bot commented Sep 1, 2022

This pull request has conflicts, please resolve those before we can evaluate the pull request.

@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Sep 6, 2022
@DanailH DanailH changed the base branch from master to next September 12, 2022 13:03
@m4theushw m4theushw force-pushed the keep-focused-cell-in-dom branch from ed1b868 to cf78d03 Compare September 14, 2022 22:22
@m4theushw m4theushw marked this pull request as ready for review September 14, 2022 23:05
@DanailH
Copy link
Member

DanailH commented Sep 28, 2022

Since we are doing this should we also consider this ticket #4282?

cherniavskii
cherniavskii reviewed Sep 28, 2022
View reviewed changes
Copy link
Member

@cherniavskii cherniavskii left a comment

Choose a reason for hiding this comment

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

Great solution!

packages/grid/x-data-grid/src/tests/keyboard.DataGrid.test.tsx
@@ -536,9 +536,9 @@ describe('<DataGrid /> - Keyboard', () => {
const input = screen.getByTestId('custom-input');
input.focus();
fireEvent.keyDown(input, { key: 'a' });
expect(renderCell.callCount).to.equal(4);
expect(renderCell.callCount).to.equal(6);
Copy link
Member

Choose a reason for hiding this comment

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

Why was this test impacted by the changes? It only renders one row, so it should not be impacted by virtualization

packages/grid/x-data-grid-pro/src/components/DataGridProVirtualScroller.tsx
@@ -425,6 +427,9 @@ const DataGridProVirtualScroller = React.forwardRef<
<GridVirtualScrollerRenderZone {...getRenderZoneProps()}>
{mainRows}
</GridVirtualScrollerRenderZone>
<GridVirtualScrollerRenderZone {...getFocusRenderZoneProps()}>
{getFocusedRow()}
Copy link
Member

Choose a reason for hiding this comment

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

I've noticed scroll jump when checkbox cell is focused.
Doesn't happen if regular cell is focused though.

Screen.Recording.2022-09-28.at.13.39.42.mov

packages/grid/x-data-grid-pro/src/components/DataGridProVirtualScroller.tsx
@@ -425,6 +427,9 @@ const DataGridProVirtualScroller = React.forwardRef<
<GridVirtualScrollerRenderZone {...getRenderZoneProps()}>
{mainRows}
</GridVirtualScrollerRenderZone>
<GridVirtualScrollerRenderZone {...getFocusRenderZoneProps()}>
{getFocusedRow()}
Copy link
Member

Choose a reason for hiding this comment

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

Moving focus to checkbox cell acts weird when focus is out of view:

Screen.Recording.2022-09-28.at.13.40.53.mov

packages/grid/x-data-grid-pro/src/components/DataGridProVirtualScroller.tsx
@@ -425,6 +427,9 @@ const DataGridProVirtualScroller = React.forwardRef<
<GridVirtualScrollerRenderZone {...getRenderZoneProps()}>
{mainRows}
</GridVirtualScrollerRenderZone>
<GridVirtualScrollerRenderZone {...getFocusRenderZoneProps()}>
Copy link
Member

Choose a reason for hiding this comment

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

We might consider similar solution for columns virtualization, since we have the same problem with horizontal scrolling

@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Oct 7, 2022
@github-actions
Copy link

github-actions bot commented Oct 7, 2022

This pull request has conflicts, please resolve those before we can evaluate the pull request.

@oliviertassinari oliviertassinari mentioned this pull request Nov 13, 2022
Merged
1 task
@yaredtsy
Copy link
Contributor

Since we are doing this should we also consider this ticket #4282?

I have submitted PR for this issue at #6838.

@yaredtsy yaredtsy mentioned this pull request Nov 29, 2022
Closed
2 tasks
@yaredtsy
Copy link
Contributor

yaredtsy commented Jan 5, 2023

I have looked at AG-grid approach to this problem and I have tried to adapt it, it solves this issue, and #6165. i have created a PR for it at #7357

@m4theushw m4theushw closed this Jul 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cherniavskii cherniavskii cherniavskii left review comments

@DanailH DanailH DanailH approved these changes

Assignees
No one assigned
Labels
component: data grid This is the name of the generic UI component, not the React module! PR: out-of-date The pull request has merge conflicts and can't be merged
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[data grid] Keyboard navigation doesn't work when active cell is out of the viewport
5 participants
@m4theushw @mui-bot @DanailH @yaredtsy @cherniavskii