108526 - preSelected radio buttons in forms aren't preselected

Status: VERIFIED FIXED

(Core :: Layout: Form Controls, defect)

Description

[timeless]

Mozilla/5.0 (Windows; U; Win98; en-US; rv:0.9.5+) Gecko/20011105.

steps: load this page, scroll down to the thing that says ( ) Leave bug as ...

expected results: it should be checked
actual results: it isn't checked

this causes knob is not defined when you try to commit comments to bugzilla and
it's very hard for users to figure out what they did wrong.

Comment 1

[timeless]

Comment results in:
Bugzilla version 2.15

This is Bugzilla: the Mozilla bug system. For more information about what
Bugzilla is and what it can do, see mozilla.org's bug pages.

Bug processed
 
Bug List: (0 of 33) First Last Prev Next   Show list      Query page      Enter
new bug
knob was not defined; this may indicate either a problem with bugzilla or a bug
in your web browser. Please help us track this down by adding coments to bug 20092.

I'm marking smoketest, and then i'm going to try to write up a list of
smoketests because we don't appear to have a set of smoketests for form usage
and we really should.

Comment 2

[timeless]

This caused me to almost mark a patch as not a patch because the [ ] patch
checkbox wasn't checked

Comment 3

[timeless]

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

Comment 4

[John Keiser (jkeiser)]

Hrm.  I didn't touch radio buttons except for submission (that I know of), but
maybe something got inadvertently put in?  I'll look at it this evening.

I can't even verify this at work since there's no Linux nightly, but if someone
with the new build (the 5th) can look at
http://www.johnkeiser.com/cgi-bin/mozform.pl and tell whether the middle radio
button on the set of radio buttons in the first column
are selected, that might help reduce the problem more.  I am pretty certain they
showed up properly selected in the 34297 builds, and they may even still show up
checked here.

If they are selected there, perhaps someone could also reduce that testcase and
then check to see if this is because of the oft-derided and much-hated
DemoteContainer(), which fixes badly formed HTML (esp. forms) and which Bugzilla
ends up invoking, presumably because of badly formed HTML.

I believe all you have to do to make such a testcase is do something like
<P><FORM><INPUT TYPE=radio NAME=blah VALUE=blah CHECKED></P></FORM> and verify
that it is different from <P><FORM><INPUT TYPE=radio NAME=blah VALUE=blah
CHECKED></FORM></P>.  (Note the malformedness of the former set of tags.)  Any
parsing gurus on the bug can tell you better, but I think that's the way you get
it to invoke.

Comment 5

[Brian Nesse (gone)]

Yes, the middle radio button in the first column (Name+Value) is selected (Mac
build pulled this morning.)

Comment 6

[Bernd]

Attachment: screenshot win98

Comment 7

[Luke Koleszar]

Attachment: Simple HTML with checked checkbox

Comment 8

[Jacek Piskozub]

So we have a testcase (adding to keywords).

Another good testcase is to try voting for any bug (for example this one).
Trying to sumit the vote is a good and quick way of deleting all your votes if
you use a new build. Bugzilla still allows doing this, unlike changing a bug
with 11/2-5 builds. Adding URL to vote-this-bug page.

Comment 9

[Asa Dotzler [:asa]]

Is this win98 only? I don't see it on win2K with 110503

Comment 10

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

this is all/all

Comment 11

[Olivier Cahagne]

Confirming on Win2k with build 2001100503.

Comment 12

[John Keiser (jkeiser)]

So is this about checkboxes or radio buttons?  Or both?

That attachment (attachment 56589 [details]) seems to be well-formed HTML, is anybody
seeing the problem there?  Or is it just on showvotes?

Comment 13

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

this is checkboxes and radiobuttons AFAICT. I think the offending code is in 
nsHTMLInputElement::SetAttr, but i'm not sure. It defenetly is the fix for bug 
108198 that is causeing this (backing it out fixed the problem).

Working on a fix as hard as I can...

Comment 14

[Johnny Stenback (:jst)]

Attachment: Proposed fix, don't reset inputs when attributes are set if there's no parent (i.e. if we're called from the content sink)

Comment 15

[John Keiser (jkeiser)]

That's a rather surprising fix.  What is happening here?  Is SetPresStateChecked
unable to change the value in presentation state when there's no parent?

Comment 16

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 56611 [details] [diff] [review]
Proposed fix, don't reset inputs when attributes are set if there's no parent (i.e. if we're called from the content sink)

r=sicking

it's a hack, but at least it'll make this a non-blocker

Comment 17

[Johnny Stenback (:jst)]

What's happening here is that we're calling Reset() every time the content sink
is setting the "value" attribute on an input, that's bad, since reset will be
called before the input has gotten its "checked" attribute.

This should all be sorted out once jkeiser is done with radio and checkboxes...

Comment 18

[Christopher Blizzard (:blizzard)]

Comment on attachment 56611 [details] [diff] [review]
Proposed fix, don't reset inputs when attributes are set if there's no parent (i.e. if we're called from the content sink)

sr=blizzard

Comment 19

[Johnny Stenback (:jst)]

Fix cheked in. Leaving bug open for John to decide what he wants to do with this
bug. Reducing severity.

Comment 20

[John Keiser (jkeiser)]

Attachment: More Permanent Fix

Comment 21

[John Keiser (jkeiser)]

sicking has pointed out that we really shouldn't reset the select when it's a
checkbox or radio *anyway*.  It should only reset text/file/password/hidden. 
Since this does work for these elements (as evidenced by the fact that they were
working before this particular patch went in), a proper fix is to only reset for
those elements.

I have tested and radio buttons, checkboxes and text boxes alike are still
working with this patch.

Comment 22

[Johnny Stenback (:jst)]

Comment on attachment 56676 [details] [diff] [review]
More Permanent Fix

sr=jst.

I would've put the "&&" and "||" on the end of the previous line and not
at the beginning of the new line (which is more common in this file). Either way...

Comment 23

[Johnny Stenback (:jst)]

Comment on attachment 56676 [details] [diff] [review]
More Permanent Fix

Rick sez r=[e-mail]

Comment 24

[Gervase Markham [:gerv]]

Anyone going to check this in, then? :-)

Gerv

Comment 25

[John Keiser (jkeiser)]

Oops, yeah, fix checked in. :)

Comment 26

[Madhur Bhatia]

verified fixed in 
win2k buildID: 2001-11-19-06trunk 
linux 7.1 buildID: 2001-11-20-08trunk