1208616 - [Windows 7 aero glass][Light theme] White text in Menu bar and TabsToolbar is barely visible on light background

Status: RESOLVED WONTFIX

(Firefox :: Theme, defect, P2)

Description

[arni2033]

Attachment: screenshot - [Dev Theme] White text in Menu bar and TabsToolbar is barely visible on light background.png

STR:   (Win7_64, Nightly 44, 32bit, ID 20150924030231, new profile, safe mode)
1. Open menu (≡) -> click "Customize"
2. Set "Developer Edition" theme, enable bookmarks toolbar, enable menu bar
3. Place bookmark items and zoom level widget to the TabsToolbar, exit customize
4. Resize window to normal mode

Result:       All those items display white-on-white text. Actually white icons aren't great as well
Expectations: Either (A) or (B)
           A) (preferable) menu bar and TabsToolbar have black background, just like in maximized mode
           B) Text color should be black in TabsToolbar and menu bar on Windows

Comment 1

[arni2033]

Attachment: video - [Dev Theme] White text in Menu bar and TabsToolbar is barely visible on light background.webm

My argument in favor of (A)

Comment 2

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

Attachment: Dark Theme - Win 7 Aero - Menu Bar.png

Comment 3

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

Attachment: Default Theme - Win 7 Aero - Menu Bar.png

Comment 4

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

I've added screenshots of Nightly 54 with Windows 7 Aero theme configured with menu bar enabled and widgets next to tabs like the reporter.

The menu bar / widgets seem more readable in my own Compact Light screenshot than the original reporter's, but if you had a really light background, the contrast would definitely go down.  Comparing it with the Default theme, it seems the Default theme uses a stronger glass / fog effect on the window plus black text which is a bit easier to read, but I am not sure if it's desired for Compact themes.

The reporter purposed making the menu bar / tabs toolbar fully black, but that seems to make the tabs kind of disappear in my opinion.  It is confusing to me that we do essentially this in maximized mode though...

Gijs, any thoughts?  I am not sure if there is a change that would be clearly better here.

Comment 5

[:Gijs (out for now; he/him)]

(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #4)
> I've added screenshots of Nightly 54 with Windows 7 Aero theme configured
> with menu bar enabled and widgets next to tabs like the reporter.
> 
> The menu bar / widgets seem more readable in my own Compact Light screenshot
> than the original reporter's, but if you had a really light background, the
> contrast would definitely go down.  Comparing it with the Default theme, it
> seems the Default theme uses a stronger glass / fog effect on the window
> plus black text which is a bit easier to read, but I am not sure if it's
> desired for Compact themes.

Yeah... if you use compact light, the text is black and that's really not a lot better when the glass is over a dark area (like in your screenshot) than the screenshots from the reporter of glass-over-white-area . So I think fog is likely the way to go here to get a consistent background for text. I assume we're disabling the fog somewhere, I bet just getting rid of that rule would help. Plus making sure that tabstrip text outside of the tabs is dark (not sure if that's going to be tricky or not).

> The reporter purposed making the menu bar / tabs toolbar fully black, but
> that seems to make the tabs kind of disappear in my opinion.  It is
> confusing to me that we do essentially this in maximized mode though...

This would be another option but I think the reason we don't do it right now is that it looks ugly in normal mode on anything before win10 because we can't make *everything* dark/white. So the window edges would still be aero-ish. But I'm not sure. Stephen would hopefully know...

Comment 6

[Stephen Horlander [:shorlander]]

(In reply to :Gijs from comment #5)
> > The reporter purposed making the menu bar / tabs toolbar fully black, but
> > that seems to make the tabs kind of disappear in my opinion.  It is
> > confusing to me that we do essentially this in maximized mode though...
> 
> This would be another option but I think the reason we don't do it right now
> is that it looks ugly in normal mode on anything before win10 because we
> can't make *everything* dark/white. So the window edges would still be
> aero-ish. But I'm not sure. Stephen would hopefully know...

Yeah we tried that initially but because of the constraints on where we can draw it mostly just ended up looking weird.

You could try adding a shadow to text on items outside of the tab strip similar to the window title or the default menu bar.

Comment 7

[Liz Henry (:lizzard) (relman/hg->git project)]

Tracking this since this was mentioned as a p2 issue for compact themes, which we plan to ship in 53. 
Gijs, is this something you'd be willing to take on, or do you know who might work on it?

Comment 8

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

I can do the work here, thought it seems like some fiddling will be need, since it's not obvious what would be best.

Comment 9

[:Gijs (out for now; he/him)]

(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #8)
> I can do the work here, thought it seems like some fiddling will be need,
> since it's not obvious what would be best.

Yeah... I would still recommend trying to not disable the fog which we use in the default theme to solve this problem. Not sure where we're removing the fog right now, and if we're just not using the fog for lwthemes or something (on the assumption that those will have an opaque background, which the compact theme doesn't). It won't look amazing, but it will be usable, so that's a good start...

Comment 10

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

Comment on attachment 8835150 [details]
Dark Theme - Win 7 Aero - Menu Bar.png

I guess I confused things a bit here, this screenshot is from the Compact Dark theme.

Comment 11

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

Attachment: Light Theme - Win 7 Aero - Menu Bar.png

Here's Compact Light for real this time.

Comment 12

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=d3751cdaca5c465ae0d8b9882b51f42335e1e0a1

Comment 13

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

Attachment: Attempt 1 - Dark Theme.png

Comment 14

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

Attachment: Attempt 1 - Dark Theme.png

Comment 15

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

Attachment: Attempt 1 - Light Theme.png

Comment 16

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

For a first attempt, I've made the following changes:

* Compact themes now apply the same glass fog effect as the Default theme
* Main menubar uses the rounded background from Default theme when the window is focused
* Main menubar always uses Compact Light's font color (#18191a) for both Light and Dark themes (since they are both using the same background color)
* No explicit changes to widgets in the tab toolbar, but they do get the fog effect behind them

Compact Dark screenshot: attachment 8840060 [details]
Compact Light screenshot: attachment 8840062 [details]

Comment 17

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

Attachment: Bug 1208616 - Improve Compact Theme toolbars with Win 7 Glass.

* Compact themes now apply the same glass fog effect as the Default theme
* Main menu bar uses the rounded background from Default theme when the window
  is focused
* Main menu bar always uses Compact Light's font color (#18191a) for both Light
  and Dark themes (since they are both using the same background color)
* No explicit changes to widgets in the tab toolbar, but they do get the fog
  effect behind them

Review commit: https://reviewboard.mozilla.org/r/114606/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/114606/

Comment 18

[:Gijs (out for now; he/him)]

Comment on attachment 8840065 [details]
Bug 1208616 - Improve Compact Theme toolbars with Win 7 Glass.

https://reviewboard.mozilla.org/r/114606/#review116654

I'm away today, but I'll look at this in more detail on Monday. From a cursory glance at the screenshots I did notice the text color in the tabstrip for the dark theme was still white with the fog being white, too, which didn't seem like it worked well.

Out of curiosity, would using dark fog for the dark theme work?

::: browser/themes/windows/compacttheme.css:82
(Diff revision 1)
> +
> +  /* When glass is used, always use the Compact Light font color (for both Light
> +     and Dark themes) for the main menu bar, which appears on top of the same
> +     rounded background from the Default theme when the window is active. */
> +  #main-menubar {
> +    color: #18191a;

From the screenshots it feels like we should do this for non-tab-items in the tabstrip as well?

Comment 19

[:Gijs (out for now; he/him)]

Comment on attachment 8840065 [details]
Bug 1208616 - Improve Compact Theme toolbars with Win 7 Glass.

https://reviewboard.mozilla.org/r/114606/#review116656

Sorry, meant to leave this at r?.

Comment 20

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

Comment on attachment 8840065 [details]
Bug 1208616 - Improve Compact Theme toolbars with Win 7 Glass.

https://reviewboard.mozilla.org/r/114606/#review116654

Yes, I think dark fog does help!  It seems like it makes the background on window active part unnecessary too.  I'll post a new attempt with these changes.

> From the screenshots it feels like we should do this for non-tab-items in the tabstrip as well?

I considered doing this, but since there isn't a single container for all these items currently, it means each widget gets its own rounded background, which looks a bit silly to me.

I'll post a new attempt with the dark fog you suggested.  In my opinion, we don't need this background portion anymore with the dark fog.

Comment 21

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

Comment on attachment 8840065 [details]
Bug 1208616 - Improve Compact Theme toolbars with Win 7 Glass.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/114606/diff/1-2/

Comment 22

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

Comment on attachment 8840065 [details]
Bug 1208616 - Improve Compact Theme toolbars with Win 7 Glass.

https://reviewboard.mozilla.org/r/114606/#review116870

Comment 23

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

Attachment: Attempt 2 - Dark Theme.png

Comment 24

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

Attachment: Attempt 2 - Light Theme.png

Comment 25

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

Okay, for attempt 2, I've changed to a dark fog for Compact Dark and the menu bar background has been removed from both.

* Compact themes now apply the same glass fog effect as the Default theme
* Compact Light theme uses same fog color as Default theme
* Compact Dark theme uses the inverse fog color
* No explicit changes to widgets in the tab toolbar, but they do get the fog effect behind them

Compact Dark screenshot: attachment 8841085 [details]
Compact Light screenshot: attachment 8841086 [details]

Comment 26

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=d666f2330af77e7daafafaac0faa68e92a9b36ac

Comment 27

[:Gijs (out for now; he/him)]

Comment on attachment 8840065 [details]
Bug 1208616 - Improve Compact Theme toolbars with Win 7 Glass.

Stephen, can you check this looks OK to you?

Comment 28

[:Gijs (out for now; he/him)]

Comment on attachment 8840065 [details]
Bug 1208616 - Improve Compact Theme toolbars with Win 7 Glass.

https://reviewboard.mozilla.org/r/114606/#review117066

I'm assuming the code is effectively copied over, so rs=me for that. The screenshots look OK to me, let's check with Stephen if he agrees. :-)

Comment 29

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

Stephen, please see the ui-review request (comment 27).  This is the last Compact Theme P2 bug on hand, so it would be nice to wrap this up, assuming it looks good to you.  Thanks!

Comment 30

[Stephen Horlander [:shorlander]]

(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #29)
> Stephen, please see the ui-review request (comment 27).  This is the last
> Compact Theme P2 bug on hand, so it would be nice to wrap this up, assuming
> it looks good to you.  Thanks!

Hi, question: Does this add the fog all the time even if you don't have menus shown or textual items in the tab strip?

Comment 31

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

(In reply to Stephen Horlander [:shorlander] from comment #30)
> (In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #29)
> > Stephen, please see the ui-review request (comment 27).  This is the last
> > Compact Theme P2 bug on hand, so it would be nice to wrap this up, assuming
> > it looks good to you.  Thanks!
> 
> Hi, question: Does this add the fog all the time even if you don't have
> menus shown or textual items in the tab strip?

Yes, with the current changes, the fog would always be enabled for Compact Theme.

Is that what you are hoping for, or did you want it to appear only when those UI elements are used?

Comment 32

[Stephen Horlander [:shorlander]]

(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #31)
> (In reply to Stephen Horlander [:shorlander] from comment #30)
> > (In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #29)
> > > Stephen, please see the ui-review request (comment 27).  This is the last
> > > Compact Theme P2 bug on hand, so it would be nice to wrap this up, assuming
> > > it looks good to you.  Thanks!
> > 
> > Hi, question: Does this add the fog all the time even if you don't have
> > menus shown or textual items in the tab strip?
> 
> Yes, with the current changes, the fog would always be enabled for Compact
> Theme.
> 
> Is that what you are hoping for, or did you want it to appear only when
> those UI elements are used?

I don't think we should go with the fog approach. It was designed for Australis and doesn't really work that well with the flatter Dev Tools design.

I think we should keep the background on the menu bar, add a dark variant and then use text-shadow for items that could potentially be placed on the tab strip so they remain readable in more situations.

e.g. https://cl.ly/273a253y1o0r

Comment 33

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

Attachment: Attempt 3 - Light Theme.png

Comment 34

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

Attachment: Attempt 3 - Light Theme (Inactive).png

Comment 35

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

Attachment: Attempt 3 - Dark Theme.png

Comment 36

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

Attachment: Attempt 3 - Dark Theme (Inactive).png

Comment 37

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

Comment on attachment 8840065 [details]
Bug 1208616 - Improve Compact Theme toolbars with Win 7 Glass.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/114606/diff/2-3/

Comment 38

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

Comment on attachment 8840065 [details]
Bug 1208616 - Improve Compact Theme toolbars with Win 7 Glass.

Here's attempt 3, which tries to implement :shorlander's proposal in comment 32.

* Background added to menu bar when window is active
* Text shadow added to widgets placed next to tabs on the glass

Compact Light: attachment 8849229 [details]
Compact Light (inactive): attachment 8849231 [details]
Compact Dark: attachment 8849234 [details]
Compact Dark (inactive): attachment 8849235 [details]

Comment 39

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=cd8c5f14815b3b8c9d340e730b5d49a92d27f8f3

Comment 40

[:Gijs (out for now; he/him)]

(I'm going to delay reviewing until Stephen gives you ui-r+ :-) )

Comment 41

[:Gijs (out for now; he/him)]

Comment on attachment 8840065 [details]
Bug 1208616 - Improve Compact Theme toolbars with Win 7 Glass.

https://reviewboard.mozilla.org/r/114606/#review124666

Based on the screenshot I would expect we'd want to use the inverted (ie white) icons on the menubar/tabstrip for the light theme on restored windows. The existing icons aren't really visible at all. :-(
The inverted icons also having black borders should ensure that they're visible even on bright background behind the glass, I think?

I'm also wondering if this code needs to be specific to restored windows? The selectors don't seem to care about this right now... Plus I'm wondering if we're sure the -moz-windows-glass media query doesn't apply on Win8/Win10.

Comment 42

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

(In reply to :Gijs from comment #41)
> Based on the screenshot I would expect we'd want to use the inverted (ie
> white) icons on the menubar/tabstrip for the light theme on restored
> windows. The existing icons aren't really visible at all. :-(
> The inverted icons also having black borders should ensure that they're
> visible even on bright background behind the glass, I think?

What are "restored windows" here?  Is that like active vs. inactive / focused vs. unfocused?  Or something about session restore...?

> I'm also wondering if this code needs to be specific to restored windows?
> The selectors don't seem to care about this right now... Plus I'm wondering
> if we're sure the -moz-windows-glass media query doesn't apply on Win8/Win10.

Looks like -moz-windows-glass means "Glass and not Win 8 or later"[1], so it seems like it's filtering to Win 7 Glass only appropriately.

[1]: http://searchfox.org/mozilla-central/rev/0079c7adf3b329bff579d3bbe6ac7ba2f6218a19/widget/windows/nsLookAndFeel.cpp#417-420

Comment 43

[:Gijs (out for now; he/him)]

(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #42)
> (In reply to :Gijs from comment #41)
> > Based on the screenshot I would expect we'd want to use the inverted (ie
> > white) icons on the menubar/tabstrip for the light theme on restored
> > windows. The existing icons aren't really visible at all. :-(
> > The inverted icons also having black borders should ensure that they're
> > visible even on bright background behind the glass, I think?
> 
> What are "restored windows" here?  Is that like active vs. inactive /
> focused vs. unfocused?  Or something about session restore...?

"restored" is a window mode on Windows, also known (even more confusingly, IMO) as "normal". As opposed to maximized, minimized or fullscreen. In other words, what I'm saying is that maybe we don't want these styles to apply for maximized windows, because IIRC we do different things with the background color of the toolbox in maximized mode (and maybe fullscreen mode, too).

Comment 44

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

I haven't been able to get back to this work with other high priority bugs in the mix.  :canuckistani, how critical is it for this to be resolved in 53 (the first release with Compact Themes)?

Comment 45

[Jeff Griffiths (:canuckistani) (:⚡︎)]

It's not critical. P1 bugs block, P2 don't.

Comment 46

[Liz Henry (:lizzard) (relman/hg->git project)]

Wontfix for 53, probably fix-optional for 54 though do request uplift if you come up with something good here.