108307 - Checkbox initial value is not set when display: none

Status: RESOLVED FIXED

(Core :: Layout: Form Controls, defect)

Description

[John Keiser (jkeiser)]

<FORM NAME=mainform>
<INPUT NAME=chk TYPE=checkbox CHECKED STYLE="display: none">
<SCRIPT>alert(document.mainform.chk.checked)</SCRIPT>
</FORM>

should show true.  Currently it shows false.  This affects its ability to be
properly submitted and manipulated with Java1

Comment 1

[jsp]

Is there anything I can do to help with this bug? I'd like to be able to
recommend a browser other than IE for use with my site, and this bug currently
prevents that. I don't have a lot of time, and I know essentially nothing of
Mozilla's code, but I'd like to move this along if I can. I'm reluctant to take
on anything complex (this is quite tangential to what I'm paid to do), but I
suppose if it were straightforward jkeiser or someone else would have taken care
of it already.

Comment 2

[John Keiser (jkeiser)]

I will have gobs more time for Mozilla starting next week, and this is near the
top of my list (after radio buttons).  Still, if you have enough time and you
want to work on it, I'd be happy to point you in the right direction.

This is one of the easier fixes I have on my plate, though it's by no means
trivial.  It involves is moving the "checked" state over from one class to
another.  Essentially, the problem is the state is stored in the frame class,
which only exists when the element is being displayed.  Thus this is where the
initialization happens as well.  It needs to be moved to content.

Frame:
http://lxr.mozilla.org/seamonkey/source/layout/html/forms/src/nsGfxCheckboxControlFrame.cpp
http://lxr.mozilla.org/seamonkey/source/layout/html/forms/src/nsGfxCheckboxControlFrame.h
(See the mChecked member variable in the .h file, that's the one to remove)

Content:
http://lxr.mozilla.org/seamonkey/source/content/html/content/src/nsHTMLInputElement.cpp
(mChecked needs to be moved to the class definition here and any calls to the
frame to determine the value need to be changed to use the local ver.)

Comment 3

[jsp]

It's unlikely that I'd get to it before next week. If I find I have the time,
I'll check in with you before I dive in.

Comment 4

[Peter Trudelle]

Could we get this targetted for early in 0.99?  If it really blocks 112716, then
we need it soon.

Comment 5

[John Keiser (jkeiser)]

Attachment: Work In Progress Patch

This is the whole thing, completely and utterly untested.  Will test and debug
tomorrow.

Comment 6

[John Keiser (jkeiser)]

That patch is nearly working.  .defaultChecked needs to be hooked up to
setAttribute("checked"), and the .defaultChecked <-> .checked relationship is a
little off (though only in its more obscure points)--attaching testcase.

Comment 7

[John Keiser (jkeiser)]

Attachment: Checkbox initialization / attribute test

Note to self: test and make sure radio buttons and onChange work properly (this
patch intersects there and fixes some onChange bugs).

Comment 8

[John Keiser (jkeiser)]

Attachment: Extended Checkbox Testcase (handles onChange and setAttribute)

Comment 9

[John Keiser (jkeiser)]

Attachment: Radio Testcase

This shows that in the forthcoming patch, radios are still working. 
defaultChecked <-> checked interaction is a little screwy, but then, it always
has been.  (Just compare behaviors.  We do about the same as before, except now
.defaultChecked doesn't change the value of .checked after you have been
clicking around on the buttons a lot.)

Comment 10

[John Keiser (jkeiser)]

Attachment: The Golden Patch

All is working here, and it passes jst-review.cgi.

Comment 11

[rods (gone)]

r=rods

Comment 12

[Johnny Stenback (:jst)]

- In nsHTMLInputElement.cpp:

@@ -223,6 +232,8 @@
   PRPackedBool             mHandlingClick;
   PRPackedBool             mValueChanged;
   char*                    mValue;
+  PRPackedBool             mCheckedChanged;
+  PRPackedBool             mChecked;

We now have 5 PRPackedBool's (and one PRInt8), which will eat up at least 16
bytes, should we combine the 5 PRPackedBool's into bitfields to save space here?
Or should we file a new bug on that?

- In nsGfxCheckboxControlFrame::GetCheckboxState():

+  nsIDOMHTMLInputElement* elem = nsnull;
+  CallQueryInterface(mContent, &elem);
+  PRBool retval = PR_FALSE;
+  elem->GetChecked(&retval);
+  return retval;

That leaks, nsCOMPtr!

Same thing in nsGfxCheckboxControlFrame::SetCheckboxState()

- In nsGfxCheckboxControlFrame::StringToCheckState():

+  if ( !aStateAsString.Equals(NS_STRING_TRUE) ) {
+    return PR_FALSE;
+  } else {
+    return PR_TRUE;
+  }

else-after-return, and why not simply:

+  return aStateAsString.Equals(NS_STRING_TRUE);

sr=jst with the above fixed.

Comment 13

[Johnny Stenback (:jst)]

s/16 bytes/8 bytes/

Comment 14

[John Keiser (jkeiser)]

OK, changes made and working, except PRPackedBool fix.  That is bug 121317. 
Will check in when tree opens.

Comment 15

[John Keiser (jkeiser)]

checked in, tested, happy.

Comment 16

[jsp]

Thanks, John. Now that this is fixed, I no longer have to tell users that our
site can't be safely viewed in Mozilla!

Comment 17

[John Keiser (jkeiser)]

Better check that you're not running afoul of its companion bug, bug 108308 :)

Comment 18

[jsp]

Thanks; I know about that bug. My site doesn't happen to hide radio controls. Of
course, I can't recommend Mozilla-based browsers unreservedly until 108308 is fixed.