search

radix-ui / primitives

Radix Primitives is an open-source UI component library for building high-quality, accessible design systems and web apps. Maintained by @workos.
https://radix-ui.com/primitives
MIT License
15.71k stars 812 forks source link

[Tabs] Screen reader focus shouldn't select the tab #1047

Open diegohaz opened 2 years ago

diegohaz commented 2 years ago

Bug report

Hello! While debugging Reakit and checking the behavior on other libraries, I found out that Radix has a similar issue.

Current Behavior

Focusing on the tab with VoiceOver automatically selects it. Therefore, users can't access the panels of previous tabs by moving the VO cursor.

Expected behavior

Screen reader focus shouldn't select the tab, only keyboard focus and clicks.

Reproducible example

I tested https://www.radix-ui.com/docs/primitives/components/tabs using VoiceOver on an iPad. But the same happens on iOS and Mac (if you use only VO+Arrow keys to move the virtual cursor).

Note that I can't access the contents of the "Account" panel by moving the VO cursor:

https://user-images.githubusercontent.com/3068563/146335454-ab2f4f74-c65f-4b3b-a6b1-d657462d8598.mov

I also couldn't move to the password panel after selecting it, but I think this is a separate issue (I guess it has something to do with VO not detecting lazily rendered elements).

Suggested solution

The VoiceOver virtual cursor will move DOM focus (by calling element.focus()) to the element if it's a <button>. There should be a distinction between focusing on tabs using arrow keys and when just calling element.focus() without arrow keys.

Alternatively, the tab could be rendered as another element, but this has other implications.

Your environment

Software Name(s) Version
Radix Package(s) n/a latest on the site
React n/a latest on the site
Browser Safari 15.1
Assistive tech VoiceOver
Operating System macOS/iOS/iPadOS
jjenzz commented 2 years ago

I've looked into this briefly. I know we followed the WAI ARIA example here but looks like we missed something because theirs does as @diegohaz suggests:

Screen reader focus shouldn't select the tab, only keyboard focus and clicks.

We could maybe take advantage of aria-owns? It seems a bit odd to activate a tab and then have to cycle through all tabs before getting to the content, but perhaps that's normal with this pattern?

I guess it could be a pain to navigate through lots of tab content just to get to the next tab too 🤔

diegohaz commented 2 years ago

We could maybe take advantage of aria-owns? It seems a bit odd to activate a tab and then have to cycle through all tabs before getting to the content, but perhaps that's normal with this pattern?

Safari doesn't support aria-owns (which is unfortunate, because it's really useful sometimes). But, yeah, I think it would be a bigger pain having to navigate through the contents to get to the next tab. Ultimately, users can choose to navigate through headings or other elements that may be inside the tab panel.

benoitgrelard commented 2 years ago

Hey @diegohaz,

Do you have any suggestions on how to detect regular focus vs. screen reader focus? How did you end up solving this in ariaKit?

🙏

diegohaz commented 2 years ago

Hey @diegohaz,

Do you have any suggestions on how to detect regular focus vs. screen reader focus?

How did you end up solving this in ariaKit?

🙏

If the focus is triggered by arrow keys or mouse down, you can activate the tab. Otherwise, the tab is focused, but not selected (like tabs with manual activation).

asherccohen commented 2 years ago

To add to this matter, I was checking the aria docs and I noticed that an outline should be visible on the tabs content only when navigating with a keyboard.

The main example on the radix docs show:

const StyledContent = styled(TabsPrimitive.Content, {
  flexGrow: 1,
  padding: 20,
  backgroundColor: 'white',
  borderBottomLeftRadius: 6,
  borderBottomRightRadius: 6,
  outline: 'none',

///Should this be :focus-within or how to solve not showing the boxshadow on mouse focus????
  '&:focus': { boxShadow: `0 0 0 2px black` },
});

It's actually a visual annoyance, but I don't want to remove the style completely.

For reference what made me realize this is a tweet about Chakra UI:

https://twitter.com/thesegunadebayo/status/1534124029010903041?t=eHU13j3B-Ep-QkDic-FG0A&s=19

benoitgrelard commented 2 years ago

Hey @asherccohen,

If you were to replace '&:focus' with '&:focus-visible' it should give you what you want I believe which is that the focus style would only show when using the keyboard, not when clicking for example.

asherccohen commented 2 years ago

Thanks for the answer. I thought the same but it actually doesn't.

:focus-within is applied both on click and keyboard navigation.

Do you suggest any other solution?

benoitgrelard commented 2 years ago

Hey @asherccohen, are you sure?

I've just made that change in the sandbox created directly from our website's demo and it works fine. You can see when I tab to the content, it shows the focus ring, but when I click into the content it doesn't.

https://user-images.githubusercontent.com/1539897/172823414-a2dab89a-ad4e-4a27-a235-546069c80655.mp4

asherccohen commented 2 years ago

Sorry my bad, I didn't notice you suggest to use :focus-visible and I was trying :focus-within 😁.

That solved it, thanks.

In addition is there any documentation about the focus ring and focus trap? I've built some custom primitives but haven't found a way to replicate their behaviour.

benoitgrelard commented 2 years ago

In addition is there any documentation about the focus ring and focus trap? I've built some custom primitives but haven't found a way to replicate their behaviour.

We don't do anything specific regarding focus ring, for focus trapping, we have some internal components that are used by the primitives but we do not document them as we do not consider part of the public API.

chad1008 commented 1 year ago

Hey there @benoitgrelard and @jjenzz! 👋 Wanted to drop a note and see if there was anything we could do to assist here. We've just run into this issue and it's a bit of a blocker for us (see the PR @diegohaz just linked from), and we'd be happy to collaborate on a fix if that would help!

figmatom commented 1 year ago

👋 Hi! We were investigating using radix Tabs, but this was an issue that is keeping us from going this route. Dug into the issue a bit, but got stumped, wanted to write some notes down.

So, the way this usually gets fixed is that the keyDown handler is prevented on the current focused item if the voiceover nav keys are being pressed. Which the RovigFocusGroup properly puts the keydown on the button. What's fascinating, is that if you remove the keydown from RovingFocusGroup, it STILL moves on arrows, but only for screenreader. Something is happening more natively to cause focus to shift, since react is not even seeing a keydown event for the arrows when the voiceover nav is used.

So, something else is causing focus changes, but it is hard to figure out WHERE