Devographics / Monorepo

Monorepo containing the State of JS apps
surveyform-sigma.vercel.app
Other
127 stars 52 forks source link

Survey: Interested / Not Interested radio buttons use invalid nesting #308

Open aardrian opened 1 year ago

aardrian commented 1 year ago

This is the HTML for the pairs of "Interested" / "Not Interested" radio buttons that follow each response in the survey:

<fieldset class="followups-v2 sentiments none-selected">
 <legend class="sr-only">
  <span data-key="followups.description.short" class="i18n-message  t">Tell us more:</span>
 </legend>
 <label class="followups-predefined-label2 sentiment positive is-not-selected" role="radio">
  <input type="radio" id="html2023__features__media_capture__experience.followup.0" class="visually-hidden form-check-input">
  <span class="sentiment-icon" aria-hidden="true"></span>
  <span data-key="followups.sentiment_interested" class="i18n-message sentiment-label t">Interested</span>
 </label>
 <label class="followups-predefined-label2 sentiment negative is-not-selected" role="radio">
  <input type="radio" id="html2023__features__media_capture__experience.followup.1" class="visually-hidden form-check-input">
  <span class="sentiment-icon" aria-hidden="true"></span>
  <span data-key="followups.sentiment_not_interested" class="i18n-message sentiment-label t">Not interested</span>
 </label>
</fieldset>

This construct is the problem:

<label […] role="radio">
  <input type="radio" […]>
  […]
</label>

This produces nested radio buttons. Not only is this an invalid HTML construct, it is also a WCAG SC 4.1.2 failure.

In action, the effects may seem to be more annoying than anything, but they can make it problematic for screen reader users to fill out this form (especially when paired with the other issues I filed). This video shows that the radio buttons are announced twice, though more accurately the radio button is announced and then its nested radio button is announced (one offers a count, the other does not). See them at 0:11 and 0:14.

https://github.com/Devographics/Monorepo/assets/1376607/58dedb70-bff5-4c9e-baa2-9e84c4f75b45

There are two distinct patterns for radio buttons on this very first page of the survey, one has this invalid nesting issue and the other has a different invalid nesting issue impacting group labels (https://github.com/Devographics/Monorepo/issues/307). Running an automated accessibility test (using axe) found 188 issues and could have caught these.

SachaG commented 1 year ago

This construct is the problem:

Is the issue the role="radio" part? I added that because when the "Interested" / "Not Interested" buttons are hidden, we give them role="presentation" so they are not read out loud. But maybe when they're visible they should just not have a role at all then?

aardrian commented 1 year ago

Yes, the radio role does not belong there and definitely cannot have any interactive content within it.

If you gave the radio inputs a presentation role that would only have kept them from being announced when the screen reader is in forms mode (context). A screen reader user would still encounter the text when otherwise moving around the page (such as when they drop out of forms mode to try to get to the third option they cannot get to using arrow keys; see related bug).

Consider always keeping those radios visible, perhaps making them disabled until the corresponding "parent" radio is selected.

SachaG commented 1 year ago

We wanted to hide them visually to avoid cluttering up the page even more. I think we chose to hide them using opacity and not just remove them from the DOM to avoid any UI jumps when you do select a parent radio button or hover over the row.

In that context what would be the best way to make sure screen readers don't read them out loud when they're not visible onscreen?

LeaVerou commented 1 year ago

We cannot make them visible, but that’s beside the point. Why is it a problem if they are announced by SR? They are, semantically, radios after all. It’s just two radios with two labels, everything else is visuals. So it shouldn’t need ARIA at all to function properly, not sure we need to override any roles. It seems to me we just need to ensure they are not read out when the corresponding parent radio is not selected, which I believe is already the case. So perhaps we can fix this by just removing the superfluous roles altogether. Does that make sense @aardrian?

cc @LJWatson

aardrian commented 1 year ago

@SachaG

We wanted to hide them visually to avoid cluttering up the page even more. I think we chose to hide them using opacity and not just remove them from the DOM to avoid any UI jumps when you do select a parent radio button or hover over the row.

You may have wanted the visibility property. Here is a useful nugget from Scott O'Hara (not tagging him because I am not pestering him on his weekend to ask for free labor):

The visibility property is an interesting one. When applied to an element with the hidden value, it will result in the element and its contents being visually hidden, though it still takes up the “space” that it would in the rendered web page. It’s just like one big visual hole and is similar to if you had applied opacity: 0 to that same content. However, unlike with opacity, visibility: hidden will result in that content not being exposed by the browser’s accessibility API (i.e., it won’t show up in the a11y tree). This is similar behavior to display: none which also hides content from the a11y tree.

What makes visibility even more different, and useful here, is that you can make parts of a visibility: hidden subtree visible again.

@LeaVerou

We cannot make them visible, but that’s beside the point.

I assume that is a design requirement?

Why is it a problem if they are announced by SR? They are, semantically, radios after all. It’s just two radios with two labels, everything else is visuals. So it shouldn’t need ARIA at all to function properly, not sure we need to override any roles.

The issue is that there is an element that uses ARIA to make it into a programmatic radio, and that element contains an HTM radio. Which means nested interactives. That manifests with screen readers with a double announcement (once for a stand-alone non-grouped non-actionable fake radio and again for its nested real radio). That may cause actual operability bugs (I only tested in NVDA/Firefox because weekend).

It seems to me we just need to ensure they are not read out when the corresponding parent radio is not selected, which I believe is already the case. So perhaps we can fix this by just removing the superfluous roles altogether. Does that make sense @aardrian?

To clarify, there is a radio group ("Never heard of it", "Heard of it", "Used it"). Each radio within that group has another radio group. Within that nested radio group is the broken nested radio construct.

So yes, the simplest fix is to remove the role from the <label>. Which it looks like someone did overnight and I have been typing all this for no reason.

Bear in mind, with the current construct I can still move through the visually-hidden radio buttons on the unselected parent question. Should I file a new issue for that or do you want to try Scott's method to programmatically hide them?

Or, perhaps leave them visible and disabled until the parent radio is selected.

LeaVerou commented 1 year ago

@SachaG

We wanted to hide them visually to avoid cluttering up the page even more. I think we chose to hide them using opacity and not just remove them from the DOM to avoid any UI jumps when you do select a parent radio button or hover over the row.

You may have wanted the visibility property.

The visibility property does not allow for transitions (I'm not sure if we're using any right now, but I think that was the intent behind using opacity).

(not tagging him because I am not pestering him on his weekend to ask for free labor):

I wouldn't expect that applies to asynchronous communication: when people are not working on the weekend, they typically don't get notified about GitHub comments either. 😄 This is a global medium, so even if it's the weekend for you, it doesn't mean it's necessarily the weekend for everyone.

But I don't think tagging him is necessary either, his blog post is pretty clear. For context, I looped @LJWatson in because she had provided feedback on an earlier version of the UI, @SachaG made some changes to address it, and I've realized we never checked back to make sure that actually improved things. 😄

@LeaVerou

We cannot make them visible, but that’s beside the point.

I assume that is a design requirement?

Both a design and UX requirement. But also, philosophically, I've always advocated that the web platform's accessibility APIs are powerful enough that screen reader accessibility can almost always be achieved without compromising visual design or UX for sighted users. Do you think that's not accurate?

The issue is that there is an element that uses ARIA to make it into a programmatic radio, and that element contains an HTM radio. Which means nested interactives. That manifests with screen readers with a double announcement (once for a stand-alone non-grouped non-actionable fake radio and again for its nested real radio). That may cause actual operability bugs (I only tested in NVDA/Firefox because weekend).

To clarify, there is a radio group ("Never heard of it", "Heard of it", "Used it"). Each radio within that group has another radio group. Within that nested radio group is the broken nested radio construct.

So yes, the simplest fix is to remove the role from the <label>.

Yup! I wasn't disagreeing with you about the problem, yes role=radio is absolutely wrong here. @SachaG: this is for marking up "fake" radios, e.g. if you have a div that you style like a radio and use JS & CSS to make it work and look like a radio, see https://developer.mozilla.org/en-US/docs/Web/Accessibility/ARIA/Roles/radio_role It's just radios nested within labels, they don't need JS to work, nor any special roles to override their built-in ones.

Bear in mind, with the current construct I can still move through the visually-hidden radio buttons on the unselected parent question. Should I file a new issue for that or do you want to try Scott's method to programmatically hide them?

Or, perhaps leave them visible and disabled until the parent radio is selected.

When they are hidden, we hide the entire fieldset with role=presentation, which I think may have been the root of the problem: I thought role=presentation applies to the whole tree, but apparently it only applies to the container.

I wonder if aria-hidden="true" and/or aria-disabled="true" on the fieldset may be more appropriate, alongside inert for browsers that support it? Basically, we want to keep this visible for sighted users (so they can select both with one click when hovering), but hide it from the accessibility tree until the parent option is selected, so that SR users don't get it announced for every single parent option they arrow through.

Which it looks like someone did overnight and I have been typing all this for no reason.

You have definitely not been typing this for no reason, even if that was an oversight: both @SachaG and I learned something new today, and you reporting this will lead to fixing a bug that would have made the experience of SR users much worse. So thank you for doing that, especially on (your) weekend!

aardrian commented 1 year ago

The visibility property does not allow for transitions (I'm not sure if we're using any right now, but I think that was the intent behind using opacity).

Gotcha.

This is a global medium, so even if it's the weekend for you, it doesn't mean it's necessarily the weekend for everyone.

In this case I know Scott's schedule. He might not want it that way, but here we are (and there he is, wishing I would stop staring).

Both a design and UX requirement. But also, philosophically, I've always advocated that the web platform's accessibility APIs are powerful enough that screen reader accessibility can almost always be achieved without compromising visual design or UX for sighted users. Do you think that's not accurate?

I think that is not accurate. That is well outside the scope of this issue, but I and others have many bugs filed against browsers for accessibility gaps in their implementations (if you can pull some strings at Chrome to fix even a few of them, users would be delighted).

I wonder if aria-hidden="true" and/or aria-disabled="true" on the fieldset may be more appropriate, alongside inert for browsers that support it? Basically, we want to keep this visible for sighted users (so they can select both with one click when hovering), ...

You will want to test that with voice and touch users (and use the radiogroup role), two groups of sighted users who do not have the affordance of hover on which this pattern relies and for whom most ARIA is meaningless.

Also, now that I am looking, those nested radios have a pixel height of 23.35. They also live within the target of another interactive control (the label for the parent radio). This means they fail WCAG 2.2 Success Criterion 2.5.8 Target Size (Minimum) as written today (including the spacing exception). It looks like someone filed a related issue on the spacing.

...but hide it from the accessibility tree until the parent option is selected, so that SR users don't get it announced for every single parent option they arrow through.

Two nuggets here as well:

If these are all getting updated, please also consider changing the value of the child <legend> to correspond to the selected option in the parent radios. "Tell us more colon" (0:10 in the above video) does nothing to confirm what selection the user has already made. This can result in false selections until the arrowing bug is resolved.

The video that follows shows the challenge with this pattern using voice control. Pardon the terrible audio; working far away from my office audio set-up.

https://github.com/Devographics/Monorepo/assets/1376607/07fedac4-9703-4808-b9a1-ff2797857fb0

SachaG commented 1 year ago

Sorry for not being clear before, yes I did remove role=radio.

And just to confirm, using visibility instead of opacity would solve the screen reader issue?

aardrian commented 1 year ago

I am not sure there is a screen reader issue. The original bug I identified was about the nested roles, which presented in screen readers and you have since fixed.

As I noted above, SR users do not get the child radios announced as they arrow through the parent options; my video above demonstrates it only happens when they press Tab (go to 0:09) to move to the next radio group. So no issue there.

However, some of the proposed solutions risk leaving voice and touch users in a potentially weird position. And the <legend> of the nested radios is not very useful.

Note what I quoted from Scott's post about also using hidden. Also note that I have no idea the design decisions; I am simply trying to make sure that users don't get confuddled across a series of suggested approaches offered in this issue.

SachaG commented 1 year ago

Oh ok, sorry for misunderstanding, and thanks for the added details.

LeaVerou commented 1 year ago

Hi @aardrian, we've made a few changes, could you please hard refresh and try again? Thank you so much!

aardrian commented 1 year ago

Getting on a plane later, so laptop packed. Fired up Chrome / TalkBack and I do not encounter the hidden child radios. So that seems good.

However:

LeaVerou commented 1 year ago

Getting on a plane later, so laptop packed. Fired up Chrome / TalkBack and I do not encounter the hidden child radios. So that seems good.

Yay!

  • I cannot select the up/down arrows (child radios) using 'explore by touch';

Could you please elaborate on this? I'm not familiar with "explore by touch".

  • the target size of each child radio is almost definitely too small (as noted above regarding WCAG SC 2.5.8);

I just pushed a fix to increase the target size above the 24px required by WCAG 2.5.8. 😊

  • the contrast for the unselected arrow looks like it fails WCAG contrast checks:

I assume you mean e.g. the second button here:

image

Even for users with perfect vision, that's only meant as a faint "shadow" of another option that used to be there, e.g. in alternate UIs it may not even be shown at all. The contrast should be decent before the selection happens though, right?

  • It feels like the hit area of the "parent" radio has been reduced to just the width of the text label, which is great to avoid selecting visually hidden child radios but gives me an inconsistent tap area as I move down the list — even though I get a visual cue in a background color change (I thought I had selected one but wondered where the arrows were and then had to glance left to the radio itself to see it had not been selected).

@SachaG could you fix that please? Even for sighted users it's a usability issue to have the whole row highlighted when you hover but nothing actually happening when you click. In my prototype I had used a ::before on the label to extend its hit target and address this, but you could also use JS that calls .click() on the radio.

Thanks again for your time and energy @aardrian, this is super useful!

aardrian commented 1 year ago

Could you please elaborate on this? I'm not familiar with "explore by touch".

With TalkBack and VoiceOver (on touch devices), you can drag your finger around the screen and the screen reader will announce what is under it. Very handy for wayfinding on complex or broken pages.

I just pushed a fix to increase the target size above the 24px required by WCAG 2.5.8. 😊

Ace. Still cannot test.

I assume you mean e.g. the second button here:

I had to wait for a good connection to load the image. In your image it would be the ”Negative experience” control.

For those rolling in who cannot see the image, it is the ”Want to use again” (green with white text) and “Negative experience” (nearly black red with gray text) pill-looking radio buttons that immediately follow the (in this case) “Used it” radio button that has been selected.

Even for users with perfect vision, that's only meant as a faint "shadow" of another option that used to be there, e.g. in alternate UIs it may not even be shown at all.

But it is still there and not disabled, so it needs to meet WCAG contrast requirements. Given a keyboard user arrows through the options, no choice is ever the final choice until they submit. In the screen shot, that is what a keyboard user sees as soon as they tab to the field on their way to choosing “Negative experience”, so it definitely cannot be disabled.

The contrast should be decent before the selection happens though, right?

Still not at computer to test, but your browser should have a contrast checker built in.