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] Sticky headers #10059

Merged
merged 212 commits into from
Jan 16, 2024
Merged

[DataGrid] Sticky headers #10059

merged 212 commits into from
Jan 16, 2024

Conversation

romgrk
Copy link
Contributor

@romgrk romgrk commented Aug 16, 2023

Closes #4506
Closes #9646
Closes #5416
Closes #10616
Closes #10339
Closes #11214
Closes #11215
Closes #9502
Closes #11691

Implement position: sticky grid headers. It's based on the work in #9589.

Before: https://deploy-preview-11650--material-ui-x.netlify.app/x/react-data-grid/#commercial-versions
After: https://deploy-preview-10059--material-ui-x.netlify.app/x/react-data-grid/#commercial-versions

Before

header tearing, header not responding to scroll intent, white screen areas

Screen.Recording.2023-12-11.at.16.34.30.mov

After

no header tearing, header responding to scroll intent, trade no white screen areas for scrollbar tearing

Screen.Recording.2023-12-11.at.16.39.11.mov

Stuff that needs to change:

  • DOM structure
  • Fake scrollbars
  • Header scrolling logic

Remaining tasks:

  • Remove transparency in colors (was set for development purposes)
  • Styles for pinned+hover, pinned+selected and pinned+hover+selected
  • The skipped test with a XXX: note
  • Discuss & agree on how to expose colors publicly (pinned background, header background)
    => We use the colors from the theme.
  • Do a performance check again (memoization is easy to disrupt)
  • Resizing is not working properly

@romgrk romgrk added the component: data grid This is the name of the generic UI component, not the React module! label Aug 16, 2023
@oliviertassinari
Copy link
Member

oliviertassinari commented Aug 17, 2023

Scrolling is so smooth 🧈

The headers now need a background-color because they float in front of the content. Not sure how to resolve this in a generic way. This change would probably require a change in our users' codebase.

Same as for pinned column: #5979. It's probably OK. At least, as a developer, I don't mind the pain to customize this, in exchange of the smoother UX.

@oliviertassinari oliviertassinari added design: ux enhancement This is not a bug, nor a new feature labels Aug 17, 2023
cherniavskii

This comment was marked as outdated.

@cherniavskii

This comment was marked as outdated.

@oliviertassinari oliviertassinari added design This is about UI or UX design, please involve a designer and removed design: ux labels Aug 18, 2023
@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Sep 22, 2023
@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Oct 3, 2023
@mui-bot

This comment was marked as outdated.

@romgrk romgrk mentioned this pull request Oct 5, 2023
45 tasks
@Janpot
Copy link
Member

Janpot commented Jan 16, 2024

This is an awesome job. I've spent so much time in a rabbit hole fixing similar issues for my own grid back in the day. I can only imagine the effort on a full featured implementation like the X grid.

@oliviertassinari
Copy link
Member

oliviertassinari commented Jan 22, 2024

Isn't this much better?

Before: https://deploy-preview-11650--material-ui-x.netlify.app/x/react-data-grid/#commercial-versions

Screen.Recording.2024-01-22.at.16.15.45.mov

After: https://deploy-preview-10059--material-ui-x.netlify.app/x/react-data-grid/#commercial-versions

Screen.Recording.2024-01-22.at.16.14.18.mov

@lauri865
Copy link
Contributor

lauri865 commented Jan 22, 2024

@oliviertassinari, try disabling overscroll-behavior, and will look and feel even more performant on iOS (as I pointed out here: #11230 (comment)). You wont have all the rows disappearing for a brief moment when you accidentally overscroll on the X-axis while actually just trying to scroll down ;)

E.g. this won't happen:
Screenshot 2024-01-22 at 16 22 07

@oliviertassinari
Copy link
Member

oliviertassinari commented Jan 22, 2024

@romgrk I found a regression, this PR broke this demo: https://next.mui.com/x/react-data-grid/row-updates/#lazy-loading


You wont have all the rows disappearing for a brief moment when you accidentally overscroll on the X-axis while actually just trying to scroll down ;)

@lauri865 I would expect #11230 to solve this. It also sounds like a performance issue, I don't get less white screen on AG Grid.

For overscroll-behavior, I wouldn't touch this. It both sounds the expected default behavior for users on iOS and a requirement to be able to propagate scroll up in the parent chain.

@romgrk
Copy link
Contributor Author

romgrk commented Jan 22, 2024

Opened the issue linked above.

@oliviertassinari
Copy link
Member

oliviertassinari commented Jan 22, 2024

@romgrk In terms of scrolling performance, I have noticed a few things on this PR.

First, the baseline, ranked by how much time it takes to render the next rows, the difference is clear on my laptop (in production and also more noticeable in dev mode):

  • 🥇 a. AG Grid: no white screens
  • 🥈 b. Before: a little bit of white screen, about 8% of the viewport
  • 🥉 c. After: more white screen, about 20% of the viewport

Now, when we look at the actual profile for 1,000 ms of scroll, we see a much different story:

  • AG Grid: no white screens, overscan of 10 rows
  • Before: a little bit of white screen, about 8% of the viewport, overscan of 3 rows
  • After: more white screen, about 20% of the viewport, overscan of 3 rows

We were 1st Before, and clearly, I think the most important metric to judge performance is idle time. However, because we are not using a high enough overscan, end-users would feel a white area. It seems clear to me that 3 is not enough https://mui.com/x/api/data-grid/data-grid/#DataGrid-prop-rowBuffer. Increasing it would be a perfect quick win.

Another opportunity I see is how we re-render emotion components at each render, e.g. ScrollbarFiller. How about we memoize or move the style outside of the re-render flow during scroll?

Now, the problem. Vertical scrolling is a more frequent use case than Horizontal scrolling, so should be prioritized in terms of UX if we can only have one. So I think for this PR to benefit end-users, we need to get to the bottom of this regression. I suspect we can fix it. If we can't, and it's because of a core tradeoff of the sticky position, then I think it would be better to stay on the previous side of the tradeoff (not using it). We are no longer ahead of AG Grid, more like a tie now (once we increase the overscan and save Emotion rendering cost).


So, I drilled deeper, there is a x2 increase in "Rendering" time in After compared to Before. This seems to come from two things:

As I can see it, it's because of this style:

Screen.Recording.2024-01-22.at.17.16.13.mov

Once this style removed, the computation bottleneck seems to then move to recalculateStyles, this is very odd to me:

More in context:

I don't know why. I ran out of time I can allocate to this 😄

@romgrk
Copy link
Contributor Author

romgrk commented Jan 22, 2024

Yes I've noticed the performance changes, I merged anyway because I wanted to get the breaking changes in. I kinda expected the layout to be more expensive for the browser due to that transform style as it's on each cell now instead of on a wrapping container, but I think it will be fairly easy to add a wrapping container for the cells and apply the style once. That in conjunction with the CSS contain: strict style should make it much more easy for the browser to layout stuff. The CSS containment styles are fairly easy to use for us because we know our dimensions ahead of time with single pixel precision thanks to the changes in the dimensions logic. We compute dimensions once and those dimensions usually don't change much, even during scrolling.

I also initially used CSS variables a lot in this PR, often in dynamic ways (e.g. calc(10px + var(--xxxx)), but ended removing them in a few places and might end up removing a lot more of them, because they also increase the layout time.

Regarding emotion, that library is absolutely atrocious for runtime performance and the sooner we can get rid of it the happier I will be. I have been trying to find a way to get emotion to do something like this:

const className = css` color: red `

instead of having to use their styled-components, but I don't know the library and our system style wrappers much so I also wasn't able to complete this during this PR. Discussing how we do styling internally on the datagrid is on my roadmap.

Lastly regarding the row overscan, I've already opened #11344 to discuss that aspect, read there for more details.

@oliviertassinari
Copy link
Member

oliviertassinari commented Jan 22, 2024

@romgrk Alright, let's continue to explore this then. Overall, it's not yet clear that we should sell it as a performance improvement in the v7 release blog post, but it feels like we are close 😁

I have been trying to find a way to get emotion to do something like this:

A nested CSS selector from the parent does something close, as long as that parent doesn't rerender. We moved many styles up to the grid root in the past because of this.

CSS variables

I don't know if they increase layout time, I wouldn't expect them to.
However, updating a CSS variable 120 times per second is slow as the browser needs to make the value propagate down all the children, not just the element itself (unless maybe if we use a custom CSS property with inheritance disabled).

@romgrk
Copy link
Contributor Author

romgrk commented Jan 22, 2024

A nested CSS selector from the parent does something close, as long as that parent doesn't rerender. We moved many styles up to the grid root in the past because of this.

That's terrible. We don't get the CSS-in-JS advantage of colocation of styles & logic, and we don't get the CSS advantage of native (clean) CSS syntax. I've been observing GridRootStyles, I also don't like that the massive styles object is recomputed, and it's recomputed quite often during normal usage of the grid despite being near the root :|

I've been watching the zero-runtime solution in the hopes that it solves all our style issues, in the meantime if there's any solution that would help with injecting CSS simply & directly, I'd definitely adopt that.

@cherniavskii
Copy link
Member

Extracted vertical scrolling performance to a separate issue: #11866

@romgrk
Copy link
Contributor Author

romgrk commented Feb 5, 2024

I've opened #11924 for the performance issues.

@romgrk
Copy link
Contributor Author

romgrk commented Feb 10, 2024

Last fix for the issues above opened in #12019. CSS variables were the culprit.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change component: data grid This is the name of the generic UI component, not the React module! design This is about UI or UX design, please involve a designer enhancement This is not a bug, nor a new feature performance v7.x
Projects
None yet
8 participants