814041 - Fix menulist (folder picker) item padding and active text color on Windows XP

Status: VERIFIED FIXED

(Toolkit :: Themes, defect)

Description

[:aceman]

Attachment: screenshot

I noticed this after landing bug 808974 in Win XP, but it may not be related.
The folder pickers have wrong padding on the first level of the menulist when the element is menuitem (not menu). The text is tacked to the icon on the left (see the yellow folder icons). There is a 0px margin-left on the text.

On the first level the highlighted item still has black text if hovering over a <menu>. It should be white as on a listitem. However, on second level, all items have black text when active.

This is in the filter dialog where the class is menulist-menupopup.

In the "new folder:" dialog the highlighted text is black even on the first level of the menu (it also has menulist-menupopup class).

Can anybody check if this is Win XP problem only? I did not notice it on Linux. Can anybody try Aero and Mac?

This theme is coming from the toolkit. Is there a problem in it or do we use something wrongly in Thunderbird?

E.g. the 0px margin is coming from menu.css and is this code:
menulist > menupopup > menuitem > .menu-iconic-text {
  margin: 0 !important;
}

Notice this only breaks the first level <menuitem> elements. Is this line needed? Because normally an item gets 2px margin:
.menu-iconic-text {
  font-weight: inherit;
  -moz-margin-start: 2px !important;
  -moz-padding-end: 2px;
}

Comment 1

[Richard Marti (:Paenglab)]

Win7 and OSX are looking good. It seems it's XP only. Later I can have a look what's wrong.

Comment 2

[[e-mail]]

(In reply to aceman from comment #0)
> Can anybody check if this is Win XP problem only? I did not notice it on
> Linux. Can anybody try Aero and Mac?
I can believe that Linux does not have the problem. I looked at the CSS and winstripe has this odd line:
menulist:-moz-focusring > menupopup > menuitem[_moz-menuactive="true"] {
This overrides the active menuitem colours but only in active menulists (exactly how you are supposed to active a menuitem in an inactive menulist is unclear), so it fails to deal with our nested menulists. (It has to do this because Windows uses different colours for active items in menus and menulists.)

> This theme is coming from the toolkit. Is there a problem in it or do we use
> something wrongly in Thunderbird?
I'd like to blame toolkit.

> E.g. the 0px margin is coming from menu.css and is this code:
> menulist > menupopup > menuitem > .menu-iconic-text {
>   margin: 0 !important;
> }
> 
> Notice this only breaks the first level <menuitem> elements. Is this line needed?
For normal text menulists, yes it is needed. If you want consistency, I would probably be happy with adding an extra selector for .menulist-menupopup (compare a number of other examples in the same file).

Comment 3

[Richard Marti (:Paenglab)]

(In reply to :aceman from comment #0)

> E.g. the 0px margin is coming from menu.css and is this code:
> menulist > menupopup > menuitem > .menu-iconic-text {
>   margin: 0 !important;
> }
> 
> Notice this only breaks the first level <menuitem> elements. Is this line
> needed?

This line is needed for normal menulists without icons. AFAIK FF isn't using icons in a menulist. Only TB and SM are using icons.

Change the line to:
menulist > menupopup > menuitem:not(.menuitem-iconic) > .menu-iconic-text {

would still work for normal menulists but not affect our menulists with icons.

Comment 4

[[e-mail]]

(In reply to Richard Marti from comment #3)
> AFAIK FF isn't using icons in a menulist.

Actually the Applications pane uses them. They hack around the problem by manually adding extra left padding on their menuitems. So maybe you can make that unnecessary too!

Comment 5

[:aceman]

Paenglab, can you look at the problems?

Comment 6

[Richard Marti (:Paenglab)]

Yes I can try it.

Comment 7

[Richard Marti (:Paenglab)]

Attachment: patch

The Applications pane (also TB's Attachments/Incoming pane) doesn't use the menuitem-iconic class, so my patch doesn't change anything there.

I had to add some selectors for [_moz-menuactive="true"] to be stronger than the rule in line 178.

Dão, if you want I can also do a separate rules block for this menulist...[_moz-menuactive="true"] rules in /* ::::: menu/menuitems in menulist popups ::::: */ block.

Comment 8

[[e-mail]]

Comment on attachment 690071 [details] [diff] [review]
patch

>-menulist > menupopup > menuitem > .menu-iconic-text {
>+menulist > menupopup > menuitem:not(.menuitem-iconic) > .menu-iconic-text {
Ordinary menuitems don't have menu-iconic-text...

Comment 9

[Richard Marti (:Paenglab)]

Attachment: Menulist DOM

(In reply to [e-mail] from comment #8)
> Comment on attachment 690071 [details] [diff] [review]
> patch
> 
> >-menulist > menupopup > menuitem > .menu-iconic-text {
> >+menulist > menupopup > menuitem:not(.menuitem-iconic) > .menu-iconic-text {
> Ordinary menuitems don't have menu-iconic-text...

This screenshot shows the menuitem without class menuitem-iconic but with menu-iconic-text.

Screenshot is from Options > Applications pane.

In chrome://global/content/xul.css line 830 there is this binding:
menulist > menupopup > menuitem {
  -moz-binding: url("chrome://global/content/bindings/menu.xml#menuitem-iconic-noaccel");
}

Comment 10

[:aceman]

This now also affects the folder picker in the toolbar widget, after checkin of bug 813218.

Paenglab, can you finish this?

Comment 11

[Richard Marti (:Paenglab)]

(In reply to :aceman from comment #10)
> Paenglab, can you finish this?

It's finished. The patch is awaiting Dao's review.

Comment 12

[Richard Marti (:Paenglab)]

Dão,

if you have some spare time, please could you review this patch?

Comment 13

[Richard Marti (:Paenglab)]

Attachment: patch unbitrotted

Patch adapted to new theme names.

I'm changing the reviewer in the hope I haven't to wait again almost 3 month without any feedback.

Comment 14

[Justin Dolske [:Dolske]]

Hmm. -moz-menuhovertext is black on Win7, whereas I'd expect it to be white from how other apps act. Is the code in widget/windows/nsLookAndFeel.cpp even doing the right thing here?

Comment 15

[Justin Dolske [:Dolske]]

Comment on attachment 720262 [details] [diff] [review]
patch unbitrotted

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

::: toolkit/themes/windows/global/menu.css
@@ +170,3 @@
>  menuitem[_moz-menuactive="true"],
> +.menulist-menupopup > menuitem[_moz-menuactive="true"],
> +menulist > menupopup > menuitem[_moz-menuactive="true"],

Why are the existing rules here insufficient?

Comment 16

[Richard Marti (:Paenglab)]

(In reply to Justin Dolske [:Dolske] from comment #14)
> Hmm. -moz-menuhovertext is black on Win7, whereas I'd expect it to be white
> from how other apps act. Is the code in widget/windows/nsLookAndFeel.cpp
> even doing the right thing here?

The nsLookAndFeel.cpp is correct but because the rule in http://hg.mozilla.org/mozilla-central/file/015da7030aab/toolkit/themes/windows/global/menu.css#l176 with color: -moz-FieldText; overrides the [_moz-menuactive="true"] rule in line 167.

(In reply to Justin Dolske [:Dolske] from comment #15)
> Comment on attachment 720262 [details] [diff] [review]
> patch unbitrotted
> 
> Review of attachment 720262 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: toolkit/themes/windows/global/menu.css
> @@ +170,3 @@
> >  menuitem[_moz-menuactive="true"],
> > +.menulist-menupopup > menuitem[_moz-menuactive="true"],
> > +menulist > menupopup > menuitem[_moz-menuactive="true"],
> 
> Why are the existing rules here insufficient?

As written above, because menulist > menupopup > menuitem from line 176 is more specific than menuitem[_moz-menuactive="true"] from line 167 and overrides the color definition also in _moz-menuactive="true" state.

Comment 17

[Justin Dolske [:Dolske]]

(In reply to Richard Marti [:Paenglab] from comment #16)
> (In reply to Justin Dolske [:Dolske] from comment #14)
> > Hmm. -moz-menuhovertext is black on Win7, whereas I'd expect it to be white
> > from how other apps act. Is the code in widget/windows/nsLookAndFeel.cpp
> > even doing the right thing here?
> 
> The nsLookAndFeel.cpp is correct but because the rule in
> http://hg.mozilla.org/mozilla-central/file/015da7030aab/toolkit/themes/
> windows/global/menu.css#l176 with color: -moz-FieldText; overrides the
> [_moz-menuactive="true"] rule in line 167.

Except that <div style="color: -moz-menuhovertext">FOO</div> is also black.

Comment 18

[Justin Dolske [:Dolske]]

Comment on attachment 720262 [details] [diff] [review]
patch unbitrotted

The more I look at menu.css the less I understand what the hell it's doing. Maybe Frank will have better luck. :/

Comment 19

[Frank Yan (:fryn)]

Comment on attachment 720262 [details] [diff] [review]
patch unbitrotted

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

(In reply to Richard Marti [:Paenglab] from comment #16)
> (In reply to Justin Dolske [:Dolske] from comment #14)
> > Hmm. -moz-menuhovertext is black on Win7, whereas I'd expect it to be white
> > from how other apps act. Is the code in widget/windows/nsLookAndFeel.cpp
> > even doing the right thing here?
-moz-menuhovertext being black on Windows 7 is correct for regular menus. See the system context menu hover text color, and compare to our text color when hovering over menu items in the content area context menu, the Firefox button menu, and the traditional menu bar menus.
However, menulists on Windows 7 have white hover text, but since we reuse -moz-menuhovertext for menulists, they end up being black in Firefox. It seems that we made the common mistake of conflating two different widgets and oversharing code between them. (See also menupopups vs arrow panels.)
Therefore, no, we're not really doing the right thing here on Windows 7, and this patch doesn't fix the Windows 7 problem.
Once again, our widget code is optimized for Windows XP classic and assumes that later OS versions' widget theming will not diverge.

`menulist > menupopup > menuitem` overrides `menuitem[_moz-menuactive="true"]`, because attribute selectors and class selectors have the same class of specificity, so the former selector has specificity 0-3-0, whereas the latter has 0-2-0.

> Except that <div style="color: -moz-menuhovertext">FOO</div> is also black.
On Windows 7, yes.

> The more I look at menu.css the less I understand what the hell it's doing.
> Maybe Frank will have better luck. :/

Yeah, I understand the file and the patch.
r+, because it solves the Windows XP problem, which is not really noticeable on Windows 7, because -moz-menuhovertext is black on Windows 7 with OS default settings.
It'd be nice to solve the Windows 7 menu hover text color and menulist hover text color divergence problem, but that is a less trivial problem for a separate bug.

Comment 20

[Frank Yan (:fryn)]

Attachment: patch for checkin

Modified commit message to clarify what it fixes.

Thanks for the patch, Richard. :)

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/753be4a4cf9f

Comment 22

[Frank Yan (:fryn)]

(In reply to Frank Yan (:fryn) from comment #19)
> `menulist > menupopup > menuitem` overrides
> `menuitem[_moz-menuactive="true"]`, because attribute selectors and class
> selectors have the same class of specificity, so the former selector has
> specificity 0-3-0, whereas the latter has 0-2-0.

This part was nonsensical. Ignore that; sorry.
That's what I get for reviewing patches at 5am.

Comment 23

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/753be4a4cf9f

Comment 24

[:aceman]

Thanks, looks great, padding and text color.