1358448 - Radio group allows to select two radio items

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[dmitri_blinov]

User Agent: Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:53.0) Gecko/20100101 Firefox/53.0
Build ID: 20170413192749

Steps to reproduce:

Please look at the following fiddle:

https://jsfiddle.net/rqjec153/1/

The source to reproduce the bug is the following:

<script>
function reload() {
  var div = document.getElementById("COFhpaPuRVWcRLgrFUCG7Q");  
    div.outerHTML = '<section id="COFhpaPuRVWcRLgrFUCG7Q"><form id="COFhpaPuRVWcRLgrFUCG7Q"></form><label><input type="radio" name="Value" form="COFhpaPuRVWcRLgrFUCG7Q" value="TRACE" checked="checked"><span>TRACE</span></label><label><input type="radio" name="Value" form="COFhpaPuRVWcRLgrFUCG7Q" value="DEBUG"><span>DEBUG</span></label><label><input type="radio" name="Value" form="COFhpaPuRVWcRLgrFUCG7Q" value="INFO"><span>INFO</span></label><label><input type="radio" name="Value" form="COFhpaPuRVWcRLgrFUCG7Q" value="WARN"><span>WARN</span></label><label><input type="radio" name="Value" form="COFhpaPuRVWcRLgrFUCG7Q" value="ERROR"><span>ERROR</span></label><label><input type="radio" name="Value" form="COFhpaPuRVWcRLgrFUCG7Q" value="FATAL"><span>FATAL</span></label></section>';
}
</script>
<section id="COFhpaPuRVWcRLgrFUCG7Q"><form id="COFhpaPuRVWcRLgrFUCG7Q"></form><label><input type="radio" name="Value" form="COFhpaPuRVWcRLgrFUCG7Q" value="TRACE" checked="checked"><span>TRACE</span></label><label><input type="radio" name="Value" form="COFhpaPuRVWcRLgrFUCG7Q" value="DEBUG"><span>DEBUG</span></label><label><input type="radio" name="Value" form="COFhpaPuRVWcRLgrFUCG7Q" value="INFO"><span>INFO</span></label><label><input type="radio" name="Value" form="COFhpaPuRVWcRLgrFUCG7Q" value="WARN"><span>WARN</span></label><label><input type="radio" name="Value" form="COFhpaPuRVWcRLgrFUCG7Q" value="ERROR"><span>ERROR</span></label><label><input type="radio" name="Value" form="COFhpaPuRVWcRLgrFUCG7Q" value="FATAL"><span>FATAL</span></label></section>
<button onclick="reload()">
  reload
</button>

This problem had started to manifest itself from late 40's versions if this is of any help.


Actual results:

The initial radio group is correct as expected. When one radio item is selected the previously selected radio item is deselected. After pressing the reload button the contents of the html section is reloaded with the same markup, and now you are able to select two items in one radio group, since the initially checked item is not deselected when other items of radio group are selected. 


Expected results:

The behaviour of the radio group should not differ when it is initially loaded or reloaded via script using outerHTML. Every time only one radio item should be selected in radio group

Comment 1

[j.j.]

Attachment: reduced testcase

hmm, identical ids are invalid HTML obviously. Did it really work in late 40's, as the reporter says?

Comment 2

[Jessica Jong [:jessica] (inactive)]

Attachment: reduced valid testcase

Although this issue can't be reproduced by using unique ids, I've found the root cause of this and attached a valid testcase.

When radios belong to a form element, the container is the form element, but when moving the elements out of the form, we don't add the radios back to a container, which should be the doc now.

Comment 3

[Jessica Jong [:jessica] (inactive)]

Attachment: patch, v1.

When a radio's form is being cleared, it will be removed from the form's radio group, so it should join back to the doc's radio group. Similarly, if a radio's form is being set, it will be added to the form's radio group, so it should leave the original radio group first.

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8862333 [details] [diff] [review]
patch, v1.


>+++ b/dom/html/nsIFormControl.h
>@@ -122,18 +122,20 @@ public:
>    */
>   virtual void SetForm(nsIDOMHTMLFormElement* aForm) = 0;
> 
>   /**
>    * Tell the control to forget about its form.
>    *
>    * @param aRemoveFromForm set false if you do not want this element removed
>    *        from the form.  (Used by nsFormControlList::Clear())
>+   * @param aReleaseOrUnbind set true if the element is being released or unbind
>+   *        from tree.
>    */
>-  virtual void ClearForm(bool aRemoveFromForm) = 0;
>+  virtual void ClearForm(bool aRemoveFromForm, bool aReleaseOrUnbind) = 0;
It is not about any release but the last release right before delete, so
might be good to rename to aUnbindOrDelete or such. Same also elsewhere.


A bit hard to follow, but this code is such.

We need some testcases.

Comment 5

[Olli Pettay [:smaug][[e-mail]]]

Hopefully wpt tests

Comment 6

[Jessica Jong [:jessica] (inactive)]

(In reply to Olli Pettay [:smaug] from comment #5)
> Hopefully wpt tests

Sure, will add some wpt tests.

Comment 7

[Jessica Jong [:jessica] (inactive)]

Attachment: patch, v2.

s/aReleaseOrUnbind/aUnbindOrDelete, carrying r+.

Comment 8

[Jessica Jong [:jessica] (inactive)]

Attachment: wpt, v1.

Here come the tests.

Comment 9

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8863606 [details] [diff] [review]
wpt, v1.

>+  test(function(){
>+    radio12.remove();
Would be worth to test what is radio12.checked after remove()

>+    assert_false(radio13.checked);
>+    assert_false(radio14.checked);
>+    radio13.checked = true;
>+    assert_true(radio13.checked);
>+    assert_false(radio14.checked);
>+    radio13.removeAttribute("form");
>+    radio14.removeAttribute("form");
>+    assert_true(radio13.checked);
>+    assert_false(radio14.checked);
>+    radio14.checked = true;
>+    assert_false(radio13.checked);
>+    assert_true(radio14.checked);
>+    radio13.setAttribute("form", "testform");
>+    radio14.setAttribute("form", "testform");
>+    radio13.checked = true;
>+    assert_true(radio13.checked);
>+    assert_false(radio14.checked);

Could you add tests for removing testform from document and what is the checked state or radio buttons then.

Comment 10

[Jessica Jong [:jessica] (inactive)]

(In reply to Olli Pettay [:smaug] from comment #9)
> Comment on attachment 8863606 [details] [diff] [review]
> wpt, v1.
> 
> >+  test(function(){
> >+    radio12.remove();
> Would be worth to test what is radio12.checked after remove()

ok, radio12.checked should still be true.

> 
> >+    assert_false(radio13.checked);
> >+    assert_false(radio14.checked);
> >+    radio13.checked = true;
> >+    assert_true(radio13.checked);
> >+    assert_false(radio14.checked);
> >+    radio13.removeAttribute("form");
> >+    radio14.removeAttribute("form");
> >+    assert_true(radio13.checked);
> >+    assert_false(radio14.checked);
> >+    radio14.checked = true;
> >+    assert_false(radio13.checked);
> >+    assert_true(radio14.checked);
> >+    radio13.setAttribute("form", "testform");
> >+    radio14.setAttribute("form", "testform");
> >+    radio13.checked = true;
> >+    assert_true(radio13.checked);
> >+    assert_false(radio14.checked);
> 
> Could you add tests for removing testform from document and what is the
> checked state or radio buttons then.

ok, radio buttons should still have the same checked state.

Comment 11

[Jessica Jong [:jessica] (inactive)]

Attachment: wpt, v2.

added more tests, see comment 9.

Comment 12

[Jessica Jong [:jessica] (inactive)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=ebf5db14ae86813d1cb9c6acc8c4a49bb997b553&group_state=expanded

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/041312e3df86
Add radio back to a radio group after moving out of a form. r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/48aec7bc9deb
Add wpt for cases when moving radio out of or into a form. r=smaug

Comment 14

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/041312e3df86
https://hg.mozilla.org/mozilla-central/rev/48aec7bc9deb