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

Universal (nearly) scrollbar styling #6026

Merged

Conversation

@telamonian
Copy link
Member

@telamonian telamonian commented Feb 22, 2019

Fixes #4867 and #3008.

Adds the ability to customize the color of the scrollbars (I'll save the width stuff for a later day, maybe). Also adds settings to enable/disable scrollbar customization.

The latest release version of Firefox (65) added support for styling scrollbars using CSS. Alongside the webkit extensions that have been around forever, this means that customizable scrollbars now have almost universal support amongst the major browsers (except for Edge, for some reason).

I've been working on a Darcula theme recently. The white scrollbars in the middle of the dark theme started to bug me, so I took at stab at implementing custom scrollbars in Jupyterlab. I also added a set of dark scrollbars to the default Dark theme. Here's what they look like:

  • Safari

image

  • Firefox

image

  • Chrome

image

I realize that custom scrollbars aren't everyone's cup of tea, and that they may cause serious technical issues for a subset of users. That's why I've added a related setting to both the theme manager and the settings manager. The custom scrollbars are turned on only if a) they're enabled in the theme and b) if the user enables them in their settings. Right now, by default the user setting is enabled.

Also, I ran into a bear of a rendering bug trying to get the custom scrollbars to work consistently in Chrome. The solution I finally came up with was to hide and unhide the shell widget at the end of the theme changing process in order to force a redraw. The hide/unhide happens under the cover of the splash screen, and it doesn't seem to have an impact on performance, so I think this is an acceptable fix. I am open to suggestions, however.

@jupyterlab-dev-mode
Copy link

@jupyterlab-dev-mode jupyterlab-dev-mode bot commented Feb 22, 2019

Thanks for making a pull request to JupyterLab!

To try out this branch on binder, follow this link: Binder

@telamonian
Copy link
Member Author

@telamonian telamonian commented Feb 22, 2019

This is a long standing issue that has been tackled (#2737, #2830) and then reverted (#2897) at least once before (I borrowed liberally from @cameronoelsen's webkit code in the old PRs). The difference now I think is the support for Firefox.

Copy link
Contributor

@jasongrout jasongrout left a comment

Thanks! I'm really excited about the scrollbars being styled nicely.

packages/apputils-extension/src/index.ts Outdated Show resolved Hide resolved
packages/apputils/src/thememanager.ts Outdated Show resolved Hide resolved
this._themeChanged.emit({
name: 'theme',
oldValue: current,
newValue: theme
});

// Need to force a redraw of the app here to avoid a Chrome rendering bug/race condition
const style = this._host.node.getAttribute('style');
Copy link
Contributor

@jasongrout jasongrout Feb 22, 2019

Can we instead use this._host.hide() and this._host.show(), the phosphor commands that essentially do this display styling?

Copy link
Contributor

@jasongrout jasongrout Feb 22, 2019

(though using the phosphor commands may trigger expensive rerenders...)

Do you have a link to the Chrome bug report, so we can keep an eye on it to know when we can remove this workaround?

Copy link
Member Author

@telamonian telamonian Feb 23, 2019

I'll test out this._host.hide() and see if it works.

No bug report, just the results of my observations from testing. Typically, at least some scrollbars (especially the ones for the panels hosted on the left and right tab bars) won't update with the rest of theme, but can be fixed with a further refresh of the browser, or even just collapsing/uncollapsing the containing widget.

As a caveat, I'm still using Chrome 68 at the moment (they dropped proper support for multiple monitors on mac in later versions, so I've been loath to move on). It would be nice if someone could test the PR without the render fix on the latest chrome/not on a mac.

Copy link
Member Author

@telamonian telamonian Feb 23, 2019

  • this._host.hide()/show() work just as well for fixing the rendering issue. My very not rigorous testing showed that hide/show took about 10% more rendering time per theme change than did directly setting display, but honestly it was probably well within the noise. Should I switch over to hide/show?

  • I just tested this out on the latest version of Chromium (74), and the issue persists. Here's an example:

image

This is after changing from the Light theme to the Dark. While the launcher scrollbar changed colors correctly, the filebrowser scrollbar remained white.

I took a quick look at Chromium's bug board for -webkit-scrollbar rendering issues, but didn't see anything similar. Given the way that the thememanager is currently dynamically loading the CSS, I'm going to go out on a limb here and guess that Jupyterlab is kind of an edge case. I'll submit a bug report.

Copy link
Contributor

@jasongrout jasongrout Feb 23, 2019

Should I switch over to hide/show?

I think so. It's definitely the more conventional "phosphor way" to do things, and may fix other things. Really, the phosphor elements are getting hidden and shown in the splash screen, and theme changes may affect measurements, so it's probably good to have a phosphor hide/show cycle so things can update.

Copy link
Contributor

@jasongrout jasongrout Feb 23, 2019

theme changes may affect measurements, so it's probably good to have a phosphor hide/show cycle so things can update.

I guess that's probably what the fitAll call does, though. Still, I think it's probably not bad to do a hide/show cycle as a more conventional way to deal with the issue.

Copy link
Member Author

@telamonian telamonian Feb 23, 2019

The code's been switched to use hide/show.

packages/apputils/src/thememanager.ts Outdated Show resolved Hide resolved
Copy link
Member

@ian-r-rose ian-r-rose left a comment

This is really cool.

packages/theme-dark-extension/src/index.ts Outdated Show resolved Hide resolved
@ellisonbg
Copy link
Contributor

@ellisonbg ellisonbg commented Feb 25, 2019

On a Mac running Sierra. I see White scrollbars in dark theme, but only when I have set the System Prefs. for "Show scroll bars" to "Always" or "Automatically based on mouse...". The "When scrolling" setting seems to work fine. I should note that if you change this system pref. you have to refresh the page (browsers don't do a good job of refreshing based on this state. In this state, the styling here is ignored (which is good, as the OS already handles these things). Here is that pane:

screen shot 2019-02-25 at 8 35 59 am

Another bug in Safari. I often use the "Show scroll bars: When scrolling" option as it hides and shows the scroll bars nicely and auto adjusts the colors based on the background (at the OS level). This setting is working fine with FF, C but with Safari in dark theme, the scroll bars are essentially invisible, rather than styled according to the browser. I can't easily take a screen shot as this is only visible during scrolling.


// Need to force a redraw of the app here to avoid a Chrome rendering
// bug that can leave the scrollbars in an invalid state
this._host.hide();
Copy link
Contributor

@ellisonbg ellisonbg Feb 25, 2019

I am ok with this workaround, but let's open a follow up issue so we have a reminder to revisit this (hopefully Chrome will fix it)

* On Firefox, only the color settings are respected.
*/

--jp-scrollbar-background-color: #3f4244;
Copy link
Contributor

@ellisonbg ellisonbg Feb 25, 2019

Can you add a note to say where this color came from - otherwise looks arbitrary.

Copy link
Member Author

@telamonian telamonian Feb 25, 2019

/* Based on the JetBrains Darcula theme */

All of this CSS is copied directly from my Darcula theme. I'm no designer, so when I couldn't find an authoritative reference for Material Dark, I figured it was safer to just leave the colors alone (they do look pretty good). If you want to point me to a spec (or just an example of a good Material Dark theme in action) I'd be happy to change these.

Copy link
Member Author

@telamonian telamonian Feb 25, 2019

note added

--jp-scrollbar-background-color: #3f4244;
--jp-scrollbar-endpad: 3px; /* the minimum gap between the thumb and the ends of a scrollbar */

--jp-scrollbar-thumb-color: 88, 96, 97; /* need to specify thumb color as an RGB triplet */
Copy link
Contributor

@ellisonbg ellisonbg Feb 25, 2019

Same here about adding note about where the color comes from.

Copy link
Member Author

@telamonian telamonian Feb 25, 2019

Same

Copy link
Member Author

@telamonian telamonian Feb 25, 2019

note added

@ellisonbg
Copy link
Contributor

@ellisonbg ellisonbg commented Feb 25, 2019

This is really impressive work. I know that @cameronoelsen wrestled with this and it was really subtle. Great work! One other question: do you know if sizing the width of the scroll bar will be possible. It is a bit wide on a multipanel app like this, especially in areas like cells, and the L/R sidebars. Doesn't have to happen for this PR, just would like to understand the future possibilities.

@telamonian
Copy link
Member Author

@telamonian telamonian commented Feb 25, 2019

@ellisonbg From what I've read, the short answer about width adjustment is that yes, it can be done. I'll test some stuff out and see if it can actually be done in practice.

@telamonian
Copy link
Member Author

@telamonian telamonian commented Feb 25, 2019

@ellisonbg Tried a bunch of stuff out, and the news looks good, but mixed. Here's what I managed in Chrome:

image

There was some trickier stuff going on with Firefox though. It seems like the scrollbar-width setting only applied to scrollbars with transparent backgrounds. Their demo here makes it clear that the feature does work as intended, though (at least for a setting of thin).

Once this PR is done with, I can work on the widths next, if you'd like.

@jasongrout
Copy link
Contributor

@jasongrout jasongrout commented Apr 2, 2019

I've been testing this. It looks great, especially when I have macOS set up to always show scroll bars.

I am seeing some issues in Chrome and Safari when I have macOS (10.14 Mojave) set up to show scrollbars only when scrolling - in those cases, with scrollbar styling turned on, I'm seeing the scrollbars always instead. When I turn off scrollbar styling in Advanced Settings, I see what I expect - the scroll thumb only appearing when actually scrolling. In Firefox, everything works like I expect, no matter what the OS scrollbars setting is.

I'm not surprised we're running into issues. Scrollbar styling is notoriously tricky. For this reason, I think we should default to off until we can fix Chrome and Safari to respect the macOS 'show scrollbars when scrolling' setting. I imagine that Chrome/Safari/'show when scrolling' is a rather prevalent combination on macOS, and I think we shouldn't break it or effectively change it into 'scrollbars always'.

@jasongrout
Copy link
Contributor

@jasongrout jasongrout commented Apr 2, 2019

If it's all right, I'll go ahead and tweak things here to turn off the extra styling by default, but merge it in so it is an option people can turn on in the next prerelease coming out very soon.

We're still seeing some issues, for example in Chrome or Safari on macOS when scrollbars are set to show only when scrolling. Essentially, this styling causes scrollbars to always show in these cases. It works fine in Firefox on macOS with the 'visible when scrolling' OS setting, though.
@jasongrout jasongrout merged commit 00d6c0c into jupyterlab:master Apr 2, 2019
7 of 9 checks passed
@lock lock bot locked as resolved and limited conversation to collaborators Aug 7, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

5 participants