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

[data grid] Fix renderContext calculation with scroll bounce / over-scroll #16297

Merged
merged 16 commits into from
Jan 28, 2025

Conversation

lauri865
Copy link
Contributor

@lauri865 lauri865 commented Jan 22, 2025

Fixes an age old bug that has made the Datagrid look really broken on iOS devices.

Turns out there's a faulty logic that prevents renderContext from being recalculated when overscrolling – it skips calculating a new render context if the scroller is out of bounds, which results in blank rows intermittently. Fixed it by clamping within the known scroll area instead to avoid any unnecessary recalculations when bouncing, while still updating the render context when needed.

Steps to reproduce:

  1. On iOS (not sure about Android) scroll down on any datagrid with sufficient rows (e.g. https://mui.com/x/), but also pull your finger to the right to pull the scroll area away from edges
  2. Notice how new rows don't get rendered, and you end up with blank screen for a while

It's very annoying, since there's no directional scroll locks, so natural motion can easily trigger even the tiniest of overscrolls and this bug looks like this (no rows displayed while actively scrolling):
https://github.com/user-attachments/assets/d431e8f3-60c0-4914-b098-ed901195e38c

I also added in a small performance optimisation to reduce the number of calls to scrollTop/scrollLeft properties (centralised to triggerUpdateRenderContext), which can cause re-measurements when called in different times (depends on browser implementation).

Fixes #14930 (was never solved)

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

@mui-bot
Copy link

mui-bot commented Jan 22, 2025

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

Generated by 🚫 dangerJS against 068cf30

@lauri865
Copy link
Contributor Author

lauri865 commented Jan 22, 2025

There's a pre-existing issue with the below grid / test – it should not have a vertical scrollbar. The issue is that the grid footer grows and forces the main container to shrink. Main container should, however follow its children's height.

https://app.argos-ci.com/mui/mui-x/builds/29689/132441281

Added flexShrink: 0 to the main container, which fixes the issue. But not sure if something else will break now?

Edit: reverted, as it broke some tests. But the issue is there still if anyone wants to take a look, will leave it out of this PR. Alternatively, it could be beneficial for GridFooter to have a fixed height, as it can cause unnecessary resizings otherwise on mount.

@zannager zannager added the component: data grid This is the name of the generic UI component, not the React module! label Jan 23, 2025
@lauri865 lauri865 force-pushed the fix-overscroll-bounce branch from 11b8382 to 4ecb631 Compare January 27, 2025 10:56
@oliviertassinari oliviertassinari added design This is about UI or UX design, please involve a designer bug 🐛 Something doesn't work browser: Safari labels Jan 27, 2025
@oliviertassinari
Copy link
Member

oliviertassinari commented Jan 27, 2025

This seems to do it:

Before

Screen.Recording.2025-01-27.at.15.53.17.mov

After:

Screen.Recording.2025-01-27.at.15.52.18.mov

On how we write tests to prevent future regressions, I'm not sure.

@oliviertassinari oliviertassinari changed the title [data grid] fix renderContext calculation with scroll bounce / over-scroll [data grid] Fix renderContext calculation with scroll bounce / over-scroll Jan 27, 2025
Copy link
Member

@KenanYusuf KenanYusuf left a comment

Choose a reason for hiding this comment

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

Thanks a lot for the PR @lauri865

lauri865 and others added 4 commits January 27, 2025 21:01
…Scrollbar.tsx

Co-authored-by: Kenan Yusuf <kenan.m.yusuf@gmail.com>
Signed-off-by: Lauri <lauri.lehtmaa@gmail.com>
…Scrollbar.tsx

Co-authored-by: Kenan Yusuf <kenan.m.yusuf@gmail.com>
Signed-off-by: Lauri <lauri.lehtmaa@gmail.com>
@KenanYusuf KenanYusuf added needs cherry-pick The PR should be cherry-picked to master after merge v7.x labels Jan 28, 2025
…VirtualScroller.tsx

Signed-off-by: Kenan Yusuf <kenan.m.yusuf@gmail.com>
@KenanYusuf
Copy link
Member

LTR

ltr.mov

RTL

rtl.mov

Pinned columns

pinned-columns.mov

@KenanYusuf KenanYusuf merged commit 4f6dc88 into mui:master Jan 28, 2025
18 checks passed
Copy link

Cherry-pick PRs will be created targeting branches: v7.x

github-actions bot pushed a commit that referenced this pull request Jan 28, 2025
…-scroll (#16297)

Signed-off-by: Lauri <lauri.lehtmaa@gmail.com>
Signed-off-by: Kenan Yusuf <kenan.m.yusuf@gmail.com>
Co-authored-by: Kenan Yusuf <kenan.m.yusuf@gmail.com>
A-s-h-o-k pushed a commit to A-s-h-o-k/mui-x that referenced this pull request Feb 4, 2025
…-scroll (mui#16297)

Signed-off-by: Lauri <lauri.lehtmaa@gmail.com>
Signed-off-by: Kenan Yusuf <kenan.m.yusuf@gmail.com>
Co-authored-by: Kenan Yusuf <kenan.m.yusuf@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
browser: Safari bug 🐛 Something doesn't work 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 needs cherry-pick The PR should be cherry-picked to master after merge v7.x
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[data grid] Stop rendering when overscrolling on iOS
5 participants