879590 - Windows classic menubar text is hard to read (black on dark-blue)

Status: RESOLVED FIXED

(Firefox :: Theme, defect)

Description

[Justin Dolske [:Dolske]]

(Filed from Australis tabs ui-review #1)

Windows Classic - menubar items are really hard to read because the text is black on dark blue.

http://i.imgur.com/aZ6bp4O.png

Comment 1

[:Gijs (he/him)]

Attachment: Tentative patch

This is a first attempt. Essentially, this uses title bar colors for the menu, as it has the titlebar as its background. For light-weight themes, I added custom colors. This also fixes the inactive window state (bug 879598).

I'll upload screenshots in a bit. There is one issue that remains, which is to have a proper window border in restored mode when using a light-weight-theme. Right now the top border is missing (probably because there's no chromemargin towards the top? Dao, do you know if I can add a magical border-top to window to fix it? I tried 2px outset ButtonFace, but it doesn't look quite right... any idea what the right thing is here? Should we be changing the chrome margin only for this case?

Comment 2

[:Gijs (he/him)]

(In reply to :Gijs Kruitbosch from comment #1)
> Created attachment 759873 [details] [diff] [review]
> Tentative patch
> 
> This is a first attempt. Essentially, this uses title bar colors for the
> menu, as it has the titlebar as its background. For light-weight themes, I
> added custom colors. This also fixes the inactive window state (bug 879598).


Found at least one issue already: light-weight themes still use the default highlight colors for _moz-menuactive. This could potentially make the menu unreadable because I've hardcoded the color values. OTOH, there's really no predicting the color values of the theme compared to those of the light-weight theme... 

Personally, for this case, I would not use the highlighted backgrounds but only use the border, which is what we currently do for non-lwtheme windows classic (on Windows 7) anyway. Alternatively, we could use the default windows classic -moz-dialog/-moz-dialogtext for _moz-menuactive.

Dao, can you advise?

Comment 3

[:Gijs (he/him)]

Attachment: Before

Comment 4

[:Gijs (he/him)]

Attachment: After

NB: note that we no longer differ in what we do when the menubar is permanently enabled or when it's temporarily shown (by pressing alt), which we did do on beta (which is where the screenshots are from). So there's just one screenshot for maximized/restored x active/inactive for the "normal" case, and then an active/inactive screenshot for the lwtheme case.

Comment 5

[:Gijs (he/him)]

Attachment: Patch v1.1

This is actually much better; There's no need for !important, and then leaving the lwtheme case alone is just fine. If we want to fix something there, we can do so in another bug. As it is, the 'after' screenshots look just the same, but without having an unreadable menu problem when you actually hover/select one of the menus. Sorry for bugspam.

Comment 6

[Matthew N. [:MattN]]

Comment on attachment 759896 [details] [diff] [review]
Patch v1.1

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

This problem isn't really Classic-specific. Can you fix this way for luna as well by moving http://hg.mozilla.org/projects/ux/file/74cfd0e16ccf/browser/themes/windows/browser.css#l1535 (as it's not specific to luna-blue) and having it use CaptionText?

Also note it seems like you will also end up fixing bug 879598 here.

Comment 7

[:Gijs (he/him)]

(In reply to Matthew N. [:MattN] from comment #6)
> Comment on attachment 759896 [details] [diff] [review]
> Patch v1.1
> 
> Review of attachment 759896 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This problem isn't really Classic-specific. Can you fix this way for luna as
> well by moving
> http://hg.mozilla.org/projects/ux/file/74cfd0e16ccf/browser/themes/windows/
> browser.css#l1535 (as it's not specific to luna-blue) and having it use
> CaptionText?

Do we want CaptionText on glass as well? If not, under what exact circumstances do we want it - just classic mode and luna/olive/silver? That selector also uses [drawintitlebar] which I believe is always true these days - or do we still support turning that off?

You're technically widening the scope of this bug considerably, but fine... :P

> 
> Also note it seems like you will also end up fixing bug 879598 here.

Yes, I did note that in comment #1 :-)

Comment 8

[Dão Gottwald [:dao]]

(In reply to :Gijs Kruitbosch from comment #7)
> Do we want CaptionText on glass as well?

Yes.

> selector also uses [drawintitlebar] which I believe is always true these
> days - or do we still support turning that off?

s/drawintitlebar/tabsintitlebar/ and no, it's not always true.

Comment 10

[:Gijs (he/him)]

Attachment: Patch v1.2

Dao, I believe this matches what we discussed yesterday, right?

I also noticed that besides bug 879598, this also seems to improve the situation for bug 863862.

Comment 11

[:Gijs (he/him)]

Comment on attachment 761931 [details] [diff] [review]
Patch v1.2

Sigh, qnew would have helped...

Comment 12

[:Gijs (he/him)]

Attachment: Patch v1.2

Correct patch this time. :-)

Comment 13

[Dão Gottwald [:dao]]

Comment on attachment 761933 [details] [diff] [review]
Patch v1.2

How does this interact with <http://hg.mozilla.org/mozilla-central/annotate/68760713a30f/toolkit/themes/windows/global/menu.css#l120>?

Comment 14

[:Gijs (he/him)]

(In reply to Dão Gottwald [:dao] from comment #13)
> Comment on attachment 761933 [details] [diff] [review]
> Patch v1.2
> 
> How does this interact with
> <http://hg.mozilla.org/mozilla-central/annotate/68760713a30f/toolkit/themes/
> windows/global/menu.css#l120>?

It overrides it. Because we also style the selected topmenu otherwise (3D-ish borders) you can still easily tell which menu is selected, and I'm not sure what else we want to do. Avoiding the override results in the same illegibility that this bug is trying to fix (it looks like it's rgb(0,0,0) on classic on Win7). We'd need to come up with another color entirely, but the Windows Theme doesn't have a "selected CaptionText" color, I'd imagine. :-\

The other option would be also overriding the menu's background color in the selected case. I suspect that won't look good, but if you like I can give that a whirl.

Comment 15

[Dão Gottwald [:dao]]

(In reply to :Gijs Kruitbosch from comment #14)
> (In reply to Dão Gottwald [:dao] from comment #13)
> > Comment on attachment 761933 [details] [diff] [review]
> > Patch v1.2
> > 
> > How does this interact with
> > <http://hg.mozilla.org/mozilla-central/annotate/68760713a30f/toolkit/themes/
> > windows/global/menu.css#l120>?
> 
> It overrides it. Because we also style the selected topmenu otherwise
> (3D-ish borders)

That depends on the OS theme. Have you tested your patch with Windows XP's Luna flavors?

Comment 16

[:Gijs (he/him)]

(In reply to Dão Gottwald [:dao] from comment #15)
> (In reply to :Gijs Kruitbosch from comment #14)
> > (In reply to Dão Gottwald [:dao] from comment #13)
> > > Comment on attachment 761933 [details] [diff] [review]
> > > Patch v1.2
> > > 
> > > How does this interact with
> > > <http://hg.mozilla.org/mozilla-central/annotate/68760713a30f/toolkit/themes/
> > > windows/global/menu.css#l120>?
> > 
> > It overrides it. Because we also style the selected topmenu otherwise
> > (3D-ish borders)
> 
> That depends on the OS theme. Have you tested your patch with Windows XP's
> Luna flavors?

I didn't before now, because VS2012 doesn't produce XP-compatible executables. I just realized that for this change, all I needed to do was copy omni.ja across. So I checked.

On Luna, the only way to tell that a menu is selected (both with and without the patch, because there's a hardcoded color:white for luna right now) is a subtly different flat background color (different shade of blue).

On Olive, the difference is more noticeable, but still, similar shade of olive-ish green background is all the info you get.

On Silver, the difference is just as badly noticeable as on Luna.

XP Windows classic seems to have 3D borders like Classic on Windows 7.

I'm not sure what to do about this problem. The patch doesn't make things worse on Luna (which already had this behaviour), but I guess it does slightly regress Olive and Silver behaviour. I suspect manually adding 3D borders won't look so great against the sort of 3D bump that modern XP themes have at the top of the window anyway. The default highlight background color is clearly visually similar to the window's background color, so there's not enough contrast there.

Comment 17

[:Gijs (he/him)]

Pushed: https://hg.mozilla.org/projects/ux/rev/06d4cc4f4377

Comment 18

[:Gijs (he/him)]

I filed bug 882623 for the issue of the selected menuitem on XP modern.

Comment 19

[:Gijs (he/him)]

https://hg.mozilla.org/mozilla-central/rev/06d4cc4f4377