1879655 - Radio button of the Satisfaction Survey on Shopping Sidebar are missing "name" attribute and radiorgoup/fieldset container

Status: VERIFIED FIXED

(Firefox :: Messaging System, defect, P1)

Description

[Anna Yeddi [:ayeddi]]

Each one of radio buttons in the survey receives keyboard focus separately while it is expected for a user to Tab to the first radio button (or to the selected one, if selection is present) and then use arrow keys to change the selection and then Tab away from the radio group.

Also, the radio group is not related to the question, thus fieldset or, at minimum, ARIA group markup should be utilized.

Comment 1

[Anna Yeddi [:ayeddi]]

Attachment: radio group on the survey.png

Adding name attribute to each radio button would mark them up as one radio group: https://html.spec.whatwg.org/multipage/input.html#radio-button-state-(type=radio)

Comment 2

[Shane Hughes [:aminomancer]]

Try dynamically adding a fieldset once we know how many radio groups there are.

See MRColorways example.

Comment 3

[Shane Hughes [:aminomancer]]

The radio buttons in this message should have had group to start with. This is already supported. That doesn't fix the issue of relating the question text to the radio buttons, which I suppose may force us to use fieldset.

Comment 4

[James Teh [:Jamie]]

(In reply to Anna Yeddi [:ayeddi] from comment #0)

Each one of radio buttons in the survey receives keyboard focus separately while it is expected for a user to Tab to the first radio button (or to the selected one, if selection is present) and then use arrow keys to change the selection and then Tab away from the radio group.

Interestingly, this is not how Gecko has behaved forever. When no radio button is selected, Gecko always tabs through all radio buttons:

data:text/html,<button>a</button><label>b<input type="radio" name="r"></label><label>c<input type="radio" name="r"></label><button>d</button>

This was long before my time here, but I guess this is because choosing the first seemed arbitrary and biased the user to the first; i.e. "if we don't have info to choose which control to tab to, don't choose one at all". FWIW, I tend to agree that this is not great for users and I'd support changing this, but that's something we'd need to take up with DOM/widget folks.

As a comparison, Chrome doesn't tab through all radios, but it's asymmetric. When you tab forward into the group, it focuses the first. When you shift+tab into the group, it chooses the last. I don't like asymmetry. :( If we're going to choose to focus a single thing, I'd prefer we were consistent, even if that is biased strictly speaking.

Comment 5

[Anna Yeddi [:ayeddi]]

(In reply to James Teh [:Jamie] from comment #4)

(In reply to Anna Yeddi [:ayeddi] from comment #0)

Each one of radio buttons in the survey receives keyboard focus separately while it is expected for a user to Tab to the first radio button (or to the selected one, if selection is present) and then use arrow keys to change the selection and then Tab away from the radio group.

Interestingly, this is not how Gecko has behaved forever. When no radio button is selected, Gecko always tabs through all radio buttons:

data:text/html,<button>a</button><label>b<input type="radio" name="r"></label><label>c<input type="radio" name="r"></label><button>d</button>

This was long before my time here, but I guess this is because choosing the first seemed arbitrary and biased the user to the first; i.e. "if we don't have info to choose which control to tab to, don't choose one at all". FWIW, I tend to agree that this is not great for users and I'd support changing this, but that's something we'd need to take up with DOM/widget folks.

As a comparison, Chrome doesn't tab through all radios, but it's asymmetric. When you tab forward into the group, it focuses the first. When you shift+tab into the group, it chooses the last. I don't like asymmetry. :( If we're going to choose to focus a single thing, I'd prefer we were consistent, even if that is biased strictly speaking.

Thank you for the details on the Gecko behavior, Jamie! TIL.

There are 2 bugs touching this subject: bug 1413213 and bug 1267488 - I'll mark them as blocking for the keyboard navigation part of the bug so thus bug could concentrate on the grouping (role="radiogroup", I believe would be better fitting here, ref to the ARIA example)

Comment 6

[Shane Hughes [:aminomancer]]

I did some more investigation and found that the radio buttons do already have name attributes. The name attribute is added here for each radio button, provided it has a group property. The survey radio buttons have that property, defined here. I think that's just not enough to form a group for a11y tools.

It's a bit confusing, because name is what we use to ensure only 1 button per group is checked and can be tab focused, but it seems to have no bearing on the accessible structure, which is apparently based solely on the ARIA role of ancestors.

This is unfortunate for our purposes because, to keep the radiogroup component generic, we don't want to add a <legend> element. Our original goal in creating the "radiogroup" component/template was that it should have purely radio buttons and/or checkboxes for children, because it's not specifically a "survey" component or a "question/answer" component. It's a reusable, generic "multi-select" component. We don't even really want to add role="radiogroup" because it's not strictly a radio group, as it can have checkboxes too or instead. But it doesn't cost us much to add role="radiogroup" dynamically when the message includes radio buttons, so I think we can do that. But yeah, ideally we'd keep the question outside of the multi-select component, which basically rules out fieldset + legend.

An easy workaround I played around with is this:

<h1 class="title">Help improve Firefox</h1>
<h2 id="question" class="subtitle">How satisfied are you with the Review Checker experience in Firefox?</h2>
<div
  class="multi-select-container"
  role="radiogroup"
  aria-labelledby="question"
  >
  <!-- radio buttons... -->
</div>

But I'm not sure if this is satisfactory, because it means both #question and .multi-select-container will have the same accessible label: "How satisfied are you with the Review Checker experience in Firefox?" So a screen reader might read that line twice (though thankfully the h2 is not focusable). I guess this is to some extent a problem either way. With fieldset + legend, the same thing could happen, but at least the legend is structurally inside the fieldset. So semantically that makes the most sense for this specific use case.

It's just problematic for the Messaging System, because all our creations revolve around reusable components and generic templates. So we don't have the luxury of doing a bespoke solution for every individual UI application; we're able to work really fast and create a high volume of output because we use the most generic templates possible, allowing these kinds of things to deployed remotely without landing any code. That's why we're hesitant to add a "survey" component, since it's one more component we'll need to maintain. If we can get 95% of the results we want with a more basic "radio/checkbox group" component + the "subtitle" component, which also works for other non-survey applications like the choices in about:welcome, then that's always preferrable.

And refactoring the radio/checkbox group component so that it's a fieldset with a visible legend inside, would make it less generic/reusable. I wouldn't rule it out entirely though, I just want to run the options by you Anna, and get a sense of whether you think we could solve this problem without changing where the subtitle/h2 is in the DOM hierarchy. If you're OK with the HTML example above, then that's highly preferrable for our purposes. But if that hierarchy is unacceptable then I suppose we will have no choice but to switch from using an h2 outside the radiogroup to a legend inside the radiogroup.

I also tested out fieldset + legend, and it works similarly, the heading just ends up inside the radiogroup instead of before the radiogroup. In some ways fieldset actually seems worse, since its implicit role is group, not radiogroup?

<h1 class="title">Help improve Firefox</h1>
<fieldset
  class="multi-select-container"
  aria-labelledby="question"
  >
  <legend id="question" class="subtitle">How satisfied are you with the Review Checker experience in Firefox?</legend>
  <!-- radio buttons... -->
</fieldset>

In this example, I still added aria-labelledby to the fieldset, because without it, its label is "", and the question only shows up in the label of the legend, which is now the first child of the fieldset. That might be fine though - I don't know much about how these are supposed to behave.

Comment 7

[Shane Hughes [:aminomancer]]

Attachment: Bug 1879655 - Fix microsurvey group structure, a11y, and randomization. r=#omc-reviewers

Change how microsurveys are structured. This ensures that screen readers
perceive a single logical collection, which contains all the radio
buttons and is labeled by the question, which is no longer defined by
subtitle but by tiles.label. This also changes how survey randomization
works. Instead of randomizing the entire set, we randomize specific
items. Any adjacent items with randomize will be randomized in-place. So
if there are 4 items with randomize, followed by 1 nonrandom item, the 4
will be randomized but the 5th will stay at the bottom. Finally, this
patch saves the randomized order so that it persists between back and
forward navigation on about:welcome. That should avoid some jank if we
show surveys in about:welcome.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a3c8904349c4
Fix microsurvey group structure, a11y, and randomization. r=omc-reviewers,emcminn

Comment 9

[Shane Hughes [:aminomancer]]

Comment on attachment 9382162 [details]
Bug 1879655 - Fix microsurvey group structure, a11y, and randomization. r=#omc-reviewers

Beta/Release Uplift Approval Request

• User impact if declined: The survey in the Review Checker Callouts v1.1 experiment will have some accessibility issues. Screen readers won't understand that the question is related to the answers, which could be confusing.
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): The patch only impacts an experimental treatment group, changing the DOM for a feature callout slightly and adding ARIA attributes where needed. The experiment's components have been verified by QA in Fx 123, but in the unlikely event something goes wrong, it can be pulled at a moment's notice. The attached nightly patch currently grafts cleanly to mozilla-beta.
• String changes made/needed:
• Is Android affected?: No

Comment 10

[Shane Hughes [:aminomancer]]

[Tracking Requested - why for this release]: See beta uplift approval request in comment 9

Comment 11

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/a3c8904349c4

Comment 12

[Rares Doghi, Desktop QA]

Verified as fixed in our latest Nightly build 125.0a1 (2024-02-28) .

Comment 13

[Dianna Smith [:diannaS] - on Leave, back on August 9th]

Comment on attachment 9382162 [details]
Bug 1879655 - Fix microsurvey group structure, a11y, and randomization. r=#omc-reviewers

Approved for 124.0b6

Comment 14

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-beta/rev/167bca2c3d7a

Comment 15

[Rares Doghi, Desktop QA]

Verified as fixed in our latest Beta 124.0b6 (64-bit).