Closed Bug 989683 Opened 6 years ago Closed 6 years ago

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

Categories

(Firefox :: Theme, defect)

All
Windows 7
defect
Not set

Tracking

()

VERIFIED FIXED
Firefox 31
Tracking Status
firefox29 --- verified
firefox30 --- verified
firefox31 --- verified

People

(Reporter: MattN, Assigned: Gijs)

References

Details

(Keywords: regression, Whiteboard: [Australis:P3])

Attachments

(1 file)

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.
Flags: in-testsuite?
Duplicate of this bug: 990709
Blocks: 990988
Whiteboard: [Australis:P3]
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.
Flags: needinfo?(MattN+bmo)
Summary: Menubar text color no longer changes in inactive windows → Menubar text color no longer changes in inactive windows (without tabsintitlebar)
Stealing. Does this look right?
Attachment #8403956 - Flags: review?(dao)
Assignee: MattN+bmo → gijskruitbosch+bugs
Flags: needinfo?(MattN+bmo)
(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 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?
(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").
(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 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
Attachment #8403956 - Flags: review?(dao) → review+
(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?
Flags: needinfo?(dao)
(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.
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.
Flags: needinfo?(dao)
Filed bug 995825.
Whiteboard: [Australis:P3] → [Australis:P3][fixed-in-fx-team]
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
Attachment #8403956 - Flags: approval-mozilla-beta?
Attachment #8403956 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/616a8d4e449e
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Whiteboard: [Australis:P3][fixed-in-fx-team] → [Australis:P3]
Target Milestone: --- → Firefox 31
Attachment #8403956 - Flags: approval-mozilla-beta?
Attachment #8403956 - Flags: approval-mozilla-beta+
Attachment #8403956 - Flags: approval-mozilla-aurora?
Attachment #8403956 - Flags: approval-mozilla-aurora+
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.
You need to log in before you can comment on or make changes to this bug.