Closed Bug 595201 Opened 9 years ago Closed 9 years ago

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

Categories

(Toolkit :: Themes, defect)

defect
Not set

Tracking

()

VERIFIED FIXED
mozilla2.0b8
Tracking Status
blocking2.0 --- betaN+

People

(Reporter: alice0775, Assigned: Margaret)

References

Details

(Whiteboard: [doorhanger])

Attachments

(3 files)

Attached image 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.
Typo in Expected Results: 
s/A notification bar appears./Readable color/
Summary: [password doorhanger]Hovering over menuitem shows black on blue instead of white on blue on WinXP and Win7/Classic → popup notification menuitem highlight is broken (black on blue) on WinXP and Win7/Classic
Depends on: 596247
No longer depends on: 596247
Duplicate of this bug: 600546
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)
Assignee: nobody → margaret.leibovic
OS: Windows 7 → All
Hardware: x86 → All
Summary: popup notification menuitem highlight is broken (black on blue) on WinXP and Win7/Classic → popup notification menuitem highlight is broken (black on blue) in winstripe and pinstripe themes
Attached patch patchSplinter Review
Attachment #479956 - Flags: review?(enndeakin)
Blocks: 577927
No longer blocks: 588309
So the problem is that the menupopup is anonymous while the menuitem isn't. We should probably fix that...
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?
(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.
(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.
(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.
(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?
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.
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.
(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.
Whiteboard: [needs review enn]
blocking2.0: --- → ?
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.
Attachment #479956 - Flags: review?(enndeakin)
(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?).
You may have to restart Minefield after switching to classic. Over here I get beveled menus without any color change, just like in Explorer.
(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.
(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.
Whiteboard: [needs review enn]
(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?
blocking2.0: ? → betaN+
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.
Attachment #488581 - Flags: review?(dao)
Whiteboard: [needs review 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.
Attachment #488581 - Flags: review?(dao)
(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?
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.
Whiteboard: [needs review dao]
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).
Attachment #479956 - Flags: review?(neil)
Attachment #479956 - Flags: review?(neil) → review+
(In reply to comment #18)
> menubar > menu[_moz-menuactive="true"]:not([disabled="true"] in menu.css seems
> to handle the text color.
Component: Theme → Themes
Product: Firefox → Toolkit
QA Contact: theme → themes
http://hg.mozilla.org/mozilla-central/rev/f774fdd362e9
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla2.0b8
Depends on: 611997
No longer depends on: 611997
Whiteboard: [doorhanger]
Depends on: 632989
Status: RESOLVED → VERIFIED
Depends on: 718392
You need to log in before you can comment on or make changes to this bug.