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] Fix horizontal scroll bar in dark mode, refactor scroll size detector #1703

Merged
merged 1 commit into from
Jun 1, 2021

Conversation

dtassone
Copy link
Member

@dtassone dtassone commented May 19, 2021

@oliviertassinari
Copy link
Member

oliviertassinari commented May 19, 2021

We could consider a simpler solution: we could remove the dark mode scrollbar customization. This has two advantages:

  1. We went in the same direction in the core because it breaks the native scrollbar width: [CssBaseline] Persistent scrollbar in dark mode material-ui#23520.
  2. This free us bandwidth in the review and follow-up. It's less code, less complexity.

@@ -71,6 +71,10 @@ export const useGridContainerProps = (

scrollBarSize.y = hasScrollY ? options.scrollbarSize! : 0;

// We recalculate the scroll x to consider the size of the y scrollbar.
hasScrollX = columnsTotalWidth + scrollBarSize.y > windowSizesRef.current.width;
scrollBarSize.x = hasScrollX ? options.scrollbarSize! : 0;
Copy link
Member

Choose a reason for hiding this comment

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

As an alternative solution. Did we consider to apply the same CSS to the scrollbar detector as the displayed scrollbar? Could this be more reliable and target the root cause better?

Copy link
Member Author

Choose a reason for hiding this comment

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

There were 2 issues

  1. we were off on the calculation of the width of the scrollbar
  2. the getScrollBarSize function didn't work properly

@oliviertassinari oliviertassinari changed the title [DataGrid] fix scroll size detector [DataGrid] Fix scroll size detector May 19, 2021
@m4theushw
Copy link
Member

The related bug still happens on mobile. I think we need to apply the CSS only on desktop. I'm using 942px as width in Chrome's Device Mode to test it.

scrollbar.mp4

@dtassone
Copy link
Member Author

The related bug still happens on mobile. I think we need to apply the CSS only on desktop. I'm using 942px as width in Chrome's Device Mode to test it.

scrollbar.mp4

it's because when you change the mode, the state of the scroll bar is not refreshed. You need to reload.
Any idea how I could watch the dark mode switch?

@m4theushw
Copy link
Member

m4theushw commented May 19, 2021

Even after refreshing the page, the scrollbar is still there. I think the whole idea in changing the color of the scrollbar on dark mode was to fix its look on Windows, because there's no dark scrollbar. However, this causes problems in other platforms. I think we have the following situations:

  • On macOS and using the light mode, the scrollbar is invisible by default. It has zero width. Although, when changing to the dark mode, the scrollbar becomes visible and it has to be accounted to calculate the rendering zone width. There's no bug here, it's just the way things work: less space available, put a scrollbar.
  • On Windows, the scrollbar is always visible not matter the mode. This way, there's no bug on this platform because the width doesn't change.
  • iOS and Android have, by default, a scrollbar that works good on both modes and it's only visible when scrolling, like macOS. Since we change the color on the dark mode, we also force it to become always visible. When there's this change between
    visible while scrolling and always visible the bug occurs.

That being said, I think we have two solutions:

  • Apply the CSS to change the scrollbar color only on dark mode and on platforms that don't have a dark scrollbar. This is a bit hacky, basically we have to check if the user is on Windows.
  • Force the scrollbar to be always visible on light mode. AG-Grid does that.

Any idea how I could watch the dark mode switch?

React.useEffect?

@oliviertassinari
Copy link
Member

oliviertassinari commented May 19, 2021

Any idea how I could watch the dark mode switch?

Don't do it (dark mode is an implementation details, nothing would predict that developers won't implement something custom). Prefer reacting to rerenders or listening to change of scrollbar width.

@dtassone
Copy link
Member Author

Not sure how you want to fix it now. But it should be fixed one way or another. It doesn't need to be the definitive solution, and we don't need to spend weeks on it...

@m4theushw
Copy link
Member

@dtassone I would remove the customization for the scrollbar on dark mode. Just noticed that browsers now have a dark scrollbar, but because the docs misses a color-scheme meta tag they don't work. cc @oliviertassinari I presume that if we use the native scrollbar and encourage users to add the tag I mentioned, the issue will be fixed.

Here's an example adding this tag on Edge:

image

@oliviertassinari
Copy link
Member

oliviertassinari commented May 27, 2021

@m4theushw Great catch. It looks like we should indeed update CssBaseline to take care of this: mui/material-ui#25016 (comment).

@dtassone dtassone merged commit c992eb3 into mui:master Jun 1, 2021
@oliviertassinari
Copy link
Member

The commit doesn't match the PR title:

Capture d’écran 2021-06-02 à 19 26 54

(It's more than a refactor, we fix a bug)

@dtassone dtassone changed the title [DataGrid] Fix scroll size detector [DataGrid] Fix horizontal scroll bar in dark mode, refactor scroll size detector Jun 3, 2021
@zannager zannager added the component: data grid This is the name of the generic UI component, not the React module! label Jan 17, 2023
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.

[DataGrid] horizontal scroll behavior is different between light mode and dark mode
4 participants