1196786 - Remove unnecessary preprocessor usage in shared devtools CSS that was ported over from platform-specific move

Status: RESOLVED FIXED

(DevTools :: Shared Components, defect)

Description

[Brian Grinstead [:bgrins]]

In Bug 896733, I mostly copied over differences between platform specific theme files into the shared files.  They probably aren't all necessary though, and some may be able to be removed.

From https://bugzilla.mozilla.org/show_bug.cgi?id=896733#c20:

(In reply to Tim Nguyen [:ntim] (mostly away until 26 August) from comment #20)
> https://reviewboard.mozilla.org/r/16173/#review14895
> 
> ::: browser/themes/shared/devtools/floating-scrollbars.css:10
> (Diff revision 1)
> > +%ifdef XP_MACOSX
> 
> The ifdef looks unnecessary as none of our platforms have a border in the
> floating scrollbars styling.
> Using border: none; shouldn't hurt too.
> 
> ::: browser/themes/shared/devtools/floating-scrollbars.css:29
> (Diff revision 1)
> > +%ifdef XP_MACOSX
> 
> The ifdef here also looks unneccessary to me since most of the rules below
> are similar.
> 
> ::: browser/themes/shared/devtools/widgets.css:223
> (Diff revision 1)
> > +%ifdef XP_WIN
> 
> I think it's safe to remove the ifdef here, since all platforms hide the
> border all the time. We can even remove this rule altogether and add
> !important to the border: none; declaration above.

Comment 1

[Tim Nguyen :ntim]

Attachment: Patch

Comment 2

[Brian Grinstead [:bgrins]]

Comment on attachment 8669367 [details] [diff] [review]
Patch

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

The changes to floating-scrollbars are the least known to me.  Can you make sure you've looked at it on at least windows and osx before/after the patch and see if you can detect any changes?  I'm happy to get rid of the preprocessor here but want to make sure we are aware of what exactly is changing (if anything).

::: devtools/client/jar.mn
@@ -286,1 @@
>      skin/themes/images/responsivemode/[e-mail] (themes/images/responsivemode/[e-mail])

Looks like the * wasn't removed for widget.css in this file

::: devtools/client/themes/floating-scrollbars.css
@@ -30,5 @@
>  slider {
>    -moz-appearance: none !important;
>  }
>  
> -thumb {

Is there a case where `thumb` matches bug `scrollbar thumb` doesn't?

::: devtools/client/themes/widgets.css
@@ +214,5 @@
>    color: hsl(210,30%,85%);
>  }
>  
>  .breadcrumbs-widget-item > .button-box {
> +  /* !important is needed to override focus ring on Windows */

How about keeping the .breadcrumbs-widget-item:-moz-focusring > .button-box rule and getting rid of the windows preprocessor directive?

Comment 3

[Tim Nguyen :ntim]

(In reply to Brian Grinstead [:bgrins] from comment #2)
> Comment on attachment 8669367 [details] [diff] [review]
> Patch
> 
> Review of attachment 8669367 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> The changes to floating-scrollbars are the least known to me.  Can you make
> sure you've looked at it on at least windows and osx before/after the patch
> and see if you can detect any changes?  I'm happy to get rid of the
> preprocessor here but want to make sure we are aware of what exactly is
> changing (if anything).
Will do. As far as I know, these changes are safe, but I'll do some extra testing to be sure.

> ::: devtools/client/jar.mn
> @@ -286,1 @@
> >      skin/themes/images/responsivemode/[e-mail] (themes/images/responsivemode/[e-mail])
> 
> Looks like the * wasn't removed for widget.css in this file
That's because there are still some preprocessor directives on top of the file.

> ::: devtools/client/themes/floating-scrollbars.css
> @@ -30,5 @@
> >  slider {
> >    -moz-appearance: none !important;
> >  }
> >  
> > -thumb {
> 
> Is there a case where `thumb` matches bug `scrollbar thumb` doesn't?
Yes, the only other usage of `thumb` is inside XUL sliders. XUL sliders can be used standalone and are used within scrollbars: https://dxr.mozilla.org/mozilla-central/source/toolkit/content/widgets/scrollbar.xml#28

So it's actually better to only target `scrollbar thumb`, otherwise standalone XUL sliders are also targeted. It may also be judicious to prefix the previous `slider` rule with `scrollbar`.

> ::: devtools/client/themes/widgets.css
> @@ +214,5 @@
> >    color: hsl(210,30%,85%);
> >  }
> >  
> >  .breadcrumbs-widget-item > .button-box {
> > +  /* !important is needed to override focus ring on Windows */
> 
> How about keeping the .breadcrumbs-widget-item:-moz-focusring > .button-box
> rule and getting rid of the windows preprocessor directive?
Will do.

Comment 4

[Tim Nguyen :ntim]

(In reply to Tim Nguyen [:ntim] from comment #3)
> (In reply to Brian Grinstead [:bgrins] from comment #2)
> > Comment on attachment 8669367 [details] [diff] [review]
> > Patch
> > 
> > Review of attachment 8669367 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > The changes to floating-scrollbars are the least known to me.  Can you make
> > sure you've looked at it on at least windows and osx before/after the patch
> > and see if you can detect any changes?  I'm happy to get rid of the
> > preprocessor here but want to make sure we are aware of what exactly is
> > changing (if anything).
> Will do. As far as I know, these changes are safe, but I'll do some extra
> testing to be sure.
> 
> > ::: devtools/client/jar.mn
> > @@ -286,1 @@
> > >      skin/themes/images/responsivemode/[e-mail] (themes/images/responsivemode/[e-mail])
> > 
> > Looks like the * wasn't removed for widget.css in this file
> That's because there are still some preprocessor directives on top of the
> file.
> 
> > ::: devtools/client/themes/floating-scrollbars.css
> > @@ -30,5 @@
> > >  slider {
> > >    -moz-appearance: none !important;
> > >  }
> > >  
> > > -thumb {
> > 
> > Is there a case where `thumb` matches bug `scrollbar thumb` doesn't?
> Yes, the only other usage of `thumb` is inside XUL sliders. XUL sliders can
> be used standalone and are used within scrollbars:
> https://dxr.mozilla.org/mozilla-central/source/toolkit/content/widgets/
> scrollbar.xml#28
:bz confirmed this via IRC.

Comment 5

[Tim Nguyen :ntim]

Attachment: Patch v2

More directives removed, and comments addressed.

Comment 6

[Tim Nguyen :ntim]

Attachment: Patch v2.1

Removes some asterisks from jar.mn

Comment 7

[Alexandre Poirot [:ochameau]]

Comment on attachment 8684707 [details] [diff] [review]
Patch v2.1

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

See my comment about namespace, otherwise f+ given I did something similar in bug 1183280.
I'm going to rebase on top of your patch, I also cleaned up splitview.css.

::: devtools/client/themes/floating-scrollbars.css
@@ +36,5 @@
>  }
> +
> +:root[platform="mac"] scrollbar thumb {
> +  background-color: rgba(0,0,0,0.2) !important;
> +}

Did you gave that a try on mac?
What I'm seeing here is that there is a namespace issue here.
"scrollbar thumb" part of the selector works as expected,
But :root[platform="mac"] doesn't.
The default namespace is xul, so this part is only going to match xul elements.
*|:root[platform="mac"] won't work either. I don't know why, but it looks like it only work with a tag name.
*|*[platform="mac"]:root is going to work.
As well as: *|html[platform=mac], *|window[platform=mac]
I haven't tried with named namespace.
So I don't know about:
  html|:root[platform=mac], xul|:root[platform=mac]
I imagine there is also other combinations that may work...

Comment 8

[Tim Nguyen :ntim]

Attachment: Patch v2.2

Fixed a few bugs I've encountered during my testing, and addressed :ochameau's comment (thanks !).

Comment 9

[Brian Grinstead [:bgrins]]

Comment on attachment 8684984 [details] [diff] [review]
Patch v2.2

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

::: devtools/client/themes/common.css
@@ +4,2 @@
>  
>  :root {

I'm inclined to leave this file alone since it's loaded in browser.xul and I don't want to change the specificity of these rules.

I'm actually looking around and don't see any reason why the :root selector plus devtools-monospace need to be in this file and can't be in a toolbox-specific file imported by the theme files.  Either way, I'd be happy if we could sort that out in another bug since most of the other stuff in this patch looks ok

::: devtools/client/themes/floating-scrollbars.css
@@ +34,5 @@
>    background-color: rgba(170,170,170,0.2) !important;
>    border-radius: 3px !important;
>  }
> +
> +*|*[platform="mac"]:root scrollbar thumb {

I can't actually find a case where this color is intended to be used... Floating scrollbars are only loaded in the toolbox for the dark theme, which is loaded as floating-scrollbars-light.css.

So this rule is overriding the 'light' scrollbar color.. which it wasn't doing before because the specificity matched that rule.  If that's the intent (I don't think it is) then it should move into that file (but I doubt it since it's changing behavior).

There appears to be a usage of floating-scrollbars.css directly from responsive mode which is probably what this meant to target, although I can't figure out how to activate that on Yosemite.

I'd actually be OK with dropping this unless if someone can figure out exactly when it was being used.

::: devtools/client/themes/netmonitor.css
@@ +817,5 @@
>    }
>  }
>  
>  /* Platform overrides (copied in from the old platform specific files) */
> +:root[platform="win"] .requests-menu-header-button > .button-box {

All of these rules are going to change specificity, so we should consider

::: devtools/client/themes/widgets.css
@@ +223,2 @@
>  .breadcrumbs-widget-item:-moz-focusring > .button-box {
> +  /* !important is needed to override focus ring on Windows */

Have you tested this to be sure it's not changing anything on osx / linux?

Comment 10

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

Comment on attachment 8684984 [details] [diff] [review]
Patch v2.2

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

::: devtools/client/themes/floating-scrollbars.css
@@ +34,5 @@
>    background-color: rgba(170,170,170,0.2) !important;
>    border-radius: 3px !important;
>  }
> +
> +*|*[platform="mac"]:root scrollbar thumb {

Responsive Design uses the following condition to decide whether to inject floating-scrollbars.css:

  let requiresFloatingScrollbars = !this.mainWindow.matchMedia("(-moz-overlay-scrollbars)").matches;

On Windows and Linux, I believe it is always injected.  On Mac with default OS settings it is not injected.  However, if you change to System Preferences -> General -> Show scroll bars -> Always, then we do inject it on Mac as well.  (Note if you flip this OS setting while Firefox is running, I think you need to open a fresh browser **window** to actually get the injection correctly in Responsive Design.)

Comment 11

[Brian Grinstead [:bgrins]]

(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #10)
> Comment on attachment 8684984 [details] [diff] [review]
> Patch v2.2
> 
> Review of attachment 8684984 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: devtools/client/themes/floating-scrollbars.css
> @@ +34,5 @@
> >    background-color: rgba(170,170,170,0.2) !important;
> >    border-radius: 3px !important;
> >  }
> > +
> > +*|*[platform="mac"]:root scrollbar thumb {
> 
> Responsive Design uses the following condition to decide whether to inject
> floating-scrollbars.css:
> 
>   let requiresFloatingScrollbars =
> !this.mainWindow.matchMedia("(-moz-overlay-scrollbars)").matches;
> 
> On Windows and Linux, I believe it is always injected.  On Mac with default
> OS settings it is not injected.  However, if you change to System
> Preferences -> General -> Show scroll bars -> Always, then we do inject it
> on Mac as well.  (Note if you flip this OS setting while Firefox is running,
> I think you need to open a fresh browser **window** to actually get the
> injection correctly in Responsive Design.)

Even if I set background-color: orange !important; in floating-scrollbars.css and floating-scrollbars-light.css, flip the system pref, restart the browser and then open a new window I cannot see any floating scrollbars in RDM.  I do see them in the toolbox when the dark theme is on in this case.

Comment 12

[Alexandre Poirot [:ochameau]]

ntim, I would like to proceed with preprocessor rules removal sooner than later as it break live reload.
I have a safe patch in bug 1183280 that is simple stupid and just rewrite existing rules with the new [platform=xxx]:root thing. Do you think you could finish up this patch in the next days or may I try to land my patch first?

Comment 13

[Tim Nguyen :ntim]

(In reply to Alexandre Poirot [:ochameau] from comment #12)
> ntim, I would like to proceed with preprocessor rules removal sooner than
> later as it break live reload.
> I have a safe patch in bug 1183280 that is simple stupid and just rewrite
> existing rules with the new [platform=xxx]:root thing. Do you think you
> could finish up this patch in the next days or may I try to land my patch
> first?

I'll try to get to it this weekend, but feel free to land your patch first.

Comment 14

[Alexandre Poirot [:ochameau]]

FYI, I rebased my patch in bug 1183280 and asked for review.

Comment 15

[Tim Nguyen :ntim]

Attachment: Patch v3

This is a very small patch, since :ochameau has done most of the work.

Comment 16

[Brian Grinstead [:bgrins]]

Comment on attachment 8721685 [details] [diff] [review]
Patch v3

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

::: devtools/client/themes/floating-scrollbars-dark-theme.css
@@ +30,2 @@
>  xul|scrollbar xul|thumb {
>    background-color: rgba(170,170,170,0.2) !important;

Looks like this is effectively only changing the background color for the thumb on mac.  Is this correct, and what we want to happen?

Comment 17

[Tim Nguyen :ntim]

(In reply to Brian Grinstead [:bgrins] from comment #16)
> Comment on attachment 8721685 [details] [diff] [review]
> Patch v3
> 
> Review of attachment 8721685 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: devtools/client/themes/floating-scrollbars-dark-theme.css
> @@ +30,2 @@
> >  xul|scrollbar xul|thumb {
> >    background-color: rgba(170,170,170,0.2) !important;
> 
> Looks like this is effectively only changing the background color for the
> thumb on mac.  Is this correct, and what we want to happen?

Is it ? That property and it's value are untouched by this patch. Am I missing something ?

Comment 18

[Brian Grinstead [:bgrins]]

Comment on attachment 8721685 [details] [diff] [review]
Patch v3

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

Never mind, after re-reading it looks like this shouldn't change anything.  r=me, but I haven't applied and tested the patch locally so I'm trusting you've done so

Comment 19

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/c244554cc68b

Comment 20

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/c244554cc68b