Focus Zone sets tabIndex to 0 for unfocusable element when it updates

[jdrush89]

Describe the bug If an element is given a tabIndex of -1 and filtered out of a Focus Zone with focusableElementFilter, it should not participate in the Focus Zone or be tabbable. This works as expected until some prop on the unfocusable element changes. When that happens, the Focus Zone will update the tabIndex of that element to be 0, making it tabbable which not desired. This seems like an edge case, but it is occurring in the ReposFileTree component, which contains expand/collapse buttons in each row that should not participate in the Focus Zone.

Note: As this is a public repo, please be careful not to include details or screenshots from unreleased GitHub products or features. In most cases, a good bug report should be able to describe the new component without including business logic or feature details, but if you must discuss context relating to an unreleased feature, please open an issue in the private Design Systems repo and link to it here.

To Reproduce Here's a simple example:

export const DefaultButton = (args: ButtonProps) => {
  const [expanded, setExpanded] = useState(false)
  const {containerRef} = useFocusZone({
    focusableElementFilter: element => element.getAttribute('data-focusable') !== 'false'
  })
  return (
    <Box ref={containerRef as RefObject<HTMLDivElement>}>
      <Button {...args}>Default</Button>
      <Button {...args}>Default2</Button>
      <Button
        {...args}
        data-focusable="false"
        tabIndex={-1}
        aria-expanded={expanded}
        onClick={() => {
          setExpanded(!expanded)
        }}
      >
        Default3
      </Button>
      <Button {...args}>Default4</Button>
    </Box>
  )
}

1. Paste into the DefaultButton storyboook example in this repo
2. Inspect the Default3 Button and notice it has a tabIndex of -1
3. Click on the Default3 Button and notice its tabIndex changes to 0

Expected behavior Elements that are being filtered out of the focus zone with focusableElementFilter should not have their tabIndex updated by it.

Desktop (please complete the following information):

• OS: MacOS
• Browser chrome
• Version 103.0.5060.134 (Official Build) (x86_64)

Additional context Add any other context about the problem here.

[siddharthkp]

Thanks for reporting!

For triaging, do you know if this is a bug the hook primer/react/useFocusZone or also in the underlying primer/behaviors/focus-zone?

Update: I think I'm missing some context here because I was not able to reproduce this issue in storybook or a sandbox. The tabIndex of Default3 stuck to -1

Link to sandbox: https://codesandbox.io/s/wandering-framework-kflqd9?file=/src/App.tsx

https://user-images.githubusercontent.com/1863771/186645011-5c3e300f-775c-49b1-8b31-0ca20d0d3cb4.mov

[jdrush89]

@siddharthkp it looks like I'm able to reproduce in your codesandbox if the button is the first thing I click. If I tab into the focusZone before clicking then it's not updating. This isn't the behavior I'm seeing on the ReposFileTree though, the expand/collapse buttons are always changing their tabIndex to 0 when clicked regardless of whether the focus has been moved into the focusZone. My guess would be this is a problem with the FocusZone itself, it seems like it's not checking the focusableElementFilter filter somewhere that it should be.

https://user-images.githubusercontent.com/955442/186749727-9839d47f-6ead-47c8-a57a-d27e073204c9.mp4

[siddharthkp]

Sorry, I'm still lost, I think we should pair on this :)

[jdrush89]

@siddharthkp sounds good, I'm down to pair next week!

[siddharthkp]

Done, sent you an invite :)

[siddharthkp]

Able to reproduce this after pairing, the problem seems to be in the focusin handler where it simply selects the interacted element without checking if it's part of focusableElements if the focusInStrategy is previous (which is the default)

source code

Talked to @colebemis about the TreeView component he is working on and it wouldn't be a problem because of a different implementation 👍

Specific to Tree View: We shouldn't use a button, here's the component a11y review and proposed dom structure.

For primer/behaviors: Opened a bug in primer/behaviors, I'd say this is low priority for now

@jdrush89 Does that sound reasonable to you?

[jdrush89]

@siddharthkp I think that works, I can update the buttons to be spans for now.

[github-actions[bot]]

Hi! This issue has been marked as stale because it has been open with no activity for 180 days. You can comment on the issue or remove the stale label to keep it open. If you do nothing, this issue will be closed in 7 days.