237138 - Disabled form controls are not always grayed out.

Status: RESOLVED FIXED

(Core Graveyard :: GFX, defect)

Description

[Baron Schwartz]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7b) Gecko/20040302 Firefox/0.8.0+
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7b) Gecko/20040302 Firefox/0.8.0+

Disabled form controls are not always grayed out.  The example I have seen is

<input type="radio" disabled="1">

Other values for the "disabled" attribute will cause it to gray out.

Reproducible: Always
Steps to Reproduce:
1.  Load the attached test case.
Actual Results:  
The form control is only grayed out sometimes.

Expected Results:  
Should have always grayed out the form control.

Comment 1

[Baron Schwartz]

Attachment: Shows disabled form controls; not all of them are grayed out.

Comment 2

[Ali Ebrahim]

Seeing this in Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:)
Gecko/20040309 too.

--> Browser

Comment 3

[Gérard Talbot]

I couldn't find a duplicate for this. I confirm this bug also because
disabled="disabled" which is perfectly valid in HTML 4 will not be rendered
accordingly. Upon further examination of 
Program Files\Common Files\mozilla.org\GRE\[]\res\forms.css file,
I can not explain this. Additional testcase coming.

Comment 4

[Gérard Talbot]

Attachment: Various situations of disabled[=string] [checked[=checked]]

MSIE 6 SP1 for windows renders all radio buttons of this testcase with a gray
background color.

Comment 5

[Gérard Talbot]

The forms.css file has:

input[type="radio"][disabled],
input[type="radio"][disabled]:active,
{
  color: GrayText ! important;
  background-color: ThreeDFace ! important;
}

So, I can not see/explain why radio buttons with disabled="disabled" do not have
a gray background when active and when not active.
Mozilla 1.7b 2004031008 here under XP Pro SP1.

Comment 6

[Gérard Talbot]

Mozilla 1.4 and Mozilla 1.5 renders all disabled[=string] radio buttons with a
gray background. So this has to be a regression.

Comment 7

[Gérard Talbot]

Attachment: Better testcase of various disabled[=string] situations

I renamed radio buttons so that each group of radio buttons has only 1 that is
checked[=checked]

Comment 8

[Boris Zbarsky [:bzbarsky]]

All three of the native theme impls screw up checking of boolean attrs on HTML
elements.  Such attrs are true if set at all, no matter what value.  See
http://lxr.mozilla.org/seamonkey/ident?i=CheckBooleanAttr for the function in
question.  Someone probably needs to check over the callers to make sure that
it's in fact only used for boolean attrs when HTML is concerned...

When native theming is off, the code in forms.css is used and this bug disappears.

Comment 9

[Boris Zbarsky [:bzbarsky]]

*** Bug 235563 has been marked as a duplicate of this bug. ***

Comment 10

[Boris Zbarsky [:bzbarsky]]

I think we should fix this for 1.7.... it's pretty bad from a usability
perspective....

Comment 11

[[e-mail]]

Not only that, but <input disabled> serializes as <input disabled="disabled">

Comment 12

[[e-mail]]

Attachment: Windows fix

I couldn't figure out how to duplicate this bug on linux as I can't persuade my
build to support native theme form controls...

Comment 13

[Unknown W. Brackets]

I know this doesn't have a lot of votes/ccs or anything, but if the patch
works... this can be a major usability problem.

After all, if a checkbox is disabled, it should look so - if it doesn't, people
might try to check it, and wonder why it won't check.  This can cause confusion
when the checkbox (or radio button...) should be disabled to indicate something.

Thanks,
-[Unknown]

Comment 14

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 144173 [details] [diff] [review]
Windows fix

Neil, how about just fixing nsNativeTheme::CheckBooleanAttr, and eliminating
the static functions in nsNativeThemeWin and nsNativeThemeGTK altogether?

Comment 15

[[e-mail]]

Attachment: Mac fix

Of course we still need to make the other two native theme implementations
inherit from the so-called shared implementation...

Comment 16

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 144173 [details] [diff] [review]
Windows fix

r+sr=bzbarsky for this to go on the 1.7 branch only.  On the trunk, the other
patch is what we want.

Could this please be approved for 1.7?	It fixes the styling of disabled
"native themed" controls in HTML to actually look disabled (most of the time,
they did not).

Comment 17

[Boris Zbarsky [:bzbarsky]]

> inherit from the so-called shared implementation...

Done for Windows.  See bug 232175.

Comment 18

[[e-mail]]

(In reply to comment #17)
>>inherit from the so-called shared implementation...
>Done for Windows.  See bug 232175.
What about nsNativeThemeGTK? Do we need a branch patch as per attachment 144173 [details] [diff] [review]
and a new bug to fix the trunk as per bug 232175 too?

Comment 19

[Boris Zbarsky [:bzbarsky]]

Ideally, yes to both.....

Comment 20

[[e-mail]]

Attachment: GTK branch fix

Comment 21

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 146453 [details] [diff] [review]
GTK branch fix

r+sr=bzbarsky.

Again, this would be nice to have on the 1.7 branch.

Comment 22

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 146407 [details] [diff] [review]
Mac fix

r+sr=bzbarsky here too.  Yay copy/paste code.

Comment 23

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 146453 [details] [diff] [review]
GTK branch fix

-  return attr.EqualsIgnoreCase("true"); // This handles the XUL case.
+  content->GetAttr(kNameSpaceID_None, aAtom, attr);
+  return attr.Equals(NS_LITERAL_STRING("true")); // This handles the XUL case.

this is no longer ignoring case, intentionally?

Comment 24

[Boris Zbarsky [:bzbarsky]]

And yes, ignoring case is totally incorrect.  XUL is completely case-sensitive.

Comment 25

[Asa Dotzler [:asa]]

Comment on attachment 144173 [details] [diff] [review]
Windows fix

a=asa (on behalf of drivers) for checkin to 1.7

Comment 26

[Asa Dotzler [:asa]]

Comment on attachment 146407 [details] [diff] [review]
Mac fix

a=asa (on behalf of drivers) for checkin to 1.7

Comment 27

[Asa Dotzler [:asa]]

Comment on attachment 146453 [details] [diff] [review]
GTK branch fix

a=asa (on behalf of drivers) for checkin to 1.7

Comment 28

[[e-mail]]

144173, 146407, 146453 checked in to the branch.

Spun off bug 241070 on the GTK issue (comment 18).

Comment 29

[Boris Zbarsky [:bzbarsky]]

Neil, are you going to check in the nsNativeTheme.cpp fix on trunk too?  We
really do want it there....

Comment 30

[[e-mail]]

Whoops, oversight :-[