853431 - Fix menulist active (hover) text color on Windows 7 and up

Status: RESOLVED FIXED

(Toolkit :: Themes, defect)

Description

[Frank Yan (:fryn)]

When hovering the cursor over an item in a menulist (for example, the Actions column items of the Applications tab of Firefox preferences), the menulist item text color should be white, but it is black.

This is because -moz-menuhovertext is black on Windows 7 and up, and we use -moz-menuhovertext for both regular menus and menulists, probably because they had very similar theming in the Windows 95/XP Classic days. Since then, the theming for the two types of widgets has diverged: regular menus use black text even on hover, whereas menulists still use white text on hover.

Since we probably want to keep -moz-menuhovertext for regular menus, since they are much more common, we should switch to something different from -moz-menuhovertext for menulists, at least on Windows 7 and up.

(I haven't tested on Windows Vista, as I don't particularly care about it.)

Comment 1

[[e-mail]]

Don't hovered menulist items use Highlight and HighlightText as their colours?

Comment 2

[Frank Yan (:fryn)]

(In reply to [e-mail] from comment #1)
> Don't hovered menulist items use Highlight and HighlightText as their
> colours?

Only in the menulist:-moz-focus-ring state, which only takes effect when the menulist is closed, I think.

menu.css is rather difficult to understand. :/

Comment 3

[Richard Marti (:Paenglab)]

In Bug 658328 I made this changes already for TB. I could do this directly in menu.css

Comment 4

[Richard Marti (:Paenglab)]

Attachment: proposed fix

This patch sets for menulists the background-color: highlight; and color: highlighttext; for all Windows versions. On XP and Win7 Classic where is no difference to menus. On Win7 Basic, Aero and Win8 the hovered text is now white.

Additionally I removed the top and bottom padding as the native menulist items are a lot more close together on all Win versions.

On Win7 not Classic and Win8 I removed also the highlight when the menulist is focused like the native menulist.

If this needs ui-r, who can I set for it?

Comment 5

[Frank Yan (:fryn)]

Comment on attachment 729171 [details] [diff] [review]
proposed fix

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

Thanks for working on this!

(In reply to Richard Marti [:Paenglab] from comment #4)
> If this needs ui-r, who can I set for it?

I'll do the ui-review after I test this on various platforms. :)

::: toolkit/themes/windows/global/menulist-aero.css
@@ +13,5 @@
> +  .menulist-label-box {
> +    background-color: transparent !important;
> +    color: inherit !important;
> +  }
> +}

Does this block apply to Windows 8?

Comment 6

[Richard Marti (:Paenglab)]

(In reply to Frank Yan (:fryn) from comment #5)
> Comment on attachment 729171 [details] [diff] [review]
> proposed fix
> 
> Review of attachment 729171 [details] [diff] [review]:
> -----------------------------------------------------------------
> ::: toolkit/themes/windows/global/menulist-aero.css
> @@ +13,5 @@
> > +  .menulist-label-box {
> > +    background-color: transparent !important;
> > +    color: inherit !important;
> > +  }
> > +}
> 
> Does this block apply to Windows 8?

Yes, -aero.css does apply to Win 8 like on Win 7.

Comment 7

[Frank Yan (:fryn)]

Comment on attachment 729171 [details] [diff] [review]
proposed fix

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

Code looks good and works well.

Tested on Windows XP, 7, and 8.
Thanks to MattN for helping me to test this on Windows XP and 7.

While this fixes color issues, it doesn't fix the padding issue:
On Windows 7 and up (and probably Vista), our menulist widget (the thing you click to open the menu, not the menu itself) has 3 pixels of extra vertical padding (2px below and 1 above, but that's probably just pixel-snapped centering) that the native menulist does not have. If you're interested, could you file a followup bug and look into that?

Comment 8

[Frank Yan (:fryn)]

Pushed to mozilla-inbound:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c15696ed3d83

Comment 9

[Richard Marti (:Paenglab)]

Filed Bug 856040 for the widget padding issue.

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/c15696ed3d83