174740 - Black checkboxes in menus (e.g. View->Toolbars->Navigation Bar)

Status: RESOLVED WONTFIX

(Core Graveyard :: GFX, defect)

Description

[Ian Anderson]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.2b) Gecko/20021014 Phoenix/0.3
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.2b) Gecko/20021014 Phoenix/0.3

I use a (rather unusual, I'll admit) windows theme that has black backgrounds
with white text. The checkboxes in many Phoenix dialogues don't respect system
colours and are thus black on black.

Reproducible: Always

Steps to Reproduce:
1. Set display preferences to have all black backgrounds and all white text.
2. Select View->Toolbars
Actual Results:  
Checkboxes are invisible.

Expected Results:  
Checkboxes are white, respecting system colours.

This is a problem with the default theme.

Comment 1

[Ian Anderson]

I neglected to mention some other places this occurs (which probably doesn't
make it clear why I said it's a problem in the default theme).

Black on black items can also be found:
-in the Profile Manager the profile icon is black
-bookmarks toolbar text is black
-checkboxes in dialogues (including prefs panel) are black

Comment 2

[Asa Dotzler [:asa]]

Setting bug status to new. This is one of two things (I'm guessing). Either we
don't have nsITheme for checks in checkboxes or we're not using it. 

Comment 3

[Asa Dotzler [:asa]]

This styling lives in mozilla/toolkit/menu.css I think 

/* ::::: checkbox menuitem ::::: */

menuitem[checked="true"] {
  list-style-image: url("chrome://global/skin/menu/menu-check.gif");
}

menuitem[checked="true"][disabled="true"] {
  list-style-image: url("chrome://global/skin/menu/menu-check-disabled.gif");
}

menuitem[checked="true"][_moz-menuactive="true"] {
  list-style-image: url("chrome://global/skin/menu/menu-check-hover.gif");
}

So right now we're using hardcoded images here. Would it be possible to replace
this with a symbol font? If so then it would pick up the font color of the
different themes and should work better.

Or am I totally wrong about how all this works?

Comment 4

[Jason Kersey]

The font that windows uses to keep stuff like checkboxes and menu arrows looking
correct isn't available on other platforms, so this will be hard.

Comment 5

[Mike Connor [:mconnor]]

Based on some testing elsewhere, this seems to be a problem with how nsITheme
gets information from the default XP themes.  It works for Windows Classic however.

I'll have a WinXP box in another week or so and can comment further.

Comment 6

[Brant Gurganus]

adding to cc

Comment 7

[Simon Paquet [:sipaq]]

Taking QA Contact

Comment 8

[Ian Anderson]

Attachment: Screenshot of menu checkmark behaviour

Comment 9

[Ian Anderson]

A number of the cases I listed above work as expected in Firebird 0.6 including
dialogue checkboxes and the bookmarks toolbar. The checkmarks that should appear
in menus, however, are still black on black even in Windows Classic.

I have attached a relevant screenshot.

Comment 10

[Ginn Chen]

Attachment: Patch to check or radio menuitem and apply to Firefox skin

Comment 11

[Ginn Chen]

Attachment: Patch to Mozilla Classic theme

We should apply the patch of Bug 243372. Unless the check and radio menuitems
will be odd to regular menuitems.

Comment 12

[Ginn Chen]

Comment on attachment 148818 [details] [diff] [review]
Patch to check or radio menuitem and apply to Firefox skin

This patch is similar to bug 118296, NS_THEME_*MENUITEM implementations.

Comment 13

[Mike Connor [:mconnor]]

so, this fixes this on GTK2, any idea on what we'd need to do to fix this on OS
X (then we can use the native Windows symbol font)

Comment 14

[Simon Paquet [:sipaq]]

The original bug was noted on a windows system? Any idea on what would need to
be done there?

Comment 15

[Ginn Chen]

Yesterday I changed the information.
245154 is similar to this; 237535 fixed something in dialogue for gtk2 build.

[e-mail] changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
          Component|Toolbars                    |GFX
            Product|Firefox                     |Browser
           Platform|PC                          |All
   Target Milestone|After Firefox 1.0           |---
            Version|unspecified                 |Trunk

Comment 16

[Brian Ryner (not reading)]

Comment on attachment 148818 [details] [diff] [review]
Patch to check or radio menuitem and apply to Firefox skin

>--- gfx/src/gtk/nsNativeThemeGTK.cpp	6 Mar 2004 21:27:32 -0000	1.50
>+++ gfx/src/gtk/nsNativeThemeGTK.cpp	19 May 2004 08:04:04 -0000
>@@ -296,16 +298,31 @@ nsNativeThemeGTK::GetGtkWidgetAndState(P
>           PRBool isOpen;
>           menuFrame->MenuIsOpen(isOpen);
>           aState->inHover = isOpen;
>         } else {
>           aState->inHover = CheckBooleanAttr(aFrame, mMenuActiveAtom);
>         }
> 
>         aState->active = FALSE;
>+        
>+        if (aWidgetType == NS_THEME_CHECKMENUITEM ||
>+            aWidgetType == NS_THEME_RADIOMENUITEM) {
>+          if (aFrame) {
>+            nsAutoString attr;
>+            nsresult res = aFrame->GetContent()->GetAttr(kNameSpaceID_None, mCheckedAtom, attr);
>+            if (res == NS_CONTENT_ATTR_NO_VALUE ||
>+               (res != NS_CONTENT_ATTR_NOT_THERE && attr.IsEmpty()))
>+              *aWidgetFlags = FALSE;
>+            else
>+              *aWidgetFlags = attr.EqualsIgnoreCase("true");
>+          } else {
>+            *aWidgetFlags = FALSE;

You should just be able to use CheckBooleanAttr() as above.


>--- toolkit/skin/gtk2/menu.css	7 Apr 2004 23:16:51 -0000	1.5
>+++ toolkit/skin/gtk2/menu.css	19 May 2004 08:04:05 -0000
>@@ -134,46 +134,24 @@ menulist > menupopup > menu {
> menulist > menupopup > menuitem > .menu-iconic-left,
> .menulist-menupopup > menu > .menu-iconic-left,
> menulist > menupopup > menu > .menu-iconic-left {
>   display: none;
> }
> 
> /* ::::: checkbox menuitem ::::: */
> 
>-menuitem[checked="true"] {
>-  list-style-image: url("chrome://global/skin/menu/menu-check.gif");
>-  -moz-image-region: auto;
>-}
>-
>-menuitem[checked="true"][disabled="true"] {
>-  list-style-image: url("chrome://global/skin/menu/menu-check-disabled.gif");
>-  -moz-image-region: auto;
>-}
>-
>-menuitem[checked="true"][_moz-menuactive="true"] {
>-  list-style-image: url("chrome://global/skin/menu/menu-check-hover.gif");
>-  -moz-image-region: auto;
>+menuitem[type="checkbox"] {
>+  -moz-appearance: checkmenuitem !important;
> }
> 

One thing you need to make sure of is that the text of the menuitem has
sufficient padding for the checkbox image.  This may already be the case, but
I'd like you to double-check.  It looks like gtk hardcodes the menu
radio/checkbox size to 12px.

sr=bryner with that addressed.

Comment 17

[Ginn Chen]

(In reply to comment #16)
> (From update of attachment 148818 [details] [diff] [review])
> >--- gfx/src/gtk/nsNativeThemeGTK.cpp	6 Mar 2004 21:27:32 -0000	1.50
> >+++ gfx/src/gtk/nsNativeThemeGTK.cpp	19 May 2004 08:04:04 -0000
> >@@ -296,16 +298,31 @@ nsNativeThemeGTK::GetGtkWidgetAndState(P
> >           PRBool isOpen;
> >           menuFrame->MenuIsOpen(isOpen);
> >           aState->inHover = isOpen;
> >         } else {
> >           aState->inHover = CheckBooleanAttr(aFrame, mMenuActiveAtom);
> >         }
> > 
> >         aState->active = FALSE;
> >+        
> >+        if (aWidgetType == NS_THEME_CHECKMENUITEM ||
> >+            aWidgetType == NS_THEME_RADIOMENUITEM) {
> >+          if (aFrame) {
> >+            nsAutoString attr;
> >+            nsresult res = aFrame->GetContent()->GetAttr(kNameSpaceID_None,
mCheckedAtom, attr);
> >+            if (res == NS_CONTENT_ATTR_NO_VALUE ||
> >+               (res != NS_CONTENT_ATTR_NOT_THERE && attr.IsEmpty()))
> >+              *aWidgetFlags = FALSE;
> >+            else
> >+              *aWidgetFlags = attr.EqualsIgnoreCase("true");
> >+          } else {
> >+            *aWidgetFlags = FALSE;
> 
> You should just be able to use CheckBooleanAttr() as above.
The difference is CheckBooleanAttr() will return TRUE if
NS_CONTENT_ATTR_NO_VALUE or attr.IsEmpty(), but if should be FALSE in this case.
> 
> 
> >--- toolkit/skin/gtk2/menu.css	7 Apr 2004 23:16:51 -0000	1.5
> >+++ toolkit/skin/gtk2/menu.css	19 May 2004 08:04:05 -0000
> >@@ -134,46 +134,24 @@ menulist > menupopup > menu {
> > menulist > menupopup > menuitem > .menu-iconic-left,
> > .menulist-menupopup > menu > .menu-iconic-left,
> > menulist > menupopup > menu > .menu-iconic-left {
> >   display: none;
> > }
> > 
> > /* ::::: checkbox menuitem ::::: */
> > 
> >-menuitem[checked="true"] {
> >-  list-style-image: url("chrome://global/skin/menu/menu-check.gif");
> >-  -moz-image-region: auto;
> >-}
> >-
> >-menuitem[checked="true"][disabled="true"] {
> >-  list-style-image: url("chrome://global/skin/menu/menu-check-disabled.gif");
> >-  -moz-image-region: auto;
> >-}
> >-
> >-menuitem[checked="true"][_moz-menuactive="true"] {
> >-  list-style-image: url("chrome://global/skin/menu/menu-check-hover.gif");
> >-  -moz-image-region: auto;
> >+menuitem[type="checkbox"] {
> >+  -moz-appearance: checkmenuitem !important;
> > }
> > 
> 
> One thing you need to make sure of is that the text of the menuitem has
> sufficient padding for the checkbox image.  This may already be the case, but
> I'd like you to double-check.  It looks like gtk hardcodes the menu
> radio/checkbox size to 12px.

in gtk 2.2, it hardcodes indicator size to 8px.
I'm just following its code.
> 
> sr=bryner with that addressed.
> 

Comment 18

[Ginn Chen]

Comment on attachment 148818 [details] [diff] [review]
Patch to check or radio menuitem and apply to Firefox skin

GTK2 fix only, different platform should have different fix.

Comment 19

[Ginn Chen]

Attachment: patch to mozilla/firefox gtk2

Firefox changed directory, and merge patch to mozilla theme into it.

Comment 20

[Ginn Chen]

Comment on attachment 151671 [details] [diff] [review]
patch to mozilla/firefox gtk2

bryner, please take a look at it again, thx.

Comment 21

[Ginn Chen]

Attachment: gtk2 patch v2.1

Changes:
support always_show_toggle style

Comment 22

[Ginn Chen]

Comment on attachment 156401 [details] [diff] [review]
gtk2 patch v2.1

review?

Comment 23

[Robin Lu]

Comment on attachment 156401 [details] [diff] [review]
gtk2 patch v2.1

the patch is OK for me

Comment 24

[Ginn Chen]

Attachment: patch checked in

Comment 25

[Ginn Chen]

Since fix for gtk2 checked in, I specify this bug to Windows/PC.

Comment 26

[Gordon P. Hemsley [:GPHemsley]]

This bug has been buried in the graveyard and has not been updated in over 5 years. It is probably safe to assume that it will never be fixed, so resolving as WONTFIX.

[Mass-change filter: graveyard-wontfix-2014-09-24]