372996 - fix native checkboxes and radio buttons on Mac OS X

Status: RESOLVED FIXED

(Core :: Widget: Cocoa, defect)

Description

[Josh Aas]

This is the bug for rewriting native checkboxes and radio buttons on Mac OS X as part of our native theme rewrite.

Comment 1

[Josh Aas]

Attachment: cleanup v1.0

The first part of this patch just removes some values that are already set correctly by default, and removes the single-initialization scheme because only one native theme is ever created anyway.

The second part of the patch just moves two blocks so that my next patch will be cleaner. It puts the code for checkbox and checkbox-small next to each other, same for radio buttons.

Comment 2

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 257636 [details] [diff] [review]
cleanup v1.0

>-    sTextfieldBGTransparent = PR_FALSE;

This variable is always false, so I'm not sure why it needs to exist.

>-    sTextfieldDisabledBGColorID = nsILookAndFeel::eColor__moz_field;

You're making this variable unneeded as well, but removing this line seems like it's a substantive change as well.

Comment 3

[Josh Aas]

Attachment: cleanup v1.1

Comment 4

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 257747 [details] [diff] [review]
cleanup v1.1

This introduces additional behavior changes (to textfields, which it makes no longer have a disabled/non-disabled color difference) without fixing the existing Mac-only one that you created.

Maybe it's better not to touch the xpwidgets/ code in this patch; it seems to have a lot of variables that might be unused; perhaps that's by design.

Comment 5

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

That said, the code in question is modifying IsWidgetStyled, so it seems like the changes to those constants are there in order to match mac-specific forms.css differences -- which we shouldn't even have anymore except for camino.  In other words, the existing code in nsNativeThemeMac and nsNativeThemeCocoa doesn't really make any sense to me.

Comment 6

[Josh Aas]

Attachment: fix v1.2

This removes only one variable from the xpwidget native theme code, and leaves the variable that makes text backgrounds different colors for enabled and disabled.

Removing sTextfieldDisabledBGColorID modification from the mac theme code is fine. That is set correctly in forms.css (and in the pinstripe theme for good measure, though that isn't really relevant here). We don't need to be overriding it in our native theme code.

Comment 7

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Comment on attachment 257796 [details] [diff] [review]
fix v1.2


>Index: xpwidgets/nsNativeTheme.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/widget/src/xpwidgets/nsNativeTheme.cpp,v
>retrieving revision 1.30
>diff -U8 -p -r1.30 nsNativeTheme.cpp
>--- xpwidgets/nsNativeTheme.cpp	7 Feb 2007 07:46:44 -0000	1.30
>+++ xpwidgets/nsNativeTheme.cpp	7 Mar 2007 23:12:39 -0000
>@@ -56,17 +56,16 @@ PRUint8  nsNativeTheme::sButtonActiveBor

>-PRBool   nsNativeTheme::sTextfieldBGTransparent = PR_FALSE;

>@@ -224,24 +223,22 @@ nsNativeTheme::IsWidgetStyled(nsPresCont
>         }
>         break;
> 
>       case NS_THEME_TEXTFIELD:
>         defaultBorderStyle = sTextfieldBorderStyle;
>         ConvertMarginToAppUnits(sTextfieldBorderSize, defaultBorderSize);
>         lookAndFeel->GetColor(sTextfieldBorderColorID,
>                               defaultBorderColor);
>-        if (!(defaultBGTransparent = sTextfieldBGTransparent)) {
>-          if (IsDisabled(aFrame))
>-            lookAndFeel->GetColor(sTextfieldDisabledBGColorID,
>-                                  defaultBGColor);
>-          else
>-            lookAndFeel->GetColor(sTextfieldBGColorID,
>-                                  defaultBGColor);
>-        }
>+        if (IsDisabled(aFrame))
>+          lookAndFeel->GetColor(sTextfieldDisabledBGColorID,
>+                                defaultBGColor);
>+        else
>+          lookAndFeel->GetColor(sTextfieldBGColorID,
>+                                defaultBGColor);
>         break;
> 
>       case NS_THEME_LISTBOX:
>       case NS_THEME_DROPDOWN:
>         defaultBorderStyle = sListboxBorderStyle;
>         ConvertMarginToAppUnits(sListboxBorderSize, defaultBorderSize);
>         lookAndFeel->GetColor(sListboxBorderColorID,
>                               defaultBorderColor);
>Index: xpwidgets/nsNativeTheme.h
>===================================================================
>RCS file: /cvsroot/mozilla/widget/src/xpwidgets/nsNativeTheme.h,v
>retrieving revision 1.20
>diff -U8 -p -r1.20 nsNativeTheme.h
>--- xpwidgets/nsNativeTheme.h	30 Aug 2006 21:47:16 -0000	1.20
>+++ xpwidgets/nsNativeTheme.h	7 Mar 2007 23:12:39 -0000
>@@ -143,17 +143,16 @@ class nsNativeTheme

>-  static PRBool                    sTextfieldBGTransparent;

I don't understand the need for this change; presumably there may be a platform that needs sTextfieldBGTransparent -- just let it default out to FALSE and leave this code in.  (As dbaron said in irc, IsWidgetStyled needs to be beat over the head with a stick, but that's a separate issue.)

Other than that this patch doesn't seem to change any of the logic at all, right?  It just removes some unneeded initializers since the variables in question are already initialized to the correct value by default, and moves some code around.

So r+ with leaving the nsNativeTheme bits alone.

Comment 8

[Josh Aas]

"fix v1.2" checked in on trunk. leaving this open for the real fix for this bug.

Comment 9

[Stuart Parmenter]

what is the "real" fix?  are there answers to vlad's questions?

Comment 10

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Marking fixed, please file a new bug for the "real fix"