Closed Bug 814041 Opened 7 years ago Closed 7 years ago

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

Categories

(Toolkit :: Themes, defect)

x86
Windows XP
defect
Not set

Tracking

()

VERIFIED FIXED
mozilla22

People

(Reporter: aceman, Assigned: Paenglab)

References

Details

(Keywords: polish)

Attachments

(3 files, 2 obsolete files)

Attached image 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;
}
Win7 and OSX are looking good. It seems it's XP only. Later I can have a look what's wrong.
(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).
(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.
(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!
Paenglab, can you look at the problems?
Yes I can try it.
Component: Theme → Themes
Product: Thunderbird → Toolkit
Attached patch patch (obsolete) — Splinter Review
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.
Assignee: nobody → richard.marti
Status: NEW → ASSIGNED
Attachment #690071 - Flags: review?(dao)
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...
Attached image Menulist DOM
(In reply to neil@parkwaycc.co.uk 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");
}
This now also affects the folder picker in the toolbar widget, after checkin of bug 813218.

Paenglab, can you finish this?
Blocks: 813218
Flags: needinfo?(richard.marti)
(In reply to :aceman from comment #10)
> Paenglab, can you finish this?

It's finished. The patch is awaiting Dao's review.
Flags: needinfo?(richard.marti)
Dão,

if you have some spare time, please could you review this patch?
Attached patch patch unbitrotted (obsolete) — Splinter Review
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.
Attachment #690071 - Attachment is obsolete: true
Attachment #690071 - Flags: review?(dao)
Attachment #720262 - Flags: review?(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 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?
(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.
(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 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. :/
Attachment #720262 - Flags: review?(dolske) → review?(fyan)
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.
Attachment #720262 - Flags: review?(fyan) → review+
Keywords: checkin-needed
Modified commit message to clarify what it fixes.

Thanks for the patch, Richard. :)
Attachment #720262 - Attachment is obsolete: true
Attachment #727665 - Flags: review+
Summary: folder pickers have wrong padding and active text color → Fix menulist (folder picker) item padding and active text color on Windows XP
Blocks: 853431
(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.
https://hg.mozilla.org/mozilla-central/rev/753be4a4cf9f
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla22
Thanks, looks great, padding and text color.
Status: RESOLVED → VERIFIED
Blocks: 1023277
You need to log in before you can comment on or make changes to this bug.