595201 - popup notification menuitem highlight is broken (black on blue) in winstripe and pinstripe themes

Status: VERIFIED FIXED

(Toolkit :: Themes, defect)

Description

[Alice0775 White]

Attachment: screenshot

Build Identifier: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0b6pre) Gecko/20100910 Firefox/4.0b6pre ID:20100910041829

The new password doorhanger notification's pulldown menu item is unreadable under Windows XP/Windows 7 Classic style.

Steps to Reproduce:
1. Log in on a any website (newly password).
2. Pull down in the New password doorhanger notification.

Actual Results:  
User is baffled, doesn't know what to do.

Expected Results:  
A notification bar appears.

Comment 1

[Alice0775 White]

Typo in Expected Results: 
s/A notification bar appears./Readable color/

Comment 3

[:Margaret Leibovic]

This is caused by the fact that there is a menupopup parent selector in the toolkit css (http://mxr.mozilla.org/mozilla-central/source/toolkit/themes/winstripe/global/menu.css#197 and http://mxr.mozilla.org/mozilla-central/source/toolkit/themes/pinstripe/global/menu.css#169).

Since gnomestripe doesn't use this parent selector (http://mxr.mozilla.org/mozilla-central/source/toolkit/themes/gnomestripe/global/menu.css#65), I propose we get rid of it in the other themes as well.

(Gavin pointed out that this has caused other problems in the past, like http://mxr.mozilla.org/mozilla-central/source/browser/themes/winstripe/browser/browser.css#166)

Comment 4

[:Margaret Leibovic]

Attachment: patch

Comment 5

[Dão Gottwald [:dao]]

So the problem is that the menupopup is anonymous while the menuitem isn't. We should probably fix that...

Comment 6

[Dão Gottwald [:dao]]

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

>--- a/toolkit/themes/winstripe/global/menu.css
>+++ b/toolkit/themes/winstripe/global/menu.css
>@@ -188,18 +188,18 @@ menubar > menu:-moz-window-inactive {
> 
> /* ::::: menu/menuitems in popups ::::: */
> 
> menupopup > menu,
> menupopup > menuitem {
>   max-width: 42em;
> }
> 
>-menupopup > menu[_moz-menuactive="true"],
>-menupopup > menuitem[_moz-menuactive="true"] {
>+menu[_moz-menuactive="true"],
>+menuitem[_moz-menuactive="true"] {
>   background-color: -moz-menuhover;
>   color: -moz-menuhovertext;
> }

Did you test this with <menu>s on a menubar e.g. with Windows classic?

Comment 7

[:Margaret Leibovic]

(In reply to comment #5)
> So the problem is that the menupopup is anonymous while the menuitem isn't. We
> should probably fix that...

How would we fix that? 

(In reply to comment #6)
> Did you test this with <menu>s on a menubar e.g. with Windows classic?

I only have Windows 7, but the menubar works correctly when I show it.

Comment 8

[:Margaret Leibovic]

(In reply to comment #7)
> (In reply to comment #6)
> > Did you test this with <menu>s on a menubar e.g. with Windows classic?
> 
> I only have Windows 7, but the menubar works correctly when I show it.

I didn't realize I could switch the theme on Windows. It does work with the Windows classic theme.

Comment 9

[Dão Gottwald [:dao]]

(In reply to comment #7)
> (In reply to comment #5)
> > So the problem is that the menupopup is anonymous while the menuitem isn't. We
> > should probably fix that...
> 
> How would we fix that?

We could probably create the popup in script where the menuitems are created.

(In reply to comment #8)
> I didn't realize I could switch the theme on Windows. It does work with the
> Windows classic theme.

Does it look the same as without the patch, though? With Windows classic, menubar items should be beveled on hover without any background or text color change.

Comment 10

[:Gavin Sharp [email: [e-mail]]]

(In reply to comment #5)
> So the problem is that the menupopup is anonymous while the menuitem isn't. We
> should probably fix that...

Why? I don't see any reason to do that if we can fix the styling to not depend on having a menupopup parent. Is there a problem with that?

Comment 11

[Dão Gottwald [:dao]]

The menupopup is a sensible dependency for stuff which you may not to apply to all <menu>s and <menuitem>s. That's why I asked about the Windows classic menubar. I'm not sure if there are other cases.

Comment 12

[:Gavin Sharp [email: [e-mail]]]

I can't think of any reasons why you'd want a menu/menuitems that doesn't highlight, and even if there were some, the presence of a menupopup seems like a weird thing to have it depend on.

Comment 13

[Dão Gottwald [:dao]]

(In reply to comment #12)
> I can't think of any reasons why you'd want a menu/menuitems that doesn't
> highlight

menubar > menu is one of those cases.

Comment 14

[Neil Deakin]

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

I think this needs an answer to Dao's concern.

It looks to me like this would have an effect on menus on menubars, although I haven't tested it.

Comment 15

[:Margaret Leibovic]

(In reply to comment #9)
> (In reply to comment #8)
> > I didn't realize I could switch the theme on Windows. It does work with the
> > Windows classic theme.
> 
> Does it look the same as without the patch, though? With Windows classic,
> menubar items should be beveled on hover without any background or text color
> change.

Testing on Windows XP with the Classic Theme without the patch, menubar items have a background/text color change on hover, so they're not saying what you say they should do.

They behave the same way with the patch applied, so this patch doesn't seem to be breaking anything, but maybe we need to file a bug to fix the menubar highlighting, since apparently it's broken right now (although I noticed some of my other applications have menubar highlighting, so maybe it's not a big problem?).

Comment 16

[Dão Gottwald [:dao]]

You may have to restart Minefield after switching to classic. Over here I get beveled menus without any color change, just like in Explorer.

Comment 17

[:Margaret Leibovic]

(In reply to comment #16)
> You may have to restart Minefield after switching to classic. Over here I get
> beveled menus without any color change, just like in Explorer.

Aha, you are correct. Well, when I restarted with my patch applied, the menus were beveled, not highlighted, on hover, so the patch doesn't seem to break them.

Comment 18

[Dão Gottwald [:dao]]

(In reply to comment #17)
> (In reply to comment #16)
> > You may have to restart Minefield after switching to classic. Over here I get
> > beveled menus without any color change, just like in Explorer.
> 
> Aha, you are correct. Well, when I restarted with my patch applied, the menus
> were beveled, not highlighted, on hover, so the patch doesn't seem to break
> them.

menubar > menu[_moz-menuactive="true"]:not([disabled="true"] in menu.css seems to handle the text color. I wonder what takes care if the background color and whether that works on Windows 2000 just like on Win 7... Maybe the answer is somewhere in nsNativeThemeWin.cpp.

Comment 19

[Justin Dolske [:Dolske]]

(In reply to comment #13)
> (In reply to comment #12)
> > I can't think of any reasons why you'd want a menu/menuitems that doesn't
> > highlight
> 
> menubar > menu is one of those cases.

Are there more? If not, seems like we should have a basic style that apply to app menu/menuitems, and then an override style for cases like this.

Otherwise, seems like the most expedient fix would be to just add selectors for menu/menuitems in a popupnotification?

Comment 20

[:Margaret Leibovic]

Attachment: alternative patch

I tried setting a style in notification.css just for the menuitems in the notification popups, but I couldn't get it to work. I assumed it's because the menuitem is being created dynamically instead of in the xbl, so I just added a class and set the style in browser.css. I don't know if this is the best approach, but it avoids having to change the styles in menu.css.

Comment 21

[Dão Gottwald [:dao]]

Comment on attachment 488581 [details] [diff] [review]
alternative patch

notification.css doesn't work because the menuitems aren't part of the anonymous content, which is also connected to why this bug happens in the first place (comment 5). global.css is your second best option.

Did you try to find an answer to comment 18? We can take the first patch if we know it'll work reliably, but for that we should understand why it would work.

Comment 22

[:Margaret Leibovic]

(In reply to comment #21)
> Did you try to find an answer to comment 18? We can take the first patch if we
> know it'll work reliably, but for that we should understand why it would work.

I didn't find an answer to comment 18. Who would be the best person to ask about that?

Comment 23

[Dão Gottwald [:dao]]

Assuming I didn't miss some CSS taking care of it, the win32 widget peers should be able to help: <https://wiki.mozilla.org/Modules/Core#Widget_-_Windows>. Neil Rashbrook would be my best bet since he knows his way around theme code as well as widget code.

Comment 24

[[e-mail]]

Yeah, that's taken care of in nsNativeThemeWin.cpp - you can see things breaking by adding "* { moz-appearance: none; }" to your userChrome.css (this used to work with Mozilla because there used to be CSS fallback).

Comment 25

[Dão Gottwald [:dao]]

(In reply to comment #18)
> menubar > menu[_moz-menuactive="true"]:not([disabled="true"] in menu.css seems
> to handle the text color.

Comment 26

[Dão Gottwald [:dao]]

http://hg.mozilla.org/mozilla-central/rev/f774fdd362e9