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 scroll to cell logic for keyboard navigating cells and drag selection with pinned columns #14550

Merged
merged 11 commits into from
Sep 23, 2024

Conversation

KenanYusuf
Copy link
Member

@KenanYusuf KenanYusuf commented Sep 9, 2024

Fixes two issues related to navigating the data grid via scroll + keyboard with pinned columns. For specific testing conditions, see the issue below.

Fixes #14545

We are using the viewportInnerSize to calculate the new scroll position, but this value does not include the width of pinned columns https://github.com/mui/mui-x/blob/master/packages/x-data-grid/src/hooks/features/dimensions/useGridDimensions.ts#L201-L204. The fix was to use viewportOuterSize instead, which does account for pinned columns.

Preview URL: https://codesandbox.io/p/sandbox/laughing-roentgen-q5l87d

Issue 1

Navigating via arrow keys moves focused items under pinned columns.

Before

1.before.mov

After

1.after.mov

Issue 2

Selecting item via drag causes the grid to scroll automatically.

Before

2.before.mov

After

2.after.mov

@KenanYusuf KenanYusuf changed the title Fix pinned scroll [DataGrid] Fix scroll to cell logic for keyboard navigating cells and drag selection with pinned columns Sep 9, 2024
@KenanYusuf KenanYusuf added component: data grid This is the name of the generic UI component, not the React module! regression A bug, but worse feature: Column pinning Related to the data grid Column pinning feature feature: Cell Selection Related to the cell selection feature labels Sep 9, 2024
@mui-bot
Copy link

mui-bot commented Sep 9, 2024

Deploy preview: https://deploy-preview-14550--material-ui-x.netlify.app/

Generated by 🚫 dangerJS against 201adf5

@KenanYusuf KenanYusuf marked this pull request as draft September 9, 2024 15:09
@KenanYusuf KenanYusuf marked this pull request as ready for review September 10, 2024 08:07
@KenanYusuf KenanYusuf requested a review from a team September 10, 2024 08:08
containerSize: number;
scrollPosition: number;
elementSize: number;
elementOffset: number;
Copy link
Member Author

@KenanYusuf KenanYusuf Sep 10, 2024

Choose a reason for hiding this comment

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

Changed the param names to be direction agnostic, since this is used for horizontal and vertical scrolling

Copy link
Contributor

@arminmeh arminmeh left a comment

Choose a reason for hiding this comment

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

I found an issue with the cell selection.

Selecting away from pinned columns works fine, but selection towards them does not trigger scroll

cell-selection-pinned-cols.mov

Reproduction code

import * as React from 'react';
import Button from '@mui/material/Button';
import { DataGridPremium } from '@mui/x-data-grid-premium';
import { useDemoData } from '@mui/x-data-grid-generator';

export default function CellSelectionGrid() {
  const { data } = useDemoData({
    dataSet: 'Commodity',
    rowLength: 10,
    maxColumns: 20,
  });

  return (
    <div style={{ width: '100%', height: 400 }}>
      <DataGridPremium
        rowSelection={false}
        cellSelection
        pinnedColumns={{"left": ['desk', 'commodity', 'traderEmail']}}
        {...data}
      />
    </div>
  );
}

@KenanYusuf
Copy link
Member Author

KenanYusuf commented Sep 11, 2024

@arminmeh I observed this too. You have to move the cursor to the edge of the outer viewport, not just passed the pinned columns.

This behaviour is consistent with v6:

Screen.Recording.2024-09-11.at.09.04.52.mov

https://stackblitz.com/edit/react-z2nfxa-d6zknf?file=Demo.tsx

@arminmeh
Copy link
Contributor

I think that the problem I have pointed out is actually another issue with the cell selection and pinned columns.
In my video you can see the selected cells together with the cells in the pinned columns.

In v6 the prop was called unstable_cellSelection
After updating your last example, I can see that the selection behavior is the same. Once you go over the pinned column, all cells in between are selected.
But in v6 they are not visible.

@KenanYusuf
Copy link
Member Author

@arminmeh I see what you mean.

There seems to be a glitch with showing selected cells underneath pinned cells too.

This is a frame from your video:

image

@KenanYusuf KenanYusuf marked this pull request as ready for review September 18, 2024 08:54
@KenanYusuf KenanYusuf requested a review from arminmeh September 18, 2024 10:11
hoverOpacity,
selectedOpacity,
selectedBackground,
backgroundColor: pinnedSelectedBackgroundColor,
Copy link
Member

@cherniavskii cherniavskii Sep 18, 2024

Choose a reason for hiding this comment

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

pinnedSelectedBackgroundColor here can be either a CSS color value or a CSS variable.
getPinnedBackgroundStyles assumes it's a CSS color value and uses the blend function, which won't work with CSS variables.
This made me realize that we missed a test for the DataGrid rendered inside of the CssVarsProvider, so I opened #14662 to add one.

We have to make sure we don't try to blend CSS variables.
This also reminds me about this PR: #12901.

Copy link
Member Author

Choose a reason for hiding this comment

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

@cherniavskii I am not sure of the best way to handle this.

color-mix would work well besides the versions of Safari that do not support it. It doesn't look like there is another way to blend colors with CSS variables otherwise.

I don't know if we can add an exception to our browser support specifically for cases where CssVarsProvider is used? 😅

The only other way I can think to achieve this effect with CSS variables is by having an inner element in pinned cells to apply the transparent hover/selected background-color to. Not sure this is viable though, considering the performance implications of having an extra element for every pinned cell. https://codepen.io/KenanYusuf/pen/WNqVXqM

Copy link

Choose a reason for hiding this comment

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

@KenanYusuf, not sure it helps, but we ran into similar problems when migrating to DataGrid v7 due to our custom styles. Some of our pinned cells had backgrounds with alpha values. Therefore, the non-pinned cells were visible behind them.

We attempted to solve this using color-mix. The result worked but was very hard to read and extend in our case.

We opted for the two-element approach you described, but instead of adding one additional element per cell, we added one sticky background element with the width of the pinned cells.

Details

grafik

We added the sticky background to the row using DataGrid slots. We had to place it between row and pinned cells using z-index, but you could probably just make it a container element of the pinned cells.

The additional sticky background always has the same background color as the row. If the row is selected using row selection, the color of the additional sticky background changes too. If only a pinned cell is selected using cell selection, only the background color of the pinned cell changes.

We didn't run into noticeable performance problems. If you consider pinned left and pinned right, it's only two more divs in each row.

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

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 20, 2024
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.

🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: data grid This is the name of the generic UI component, not the React module! feature: Cell Selection Related to the cell selection feature feature: Column pinning Related to the data grid Column pinning feature regression A bug, but worse
Projects
Status: Done
5 participants