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

RESOLVED FIXED in Firefox 47

Status

defect
RESOLVED FIXED
4 years ago
4 months ago

People

(Reporter: bgrins, Assigned: ntim)

Tracking

(Blocks 1 bug)

Trunk
Firefox 47
Dependency tree / graph

Firefox Tracking Flags

(firefox43 affected, firefox47 fixed)

Details

(Whiteboard: [devtools-css])

Attachments

(1 attachment, 4 obsolete attachments)

Reporter

Description

4 years ago
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.
Reporter

Updated

4 years ago
Whiteboard: [devtools-css]
Assignee

Comment 1

4 years ago
Posted patch Patch (obsolete) — Splinter Review
Assignee: nobody → ntim.bugs
Status: NEW → ASSIGNED
Attachment #8669367 - Flags: review?(bgrinstead)
Reporter

Comment 2

4 years ago
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/responsiveui-screenshot@2x.png (themes/images/responsivemode/responsiveui-screenshot@2x.png)

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?
Attachment #8669367 - Flags: review?(bgrinstead)
Assignee

Comment 3

4 years ago
(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/responsiveui-screenshot@2x.png (themes/images/responsivemode/responsiveui-screenshot@2x.png)
> 
> 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.
Assignee

Comment 4

4 years ago
(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/responsiveui-screenshot@2x.png (themes/images/responsivemode/responsiveui-screenshot@2x.png)
> > 
> > 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.
Assignee

Updated

4 years ago
Assignee

Comment 5

4 years ago
Posted patch Patch v2 (obsolete) — Splinter Review
More directives removed, and comments addressed.
Attachment #8669367 - Attachment is obsolete: true
Attachment #8684705 - Flags: review?(bgrinstead)
Assignee

Comment 6

4 years ago
Posted patch Patch v2.1 (obsolete) — Splinter Review
Removes some asterisks from jar.mn
Attachment #8684705 - Attachment is obsolete: true
Attachment #8684705 - Flags: review?(bgrinstead)
Attachment #8684707 - Flags: review?(bgrinstead)
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...
Attachment #8684707 - Flags: feedback+
Assignee

Comment 8

4 years ago
Posted patch Patch v2.2 (obsolete) — Splinter Review
Fixed a few bugs I've encountered during my testing, and addressed :ochameau's comment (thanks !).
Attachment #8684707 - Attachment is obsolete: true
Attachment #8684707 - Flags: review?(bgrinstead)
Attachment #8684984 - Flags: review?(bgrinstead)
Reporter

Comment 9

4 years ago
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?
Attachment #8684984 - Flags: review?(bgrinstead)
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.)
Reporter

Comment 11

4 years ago
(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.
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?
Flags: needinfo?(ntim.bugs)
Assignee

Comment 13

4 years ago
(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.
Flags: needinfo?(ntim.bugs)
FYI, I rebased my patch in bug 1183280 and asked for review.
Assignee

Comment 15

3 years ago
Posted patch Patch v3Splinter Review
This is a very small patch, since :ochameau has done most of the work.
Attachment #8684984 - Attachment is obsolete: true
Attachment #8721685 - Flags: review?(bgrinstead)
Reporter

Comment 16

3 years ago
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?
Assignee

Comment 17

3 years ago
(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 ?
Reporter

Comment 18

3 years ago
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
Attachment #8721685 - Flags: review?(bgrinstead) → review+
Assignee

Updated

3 years ago
Keywords: checkin-needed

Comment 20

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/c244554cc68b
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 47

Updated

11 months ago
Product: Firefox → DevTools
Assignee

Updated

4 months ago
Component: Framework → CSS and Themes
You need to log in before you can comment on or make changes to this bug.