1685926 - Radio buttons with no form owner are not grouped

Status: RESOLVED FIXED

(Core :: DOM: Forms, defect, P2)

Description

[matt]

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 11_1_0) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/87.0.4280.141 Safari/537.36

Steps to reproduce:

I created a series of tests to address issue #2551 (https://github.com/web-platform-tests/wpt/issues/2551) of the Web Platform Tests project. One of the tests is failing when run against Firefox 86. The test consists of two radio button inputs that should be grouped according to the criteria in the HTML spec (type=radio, same tree, identical name attribute values, and no form owner).

The test results can be viewed in the Firefox wpt.fyi link for the WPT pull request (https://github.com/web-platform-tests/wpt/pull/27029). The relevant test is labeled "Radio buttons in different groups (because they have different form owners or no form owner) do not affect each other's checkedness".

Actual results:

The test reveals that two radio buttons meeting all the criteria to be in the same group (type=radio, same tree, identical name attribute values, and no form owner) are not being properly grouped. The checked status of the two buttons is not mutually exclusive.

Link to the test results is https://wpt.fyi/results/html/semantics/forms/the-input-element/radio.html?diff&filter=ADC&run_id=5755001215385600&run_id=5737665720745984

Expected results:

Radio buttons meeting all the criteria to be in the same group (in particular the scenario where type=radio, same tree, identical name attribute values, and no form owner), should have mutually exclusive checked status values. When one of the buttons in the group is checked, the others should become unchecked.

Comment 1

[matt]

I did do a search prior to creating this bug and determined that there is a an existing bug that seems to be identical, but it is has a status RESOLVED FIXED.

Comment 2

[Anne (:annevk)]

I suspect this regressed somewhere in the past 18 years then. 😊

Comment 3

[Kagami Rosylight [:saschanaz] (they/them)]

This is because GetRadioGroupContainer() depends on the root node being either Document or ShadowRoot, and thus returns nullptr for disconnected nodes. nsIRadioGroupContainer currently is only implemented by Document, ShadowRoot and HTMLFormElement, and the fix may:

1. Make every node inherit nsIRadioGroupContainer, or
2. Modify nsIRadioGroupContainer implementation to group radio buttons also by root node.

Comment 6

[Kagami Rosylight [:saschanaz] (they/them)]

Note that the tests fail on both Gecko and WebKit: https://wpt.fyi/results/html/semantics/forms/the-input-element/radio.html?label=experimental&label=master&aligned

Comment 8

[Kagami Rosylight [:saschanaz] (they/them)]

Not working on this now, please feel free to take it.

Comment 9

[BugBot [:suhaib / :marco/ :calixte]]

The severity field for this bug is relatively low, S3. However, the bug has 3 duplicates.
:kmag, could you consider increasing the bug severity?

For more information, please visit auto_nag documentation.

Comment 10

[Kris Maglione [:kmag]]

I'm still inclined to leave this at P3. I'm not sure that it matters much in the real world, and WebKit apparently has the same behavior. But it could in principle cause problems when people are constructing dynamic DOM trees without <form> elements and then manipulating the input values before inserting it into the document. It wouldn't be a problem when the elements are within a <form> element in the orphan tree or document fragment, which I would hope to be the usual case.

But given that it works on Chrome, maybe we should make it higher priority for compatibility reasons, especially given that it's been reported more than once.

Comment 11

[Kris Maglione [:kmag]]

Moving back to S3 after some offline discussion. Two of the dupes are about spec inconsistencies and WPT failures. Only one is from an actual user, and I'm not convinced that this will actually come up very often in the real world. We should still get it fixed, though, and hopefully it shouldn't be too difficult.

Comment 12

[Hsin-Yi Tsai (she/her) away [:hsinyi]]

(In reply to Kagami :saschanaz from comment #6)

Note that the tests fail on both Gecko and WebKit: https://wpt.fyi/results/html/semantics/forms/the-input-element/radio.html?label=experimental&label=master&aligned
Tests pass on Webkit already.

Comment 13

[Hsin-Yi Tsai (she/her) away [:hsinyi]]

Adam is looking into this.

Comment 14

[Adam Vandolder [:avandolder]]

Attachment: Bug 1685926 - Group disconnected radio buttons together. r?saschanaz,#dom-core

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/37ade77d181e
Group disconnected radio buttons together. r=saschanaz,smaug

Comment 16

[Cristian Tuns]

Backed out for causing build bustages in PerformanceEventTiming.cpp

• Backout link
• Push with failures
• Failure Log
• Failure line: /builds/worker/checkouts/gecko/dom/performance/PerformanceEventTiming.cpp:66:21: error: no member named 'dom_enable_event_timing' in namespace 'mozilla::StaticPrefs'

Comment 17

[Kagami Rosylight [:saschanaz] (they/them)]

Heh, it lacked the proper header forever since bug 1667836!

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/8f8492410799
Group disconnected radio buttons together. r=saschanaz,smaug

Comment 19

[Natalia Csoregi [:nataliaCs]]

Backed out for causing bustage on SVGAnimatedPathSegList.cpp

• backout: https://hg.mozilla.org/integration/autoland/rev/7d6ecf4cbe3727220b6ae83ccdf2d33118a4743e
• push: https://treeherder.mozilla.org/jobs?repo=autoland&group_state=expanded&revision=8f84924107993a8749b11d74bb70c84e5aab81d3&selectedTaskRun=Opz7_uoAQwKyKOiiSRnwSA.1
• failure log: https://treeherder.mozilla.org/logviewer?job_id=430606272&repo=autoland&lineNumber=25782

[task 2023-09-28T07:24:27.383Z] 07:24:27     INFO -  /builds/worker/fetches/sccache/sccache /builds/worker/fetches/clang/bin/clang++ --sysroot /builds/worker/fetches/sysroot-x86_64-linux-gnu -o SVGAnimatedPathSegList.o -c  -I/builds/worker/workspace/obj-build/dist/stl_wrappers -I/builds/worker/workspace/obj-build/dist/system_wrappers -include /builds/worker/checkouts/gecko/config/gcc_hidden.h -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=2 -fstack-protector-strong -fstack-clash-protection -DNDEBUG=1 -DTRIMMED=1 -DMOZ_HAS_MOZGLUE -DMOZILLA_INTERNAL_API -DIMPL_LIBXUL -DSTATIC_EXPORTABLE_JS_API -I/builds/worker/checkouts/gecko/dom/svg -I/builds/worker/workspace/obj-build/dom/svg -I/builds/worker/workspace/obj-build/ipc/ipdl/_ipdlheaders -I/builds/worker/checkouts/gecko/ipc/chromium/src -I/builds/worker/checkouts/gecko/dom -I/builds/worker/checkouts/gecko/dom/base -I/builds/worker/checkouts/gecko/dom/html -I/builds/worker/checkouts/gecko/dom/smil -I/builds/worker/checkouts/gecko/dom/svg -I/builds/worker/checkouts/gecko/dom/xml -I/builds/worker/checkouts/gecko/layout/base -I/builds/worker/checkouts/gecko/layout/generic -I/builds/worker/checkouts/gecko/layout/style -I/builds/worker/checkouts/gecko/layout/svg -I/builds/worker/checkouts/gecko/layout/xul -I/builds/worker/workspace/obj-build/dist/include -I/builds/worker/workspace/obj-build/dist/include/nspr -I/builds/worker/workspace/obj-build/dist/include/nss -DMOZILLA_CLIENT -include /builds/worker/workspace/obj-build/mozilla-config.h -D_GLIBCXX_USE_CXX11_ABI=0 -fno-sized-deallocation -fno-aligned-new -fcrash-diagnostics-dir=/builds/worker/artifacts -fno-exceptions -fPIC -fno-rtti -ffunction-sections -fdata-sections -fno-exceptions -fno-math-errno -pthread -pipe -gdwarf-4 -Xclang -load -Xclang /builds/worker/workspace/obj-build/build/clang-plugin/libclang-plugin.so -Xclang -add-plugin -Xclang moz-check -O2 -fno-omit-frame-pointer -funwind-tables -Werror -Wall -Wbitfield-enum-conversion -Wdeprecated-this-capture -Wempty-body -Wformat-type-confusion -Wignored-qualifiers -Wpointer-arith -Wshadow-field-in-constructor-modified -Wsign-compare -Wtautological-constant-in-range-compare -Wtype-limits -Wno-error=tautological-type-limit-compare -Wunreachable-code -Wunreachable-code-return -Wunused-but-set-parameter -Wno-invalid-offsetof -Wclass-varargs -Wempty-init-stmt -Wfloat-overflow-conversion -Wfloat-zero-conversion -Wloop-analysis -Wno-range-loop-analysis -Wc++2a-compat -Wenum-compare-conditional -Wenum-float-conversion -Wno-error=deprecated -Wno-error=deprecated-anon-enum-enum-conversion -Wno-error=deprecated-enum-enum-conversion -Wno-error=deprecated-pragma -Wno-error=deprecated-this-capture -Wcomma -Wimplicit-fallthrough -Wstring-conversion -Wno-inline-new-delete -Wno-error=deprecated-declarations -Wno-error=array-bounds -Wno-error=free-nonheap-object -Wno-error=atomic-alignment -Wno-error=deprecated-builtins -Wformat -Wformat-security -Wno-psabi -Wthread-safety -Wno-error=builtin-macro-redefined -Wno-unknown-warning-option -fno-strict-aliasing -ffp-contract=off  -MD -MP -MF .deps/SVGAnimatedPathSegList.o.pp   /builds/worker/checkouts/gecko/dom/svg/SVGAnimatedPathSegList.cpp
[task 2023-09-28T07:24:27.384Z] 07:24:27    ERROR -  /builds/worker/checkouts/gecko/dom/svg/SVGAnimatedPathSegList.cpp:37:20: error: no member named 'dom_svg_pathSeg_enabled' in namespace 'mozilla::StaticPrefs'
[task 2023-09-28T07:24:27.384Z] 07:24:27     INFO -     37 |   if (StaticPrefs::dom_svg_pathSeg_enabled()) {
[task 2023-09-28T07:24:27.384Z] 07:24:27     INFO -        |       ~~~~~~~~~~~~~^
[task 2023-09-28T07:24:27.384Z] 07:24:27    ERROR -  /builds/worker/checkouts/gecko/dom/svg/SVGAnimatedPathSegList.cpp:63:20: error: no member named 'dom_svg_pathSeg_enabled' in namespace 'mozilla::StaticPrefs'
[task 2023-09-28T07:24:27.384Z] 07:24:27     INFO -     63 |   if (StaticPrefs::dom_svg_pathSeg_enabled()) {
[task 2023-09-28T07:24:27.384Z] 07:24:27     INFO -        |       ~~~~~~~~~~~~~^
[task 2023-09-28T07:24:27.384Z] 07:24:27    ERROR -  /builds/worker/checkouts/gecko/dom/svg/SVGAnimatedPathSegList.cpp:98:20: error: no member named 'dom_svg_pathSeg_enabled' in namespace 'mozilla::StaticPrefs'
[task 2023-09-28T07:24:27.385Z] 07:24:27     INFO -     98 |   if (StaticPrefs::dom_svg_pathSeg_enabled()) {
[task 2023-09-28T07:24:27.385Z] 07:24:27     INFO -        |       ~~~~~~~~~~~~~^
[task 2023-09-28T07:24:27.385Z] 07:24:27    ERROR -  /builds/worker/checkouts/gecko/dom/svg/SVGAnimatedPathSegList.cpp:121:20: error: no member named 'dom_svg_pathSeg_enabled' in namespace 'mozilla::StaticPrefs'
[task 2023-09-28T07:24:27.385Z] 07:24:27     INFO -    121 |   if (StaticPrefs::dom_svg_pathSeg_enabled()) {
[task 2023-09-28T07:24:27.385Z] 07:24:27     INFO -        |       ~~~~~~~~~~~~~^
[task 2023-09-28T07:24:27.385Z] 07:24:27     INFO -  4 errors generated.
[task 2023-09-28T07:24:27.385Z] 07:24:27    ERROR -  gmake[4]: *** [/builds/worker/checkouts/gecko/config/rules.mk:659: SVGAnimatedPathSegList.o] Error 1

Comment 20

[Kagami Rosylight [:saschanaz] (they/them)]

Oh no, this is why we need to get misc-include-cleaner back. (Removed by bug 1838396 because it was too aggressive)

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/64e79cecbacd
Group disconnected radio buttons together. r=saschanaz,smaug

Comment 22

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/64e79cecbacd

Comment 23

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/42240 for changes under testing/web-platform/tests

Comment 24

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Upstream PR merged by moz-wptsync-bot