Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Label] ARIA issue detected by Lighthouse #638

Closed
rafgraph opened this issue May 5, 2021 · 6 comments
Closed

[Label] ARIA issue detected by Lighthouse #638

rafgraph opened this issue May 5, 2021 · 6 comments

Comments

@rafgraph
Copy link

rafgraph commented May 5, 2021

Bug report

Hi, I'm using the <Label> component on my site and Google's Lighthouse detected an ARIA issue with it. I wasn't sure if it is how I'm using it or intrinsic to the component itself, so I ran Lighthouse on the docs page for <Label> and got the same result.

Current Behavior

Lighthouse detects an ARIA issue with <Label>.

Expected behavior

<Label> should pass all Lighthouse ARIA tests.

Reproducible example

Run Lighthouse on the docs page: https://radix-ui.com/primitives/docs/utilities/label

Additional context

Screen Shot 2021-05-05 at 11 08 10 AM

@jjenzz
Copy link
Contributor

jjenzz commented May 8, 2021

We are aware that role="label" is not valid (yet) but it improves accessibility today in our testing.

Here's the results of our testing:

image

alt: https://discord.com/channels/752614004387610674/803656831704629298/816758848865435698

You might improve your lighthouse score if you use native label but will lose out on the accessibility gains as mentioned above.

@rafgraph
Copy link
Author

rafgraph commented May 9, 2021

Hi, thanks for the explanation, I appreciate your thoroughness! That's been my hesitation with 3rd party component libraries, on the surface they're great, but they end up causing more trouble then they are worth with a lack of attention to detail. Also, it might be helpful to add that explanation to the docs for the next person.

A couple of follow up questions:

Have you tested it with touch device screen readers for users navigating by touch?

I'm use the Radix Checkbox and Label together, I'm curious what the tradeoffs are of the Radix approach (<button> checkbox, <span> label) compared to the more traditional approach of <input> with opacity: 0 and a <label> (as described here and here)?

Also, I initially tried using a regular <label> with the Radix Checkbox, but ran into a bug where the onCheckedChange event is called twice when clicking the checkbox. I didn't report this because switching to the Radix Label fixed this, but here's a codesandbox with the issue.

@jjenzz
Copy link
Contributor

jjenzz commented May 10, 2021

My pleasure 🙂 and good idea on the docs. You're not the first person to question this so definitely seems it would useful.

Have you tested it with touch device screen readers for users navigating by touch?

Yes, with VoiceOver only atm.

[...] I'm curious what the tradeoffs are of the Radix approach (<button> checkbox, <span> label) compared to the more traditional approach of <input> with opacity: 0 and a <label>

There are no tradeoffs as far as I am aware, but with the button approach you get an improved DX.

Our input only exists to allow event propagation when used in forms. If it wasn't for that, we wouldn't render an input at all. You'll notice that the button has all the necessary aria attributes for SRs to communicate it as the checkbox. We chose this approach for a few reasons:

  1. The inline styles we would have to supply for the traditional approach to work out of the box wouldn't be minimal and we try to avoid too many of these if we can. We want them to be as style-free as possible.
  2. The button approach allows consumers more intuitive and flexible styling. They can display: flex, display: grid etc. and it would position the indicator as expected.
  3. The input approach relies solely on absolute positioning and the correct stacking context for it to work. That could very easily be broken by consumers if they weren't paying attention to our implementation detail and we try to make sure you don't need to care what's going on under the hood.
  4. The input approach would mean spreading the props on a parent div in order for you to style the box that wraps the indicator. This means many input DOM attributes that consumers might try to apply (e.g. disabled, value etc.) either wouldn't work or wouldn't be valid HTML and would have to be totally reimplemented. Button's however, already support many attributes inputs support and can still be wrapped in native labels.
  5. We didn't spread props on the input because see 1, 2 and 3.
  6. We avoid redirecting props to different elements internally to save confusion. document.querySelector('.className') should return the same DOM node that you get by attaching a ref. Similarly, events such as onClick should give you the same element via event.currentTarget as ref.current, etc.

I'm sure there were other reasons but I can't think of them all off the top of my head, it was a while ago 😅 but hopefully that helps gather some understanding of how we ended up where we are.

[...] but ran into a bug where the onCheckedChange event is called twice when clicking the checkbox

This is an issue when clicking the indicator. We are aware of it but haven't had a chance to look into it yet. Original bug report is here #512

@rafgraph
Copy link
Author

Hi @jjenzz, thanks for explaining why you chose the <button> approach. Knowing these details gives me more confidence in using the component (this why might useful in the docs too, since it's a non-standard approach). I can say that I've already used display: flex to help position the indicator.

One tradeoff might be #609. It seems like there's some complex wiring of events that requires not using the React event system, so calling stopPropagation() or preventDefault() on a click event on a child of <Label>, e.g. an anchor link, doesn't work. Hopefully this is something that can be fixed so <Label> uses the React event system.

I'm going to close this issue because the Lighthouse ARIA tests should indicate real world accessibility performance, and clearly that is not the case. If possible it might be worth it to try and get role="label" approved by Lighthouse given its real world performance.

@jjenzz
Copy link
Contributor

jjenzz commented May 10, 2021

One tradeoff might be #609

I think the bugs reported are probably most likely due to some dodgy code on my part that needs fixing. I would be surprised if it has anything to do with the fact we use a button but don't quote me on that haha.

@jjenzz
Copy link
Contributor

jjenzz commented May 31, 2021

@rafgraph you might be interested in the new changes I'm working on here #672

I'm hoping to apply similar changes across our other components like Switch and RadioGroup.

These changes improve the native label accessibility with our custom controls so now, the only real reason to chose the custom label once this is merged would be because it avoids the unnecessary text selection on click, or if you are trying to build your own custom control that needs to be labelled.

CleanShot 2021-05-31 at 13 22 55

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants