612234 - [gtk2] Menu item rendered with wrong color in open or hover state

Status: RESOLVED FIXED

(Core :: Widget: Gtk, defect)

Description

[Ginn Chen]

Attachment: High Contrast theme - Firefox 3.6.x

With Clearlooks or High Contrast theme, open menu, the background and/or foreground color of menuitem with open/hover state is wrong.

Comment 1

[Ginn Chen]

Attachment: Clearlooks - Firefox 3.6.x

Comment 2

[Ginn Chen]

Attachment: High Contrast theme - Firefox 4.0

Comment 3

[Ginn Chen]

Attachment: Clearlooks - Firefox 4.0

Comment 4

[Joe Drew (not getting mail)]

I'm hoping this is easy, Karl.

Comment 5

[Karl Tomlinson (:karlt)]

I'm not reproducing the missing menu item in attachment 490539 [details] (Clearlooks), nor am I reproducing the wrong bg color on the open menu title.
However, I do see the wrong fg color on the open menu title.

Comment 6

[Ginn Chen]

(In reply to comment #5)
> I'm not reproducing the missing menu item in attachment 490539 [details] (Clearlooks),
> nor am I reproducing the wrong bg color on the open menu title.

Perhaps the theme on Solaris is a little different.

> However, I do see the wrong fg color on the open menu title.

Just found it is a recent regression.
The regression window is between changeset 57199 (Nov 10 nightly) to changeset 57324 (Nov 11 nightly).

Comment 7

[Karl Tomlinson (:karlt)]

http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=df1d1ff6b489&tochange=0f17e5f1eb01

Comment 8

[Ginn Chen]

The fg color and bg color issue are caused by different commits.

The fg color issue with High Contrast theme is caused by
http://hg.mozilla.org/mozilla-central/rev/4563259585a2

Now I'm trying to find out the regression window of the bg color issue with Clearlook theme.

Comment 9

[Ginn Chen]

The bg color of Clearlook theme issue is caused by
http://hg.mozilla.org/mozilla-central/rev/f780f3d3add9

Comment 10

[Ian Neal]

(In reply to comment #8)
> The fg color and bg color issue are caused by different commits.
> 
> The fg color issue with High Contrast theme is caused by
> http://hg.mozilla.org/mozilla-central/rev/4563259585a2
> 
> Now I'm trying to find out the regression window of the bg color issue with
> Clearlook theme.

Not sure why that commit would cause the issue as presumably the File menu element is not disabled at that point?

Comment 11

[Ginn Chen]

Ian,

I guess it is because menubar > menu[_moz-menuactive="true"]:not([disabled="true"]) is more specific than menubar > menu[open].

Before your patch, the color for menuitem should be

1) Normal,
75 menubar > menu {
76   padding: 0px 4px;
77   color: -moz-menubartext;
78 }

2) Mouse on menu but not dropdown the menu
89 menubar > menu[_moz-menuactive="true"] {
90   color: -moz-menubartext;
91 }

3) Menu is dropdown
93 menubar > menu[open] {
94   color: -moz-menubarhovertext;
95   background-color: -moz-menuhover;
96 }

Because you're adding another rule for condition 2), it is more specific than condition 3).

You can add "! important" or ":not([disabled="true"]) " for condition 3) to fix this problem.

Comment 12

[Ginn Chen]

(In reply to comment #9)
> The bg color of Clearlook theme issue is caused by
> http://hg.mozilla.org/mozilla-central/rev/f780f3d3add9

This is caused by a series of bugs.
1) In Bug 598561 Don't rename pixman functions when system pixman is in use, we didn't define MOZ_TREE_PIXMAN, therefore we didn't rename pixman functions even with our internal pixman library.
2) We didn't rename all pixman export functions
3) Although in pixman.h, it doesn't have PIXMAN_EXPORT, but for Sun Studio compiler, pixman functions are still compiled as global scope.

With these bugs and a certain combination of system cairo, pixman library and mozilla internal pixman library, we got the wrong background.

Comment 13

[Ginn Chen]

Attachment: fix issue 1) in comment #12

Comment 14

[Ginn Chen]

Attachment: fix issue 2) in comment #12

Comment 15

[Ginn Chen]

Attachment: fix issue 3) in comment #12

Comment 16

[Ginn Chen]

Attachment: fix the fg color of menu

Comment 17

[Dão Gottwald [:dao]]

Comment on attachment 492933 [details] [diff] [review]
fix the fg color of menu

>diff --git a/toolkit/themes/gnomestripe/global/menu.css b/toolkit/themes/gnomestripe/global/menu.css
>--- a/toolkit/themes/gnomestripe/global/menu.css
>+++ b/toolkit/themes/gnomestripe/global/menu.css
>@@ -85,17 +85,17 @@ menubar:-moz-lwtheme > menu:not([open="t
>   color: inherit;
>   text-shadow: inherit;
> }
> 
> menubar > menu[_moz-menuactive="true"]:not([disabled="true"]) {

Can you add :not([open]) here? And :not(:-moz-lwtheme) while you're at it?

>   color: -moz-menubartext;
> }
> 
>-menubar > menu[open] {
>+menubar > menu[open]:not([disabled="true"]) {
>   color: -moz-menubarhovertext;
>   background-color: -moz-menuhover;
> }

Comment 18

[Ginn Chen]

Attachment: fix the fg color of menu v2

Comment 19

[Ian Neal]

(In reply to comment #17)
> Comment on attachment 492933 [details] [diff] [review]
> fix the fg color of menu
> 
> >diff --git a/toolkit/themes/gnomestripe/global/menu.css b/toolkit/themes/gnomestripe/global/menu.css
> >--- a/toolkit/themes/gnomestripe/global/menu.css
> >+++ b/toolkit/themes/gnomestripe/global/menu.css
> >@@ -85,17 +85,17 @@ menubar:-moz-lwtheme > menu:not([open="t
> >   color: inherit;
> >   text-shadow: inherit;
> > }
> > 
> > menubar > menu[_moz-menuactive="true"]:not([disabled="true"]) {
> 
> Can you add :not([open]) here? And :not(:-moz-lwtheme) while you're at it?
> 
Did you mean as well as or instead of?

Comment 20

[Dão Gottwald [:dao]]

Instead of what attachment 492933 [details] [diff] [review] did.

Comment 22

[Ginn Chen]

http://hg.mozilla.org/mozilla-central/rev/a8cccc6bf4a2
http://hg.mozilla.org/mozilla-central/rev/7ba6c4492ec0
http://hg.mozilla.org/mozilla-central/rev/b0f8b6159c81
http://hg.mozilla.org/mozilla-central/rev/837d7b346a64

Comment 24

[[e-mail]]

Comment on attachment 492291 [details] [diff] [review]
fix issue 3) in comment #12

>+/* In libxul builds we don't ever want to export pixman symbols */
>+#ifdef cairo_public
>+#   define PIXMAN_EXPORT cairo_public
This broke my non-libxul build where cairo_public defines to extern __declspec(dllexport) but PIXMAN_EXPORT used to define to nothing.
Should this say #ifdef MOZ_ENABLE_LIBXUL instead?

Comment 25

[Ginn Chen]

Attachment: fix non libxul build

Just noticed I forgot to add the new file in my last commit.

Comment 26

[[e-mail]]

Comment on attachment 494589 [details] [diff] [review]
fix non libxul build

I can confirm that this patch fixes my Windows non-libxul build.
[No idea as to its effect on any other sort of build though.]

>-		*n++ = bswap_32 (*d++);
>+		*n++ = bswap_32 (*d);
>+        d++;
[Not intended to be part of this patch I take it!]

Comment 27

[Ginn Chen]

Fix for non libxul build and add the missing patch file
http://hg.mozilla.org/mozilla-central/rev/bfec67c46b7c