1235780 - Remove preprocessing from floating scrollbar css

Status: RESOLVED FIXED

(DevTools :: Framework, defect)

Comment 1

[Alexandre Poirot [:ochameau]]

Attachment: patch v1

Comment 2

[Brian Grinstead [:bgrins]]

Comment on attachment 8702884 [details] [diff] [review]
patch v1

Review of attachment 8702884 [details] [diff] [review]:
-----------------------------------------------------------------

Just a couple of questions

::: devtools/client/themes/floating-scrollbars.css
@@ -1,1 @@
> -@namespace url("http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul");

Why give the namespace a name here?  Were you unable to do the :root selector without doing so?  If so, could we do `[platform="mac"] scrollbar`, etc (removing :root from the selector)

@@ +42,5 @@
>    background-color: rgba(0,0,0,0.2);
>    border-radius: 3px;
>  }
> +
> +:root[platform="win"] xul|thumb,

Why change `scrollbar thumb` to just `thumb`?

Comment 3

[Alexandre Poirot [:ochameau]]

(In reply to Brian Grinstead [:bgrins] from comment #2)
> Comment on attachment 8702884 [details] [diff] [review]
> patch v1
> 
> Review of attachment 8702884 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Just a couple of questions
> 
> ::: devtools/client/themes/floating-scrollbars.css
> @@ -1,1 @@
> > -@namespace url("http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul");
> 
> Why give the namespace a name here?  Were you unable to do the :root
> selector without doing so?  If so, could we do `[platform="mac"] scrollbar`,
> etc (removing :root from the selector)

Yes, that's to workaround the root with html documents.
I got this trick suggested by Vivien, a old XUL guru and it allows to magically work. This is common to use explicit xul selector everywhere when we mixup namespaces like this.

But I just realized this doesn't work on Responsive mode. We inject this stylesheet in a content document, which doesn't have the platform attribute set. I'll split this stylesheet per platform and inject the good one based. We already inject it dynamically (light-theme and responsive mode)

> 
> @@ +42,5 @@
> >    background-color: rgba(0,0,0,0.2);
> >    border-radius: 3px;
> >  }
> > +
> > +:root[platform="win"] xul|thumb,
> 
> Why change `scrollbar thumb` to just `thumb`?

To simplify, as sometime we were already using just `thumb` few lines before for Mac.
I thought <xul:thumb> would only be used for scrollbars, but it is also used in <xul:scale> used in canvas debugger. So I'll readd the additional xul|scrollbar everywhere and stay consitant to one specific selector.

Comment 4

[Alexandre Poirot [:ochameau]]

Attachment: patch v2

This patch complexify the floating scrollbars a bit,
but reflects how complex our rules are.

Here is the CSS hierarchy (@import hierarchy):

- floating-scrollbars-theme-mac.css (additional overload just for mac and only for toolbox, not for responsive)
  > floating-scrollbars-mac.css (mac specifics, shared by responsive and toolbox)
    > floating-scollbars.css (common overrides, shared by responsive and toolbox and all OSes)
- floating-scrollbars-non-mac.css (windows/linux specifics, shared by responsive and toolbox)
  > floating-scrollbars.css

Note that I also removed unecessary !important in `scrollbar thumb` rules.

Verified on Mac and Linux.

Comment 5

[Brian Grinstead [:bgrins]]

Why is floating-scrollbars-theme-mac.css needed?  Seems that we were already loading all of these rules inside of responsive design via floating-scrollbars.css.  If we didn't have that file then the hierarchy would be easier to follow.

Comment 6

[Alexandre Poirot [:ochameau]]

(In reply to Brian Grinstead [:bgrins] from comment #5)
> Why is floating-scrollbars-theme-mac.css needed?  Seems that we were already
> loading all of these rules inside of responsive design via
> floating-scrollbars.css.  If we didn't have that file then the hierarchy
> would be easier to follow.

Because we want floating-scrollbars-mac.css, with, for example, its `border:none` on `scrollbar`.
But then this files also overloads the background of `scrollbar thumb` to `background-color: rgba(0,0,0,0.2)`.

I think we can only simplify this by changing the behavior/final look of our scrollbars.
Like, using the exact same styles between responsive design and toolboxes.
Then, we could get rid of floating-scrollbars-theme-mac.css.
I imagine we ended up with this complexity to match the precise look of system floating scrollbars on Mac and also help seeing them on white backgrounds. Note that scrollbars are light color on Windows and are very hard to see on light background.

Comment 7

[Brian Grinstead [:bgrins]]

(In reply to Alexandre Poirot [:ochameau] from comment #6)
> (In reply to Brian Grinstead [:bgrins] from comment #5)
> > Why is floating-scrollbars-theme-mac.css needed?  Seems that we were already
> > loading all of these rules inside of responsive design via
> > floating-scrollbars.css.  If we didn't have that file then the hierarchy
> > would be easier to follow.
> 
> Because we want floating-scrollbars-mac.css, with, for example, its
> `border:none` on `scrollbar`.
> But then this files also overloads the background of `scrollbar thumb` to
> `background-color: rgba(0,0,0,0.2)`.
> 
> I think we can only simplify this by changing the behavior/final look of our
> scrollbars.
> Like, using the exact same styles between responsive design and toolboxes.
> Then, we could get rid of floating-scrollbars-theme-mac.css.

Aren't we sharing exact styles between the responsive design and toolboxes right now?

Comment 8

[Brian Grinstead [:bgrins]]

ni for Comment 7.  It seems like this is adding a new distinction that didn't exist before, between toolbox floating scrollbars and responsive design floating scrollbars.  Maybe I'm missing something but I don't think we should change any UI with this work.

Comment 9

[Alexandre Poirot [:ochameau]]

This patch is complex for keeping the exact same behavior than before.
It would have been much simplier if we would have changed the UI.
That what I tried to highlight in comment 6.

Responsive mode loads floating-scrollbars-mac.css or floating-scrollbars-nonmac.css
While toolboxes load floating-scrollbars-theme-mac.css or floating-scrollbars-nonmac.css if dark theme.
The scrollbars look is only different between responsive mode and toolboxes on mac, before and after this patch.

Comment 10

[Brian Grinstead [:bgrins]]

(In reply to Alexandre Poirot [:ochameau] from comment #9)
> This patch is complex for keeping the exact same behavior than before.
> It would have been much simplier if we would have changed the UI.
> That what I tried to highlight in comment 6.
> 
> Responsive mode loads floating-scrollbars-mac.css or
> floating-scrollbars-nonmac.css
> While toolboxes load floating-scrollbars-theme-mac.css or
> floating-scrollbars-nonmac.css if dark theme.
> The scrollbars look is only different between responsive mode and toolboxes
> on mac, before and after this patch.

OK so if it's only different between those two modes on Mac, then I agree we should do a UI review of what it looks like now and see about consolidating into a single look on OSX since that will greatly simplify implementation.

Or, what about completely removing floating scrollbars in responsive design mode?  Ryan, maybe you have some context as to why they are there right now?  The scrollbars are in the page's viewport - I don't see why we are overriding them.

Comment 11

[Brian Grinstead [:bgrins]]

Removing floating scrollbars in RDM would also simplify this patch a lot because we could go back to using [platform] selectors in a single CSS file.

Comment 12

[Alexandre Poirot [:ochameau]]

(In reply to Brian Grinstead [:bgrins] from comment #10)
> OK so if it's only different between those two modes on Mac, then I agree we
> should do a UI review of what it looks like now and see about consolidating
> into a single look on OSX since that will greatly simplify implementation.

I would like to proceed with preprocessing removal rather than waiting for UI reviews.
That's why I invested time in providing a patch that shouldn't change anything.

From what I've seen, paul originaly just implemented some design from shortlander.
But I couldn't find any image/pdf/document from him.
So may be paul just wanted to match the style of native MacOS scrollbars, that are dark and not light.
floating-scrollbars is only for dark theme and are light.
Note in RDM it is often helpfull to have them dark as the content is often with a white background and it is hard to see the light scrollbars on windows and linux.
But again, please help me get rid of preprocessing sooner than later.

Comment 13

[Brian Grinstead [:bgrins]]

(In reply to Alexandre Poirot [:ochameau] from comment #12)
> (In reply to Brian Grinstead [:bgrins] from comment #10)
> > OK so if it's only different between those two modes on Mac, then I agree we
> > should do a UI review of what it looks like now and see about consolidating
> > into a single look on OSX since that will greatly simplify implementation.
> 
> I would like to proceed with preprocessing removal rather than waiting for
> UI reviews.
> That's why I invested time in providing a patch that shouldn't change
> anything.
> 
> From what I've seen, paul originaly just implemented some design from
> shortlander.
> But I couldn't find any image/pdf/document from him.
> So may be paul just wanted to match the style of native MacOS scrollbars,
> that are dark and not light.
> floating-scrollbars is only for dark theme and are light.
> Note in RDM it is often helpfull to have them dark as the content is often
> with a white background and it is hard to see the light scrollbars on
> windows and linux.
> But again, please help me get rid of preprocessing sooner than later.

I understand that, but we should at least consider removing the floating scrollbars in RDM since it would simplify the implementation greatly and should also improve the UX of RDM against light backgrounds.  This patch is adding a lot of complexity that could be removed in that case - we could go back to using [platform] and not having all the extra scrollbars files.

Comment 14

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

It is important for RDM to use floating scrollbars of *some* kind or another.  It's meant to simulate the look of a device screen, which (nearly?) always have floating scrollbars.

On Mac, the existing styles for RDM actually on target a very specific case.  For the average Mac user, they will see OS generated floating scrollbars inside RDM, and these styles have no effect on them.  You can tell you have the OS generated ones because they are white on dark backgrounds and dark on white backgrounds.  Clearly this is not possible from the single background color in CSS.

The Mac floating scrollbar styles we are discussing here only apply for users who have changed System Preferences -> General -> Show scroll bars to Always.  This means they see fixed scrollbars in all OS applications that take up actual space.  These users are the only ones for which the Mac floating scrollbar styles actually apply: they create something similar to the floating OS scrollbars, except they are always black of course.  So this means RDM gets special floating scrollbars for this case, which differs from the scrollbars these users see anywhere else.

(If you wish to test the difference in Mac OS scrollbar settings, you *must* close and open a fresh tab after changing the OS settings.  Reloading is not enough.)

For Windows and Linux, the styles are much more important since (AFAIK) they always apply to create the floating scrollbars.

So, does it simplify things to use the same scrollbar colors for all OSes in RDM?  For Mac, it won't be a change for the default OS settings anyway, so most users will see no difference.  Also, the RDM scrollbar color should probably be black on all OSes, since like Alex said most pages are probably light.  (But changing that color can be done separately from this bug, just an idea.)

Overall, we do need to keep some kind of floating scrollbar for all OSes in RDM.  But yes, let's try to simplify down from the current patch here.  It seems likely that we will want to a separate scrollbar color in RDM vs. toolbox, since in RDM it covers page content vs. the toolbox which has a known theme color we can control.

Comment 15

[Brian Grinstead [:bgrins]]

(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #14)
> So, does it simplify things to use the same scrollbar colors for all OSes in
> RDM?  For Mac, it won't be a change for the default OS settings anyway, so
> most users will see no difference.  Also, the RDM scrollbar color should
> probably be black on all OSes, since like Alex said most pages are probably
> light.  (But changing that color can be done separately from this bug, just
> an idea.)
> 
> Overall, we do need to keep some kind of floating scrollbar for all OSes in
> RDM.  But yes, let's try to simplify down from the current patch here.  It
> seems likely that we will want to a separate scrollbar color in RDM vs.
> toolbox, since in RDM it covers page content vs. the toolbox which has a
> known theme color we can control.

Somehow merging floating-scrollbars-theme-mac.css and floating-scrollbars-mac.css sounds good to me.

If I'm undestanding correctly, here is the plan:

Toolbox (dark theme only) and Responsive Design Mode will both use the same light colored floating scrollbars.  There will be a separate file for mac and non-mac.  There will be a plan to later convert these to be dark scrollbars for RDM - probably using `root:not(.theme-dark)` or a domain specific selector to distinguish between the two modes.

I'm still not sure how this all interacts with floating-scrollbars-light.css..  Does that file need to be deleted in this patch?

Comment 16

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

(In reply to Brian Grinstead [:bgrins] from comment #15)
> (In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #14)
> > So, does it simplify things to use the same scrollbar colors for all OSes in
> > RDM?  For Mac, it won't be a change for the default OS settings anyway, so
> > most users will see no difference.  Also, the RDM scrollbar color should
> > probably be black on all OSes, since like Alex said most pages are probably
> > light.  (But changing that color can be done separately from this bug, just
> > an idea.)
> > 
> > Overall, we do need to keep some kind of floating scrollbar for all OSes in
> > RDM.  But yes, let's try to simplify down from the current patch here.  It
> > seems likely that we will want to a separate scrollbar color in RDM vs.
> > toolbox, since in RDM it covers page content vs. the toolbox which has a
> > known theme color we can control.
> 
> Somehow merging floating-scrollbars-theme-mac.css and
> floating-scrollbars-mac.css sounds good to me.

The current set of files in this patch is really confusing... I get lost each time I try to reason about what is used where.

The file "floating-scrollbars-mac.css" is really "floating-scrollbars-responsive-mac.css" I guess (for RDM only)?

I think we do want at least:

* floating-scrollbars-responsive.css
* floating-scrollbars-toolbox.css

...or something.  They will want to be different colors.

Maybe we should talk about this on IRC / Vidyo.

> If I'm undestanding correctly, here is the plan:
> 
> Toolbox (dark theme only) and Responsive Design Mode will both use the same
> light colored floating scrollbars. There will be a separate file for mac
> and non-mac.

If we had a separate file for floating-scrollbars-toolbox.css, then it could use the platform selectors, and stay as one file.  That might be easier.  So, we may duplicate some things between -responsive.css and -toolbox.css, but it may also be easier to follow too.

> There will be a plan to later convert these to be dark
> scrollbars for RDM - probably using `root:not(.theme-dark)` or a domain
> specific selector to distinguish between the two modes.

I don't think RDM scrollbars care much about the DevTools theme...?  They want to always be there.  The color only matters based on the page content under them.  Currently they are a mix of colors across platforms, but probably should switch to black everywhere.

Comment 17

[Alexandre Poirot [:ochameau]]

Attachment: patch v3

Based on comment 16, with just two distinct files.

Note that if don't split per platform, with a mac AND a non-mac file for RDM,
we have to flatten the per-platform rules. We don't have just the background color.
  http://mxr.mozilla.org/mozilla-central/source/devtools/client/themes/floating-scrollbars.css#10
  http://mxr.mozilla.org/mozilla-central/source/devtools/client/themes/floating-scrollbars.css#29
But it looks like, we can just flatten/merge everything and have the same end result,
at least on Linux, I don't have Mac next to me.

I still kept two distinct files, we could have merge these two with using root:not(.theme-dark)
just for the dark background color in RDM, but it feels weak?
These two files are basically the same thing, except:
  - the `scrollbar thumb { background-color: }` which is light on toolbox, and dark on RDM
  - explicit namespace in toolbox css, that to make :root work against html docs.
    (we can get rid of that, if we remove any usage of :root in the CSS,
     by merging all per-platform rules, like what I did for RDM css)

Comment 18

[Brian Grinstead [:bgrins]]

(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #16)
> 
> > There will be a plan to later convert these to be dark
> > scrollbars for RDM - probably using `root:not(.theme-dark)` or a domain
> > specific selector to distinguish between the two modes.
> 
> I don't think RDM scrollbars care much about the DevTools theme...?  They
> want to always be there.  The color only matters based on the page content
> under them.  Currently they are a mix of colors across platforms, but
> probably should switch to black everywhere.

I intended for :not(.theme-dark) to be a way to select 'is in RDM' since the only time floating scrollbars are on in devtools toolbox is when the dark theme is applied.  As Alex points out in Comment 17 it's a little flaky because a web page could presumably add that class to the root element and we'd incorrectly think we were in the toolbox.

Maybe we could use the @document selector in addition to it and look for chrome / devtools paths somehow: https://developer.mozilla.org/en-US/docs/Web/CSS/@document.

Comment 19

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8707438 [details] [diff] [review]
patch v3

Review of attachment 8707438 [details] [diff] [review]:
-----------------------------------------------------------------

Great, this version is much easier to think about!

Personally I am fine with two separate files, I feel like they are for fairly independent problems, even if most of the contents are the same.

But, feel free to try @document or other ideas if you like!

I'll check this across platforms once the selectors get fixed.

::: devtools/client/themes/floating-scrollbars-dark-theme.css
@@ +32,5 @@
> +}
> +
> +:root[platform="mac"] xul|thumb {
> +  -moz-appearance: none !important;
> +  background-color: rgba(0,0,0,0.2);

The toolbox dark theme case (used to be "floating-scrollbars-light.css") would set background-color: rgba(170,170,170,0.2) for all platforms, so this line should be removed.

::: devtools/client/themes/floating-scrollbars-responsive-design.css
@@ +8,5 @@
>    z-index: 2147483647;
>    padding: 2px;
> +}
> +
> +scrollbar {

Same selector, collapse with above

@@ +39,5 @@
>    border-radius: 3px !important;
>  }
> +
> +scrollbar scrollbarbutton,
> +scrollbar scrollbarbutton {

scrollbar gripper

Comment 20

[Alexandre Poirot [:ochameau]]

I tried experimenting with @-moz-document, but I have some issues with codemirror iframe which are data:text/html :x

Comment 21

[Alexandre Poirot [:ochameau]]

I can do:
 @-moz-document url-prefix(chrome://devtools/content/) { ... }
But then it doesn't match codemirror iframe.

Or at the opposite, do:
 @-moz-document url-prefix(https://), url-prefix(http://) { ... }
But that will misbehaves if loading anything else in responsive mode like file:// about:// chrome:// ...

Comment 22

[Brian Grinstead [:bgrins]]

(In reply to Alexandre Poirot [:ochameau] from comment #20)
> I tried experimenting with @-moz-document, but I have some issues with
> codemirror iframe which are data:text/html :x

In that case I'd be fine with 2 separate, mostly identical files as suggested in Comment 19.  The files aren't big and rarely change so that seems like the simplest solution.

Comment 23

[Alexandre Poirot [:ochameau]]

Attachment: patch v4

Just addressed the comments.
I must say the single css using -moz-document was much much better,
especially when based on floating-scrollbars-responsive-design.css, without :root, not explicit namespace.
But it also feels weak, I haven't found nice and solid url selectors.

Comment 24

[Brian Grinstead [:bgrins]]

Comment on attachment 8710505 [details] [diff] [review]
patch v4

Review of attachment 8710505 [details] [diff] [review]:
-----------------------------------------------------------------

::: devtools/client/themes/floating-scrollbars-dark-theme.css
@@ +1,3 @@
> +@namespace xul url("http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul");
> +
> +xul|scrollbar {

Why is the explicit `xul|` namespace needed again?

Comment 25

[Alexandre Poirot [:ochameau]]

To make the :root selections magically work for html and xul documents.
If we don't set a default namespace, it looks like document one is automatically set.

Comment 26

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8710505 [details] [diff] [review]
patch v4

Review of attachment 8710505 [details] [diff] [review]:
-----------------------------------------------------------------

While testing this, I realized that our current behavior in the toolbox is strange.  We only insert floating scrollbars for the dark theme, but leave the light theme up to the OS, which may or may not float.  I would assume the toolbox would want floating all the time, but just with different colors per theme.

Anyway, this change appears to recreate the existing behavior correctly.  I only had time to check Mac, hopefully you've checked Win / Linux?

Comment 27

[Alexandre Poirot [:ochameau]]

Just verified on windows. And I already verified this on linux while working on it.

Comment 28

[Alexandre Poirot [:ochameau]]

https://hg.mozilla.org/integration/fx-team/rev/1bcd7705d2dc1610850ae01c221f55a6249a14cd
Bug 1235780 - Remove preprocessing from floating scrollbar css. r=jryans

Comment 29

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/1bcd7705d2dc