1837540 - Keyboard navigation order of radio buttons is not guaranteed to be in tree-order

Status: RESOLVED FIXED

(Core :: DOM: UI Events & Focus Handling, defect)

Description

[Bart Nagel]

Attachment: bug test case HTML

User Agent: Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:109.0) Gecko/20100101 Firefox/113.0

Steps to reproduce:

See live test cases at http://firefox-radio-order-bug-testcase.surge.sh/ (also attached) showing buggy and working cases. See source. There is one buggy form, and two working forms, with the bug worked around in different ways.

Scenario:

I have a form element with an ID.
Before, outside this form element in DOM order are some radio controls.

Example:
<label><input type="radio" name="radio-group" value="1" form="myform">1</label>
<label><input type="radio" name="radio-group" value="2" form="myform">2</label>
...
<form id="myform"></form>

I am navigating the form with the keyboard: tab, shift-tab, and cursors.

Actual results:

The navigation order of the radio buttons when moving between them with cursor keys seems random, and is different each time I refresh the page. Tab order between radio groups is fine.

Expected results:

Predictable, logical order when navigating between radio buttons with the keyboard.

---

Known workarounds:

1. Putting the form element before the inputs (and still referencing it with the form="formid" attribute).
2. Putting the input elements inside the form tag, and not using the form="formid" attribute.

Both of these are demonstrated in the test case file.

Comment 1

[Bart Nagel]

Bug does not exist in Chrome.

Comment 2

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

The Bugbug bot thinks this bug should belong to the 'Core::DOM: Core & HTML' component, and is moving the bug to that component. Please correct in case you think the bot is wrong.

Comment 3

[Edgar Chen [:edgar]]

I could also reproduce this issue on 114, but not on 80, I will try to find a regression window.

Comment 4

[Alice0775 White]

Regression window:
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=deaebc1c5c0e813b9df285903a5195f056c231a8&tochange=75e6eca97890cf3b5e4ee753daca5abae683397d

Comment 5

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

Set release status flags based on info from the regressing bug 1707126

:saschanaz, since you are the author of the regressor, bug 1707126, could you take a look? Also, could you set the severity field?

For more information, please visit BugBot documentation.

Comment 6

[Edgar Chen [:edgar]]

(In reply to Alice0775 White from comment #4)

Regression window:
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=deaebc1c5c0e813b9df285903a5195f056c231a8&tochange=75e6eca97890cf3b5e4ee753daca5abae683397d

Thanks, Alice!

Comment 7

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

Hmm, it has been a while since I last touched radio buttons. Edgar, is this something in your current area that you can take a look? If not I can at least put it in my backlog.

Comment 8

[Edgar Chen [:edgar]]

Adam is working on radio group recently, maybe he has some idea.
(If not, feel free to bounce back to me)

Comment 9

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

Set release status flags based on info from the regressing bug 1707126

Comment 10

[Adam Vandolder [:avandolder]]

The issue appears to be that prior to D113589, when getting the next radio button, a form element would look up the corresponding buttons within its control collection, which had the buttons stored in order of appearance within the document. Now, however, it uses the same lookup as documents, which gets the buttons from the RadioGroupManagers table. When the form comes after a button, the button is added to this table via a ContentChangeCallback which is stored in an IdentifierMap, and since iteration over a hashmap is non-deterministic, so is the order the buttons get added to the group.

It should be possible to change the callbacks to be stored in a fixed-order structure instead, so the iteration is always deterministic.

Comment 11

[Adam Vandolder [:avandolder]]

Attachment: Bug 1837540 - Make radio button group navigation follow tree-order. r=#dom-core

Previously, the order was non-deterministic due to the callbacks being
iterated in the order they were stored in a hashtable. This patch adds a
LinkedList over the callback entries, tracking their order of insertion
while still making them available through a hashtable.

Comment 12

[Adam Vandolder [:avandolder]]

Attachment: Larger test case showing additional failing cases

So it turns out this bug is actually a larger issue. Currently, no in cases is tree-order being checked during radio button navigation, and so if buttons are moved or added in between existing ones in a group, the keyboard navigation will still follow the order the elements were inserted. This occurs whether the form element comes after the inputs or not, and also if the inputs are not part of a form.

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/013278adabc7
Make radio button group navigation follow tree-order. r=sefeng,smaug

Comment 14

[Narcis Beleuzu [:NarcisB]]

Backed out for build bustages on mochitest.ini

Backout link: https://hg.mozilla.org/integration/autoland/rev/63736f30e2c65a00bc23ba7317b581b5f9f4a53f
Log link: https://treeherder.mozilla.org/logviewer?job_id=424712302&repo=autoland&lineNumber=1106

Comment 15

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

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

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/855e68839009
Make radio button group navigation follow tree-order. r=sefeng,smaug

Comment 17

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

Upstream PR was closed without merging

Comment 18

[Atila Butkovits]

https://hg.mozilla.org/mozilla-central/rev/855e68839009

Comment 19

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

Upstream PR merged by moz-wptsync-bot

Comment 20

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

The patch landed in nightly and beta is affected.
:avandolder, is this bug important enough to require an uplift?

• If yes, please nominate the patch for beta approval.
• If no, please set status-firefox117 to wontfix.

For more information, please visit BugBot documentation.