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] Improve scrollbar deadzone with overlay scrollbars #15961

Merged
merged 7 commits into from
Jan 15, 2025

Conversation

lauri865
Copy link
Contributor

@lauri865 lauri865 commented Dec 20, 2024

On OS X and potentially other platforms that hide scrollbars when not scrolling, virtual scrollbars are effectively overlapping. When scrollbar is scrolled to the end, there's a dead zone where you cannot drag the scroll handle at all until you manually scroll it out of the dead zone, and scrolling works in only in one direction (whichever comes last in the DOM, currently horizontal scrollbar).

This PR adds two improvements:

  1. Vertical scrollbar comes after horizontal scrollbar, making it render on top. It's more likely that there's more vertical content than horizontal content, hence the handle is smaller and the issue is more pronounced (it's more likely that the whole scroll handle is hidden in the dead zone).
  2. On hovering scroll track, the zIndex is incremented by +1, making the deadzone go away. You still cannot scroll from the deadzone itself in one of the directions (previously vertically, now horizontally), but the it's at least an easy win for now, and if you start hovering/scrolling outside of the dead zone, it works in both directions now.

@lauri865
Copy link
Contributor Author

lauri865 commented Dec 20, 2024

The issue:

deadzone.mp4

@mui-bot
Copy link

mui-bot commented Dec 20, 2024

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

Generated by 🚫 dangerJS against e6bf89a

@zannager zannager added the component: data grid This is the name of the generic UI component, not the React module! label Dec 30, 2024
@romgrk romgrk added the needs cherry-pick The PR should be cherry-picked to master after merge label Jan 3, 2025
@romgrk
Copy link
Contributor

romgrk commented Jan 8, 2025

Fyi, about the case that makes the CI fail, sometimes those act() warnings come from a different case in the same file than the one marked as failing, you can try to pinpoint which one by using it.only(...). act() is a react/RTL hack so it's finicky to deal with, not sure why this diff triggers that warning.

image

@lauri865
Copy link
Contributor Author

lauri865 commented Jan 9, 2025

All the tests were passing when I first opened this PR. It's very unlikely that such a simple change would break any tests to begin with.

Since then, merged master to the PR twice:

  1. First time Argos test failed (scrollbar size significantly different)
  2. Second time Argos test passed (no change to this PR), but the above failed (which might not pop up when merging again).

Seems unrelated to the nature of the change, so I don't feel like debugging the test system to be honest.

@lauri865
Copy link
Contributor Author

lauri865 commented Jan 9, 2025

Now there are 576 Argos changes 🫠

FWIW, all the unit tests are (still) passing locally, so I'm not sure how I could even debug the CI failure to begin with.

@romgrk
Copy link
Contributor

romgrk commented Jan 9, 2025

The argos issue is present in all open PR:s, should be dealt with shortly. And if the other failure is a flaky test we'll ignore it and deal with it independently.

@lauri865
Copy link
Contributor Author

Ready to merge. The argos screenshot is flaky – it's also jumpy in live, so a small timing difference can generate a different screenshot it seems: https://mui.com/x/react-data-grid/server-side-data/row-grouping/

@romgrk romgrk merged commit 6dc3f84 into mui:master Jan 15, 2025
17 of 18 checks passed
@cherniavskii cherniavskii changed the title [data grid] improve scrollbar deadzone with overlay scrollbars [DataGrid] Improve scrollbar deadzone with overlay scrollbars Jan 16, 2025
cherniavskii pushed a commit to cherniavskii/mui-x that referenced this pull request Jan 16, 2025
@cherniavskii cherniavskii added the bug 🐛 Something doesn't work label Jan 16, 2025
@cherniavskii
Copy link
Member

@romgrk FYI, the target version label is required for automatic cherry-pick to work. In this case, it's v7.x.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug 🐛 Something doesn't work component: data grid This is the name of the generic UI component, not the React module! needs cherry-pick The PR should be cherry-picked to master after merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants