feat(Popover): isModal decides whether the popover holds Tab (AST-116 FR12–FR16) by nynexman4464 · Pull Request #11156 · meta/astryx

nynexman4464 · 2026-11-11T02:49:47Z

Blocked on spec approval. Stacked on #11121 → #11133 → #11132 → #11122 → #11117 → #11112 (AST-116, draft). Do not merge before AST-116 is approved and promoted. It also amends component:Popover DEC-9 for isModal: false, which needs the Popover spec owner's approval.

User need

A coachmark or tour step points at a control the person should be able to reach and use. Today every Popover holds Tab while open. isModal={false} only removes aria-modal, so a non-modal popover still traps the keyboard inside itself, and the person cannot Tab to the control it describes.

Problem and solution fit

isModal now decides modality (AST-116 FR12–FR16, DEC-5).

true (default, unchanged): Tab and Shift+Tab cycle inside the open popover, and a dialog popover carries aria-modal.
false:
- The popover holds nothing; Tab and Shift+Tab move in the browser's order. No popover-and-anchor trap exists.
- Escape closes it only while focus is inside it or on its anchor or trigger. Otherwise the press passes to the layer or page beneath.
Both:
- A Tab pressed outside is never pulled in.
- Focus entry and focus return are the same either way.
- hasLightDismiss and onOutsidePress stay separate.
Sheet: on a compact touch screen, a non-modal popover presents a BottomSheet with no scrim (hasScrim={false}), so the adaptive sheet never turns it modal again.

Current specification

spec:AST-116 FR12–FR16, FR22 and DEC-5 (draft, #11112). DEC-5 supersedes component:Popover DEC-9 for isModal: false.

Public delta and caller burden

No new prop. isModal keeps its name, type and default.
Behavior change: isModal={false} stops holding Tab. The changeset carries the release note.
Audit of callers in this repo that set isModal: false:
- PowerSearch's token editor, a dialog popover: Tab now leaves the editor in page order, matching its description as a non-modal dialog. Its suites pass unchanged.
- Popover's own tests, updated.
- No story, template block or lab component sets it.
Internal: useFocusTrapInternal gains isContaining and isEscapeInScope; the latter reaches the shared dismissal stack's isPresent.

<Popover anchor={targetRef} isOpen={step === 2} onOpenChange={endTour}
  isModal={false} hasLightDismiss={false} label="Filters"
  content={<StepBody />} />

Design and alternatives

Rejected (DEC-5): a separate focus-region input with a value that cycles the popover and its anchor. It would be a second name for modality, and a trap the platform does not have.

Evidence

jsdom: Popover modality suite (10):
- modal and non-modal leave an outside Tab to the page;
- modal cycles;
- non-modal lets Tab leave forward, to the next page control, and backward, to the trigger, while staying open;
- only modal carries aria-modal;
- an Escape pressed elsewhere passes a non-modal popover, and one on its trigger closes it.
Presentation suite: a modal popover presents a modal sheet, and a non-modal one never calls showModal().
Real browser, Chromium 149 and Firefox 151 (app and script): all 7 checks pass.
- A non-modal coachmark opens with focus inside. Tab moves past it into the page and never wraps, and Tab from the highlighted control is not trapped.
- Escape elsewhere leaves it open; Escape on the anchor closes it.
- A modal popover holds Tab (M2 → close control → M1) and carries aria-modal.
- The engines differ in one place: after the coachmark's last control, Chromium moves to the anchor and Firefox to the next control in DOM order. Both are the browser's own order, and neither traps.
Not yet checked: WebKit. modal-drive.mjs is in the bundle for a macOS run.
Full core ui project: 824 files, 21,896 tests passed.

Scope

Components: Popover, usePopover, useFocusTrap (internal)

One capability is added.
Unrelated fixes, cleanup, and policy changes were removed or split.
Consumer docs, focused tests, and migration evidence ship with the feature.
Public text and artifacts contain no internal Meta context.

Testing

pnpm exec vitest run --project ui packages/core/src/Popover
pnpm exec vitest run --project ui --maxWorkers=6 packages/core: 824 files, 21,896 tests passed
tsc --noEmit (core), eslint: clean

vercel · 2026-11-11T02:49:52Z

The latest updates on your projects. Learn more about Vercel for GitHub.

Project	Deployment	Actions	Updated
astryx	Ready	Preview	Nov 11, 2026 3:55pm UTC

astracat-bot · 2026-11-11T02:50:17Z

AI review status for this pull request.

Review status	Updated
🔄 In progress (for maintainers only)	Nov 11, 2026, 3:43 PM UTC

astracat-bot

Approval waits on an engineering owner's spec decision: current component:Popover DEC-9 and AR3 make every Popover hold Tab, modal or not, and this pull request changes isModal={false} on the strength of draft spec:AST-113 DEC-5 and FR12–FR16, including a non-modal sheet in place of DEC-8's modal sheet. The owner also needs to settle whether this change to released behavior ships as a patch [feat] (an accessibility fix under spec:AST-017 FR53) or as [breaking] in a declared minor (FR7, FR25). An engineering owner records the decision by commenting /approve-spec <full head SHA>; as an engineering owner, the author may do so.

Whatever the decision, these need fixing:

With the default hasLightDismiss, pressing Escape on another page control still closes a non-modal popover in Chromium and Firefox. When the shared stack skips the layer, the browser's own popover="auto" close request fires (usePopover.tsx:634, :665), although the docs, the changeset and family:overlay-dismissal FR3 say the popover stays open.
The docs and the changeset say Tab moves "from the anchor (else the trigger) into the popover and on to what follows the anchor". Without a separate anchor, Tab from the trigger skips the open popover in Chromium and Firefox, and in Chromium Tab from its last control returns to the trigger (Popover.doc.mjs:135, Popover.tsx:354-357, usePopover.tsx:309-311).
The presentation docs this pull request rewrites still say the sheet always closes on a tap outside. A non-modal sheet (hasScrim={false}, Popover.tsx:1147) has no backdrop and ignores that tap (Popover.doc.mjs:176, Popover.tsx:421-422, Popover.tsx:1133-1137).
component:Popover VG1 asks for real-browser evidence when focus or light-dismiss behavior changes, and the jsdom tests cannot see either failure above. Please add non-modal cases to packages/core/src/Popover/__tests__/PopoverKeyboard.interaction.browser.spec.ts: Tab from the trigger and from a separate anchor, and Escape on another control under the default hasLightDismiss.

Automated review (maintainer link)

astracat-bot · 2026-11-11T03:18:14Z

      anchorOptionRef.current == null ? null : boundAnchorRef.current,
    layerId: layer.id,
+    isContaining: isModal,
+    isEscapeInScope: isModal ? undefined : isFocusInNonModalScope,


With the default hasLightDismiss, this popover is still a native popover="auto" (lightDismiss at line 634). When isEscapeInScope returns false, the shared stack skips the layer and does not preventDefault the key, so the browser's own Escape close request hides the popover anyway. In Chromium and Firefox, Escape pressed on another page control closes a non-modal popover unless the caller also passes hasLightDismiss={false}. One fix: keep a non-modal popover off native light dismiss (lightDismiss: hasLightDismiss && !watchesOutsidePress && isModal) and send its outside presses through the useOutsidePress watch, as onOutsidePress already does.

astracat-bot · 2026-11-11T03:18:14Z

          type: 'boolean',
          description:
-            'Whether a dialog-style popover sets aria-modal. Only applies when role is dialog.',
+            'Whether the popover is modal. true holds Tab inside the open popover and marks a dialog popover aria-modal. false holds nothing: Tab moves from the anchor (else the trigger) into the popover and on to what follows the anchor, and Escape closes it only while focus is inside it or on its anchor or trigger. Use false for a coachmark or a tour step. As a sheet, false presents a sheet with no scrim that leaves the page interactive.',


"Tab moves from the anchor (else the trigger) into the popover" holds only with a separate anchor. With no anchor, Tab from the trigger skips the open popover in Chromium and Firefox, and in Chromium Tab from the popover's last control goes back to the trigger. Make the trigger case match, or narrow this sentence (and the JSDoc and the changeset) to what ships.

astracat-bot · 2026-11-11T03:18:14Z

          type: "'popover' | 'bottom-sheet' | 'adaptive'",
          description:
-            "The surface that holds the content. 'popover' anchors it to the trigger, 'bottom-sheet' presents it in a modal BottomSheet labeled by label, and 'adaptive' uses the BottomSheet on a compact touch screen (a coarse pointer on a screen the theme counts as compact: narrower than its md width point or shorter than 500 px, so a phone in either orientation) and the anchored popover elsewhere. In a sheet, placement, alignment, width, role, isModal and hasAutoFocus do not apply, and xstyle, className and style style the sheet's panel; the sheet always light-dismisses (a tap outside it or a swipe down closes it, whatever hasLightDismiss says), Escape follows hasEscapeDismiss, and the fallback close stays hidden. The surface is chosen as it opens and kept until it closes.",
+            "The surface that holds the content. 'popover' anchors it to the trigger, 'bottom-sheet' presents it in a modal BottomSheet labeled by label, and 'adaptive' uses the BottomSheet on a compact touch screen (a coarse pointer on a screen the theme counts as compact: narrower than its md width point or shorter than 500 px, so a phone in either orientation) and the anchored popover elsewhere. In a sheet, placement, alignment, width, role and hasAutoFocus do not apply, and xstyle, className and style style the sheet's panel; the sheet always light-dismisses (a tap outside it or a swipe down closes it, whatever hasLightDismiss says), Escape follows hasEscapeDismiss, and the fallback close stays hidden. The surface is chosen as it opens and kept until it closes.",


For isModal={false} this is no longer true: the sheet is rendered with hasScrim={false} (Popover.tsx:1147), and BottomSheet dismisses on a tap only through its backdrop when it has a scrim, so a tap outside a non-modal sheet does nothing. Please say which exits a non-modal sheet has (swipe down, the close control, Escape).

astracat-bot · 2026-11-11T03:18:14Z

+  // Tab and Shift+Tab wrap inside the container while the trap is active and
+  // containing.


Non-blocking (INV29): this comment restates isActive && isContaining on the next line. Consider dropping it.

astracat-bot

Approval waits on an engineering owner's spec decision: current component:Popover DEC-9 and AR3 make every Popover hold Tab, modal or not, and DEC-8 makes its sheet modal, while this pull request follows draft spec:AST-113 DEC-5 and FR12–FR16 for isModal={false}. The same decision needs to settle the release tier under spec:AST-017: an accessibility fix shipped as a patch [fix] (FR53), or a [breaking] change in a declared minor (FR7, FR25). An engineering owner records it by commenting /approve-spec <full head SHA> on this pull request; as an engineering owner, the author may do so.

Whatever the decision, these need fixing:

With the default hasLightDismiss, pressing Escape on another page control still closes a non-modal popover in Chromium and Firefox, because the browser's own popover="auto" close request fires when the shared stack skips the layer (usePopover.tsx:634, :665), although the docs and the changeset say it stays open.
The docs and the changeset say Tab moves "from the anchor (else the trigger) into the popover and on to what follows the anchor", but with no separate anchor, Tab from the trigger skips the open popover in Chromium and Firefox, and in Chromium Tab from its last control returns to the trigger (Popover.doc.mjs:135, Popover.tsx:355-357, usePopover.tsx:309-311).
The presentation docs still say every sheet is modal and closes on a tap outside, yet a non-modal sheet has no scrim and stays open when tapped above (Popover.doc.mjs:176, Popover.tsx:417-422, Popover.tsx:1133-1136).
Miss: a non-modal sheet closes on an Escape pressed on a page control behind it, because BottomSheet's own dismissal registration has no focus scope (Popover.tsx:1147, BottomSheet.tsx:892-895), contrary to "Escape closes it only while focus is inside it or on its anchor or trigger".
Miss: the changeset tags this change to released behavior [feat], which spec:AST-017 FR7 keeps for compatible capabilities, so it needs [fix] with its accessibility target (FR53) or [breaking] with a migration (FR7), whichever tier is decided.
component:Popover VG1 asks for real-browser evidence when focus or light-dismiss behavior changes, and the jsdom test at Popover.test.tsx:1960 passes the Escape case that fails in real engines, so please add a real-browser spec under packages/core/src/Popover/__tests__/ for Escape elsewhere under the default hasLightDismiss, and for Tab from a trigger and from a separate anchor.

Automated review (maintainer link)

astracat-bot · 2026-11-11T08:15:13Z

        onOpenChange={setSheetOpen}
+        // A non-modal popover stays non-modal as a sheet: no scrim, the page
+        // stays interactive, and focus is not held.
+        hasScrim={isModal !== false}


Miss (not raised in the earlier review of this same patch): this makes the sheet non-modal, but its Escape is not scoped to focus. BottomSheet joins the dismissal stack with its own presence check (BottomSheet.tsx:892-895), which ignores where focus is, so an Escape pressed on a page control behind an open non-modal sheet closes it (Chromium, 390 × 844). The isModal docs ("Escape closes it only while focus is inside it or on its anchor or trigger") and draft FR16 ("Escape while it holds focus") say otherwise. Scope the sheet's Escape the same way as the anchored popover's, or document the sheet's own rule.

astracat-bot · 2026-11-11T08:15:13Z

+'@astryxdesign/core': patch
+---
+
+[feat] Popover `isModal` now decides whether the popover holds focus. `isModal` (the default) is unchanged: Tab and Shift+Tab cycle inside the open popover, and a dialog popover carries `aria-modal`. `isModal={false}` holds nothing. Tab moves from the anchor or trigger into the popover and on to what follows it, and Escape closes the popover only while focus is inside it or on its anchor or trigger. Use it for a coachmark or a tour step that lets the person reach the control it describes. On a compact touch screen, a non-modal popover presents a sheet with no scrim. **Behavior change:** before, `isModal={false}` removed only `aria-modal` and still kept Tab inside the popover. If a popover needs Tab held, leave `isModal` at its default.


Miss (not raised in the earlier review of this same patch): spec:AST-017 FR7 keeps [feat] for compatible capabilities and makes a behavior-correcting incompatibility [breaking] (a minor under 0.x); FR53 ships an accessibility fix as a patch [fix] that names its accessibility target. This entry changes released isModal={false} behavior, so whichever tier the owner rules, the tag needs to be [fix] with the target or [breaking] with a migration, not [feat].

astracat-bot

Approval waits on an engineering owner's spec decision: current component:Popover DEC-9 and AR3 make every Popover hold Tab, modal or not, and DEC-8 makes its phone sheet modal, while this pull request intentionally changes that for isModal={false} on the strength of spec:AST-113 DEC-5 and FR12–FR16, which are still draft. An engineering owner needs to approve or reject that direction and rule whether the change to released isModal={false} behavior ships as a patch accessibility fix (spec:AST-017 FR53) or as [breaking] in a declared minor (FR7, FR25), by commenting /approve-spec <full head SHA> on this pull request; the author may do so as an engineering owner. Whatever the ruling, these need changes before approval:

With the default hasLightDismiss, Escape pressed on another page control still closes a non-modal popover in Chromium and Firefox, because the native popover="auto" close request fires when the shared stack declines the press (usePopover.tsx:634, :665); keep a non-modal popover off native light dismiss and route its outside presses through the existing outside-press watch.
The docs, JSDoc and changeset say Tab moves "from the anchor (else the trigger) into the popover", but with no separate anchor Tab from the trigger skips the open popover in Chromium and Firefox (Popover.doc.mjs:135); make the trigger case match or narrow the sentence.
The presentation docs and the sheet comment still say every sheet is modal and closes on a tap outside, which a non-modal sheet with no scrim does not do (Popover.doc.mjs:176, Popover.tsx:417-422, :1133-1137); list the non-modal sheet's exits.
Miss: a non-modal sheet closes on an Escape pressed anywhere on the page, because BottomSheet's presence check has no focus scope (Popover.tsx:1147, BottomSheet.tsx:892-895); scope it like the anchored popover or document the sheet's rule.
Miss: the changeset tags a changed released behavior [feat], which spec:AST-017 FR7 reserves for compatible capabilities; retag it [fix] with its accessibility target or [breaking] with a migration, to match the owner's ruling.
component:Popover VG1 asks for real-browser evidence when focus or light-dismiss behavior changes, and the new jsdom test at Popover.test.tsx:1960 passes a case that fails in real browsers; add a real-browser spec under packages/core/src/Popover/__tests__/ for Escape elsewhere and Tab from the trigger and from a separate anchor.

Automated review (maintainer link)

…al follows page order (spec:AST-113 FR12–FR16)

meta-cla Bot added the CLA Signed This label is managed by the Meta Open Source bot. label Nov 11, 2026

vercel Bot deployed to Preview November 11, 2026 02:59

View deployment

astracat-bot Bot reviewed Nov 11, 2026

View reviewed changes

nynexman4464 force-pushed the nynexman4464/feat/popover-return-focus branch from 244aa61 to 18cc3b1 Compare November 11, 2026 07:44

nynexman4464 force-pushed the nynexman4464/feat/popover-is-modal branch from fd92d14 to cc68bb8 Compare November 11, 2026 07:44

vercel Bot deployed to Preview November 11, 2026 07:53

View deployment

astracat-bot Bot reviewed Nov 11, 2026

View reviewed changes

nynexman4464 force-pushed the nynexman4464/feat/popover-return-focus branch from 18cc3b1 to 923cd1e Compare November 11, 2026 09:29

nynexman4464 force-pushed the nynexman4464/feat/popover-is-modal branch from cc68bb8 to abef85d Compare November 11, 2026 09:29

vercel Bot deployed to Preview November 11, 2026 09:36

View deployment

astracat-bot Bot reviewed Nov 11, 2026

View reviewed changes

cixzhang mentioned this pull request Nov 11, 2026

fix(focus region): leave Ctrl+Tab and Meta+Tab to the page #11247

Merged

4 tasks done

feat(Popover): isModal decides whether the popover holds Tab; non-mod…

8872b22

…al follows page order (spec:AST-113 FR12–FR16)

nynexman4464 force-pushed the nynexman4464/feat/popover-is-modal branch from abef85d to 8872b22 Compare November 11, 2026 15:43

nynexman4464 force-pushed the nynexman4464/feat/popover-return-focus branch from 923cd1e to 2ffbdf5 Compare November 11, 2026 15:43

nynexman4464 changed the title ~~feat(Popover): isModal decides whether the popover holds Tab (AST-113 FR12–FR16)~~ feat(Popover): isModal decides whether the popover holds Tab (AST-116 FR12–FR16) Nov 11, 2026

vercel Bot deployed to Preview November 11, 2026 15:55

View deployment

Navigation Menu

Repository navigation

feat(Popover): isModal decides whether the popover holds Tab (AST-116 FR12–FR16) - #11156

feat(Popover): isModal decides whether the popover holds Tab (AST-116 FR12–FR16)#11156
nynexman4464 wants to merge 1 commit into
nynexman4464/feat/popover-return-focusfrom
nynexman4464/feat/popover-is-modal

nynexman4464 commented Nov 11, 2026 •

edited

Loading

Uh oh!

vercel Bot commented Nov 11, 2026 •

edited

Loading

Uh oh!

astracat-bot Bot commented Nov 11, 2026 •

edited

Loading

Uh oh!

astracat-bot Bot left a comment

Uh oh!

astracat-bot Bot Nov 11, 2026

Uh oh!

astracat-bot Bot Nov 11, 2026

Uh oh!

astracat-bot Bot Nov 11, 2026

Uh oh!

astracat-bot Bot Nov 11, 2026

Uh oh!

astracat-bot Bot left a comment

Uh oh!

astracat-bot Bot Nov 11, 2026

Uh oh!

astracat-bot Bot Nov 11, 2026

Uh oh!

astracat-bot Bot left a comment

Uh oh!

This branch was successfully deployed

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

		// Tab and Shift+Tab wrap inside the container while the trap is active and
		// containing.

Conversation

nynexman4464 commented Nov 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

User need

Problem and solution fit

Current specification

Public delta and caller burden

Design and alternatives

Evidence

Scope

Testing

Uh oh!

vercel Bot commented Nov 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

astracat-bot Bot commented Nov 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

astracat-bot Bot left a comment

Choose a reason for hiding this comment

Uh oh!

astracat-bot Bot Nov 11, 2026

Choose a reason for hiding this comment

Uh oh!

astracat-bot Bot Nov 11, 2026

Choose a reason for hiding this comment

Uh oh!

astracat-bot Bot Nov 11, 2026

Choose a reason for hiding this comment

Uh oh!

astracat-bot Bot Nov 11, 2026

Choose a reason for hiding this comment

Uh oh!

astracat-bot Bot left a comment

Choose a reason for hiding this comment

Uh oh!

astracat-bot Bot Nov 11, 2026

Choose a reason for hiding this comment

Uh oh!

astracat-bot Bot Nov 11, 2026

Choose a reason for hiding this comment

Uh oh!

astracat-bot Bot left a comment

Choose a reason for hiding this comment

Uh oh!

This branch was successfully deployed

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

nynexman4464 commented Nov 11, 2026 •

edited

Loading

vercel Bot commented Nov 11, 2026 •

edited

Loading

astracat-bot Bot commented Nov 11, 2026 •

edited

Loading