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

VERIFIED FIXED in mozilla2.0b8

Status

()

Toolkit
Themes
VERIFIED FIXED
8 years ago
7 years ago

People

(Reporter: Alice0775 White, Assigned: Margaret)

Tracking

Trunk
mozilla2.0b8
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(blocking2.0 betaN+)

Details

(Whiteboard: [doorhanger])

Attachments

(3 attachments)

(Reporter)

Description

8 years ago
Created attachment 474073 [details]
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.
(Reporter)

Comment 1

8 years ago
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
(Assignee)

Updated

8 years ago
Duplicate of this bug: 600546
(Assignee)

Comment 3

8 years ago
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
(Assignee)

Comment 4

8 years ago
Created attachment 479956 [details] [diff] [review]
patch
Attachment #479956 - Flags: review?(enndeakin)
(Assignee)

Updated

8 years ago
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?
(Assignee)

Comment 7

8 years ago
(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.
(Assignee)

Comment 8

8 years ago
(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.
(Assignee)

Updated

8 years ago
Whiteboard: [needs review enn]
(Assignee)

Updated

8 years ago
blocking2.0: --- → ?

Comment 14

8 years ago
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)
(Assignee)

Comment 15

8 years ago
(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.
(Assignee)

Comment 17

8 years ago
(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.
(Assignee)

Updated

8 years ago
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+
(Assignee)

Comment 20

8 years ago
Created attachment 488581 [details] [diff] [review]
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.
Attachment #488581 - Flags: review?(dao)
(Assignee)

Updated

8 years ago
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)
(Assignee)

Comment 22

8 years ago
(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.
(Assignee)

Updated

8 years ago
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).

Updated

8 years ago
Attachment #479956 - Flags: review?(neil)

Updated

8 years ago
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
Last Resolved: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla2.0b8

Updated

8 years ago
Depends on: 611997
No longer depends on: 611997
Whiteboard: [doorhanger]

Updated

8 years ago
Depends on: 632989
Status: RESOLVED → VERIFIED

Updated

7 years ago
Depends on: 718392
You need to log in before you can comment on or make changes to this bug.