989683 - Menubar text color no longer changes in inactive windows (without tabsintitlebar)

Status: VERIFIED FIXED

(Firefox :: Theme, defect)

Description

[Matthew N. [:MattN]]

Will investigating bug 981569, I noticed that the menubar color no longer changes on inactive windows on Windows 7 (at least in classic mode).

The issue is that http://hg.mozilla.org/mozilla-central/annotate/0e19655e93df/browser/themes/windows/browser.css#l111 is more specific than 
menubar > menu:-moz-window-inactive and that the proper color isn't set on the menubar (to be inheritted), only on the menu.

Comment 2

[:Gijs (he/him)]

Is the simplest solution to just set the selector at http://hg.mozilla.org/mozilla-central/annotate/0e19655e93df/browser/themes/windows/browser.css#l111 to only apply in the tabsintitlebar case for non-compositor themes (and always for compositor themes, using a media query + the requisite ifdef AERO)? That's when we are guaranteed to set the right foreground color in the blocks above and in browser-aero.css, right? I would imagine that would also fix bug 990988.

Comment 3

[:Gijs (he/him)]

Attachment: restrict how we inherit the menubar text color to tabsintitlebar cases on non-aero,

Stealing. Does this look right?

Comment 4

[:Gijs (he/him)]

(In reply to :Gijs Kruitbosch from comment #2)
> I would imagine that would also fix bug 990988.

I can confirm this is the case for the patch that's waiting for review.

Comment 5

[Dão Gottwald [:dao]]

Comment on attachment 8403956 [details] [diff] [review]
restrict how we inherit the menubar text color to tabsintitlebar cases on non-aero,

>+  /* Use a different color only on Windows 8 and higher for inactive windows.
>+   * On aero, the menubar fog disappears for inactive windows, and renders gray
>+   * illegible.
>+   */
>+  @media not all and (-moz-os-version: windows-vista) {
>+    @media not all and (-moz-os-version: windows-win7) {
>+      #toolbar-menubar:not(:-moz-lwtheme):-moz-window-inactive {
>+        color: #aaa;
>+      }
>+    }
>+  }

Why is this in the -moz-windows-compositor block?

Comment 6

[:Gijs (he/him)]

(In reply to Dão Gottwald [:dao] from comment #5)
> Comment on attachment 8403956 [details] [diff] [review]
> restrict how we inherit the menubar text color to tabsintitlebar cases on
> non-aero,
> 
> >+  /* Use a different color only on Windows 8 and higher for inactive windows.
> >+   * On aero, the menubar fog disappears for inactive windows, and renders gray
> >+   * illegible.
> >+   */
> >+  @media not all and (-moz-os-version: windows-vista) {
> >+    @media not all and (-moz-os-version: windows-win7) {
> >+      #toolbar-menubar:not(:-moz-lwtheme):-moz-window-inactive {
> >+        color: #aaa;
> >+      }
> >+    }
> >+  }
> 
> Why is this in the -moz-windows-compositor block?

Because that's where we set the black/white color to begin with. I'd rather not risk us doing only half of the set in future (also, not 100% sure if win8's high contrast themes do or do not use compositor, so that might even be "now" rather than "future").

Comment 7

[:Gijs (he/him)]

(In reply to :Gijs Kruitbosch from comment #6)
> (also, not 100% sure if win8's high contrast themes do or do not use compositor)

They do, or at least (-moz-windows-compositor) matches on a window after changing theme to the high contrast one.

Comment 8

[Dão Gottwald [:dao]]

Comment on attachment 8403956 [details] [diff] [review]
restrict how we inherit the menubar text color to tabsintitlebar cases on non-aero,

>+  @media not all and (-moz-os-version: windows-vista) {
>+    @media not all and (-moz-os-version: windows-win7) {
>+      #toolbar-menubar:not(:-moz-lwtheme):-moz-window-inactive {
>+        color: #aaa;
>+      }
>+    }
>+  }

Can you use ThreeDShadow here like in menu.css?

>+@media (-moz-windows-compositor) {
>+  #main-menubar > menu:not(:-moz-lwtheme) {
>+    color: inherit
>+  }

missing semicolon

Comment 9

[:Gijs (he/him)]

(In reply to Dão Gottwald [:dao] from comment #8)
> Comment on attachment 8403956 [details] [diff] [review]
> restrict how we inherit the menubar text color to tabsintitlebar cases on
> non-aero,
> 
> >+  @media not all and (-moz-os-version: windows-vista) {
> >+    @media not all and (-moz-os-version: windows-win7) {
> >+      #toolbar-menubar:not(:-moz-lwtheme):-moz-window-inactive {
> >+        color: #aaa;
> >+      }
> >+    }
> >+  }
> 
> Can you use ThreeDShadow here like in menu.css?

Maybe, but why would we want to use a themed color when the background color is fixed? That doesn't seem right. Am I missing something?

Comment 10

[:Gijs (he/him)]

(In reply to :Gijs Kruitbosch from comment #9)
> (In reply to Dão Gottwald [:dao] from comment #8)
> > Comment on attachment 8403956 [details] [diff] [review]
> > restrict how we inherit the menubar text color to tabsintitlebar cases on
> > non-aero,
> > 
> > >+  @media not all and (-moz-os-version: windows-vista) {
> > >+    @media not all and (-moz-os-version: windows-win7) {
> > >+      #toolbar-menubar:not(:-moz-lwtheme):-moz-window-inactive {
> > >+        color: #aaa;
> > >+      }
> > >+    }
> > >+  }
> > 
> > Can you use ThreeDShadow here like in menu.css?
> 
> Maybe, but why would we want to use a themed color when the background color
> is fixed? That doesn't seem right. Am I missing something?

So the background color on high-contrast themes isn't fixed, for one (id est, varies with the theme). That said, I'm not sure if ThreeDShadow would help, in that case, and/or if we should worry about that (because e.g. tabs have the wrong foreground color; they're also black when they should be white). Probably makes more sense to followup that work.

Comment 11

[:Gijs (he/him)]

remote:   https://hg.mozilla.org/integration/fx-team/rev/616a8d4e449e

With ThreeDShadow + nit fixed. I'll file a followup bug for the high contrast theme stuff that I ran into.

Comment 12

[:Gijs (he/him)]

Filed bug 995825.

Comment 13

[Matthew N. [:MattN]]

Comment on attachment 8403956 [details] [diff] [review]
restrict how we inherit the menubar text color to tabsintitlebar cases on non-aero,

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 985267
User impact if declined: The menubar doesn't have inactive styling on Windows
Testing completed (on m-c, etc.): m-c soon
Risk to taking this patch (and alternatives if risky): Low risk making a previous fix (bug 985267) more specific.
String or IDL/UUID changes made by this patch: None

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/616a8d4e449e

Comment 15

[Mike de Boer [:mikedeboer]]

Aurora: https://hg.mozilla.org/releases/mozilla-aurora/rev/f40e90a9bdfe

Beta: https://hg.mozilla.org/releases/mozilla-beta/rev/bf8adf5a7040

Comment 16

[Ioana (away)]

Reproduced with the 03/31 Aurora on Windows 7 64bit (Windows Classic theme). Verified as fixed on the same OS, with Firefox 29.0b9 and the 04/17 Aurora and Nightly.