589121 - Hovering over menuitem shows black on blue instead of white on blue on WinXP and Win7/Classic

Status: RESOLVED FIXED

(Firefox :: Theme, defect)

Description

[Alice0775 White]

Attachment: screenshot

Build Identifier: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0b5pre) Gecko/20100820 Minefield/4.0b5pre ID:20100820020849
http://hg.mozilla.org/mozilla-central/rev/1504917ed42e

In Windows Classic Style, 
Hovering menuitem shows black on blue instead of white on blue.

Text color should be white.

This problem looks like Bug 571871.

Comment 1

[Mike Beltzner [:beltzner, not reading bugmail]]

Are we supporting the menubutton on Windows Classic?

Comment 2

[Jonathan Haas]

(In reply to comment #1)
> Are we supporting the menubutton on Windows Classic?

I think we should. At least the draw-in-titebar stuff should already work in the classic theme (when the patch is checked in again). And disabling the button when the user switches the theme seems more difficult than just supporting it.

Comment 3

[Dão Gottwald [:dao]]

(In reply to comment #1)
> Are we supporting the menubutton on Windows Classic?

Yes. This bug affects more than classic, though.

Comment 4

[Dão Gottwald [:dao]]

The patch in bug 589163 fixes this for some items.

Comment 5

[alex_mayorga]

(In reply to comment #1)
(From https://bugzilla.mozilla.org/show_bug.cgi?id=580738#c3)
> The "menu button" jumplist? doesn't exist on "Windows Classic"
> theme (see https://bugzilla.mozilla.org/attachment.cgi?id=467794).
> I'm not UI expert, but I believe for classic theme only the elements on
> Classic.xaml should be used (see
> http://msdn.microsoft.com/en-us/library/aa970773.aspx)

Comment 7

[Jim Mathies [:jimm]]

Attachment: another ss

Comment 8

[MrX1980]

With the beta 5 (build 1] it is ok with Win7 Basic/Aero Design.

Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0b5) Gecko/20100101 Firefox/4.0b5
(the build date is wrong, should be 20100831)

Comment 9

[Gabriele Best [:Gabri]]

Attachment: Patch v1

I think it works properly.

Comment 10

[Gabriele Best [:Gabri]]

Attachment: Patch v1 + little fix

Little fix about edit menuitem.

Comment 11

[Dão Gottwald [:dao]]

Comment on attachment 472476 [details] [diff] [review]
Patch v1 + little fix

This looks like it will slow down interaction with every menu or menuitem in the window... not cool.

Comment 12

[Gabriele Best [:Gabri]]

Attachment: Patch v2

Dao now CSS rules seems to be better.
I hope now it's cool ;)

Comment 13

[Dão Gottwald [:dao]]

Comment on attachment 472566 [details] [diff] [review]
Patch v2

>     <menupopup id="appmenu-popup"
>                onpopupshowing="updateEditUIVisibility();">
>       <hbox>
>-        <vbox id="appmenuPrimaryPane">
>+        <vbox id="appmenuPrimaryPane" class="split-menuitem">
>           <hbox flex="1"
>                 class="split-menuitem">
>             <menuitem id="appmenu_newTab"
>-                      class="split-menuitem-item"
>                       flex="1"
>                       label="&tabCmd.label;"
>                       command="cmd_newNavigatorTab"/>

appmenuPrimaryPane isn't a split menuitem. Setting the split-menuitem class there is not cool ;-)

Thanks for looking into this, but I already have a pretty simple patch, just need to test it.

Comment 14

[Gabriele Best [:Gabri]]

Ok, thanks Dao! :)

Comment 15

[Dão Gottwald [:dao]]

Attachment: patch

Comment 16

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

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

Why is the -moz-padding-end removal OK?
Why do we need to specify these styles at all anyways? Can the answer to that be in a comment?

Comment 17

[Dão Gottwald [:dao]]

(In reply to comment #16)
> Comment on attachment 472574 [details] [diff] [review]
> patch
> 
> Why is the -moz-padding-end removal OK?

It was there to align the keyboard shortcuts. They're gone, so it's not needed anymore, and the selector doesn't actually match anything anymore.

> Why do we need to specify these styles at all anyways? Can the answer to that
> be in a comment?

See bug 571871 comment 6. I can add a comment.

Comment 18

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

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

please do add the comment

Comment 19

[Dão Gottwald [:dao]]

http://hg.mozilla.org/mozilla-central/rev/6cd1f572ab15

Comment 20

[alex_mayorga]

I now see white on blue on WinXP Classic using Mozilla/5.0 (Windows NT 6.0; rv:2.0b6pre) Gecko/20100910 Firefox/4.0b6pre ID:20100910041829

Thanks Mr. Gottwald!