navigation in useRadioGroup using keyboard arrows break if there is another focusable element withing the group

[Pustelto]

🐛 Bug Report

Hi,

I had to create this pattern in one project: https://design-system.service.gov.uk/components/radios/conditional-reveal/index.html. But when I tried to navigate between the radios using arrow keys the focus was last and value of the radio group was set to null. This is because the getFocusableTreeWalker select any focusable element, even in this case it should really select only radio buttons.

🤔 Expected Behavior

After pressing arrow key, if the radio button is focused, I would expect the next radio to be selected and focused, even if there is some other focusable element. in between those two radio buttons.

😯 Current Behavior

Focus is removed from the radio group, because it is trying to focus non-radio element which is removed from the dom.

💁 Possible Solution

I have add another option to the getFocusableTreeWalker using patch-package. Which is a callback used to modify default selectors. Whatever is returned from this callback will be used as a new selector for tree walker.

export function getFocusableTreeWalker(root, opts, scope) {
  let selector = opts?.tabbable ? TABBABLE_ELEMENT_SELECTOR : FOCUSABLE_ELEMENT_SELECTOR;
  if (opts.selectorModifier) {
    selector = opts.selectorModifier(selector)
  }
  let walker = document.createTreeWalker(root, NodeFilter.SHOW_ELEMENT, {
    acceptNode(node) {
      ...


// Then in the useRadioGroup function in the onKeyDown method

let walker = getFocusableTreeWalker(e.currentTarget, {
      from: e.target,
      selectorModifier: function( selector ) { 
        return "input[type=radio]:not([disabled]):not([type=hidden]):not([hidden])" 
      }
    });

🔦 Context

This is one of the issues I have faced when trying to create conditional reveal from the example above. I had to use patch-package to make this work as intended. But I would like to be this fixed in the library.

💻 Code Sample

https://codesandbox.io/s/silly-violet-ow97m?file=/src/App.js:674-693

🌍 Your Environment

Software	Version(s)
react-spectrum	3.4.1
Browser	Chrome 93
Operating System	macOS

[devongovett]

Hmm that's interesting. The pattern makes sense, but I'm pretty sure the radiogroup role can only have radio children according to the ARIA spec (see "Required owned elements"). It is possible to have radios outside a radio group though. I will need to check with our accessibility experts and do some research and get back to you.

[devongovett]

So I chatted with our a11y experts, and confirmed that the radiogroup role only allows radio children. This pattern isn't really supported by ARIA.

You have two options:

1. Place the conditional content outside (e.g. after) the radiogroup rather than inline with each radio.
2. Don't use the radiogroup role, and either use a <fieldset> or role="group" instead. This is what the UK design system implementation you linked does.

The first option is preferable because role="group"/<fieldset> do not support all of the properties that role="radiogroup" does, specifically aria-required, aria-invalid, aria-readonly, and aria-orientation. If we switched our implementation to use role="group" instead, we could not expose these states. I think we'd rather keep the useRadioGroup hook specific to actual radio groups rather than general purpose groups.

If you must make the expanded content inline, then I would follow what the gov.uk example is doing for the most part, and use native input elements and a fieldset. Note however, that they are using aria-expanded on their radios, which is not a valid property for radios (see https://www.w3.org/TR/wai-aria-1.2/#radio), so you should avoid that.

[Pustelto]

Hi, thanks for the detailed and valuable information. Will, take this into account.