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

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

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

@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 c96cdfe

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

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

4 participants