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

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

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).

Related issue #14930 (was never solved)

@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 499d876

@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
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!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants