Radio group allows to select two radio items

RESOLVED FIXED in Firefox 55

Status

()

Core
DOM: Core & HTML
RESOLVED FIXED
8 months ago
8 months ago

People

(Reporter: dmitri_blinov, Assigned: jessica)

Tracking

11 Branch
mozilla55
Points:
---

Firefox Tracking Flags

(firefox55 fixed)

Details

Attachments

(4 attachments, 2 obsolete attachments)

(Reporter)

Description

8 months ago
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

Updated

8 months ago
Blocks: 92264
status-firefox55: --- → affected
Component: Untriaged → Layout: Form Controls
Product: Firefox → Core
Version: 53 Branch → 11 Branch

Updated

8 months ago
Status: UNCONFIRMED → NEW
Ever confirmed: true

Updated

8 months ago
Has Regression Range: --- → yes
Has STR: --- → yes

Comment 1

8 months ago
Created attachment 8860384 [details]
reduced testcase

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

Comment 2

8 months ago
Created attachment 8860850 [details]
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.
(Assignee)

Updated

8 months ago
Assignee: nobody → jjong
Component: Layout: Form Controls → DOM: Core & HTML
(Assignee)

Comment 3

8 months ago
Created attachment 8862333 [details] [diff] [review]
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.
Attachment #8862333 - Flags: review?(bugs)
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.
Attachment #8862333 - Flags: review?(bugs) → review+
Hopefully wpt tests
(Assignee)

Comment 6

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

Sure, will add some wpt tests.
(Assignee)

Comment 7

8 months ago
Created attachment 8863605 [details] [diff] [review]
patch, v2.

s/aReleaseOrUnbind/aUnbindOrDelete, carrying r+.
Attachment #8862333 - Attachment is obsolete: true
Attachment #8863605 - Flags: review+
(Assignee)

Comment 8

8 months ago
Created attachment 8863606 [details] [diff] [review]
wpt, v1.

Here come the tests.
Attachment #8863606 - Flags: review?(bugs)
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.
Attachment #8863606 - Flags: review?(bugs) → review+
(Assignee)

Comment 10

8 months ago
(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.
(Assignee)

Comment 11

8 months ago
Created attachment 8864000 [details] [diff] [review]
wpt, v2.

added more tests, see comment 9.
Attachment #8863606 - Attachment is obsolete: true
Attachment #8864000 - Flags: review+
(Assignee)

Comment 12

8 months ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=ebf5db14ae86813d1cb9c6acc8c4a49bb997b553&group_state=expanded
Keywords: checkin-needed

Comment 13

8 months ago
Pushed by cbook@mozilla.com:
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
Keywords: checkin-needed

Comment 14

8 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/041312e3df86
https://hg.mozilla.org/mozilla-central/rev/48aec7bc9deb
Status: NEW → RESOLVED
Last Resolved: 8 months ago
status-firefox55: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.