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

[CssBaseline] Persistent scrollbar in dark mode #23520

Closed
2 tasks done
mbrookes opened this issue Nov 14, 2020 · 23 comments · Fixed by #24780
Closed
2 tasks done

[CssBaseline] Persistent scrollbar in dark mode #23520

mbrookes opened this issue Nov 14, 2020 · 23 comments · Fixed by #24780
Labels
component: CssBaseline The React component new feature New feature or request ready to take Help wanted. Guidance available. There is a high chance the change will be accepted

Comments

@mbrookes
Copy link
Member

  • The issue is present in the latest release.
  • I have searched the issues of this repository and believe that this is not a duplicate.

Current Behavior 😯

image

Expected Behavior 🤔

image

Steps to Reproduce 🕹

Steps:

  1. Visit https://next.material-ui.com/
  2. Enable dark mode
  3. Notice scrollbar and layout shift

Your Environment 🌎

Tech Version
Material-UI v5.0.0-alpha.16
Browser Google Chrome Version 86.0.4240.111 (Official Build) (x86_64)
OS MacOS Catalin 10.15.7
@mbrookes mbrookes added status: waiting for maintainer These issues haven't been looked at yet by a maintainer docs Improvements or additions to the documentation and removed status: waiting for maintainer These issues haven't been looked at yet by a maintainer labels Nov 14, 2020
@mbrookes
Copy link
Member Author

Related? #23407

@oliviertassinari
Copy link
Member

oliviertassinari commented Nov 14, 2020

Interesting. In my configuration of macOS, I force the scrollbar (why? to be closer to the UX end-users have on Windows), so I couldn't see this change of behavior when working on #23407.

@oliviertassinari oliviertassinari added component: CssBaseline The React component and removed docs Improvements or additions to the documentation labels Nov 14, 2020
@oliviertassinari
Copy link
Member

@eps1lon I start to think that https://github.com/mui-org/material-ui-x/issues/510 could be interesting. YouTube, Facebook, Gmail are using custom scrollbars. This is a strong signal.

@efoken
Copy link

efoken commented Nov 14, 2020

Having the same issue 😕 As soon as the scrollbars are styled with CSS, they are always visible on macOS, so I think this styling should be optional

@oliviertassinari oliviertassinari changed the title [docs] Persistent scrollbar in dark mode [CssBaseline] Persistent scrollbar in dark mode Nov 19, 2020
@carlhopf
Copy link

carlhopf commented Dec 7, 2020

By default MacOS has auto hiding scrollbars and I believe most users will expect this behaviour.

Since there's no way to un-do or override the ::-webkit-scrollbar styling to get back to system default behaviour, I also think the scrollbar styling should be optional (or a way to turn it off) 😄

@oliviertassinari
Copy link
Member

Yeah ok, it seems too opinionated to be done by default for macOS users. Maybe an opt-out flag?

@mbrookes would it work with you if we keep it enabled by default?

@mbrookes
Copy link
Member Author

mbrookes commented Dec 7, 2020

Would OS detection be too icky? It's never going to be nice on Mac while it's enabled, and the system default scroll bar is perfectly fine for both light and dark mode, so Macs don't need the custom scrollbar styles.

@oliviertassinari
Copy link
Member

oliviertassinari commented Dec 7, 2020

Would OS detection be too icky? It's never going to be nice on Mac while it's enabled

Well, I'm personally not using the autohide scrollbar feature of macOS, I like it just fine with the scrollbar always visible. Maybe because I was used to it for so many years on Windows. As far as I know, CSS OS detection isn't possible.

So opt-in flag? Should we enable it for the docs? Note that we have also the same CSS logic applied on the DataGrid. It could be interesting to export the CSS object to not duplicate it.

@mbrookes
Copy link
Member Author

mbrookes commented Dec 8, 2020

As far as I know, CSS OS detection isn't possible.

I meant in JS, to determine whether to apply the CSS, but... SSR

So opt-in flag?

Preferably

Should we enable it for the docs?

I don't know. Just how bad do scrollbars look on Windows in dark mode?

@carlhopf
Copy link

carlhopf commented Dec 8, 2020

👍 for opt-in flag to preserve system-default behaviour.

but definitely agree that scrollbars look bad on Windows, especially in dark mode.

maybe there is a way to detect permanent scrollbar visibility and OS, then apply webkit styles. seems tricky though

@eps1lon
Copy link
Member

eps1lon commented Dec 8, 2020

@eps1lon I start to think that mui-org/material-ui-x#510 could be interesting. YouTube, Facebook, Gmail are using custom scrollbars.

I want to emphasize that my main concern with scrollbars is scrolling performance. However, this is mainly an issue on mobile devices. I'm not aware of scrolling issues on desktop though it might be one.

Scrolling is also a crucial part of the UX so any component needs thorough manual testing in different environments. Since this is integral to the documentation we can't just ship something we want users to test for us.

So please consider how much you want to invest into "pretty scrollbars" and if it isn't more important that you can smoothly scroll.

@oliviertassinari
Copy link
Member

oliviertassinari commented Dec 8, 2020

I don't know. Just how bad do scrollbars look on Windows in dark mode?

@mbrookes you can experience it with BrowserStack. It's significantly worse on Windows than it's on macOS with, at least, from my perspective.

Capture d’écran 2020-12-18 à 14 33 27

We can also weight the difference with the traffic coming from Windows vs. macOS, I haven't looked at the data but I imagine Windows is the dominant OS among developers.

@mbrookes
Copy link
Member Author

mbrookes commented Dec 8, 2020

It seems we should just be using color-scheme. After that, it's down to Browser support. This will also fix native form elements, such as select, which don't currently render correctly in dark mode. Tested in Edge 87 on Windows 10.

@oliviertassinari
Copy link
Member

@mbrookes I don't understand. What's the link with color-scheme and native elements?

@mbrookes
Copy link
Member Author

mbrookes commented Dec 8, 2020

Browsers set native element color (including scroll bar) according to the page meta or css color-scheme.

https://web.dev/color-scheme/#the-color-scheme-css-property

@oliviertassinari
Copy link
Member

oliviertassinari commented Dec 8, 2020

  1. I wasn't aware of the existence of the color-schema CSS property. I doubt we should use it. It doesn't seem to place nicely with developers that want to force a specific theme, independently of what the OS uses.
  2. I have looked at the <select> in Edge 87, Windows 10. I couldn't spot anything specifically wrong:

Capture d’écran 2020-12-09 à 00 02 19

https://material-ui.com/components/selects/

  1. What if we only changed the scrollbar on nested elements, not the body? It's the tradeoff of stackoverflow.com. Nested elements is where it sucks the most.
  2. For the opt-in, what if we made it an exported function?
import { darkScrollbar } from '@material-ui/system';

const theme = {
  components: {
    MuiCssBaseline: {
      styleOverrides: {
        '@global': darkScrollbar(),
      },
    },
  },
}

A function we can then share with the data grid.

@carlhopf
Copy link

carlhopf commented Dec 9, 2020

The scrolling Select on Chrome 87 MacOS, MUI alpha 18, dark palette.

Screenshot 2020-12-09 at 20 03 16

Removed styles for system-default, scrollbar only visible temporarily while scrolling

Screenshot 2020-12-09 at 20 08 42

@oliviertassinari oliviertassinari added new feature New feature or request ready to take Help wanted. Guidance available. There is a high chance the change will be accepted labels Dec 12, 2020
@oliviertassinari
Copy link
Member

oliviertassinari commented Dec 12, 2020

I'm adding the "good to take" label to move forward with option 4. from #23520 (comment):

  • We make the feature opt-in.
  • We turn the feature on it in the documentation.
  • Once released, we used it scoped to the data grid (to replace the duplication of code).

@dborstelmann
Copy link
Contributor

Any updates to this? Just upgraded to v5 (which we need for pickers fixes) and this will definitely block our launch. Happy to use any workarounds in the meantime.

@oliviertassinari
Copy link
Member

@dborstelmann What would be perfect is to execute on #23520 (comment). I'm not sure that there is an easy workaround. It requires resetting the CSS properties set back to their default.

@dborstelmann
Copy link
Contributor

I'm not super familiar with how this works or I'd be happy to make a PR. Is it simple enough I can help out in some way?

@dborstelmann
Copy link
Contributor

I see the code got added in #23407 but I'm not sure how to make that CSS change optional

@dborstelmann
Copy link
Contributor

I think I can do #23520 (comment). I'll send a PR soon.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: CssBaseline The React component new feature New feature or request ready to take Help wanted. Guidance available. There is a high chance the change will be accepted
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants