Repository navigation
fix(Popover): an open popover no longer pulls in a Tab pressed elsewhere on the page - #8611
Conversation
…ere on the page useFocusTrap holds focus that is inside its container and leaves a Tab that starts outside to the page (component:Popover DEC-9). Fixes #7687.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
AI review status for this pull request.
|
PR Analysis Report
|
meta-cla
Bot
added
the
CLA Signed
![astracat-bot[bot]](https://avatars.githubusercontent.com/in/4796231?s=60&v=4){kind=small}
There was a problem hiding this comment.
Changes requested: Tabs that start on a page control now follow the page instead of being pulled into an open popover, as DEC-9 asks, and Tab inside the popover still cycles. One regression needs fixing first: the new guard at packages/core/src/hooks/useFocusTrap.ts:429 treats a focused SVG control inside the popover (for example an <svg tabindex="0"> chart point) as outside, so Tab from it as the last control leaves the popover instead of wrapping, which AR3 does not allow. Checking container.contains(active) without requiring an HTMLElement, plus a test with an SVG control as the last tabbable, should resolve it.
[Automated review]
| if (event.key === 'Tab') { | ||
| // Mark that keyboard navigation is happening | ||
| const active = doc.activeElement; | ||
| if (!isHTMLElement(active) || !container.contains(active)) { |
There was a problem hiding this comment.
This guard also treats focus on a non-HTML control inside the container as outside: an SVGElement (an <svg tabindex="0">, an SVG <a href>) is not an HTMLElement, although the trap's focusable selector lists it. With such a control as the popover's last tabbable, Tab from it now leaves the popover (to the trigger, in Chromium and WebKit), where the base wrapped to the first control. Testing the node is enough here:
const active = doc.activeElement;
if (active == null || !container.contains(active)) {
return;
}A useFocusTrap.test.tsx case whose last tabbable is an SVG control would pin it.
There was a problem hiding this comment.
Recommending approval: a Tab or Shift+Tab pressed on a page control now stays with the page while focus inside an open popover still cycles, as component:Popover DEC-9 and AR3 require, and the earlier escape from a focused SVG control is fixed and covered by a test. The new outside-Tab tests fail against the previous focus trap and pass with this change, and Chromium and WebKit checks show the same result. One note for a later change, not this one: with hasAutoFocus={false} and Popover's default wrapped trigger, Tab from the trigger no longer reaches the open popover, because the popover's invoker is the trigger's wrapper rather than the button, so passing the button as the invoker would be a worthwhile follow-up.
[Automated review]
Main's meta#8611 (component:Popover DEC-9) made useFocusTrap leave a Tab that starts outside its container to the page. This branch's nested-trap check runs after it: a Tab inside a trap nested in this one's container is still inside this container, so the outer trap still leaves it to the inner one. The doc keeps both sentences.
User impact
Keyboard users on any page with an open Popover. Today a Tab pressed anywhere on the page, even on a control nowhere near the popover, is pulled into the popover. For example, a list next to an open non-modal comment popover can't be reached by Tab. After this change a Tab outside the popover moves to the next control on the page. Focus inside the popover still cycles, and Escape still closes it and returns focus to its trigger.
Problem and solution fit
useFocusTrapmarks every Tab on the document as keyboard navigation before it checks where focus is. Its focus listener then sees focus land outside the container and redirects it inside.The fix marks keyboard navigation only when the Tab starts inside the container. Tabs that start outside are left to the page. Containment of focus that is already inside does not change.
Expected behavior and authority
component:PopoverDEC-9 and AR3 (#8608): every Popover is a focus region, modal or not. Containment holds focus that is inside the popup, and a Tab that starts outside it is never redirected into it.Smallest restoration
One early return in
useFocusTrap's Tab handler, plus doc wording for the hook. No new prop, no change toisModal, Escape, light dismiss, initial focus, or focus return. Role-neutral popups (menus, listboxes) keep their own Tab handling.Evidence
useFocusTrap"outside a trap with tabbable controls" test fail, because focus is pulled into the popover.Scope
Testing
pnpm vitest run packages/core/src/hooks packages/core/src/Popoverplus the Drawer, BottomSheet, DropdownMenu, TabList, Breadcrumbs, SideNav, TopNav, Typeahead, PowerSearch, Selector, ComplexSelector, Chat, and date input suites.Fixes #7687