From 8872b22b47c4ed8fd10ae56764baed074d89ba66 Mon Sep 17 00:00:00 2001 From: nynexman4464 <1675576+nynexman4464@users.noreply.github.com> Date: Tue, 10 Nov 2026 18:48:00 -0800 Subject: [PATCH] =?UTF-8?q?feat(Popover):=20isModal=20decides=20whether=20?= =?UTF-8?q?the=20popover=20holds=20Tab;=20non-modal=20follows=20page=20ord?= =?UTF-8?q?er=20(spec:AST-113=20FR12=E2=80=93FR16)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- .changeset/popover-is-modal.md | 7 ++ packages/core/src/Popover/Popover.doc.mjs | 8 +- packages/core/src/Popover/Popover.test.tsx | 101 ++++++++++++++---- packages/core/src/Popover/Popover.tsx | 14 ++- .../src/Popover/PopoverPresentation.test.tsx | 23 ++++ packages/core/src/Popover/usePopover.doc.mjs | 5 +- packages/core/src/Popover/usePopover.tsx | 37 ++++++- packages/core/src/hooks/useFocusTrap.ts | 20 +++- 8 files changed, 179 insertions(+), 36 deletions(-) create mode 100644 .changeset/popover-is-modal.md diff --git a/.changeset/popover-is-modal.md b/.changeset/popover-is-modal.md new file mode 100644 index 0000000000000..85b6a1b75519e --- /dev/null +++ b/.changeset/popover-is-modal.md @@ -0,0 +1,7 @@ +--- +'@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. + +@nynexman4464 diff --git a/packages/core/src/Popover/Popover.doc.mjs b/packages/core/src/Popover/Popover.doc.mjs index ba333eacbe4ca..3c73524f7c265 100644 --- a/packages/core/src/Popover/Popover.doc.mjs +++ b/packages/core/src/Popover/Popover.doc.mjs @@ -132,7 +132,7 @@ export const docs = { name: 'isModal', 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.', default: 'true', }, { @@ -173,7 +173,7 @@ export const docs = { name: 'presentation', 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.", default: "'popover'", }, { @@ -419,7 +419,7 @@ export const docsZh = { name: 'isModal', type: 'boolean', description: - '对话框式弹出框是否设置 aria-modal。仅在 role 为 dialog 时适用。', + '弹出框是否为模态。true 时 Tab 在打开的弹出框内循环，对话框式弹出框带有 aria-modal。false 时不限制焦点：Tab 从锚点（否则为触发器）进入弹出框，再移到锚点之后的元素；仅当焦点在弹出框内或其锚点、触发器上时，Escape 才会关闭它。教学提示或导览步骤请使用 false。作为底部面板时，false 呈现无遮罩、页面仍可交互的面板。', default: 'true', }, { @@ -459,7 +459,7 @@ export const docsZh = { name: 'presentation', type: "'popover' | 'bottom-sheet' | 'adaptive'", description: - "承载内容的界面。'popover' 锚定在触发器上，'bottom-sheet' 以模态 BottomSheet 呈现（由 label 命名），'adaptive' 在紧凑触屏（粗指针，且屏幕按主题定义为紧凑：宽度小于其 md 断点或高度小于 500 px，即任意方向的手机）上使用 BottomSheet，在其他设备上使用锚定的弹出框。在底部面板中，placement、alignment、width、role、isModal 和 hasAutoFocus 不生效，xstyle、className 和 style 作用于面板；底部面板始终支持轻触关闭（点按面板外部或向下轻扫即可关闭，不受 hasLightDismiss 影响），Escape 遵循 hasEscapeDismiss，后备关闭按钮保持隐藏。界面在打开时选定，并保持到关闭。", + "承载内容的界面。'popover' 锚定在触发器上，'bottom-sheet' 以模态 BottomSheet 呈现（由 label 命名），'adaptive' 在紧凑触屏（粗指针，且屏幕按主题定义为紧凑：宽度小于其 md 断点或高度小于 500 px，即任意方向的手机）上使用 BottomSheet，在其他设备上使用锚定的弹出框。在底部面板中，placement、alignment、width、role 和 hasAutoFocus 不生效，xstyle、className 和 style 作用于面板；底部面板始终支持轻触关闭（点按面板外部或向下轻扫即可关闭，不受 hasLightDismiss 影响），Escape 遵循 hasEscapeDismiss，后备关闭按钮保持隐藏。界面在打开时选定，并保持到关闭。", default: "'popover'", }, ], diff --git a/packages/core/src/Popover/Popover.test.tsx b/packages/core/src/Popover/Popover.test.tsx index 8ec0376d29788..91796f4035464 100644 --- a/packages/core/src/Popover/Popover.test.tsx +++ b/packages/core/src/Popover/Popover.test.tsx @@ -1822,7 +1822,7 @@ describe('Popover touch presentation', () => { }); }); -describe('Popover focus region (DEC-9)', () => { +describe('Popover modality (DEC-9, AST-116 FR12–FR15)', () => { function Page({isModal}: {isModal: boolean}) { return (
@@ -1882,27 +1882,6 @@ describe('Popover focus region (DEC-9)', () => { expect(screen.getByRole('button', {name: 'Elsewhere 1'})).toHaveFocus(); }); - it('cycles Tab and Shift+Tab inside the open popover', async () => { - const user = userEvent.setup(); - render(); - await openDetails(); - const first = screen.getByRole('button', { - name: 'First inside', - hidden: true, - }); - const last = screen.getByRole('button', { - name: 'Last inside', - hidden: true, - }); - - await user.tab(); - expect(last).toHaveFocus(); - await user.tab(); - expect(first).toHaveFocus(); - await user.tab({shift: true}); - expect(last).toHaveFocus(); - }); - it('closes on Escape from inside and returns focus to the trigger', async () => { const user = userEvent.setup(); render(); @@ -1915,6 +1894,84 @@ describe('Popover focus region (DEC-9)', () => { expect(trigger).toHaveFocus(); }); }); + + it('cycles Tab and Shift+Tab inside a modal popover', async () => { + const user = userEvent.setup(); + render(); + await openDetails(); + const first = screen.getByRole('button', { + name: 'First inside', + hidden: true, + }); + const last = screen.getByRole('button', { + name: 'Last inside', + hidden: true, + }); + + await user.tab(); + expect(last).toHaveFocus(); + await user.tab(); + expect(first).toHaveFocus(); + await user.tab({shift: true}); + expect(last).toHaveFocus(); + }); + + it('lets Tab and Shift+Tab leave a non-modal popover in page order', async () => { + const user = userEvent.setup(); + render(); + await openDetails(); + const first = screen.getByRole('button', { + name: 'First inside', + hidden: true, + }); + const trigger = screen.getByRole('button', {name: 'Open details'}); + + // jsdom matches no `:popover-open`, so the tab sequence skips the + // popover's controls; start from its last control. + screen.getByRole('button', {name: 'Last inside', hidden: true}).focus(); + await user.tab(); + // The popover renders after its trigger, so what follows it in the page + // follows its last control. + expect(screen.getByRole('button', {name: 'Elsewhere 1'})).toHaveFocus(); + // Still open: leaving focus does not close a popover without light + // dismiss being involved. + expect(trigger).toHaveAttribute('aria-expanded', 'true'); + + first.focus(); + await user.tab({shift: true}); + expect(trigger).toHaveFocus(); + }); + + it('marks only a modal dialog popover aria-modal', async () => { + const {unmount} = render(); + await openDetails(); + expect(screen.getByRole('dialog', {hidden: true})).toHaveAttribute( + 'aria-modal', + 'true', + ); + unmount(); + render(); + await openDetails(); + expect(screen.getByRole('dialog', {hidden: true})).not.toHaveAttribute( + 'aria-modal', + ); + }); + + it('lets an Escape pressed elsewhere pass a non-modal popover', async () => { + const user = userEvent.setup(); + render(); + await openDetails(); + const trigger = screen.getByRole('button', {name: 'Open details'}); + + screen.getByRole('button', {name: 'Elsewhere 2'}).focus(); + await user.keyboard('{Escape}'); + expect(trigger).toHaveAttribute('aria-expanded', 'true'); + + // On its trigger, the popover takes Escape again. + trigger.focus(); + await user.keyboard('{Escape}'); + expect(trigger).toHaveAttribute('aria-expanded', 'false'); + }); }); describe('usePopover trigger ref', () => { diff --git a/packages/core/src/Popover/Popover.tsx b/packages/core/src/Popover/Popover.tsx index c390cd22dc4d9..ca026937f43e7 100644 --- a/packages/core/src/Popover/Popover.tsx +++ b/packages/core/src/Popover/Popover.tsx @@ -351,8 +351,13 @@ export interface PopoverProps extends Pick< role?: 'dialog' | 'none'; /** - * Whether a dialog-style popover is modal (`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 (`presentation`), `false` presents a sheet with no scrim that + * leaves the page interactive. * * @default true */ @@ -409,7 +414,7 @@ export interface PopoverProps extends Pick< * either orientation) and the anchored popover elsewhere, which is what iOS * does with a popover by default. * - * In a sheet, `placement`, `alignment`, `width`, `role`, `isModal` and + * In a sheet, `placement`, `alignment`, `width`, `role` and * `hasAutoFocus` do not apply (the sheet is a modal dialog with its own * focus handling); `xstyle`, `className` and `style` style the sheet's * panel, as they style the anchored surface; `padding` insets the @@ -1137,6 +1142,9 @@ export function Popover({ id={sheetId} isOpen={isSheetOpen} 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} label={label ?? triggerText} height="tall" padding={padding} diff --git a/packages/core/src/Popover/PopoverPresentation.test.tsx b/packages/core/src/Popover/PopoverPresentation.test.tsx index fb4881914ef2f..9681993566d5e 100644 --- a/packages/core/src/Popover/PopoverPresentation.test.tsx +++ b/packages/core/src/Popover/PopoverPresentation.test.tsx @@ -105,7 +105,9 @@ function Filters({ hasLightDismiss, hasEscapeDismiss, hasCloseButton, + isModal, }: { + isModal?: boolean; presentation?: PopoverPresentation; onOpenChange?: (isOpen: boolean) => void; hasLightDismiss?: boolean; @@ -120,6 +122,7 @@ function Filters({ hasLightDismiss={hasLightDismiss} hasEscapeDismiss={hasEscapeDismiss} hasCloseButton={hasCloseButton} + isModal={isModal} content={}> @@ -473,6 +476,26 @@ describe('Popover presentation', () => { }); }); + it('a modal popover presents a modal sheet', async () => { + render(); + await tapTrigger(); + await vi.waitFor(() => { + expect(sheetDialog()).toHaveAttribute('open'); + }); + expect(HTMLDialogElement.prototype.showModal).toHaveBeenCalled(); + }); + + it('a non-modal popover presents a sheet with no scrim, never a modal one (FR16)', async () => { + isCompactTouch = true; + render(); + await tapTrigger(); + await vi.waitFor(() => { + expect(sheetDialog()).not.toBeNull(); + }); + expect(HTMLDialogElement.prototype.showModal).not.toHaveBeenCalled(); + expect(sheetDialog()).not.toHaveAttribute('aria-modal', 'true'); + }); + it('closing the sheet reports it and collapses the trigger', async () => { isCompactTouch = true; const onOpenChange = vi.fn(); diff --git a/packages/core/src/Popover/usePopover.doc.mjs b/packages/core/src/Popover/usePopover.doc.mjs index 52346d5f6d34d..7f9fa3979ab8b 100644 --- a/packages/core/src/Popover/usePopover.doc.mjs +++ b/packages/core/src/Popover/usePopover.doc.mjs @@ -102,7 +102,7 @@ export const docs = { name: 'isModal', type: 'boolean', description: - 'Whether a dialog-role popover is modal (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 follows the browser order from the anchor or trigger into the popover and on, and Escape closes it only while focus is inside it or on its anchor or trigger. A Tab pressed outside is never pulled in.', default: 'true', }, { @@ -241,7 +241,8 @@ export const docsDense = { closeButtonLabel: 'label for hidden close button.', dialogLabel: 'accessible label for popover dialog (role="dialog" only).', role: 'content wrapper ARIA role; "none" for listbox/menu popups.', - isModal: 'whether a dialog-role popover is modal (aria-modal).', + isModal: + 'true: Tab held inside + aria-modal; false: page Tab order, Escape only while focus in popover/anchor/trigger.', hasSurface: 'apply default surface background/radius/shadow.', padding: 'spacing-scale padding on the painted surface; omit for none.', surfaceTarget: diff --git a/packages/core/src/Popover/usePopover.tsx b/packages/core/src/Popover/usePopover.tsx index c16d847625926..a2ed392c378a5 100644 --- a/packages/core/src/Popover/usePopover.tsx +++ b/packages/core/src/Popover/usePopover.tsx @@ -302,9 +302,17 @@ export interface UsePopoverOptions { role?: 'dialog' | 'none'; /** - * Whether the dialog is modal (`aria-modal`). Only applies when `role` is - * `'dialog'`. Set to `false` for non-modal dialogs that do not inert the rest - * of the page. + * Whether the popover is modal. + * + * - `true`: Tab and Shift+Tab cycle inside the open popover, and a dialog + * popover carries `aria-modal`. + * - `false`: the popover holds nothing. Tab moves in the browser's order: + * from the anchor (else the trigger) into the popover, and from its last + * control to what follows the anchor. Escape closes it only while focus + * is inside it or on its anchor or trigger. Use it for a coachmark or a + * tour step, whose reader must reach the control it describes. + * + * A Tab pressed outside the popover is never pulled into it. * * @default true */ @@ -589,6 +597,26 @@ function usePopoverImplementation( // A caller that may cancel an outside dismissal gets the outside-press // watch in place of native light dismiss, which no script can cancel. const watchesOutsidePress = hasLightDismiss && onOutsidePress != null; + // A non-modal popover takes Escape only while it holds focus: inside it, or + // on the anchor or trigger it belongs to. Read at press time. + const surfaceHolderRef = + useRef | null>(null); + const isFocusInNonModalScope = useCallback((): boolean => { + const surface = surfaceHolderRef.current?.current ?? null; + const doc = surface?.ownerDocument ?? document; + const active = doc.activeElement; + if (active == null) { + return false; + } + const anchor = boundAnchorRef.current; + const trigger = triggerElementRef.current; + return ( + surface?.contains(active) === true || + anchor?.contains(active) === true || + trigger?.contains(active) === true + ); + }, []); + // Focus returns as the popover starts to close, while it still holds focus: // the browser restores focus only for an auto popover whose opener is still // there, and then with a focus ring whatever the last input was. The trap @@ -633,7 +661,10 @@ function usePopoverImplementation( getAnchor: () => anchorOptionRef.current == null ? null : boundAnchorRef.current, layerId: layer.id, + isContaining: isModal, + isEscapeInScope: isModal ? undefined : isFocusInNonModalScope, }); + surfaceHolderRef.current = contentRef; returnFocusRef.current = returnFocus; const focusInitialTarget = useCallback(() => { diff --git a/packages/core/src/hooks/useFocusTrap.ts b/packages/core/src/hooks/useFocusTrap.ts index 06f373e461460..3eec7e1ab6261 100644 --- a/packages/core/src/hooks/useFocusTrap.ts +++ b/packages/core/src/hooks/useFocusTrap.ts @@ -583,6 +583,18 @@ interface InternalFocusTrapOptions extends UseFocusTrapOptions { getAnchor?: () => Element | null; /** The id the trigger's `aria-controls` names. */ layerId?: string; + /** + * Whether Tab and Shift+Tab cycle inside the container while it is active. + * A non-modal overlay leaves Tab to the page's own order. + * @default true + */ + isContaining?: boolean; + /** + * Whether the overlay takes an Escape press now, asked at press time. A + * non-modal overlay takes Escape only while it holds focus; otherwise the + * press passes to the layer or page beneath. + */ + isEscapeInScope?: () => boolean; } export interface InternalFocusTrapReturn< @@ -607,6 +619,8 @@ export function useFocusTrapInternal( getTrigger, getAnchor, layerId, + isContaining = true, + isEscapeInScope, } = options; const getRestoreTargetRef = useRef(getRestoreTarget); getRestoreTargetRef.current = getRestoreTarget; @@ -623,8 +637,9 @@ export function useFocusTrapInternal( const containerRef = useRef(null); - // Tab and Shift+Tab wrap inside the container while the trap is active. - useFocusRegionContainment(containerRef, isActive); + // Tab and Shift+Tab wrap inside the container while the trap is active and + // containing. + useFocusRegionContainment(containerRef, isActive && isContaining); // Join the shared layer dismissal stack. The trap no longer listens for // Escape itself: the stack owns one listener and routes each press to the @@ -646,6 +661,7 @@ export function useFocusTrapInternal( // content; hand the stack the container instead so two DOM-nested traps // still resolve in the right order. getContainer: () => containerRef.current, + isPresent: isEscapeInScope, }); useFocusTrapEscapeCompatibilitySignal(isEscapeTrap);