[Combobox] Tab sequence gets stuck in popover interactive elements

[indiesquidge]

🐛 Bug report

Current Behavior

When tabbing to interactive elements in the popover, the tab sequence gets stuck in the popover

Compare this to the popover itself, which does not track the tab sequence.

Interestingly, the tab sequence issue is not reproducible on the website examples, despite creating a clean slate with cd website/ && rm -rf node_modules && yarn && yarn dev.

Expected behavior

There is nothing mentioned on WAI ARIA about trapping focus in the listbox popover, so I would expect the tab sequence to predictably follow the browser default sequence, i.e. how the current Reach UI popover component behaves (as shown above).

Reproducible example

CodeSandbox Template

Running the Storybook examples locally will also reproduce this issue

Suggested solution(s)

Even after tinkering locally, its not clear to me why the tab sequence is trapped in the combobox popover (and also why it isn't on the website examples).

Additional context

Your environment

Software	Name(s)	Version
Reach Package	combobox	0.10.0
React	-	16.13.0
Browser	Chrome	80.0.3987.163
Assistive tech	-	-
Node	-	v13.12.0
npm/yarn	yarn	1.22.4
Operating System	macOS	10.15.4

[indiesquidge]

This is perhaps a separate issue, but a fresh build of the website also warns that certain @reach/utils aren't exported (even though they are, and browsing my local node_modules even shows that they are). Something feels a bit off about the build of the website locally (or I'm just setting up the dev env wrong), which could explain why I can't reproduce the combobox issue on the website examples.

[chaance]

There has been some discussion on how we handle this by default in various components. All of the ARIA examples I've seen that implement popovers with interactive content do in fact lock focus in the popover, though some UI specs don't mention it explicitly (particularly if the example doesn't use nested tabbable elements). My general feeling is aligned with yours here, and that's the default approach taken in listbox currently. I would like to get some more feedback from users in the a11y community here.

That said, no idea why the behavior wouldn't be consistent between the docs and local build. Haven't had a problem building the site locally on my end or in CI 🤔

[raunofreiberg]

I think the root of the problem lies in Popover. Hear me out!

All of the examples where tab sequence is not disrupted have focusable elements before and after the component.

I added a sibling input element to the reproducible example and the tab sequence is correct:
https://codesandbox.io/s/hardcore-dewdney-0htlo

Or, using portal={false} on ComboboxPopover also fixes the problem since Popover is not used: https://codesandbox.io/s/distracted-hofstadter-9633t

---

Alas, if you reduce the Popover example down to a single textarea, the same issue appears:
https://codesandbox.io/s/new-darkness-uipul

I'll try to take a closer look later, but on first guess, the devil is somewhere here:

reach-ui/packages/popover/src/index.tsx

Line 193 in 52d721c

function useSimulateTabNavigationForReactTree<

[raunofreiberg]

I dunno if this makes sense, but these changes to focusLastTabbableInPopover seem to fix tabbing back and forth between the trigger and elements inside the Popover.

function focusLastTabbableInPopover(event: KeyboardEvent) {
  const elements = popoverRef.current && tabbable(popoverRef.current);

+  // The actual last tabbable should be the trigger.
+  // We push it manually to the array because it is outside of the portal of popover.
+  triggerRef.current && elements?.push(triggerRef.current);

  const last = elements && elements[elements.length - 1];

  if (last) {
    event.preventDefault();
    last.focus();
  }
}

[michaeldfoley]

I too believe that the issue relates to Popover, specifically cases where the targetRef is the last tabbable element outside the popover. When this is the case, getElementAfterTrigger returns the first tabbable element inside the portal causing the focus to stay trapped inside the popover. What we want is for getElementAfterTrigger to return an element only if it is not inside the popover.

I submitted a PR (#609) that should address the issue. I recreated the original example referenced in this issue and the focus now moves outside the popover as expected.

[chaance]

Should be fixed in 0.10.4. Please let me know if you continue to have issues.