Skip to content
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions .changeset/popover-is-modal.md
Original file line number Diff line number Diff line change
@@ -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.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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].


@nynexman4464
8 changes: 4 additions & 4 deletions packages/core/src/Popover/Popover.doc.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -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.',

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"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.

default: 'true',
},
{
Expand Down Expand Up @@ -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.",

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

default: "'popover'",
},
{
Expand Down Expand Up @@ -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',
},
{
Expand Down Expand Up @@ -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'",
},
],
Expand Down
101 changes: 79 additions & 22 deletions packages/core/src/Popover/Popover.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -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 (
<div>
Expand Down Expand Up @@ -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(<Page isModal={isModal} />);
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(<Page isModal={isModal} />);
Expand All @@ -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(<Page isModal />);
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(<Page isModal={false} />);
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(<Page isModal />);
await openDetails();
expect(screen.getByRole('dialog', {hidden: true})).toHaveAttribute(
'aria-modal',
'true',
);
unmount();
render(<Page isModal={false} />);
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(<Page isModal={false} />);
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', () => {
Expand Down
14 changes: 11 additions & 3 deletions packages/core/src/Popover/Popover.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -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
*/
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

label={label ?? triggerText}
height="tall"
padding={padding}
Expand Down
23 changes: 23 additions & 0 deletions packages/core/src/Popover/PopoverPresentation.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -105,7 +105,9 @@ function Filters({
hasLightDismiss,
hasEscapeDismiss,
hasCloseButton,
isModal,
}: {
isModal?: boolean;
presentation?: PopoverPresentation;
onOpenChange?: (isOpen: boolean) => void;
hasLightDismiss?: boolean;
Expand All @@ -120,6 +122,7 @@ function Filters({
hasLightDismiss={hasLightDismiss}
hasEscapeDismiss={hasEscapeDismiss}
hasCloseButton={hasCloseButton}
isModal={isModal}
content={<button type="button">Apply</button>}>
<Button label="Filter">Filter</Button>
</Popover>
Expand Down Expand Up @@ -473,6 +476,26 @@ describe('Popover presentation', () => {
});
});

it('a modal popover presents a modal sheet', async () => {
render(<Filters presentation="bottom-sheet" />);
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(<Filters presentation="adaptive" isModal={false} />);
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();
Expand Down
5 changes: 3 additions & 2 deletions packages/core/src/Popover/usePopover.doc.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -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',
},
{
Expand Down Expand Up @@ -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:
Expand Down
37 changes: 34 additions & 3 deletions packages/core/src/Popover/usePopover.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -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
*/
Expand Down Expand Up @@ -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<React.RefObject<HTMLDivElement | null> | 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
Expand Down Expand Up @@ -633,7 +661,10 @@ function usePopoverImplementation(
getAnchor: () =>
anchorOptionRef.current == null ? null : boundAnchorRef.current,
layerId: layer.id,
isContaining: isModal,
isEscapeInScope: isModal ? undefined : isFocusInNonModalScope,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

});
surfaceHolderRef.current = contentRef;
returnFocusRef.current = returnFocus;

const focusInitialTarget = useCallback(() => {
Expand Down
20 changes: 18 additions & 2 deletions packages/core/src/hooks/useFocusTrap.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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<
Expand All @@ -607,6 +619,8 @@ export function useFocusTrapInternal<T extends HTMLElement = HTMLElement>(
getTrigger,
getAnchor,
layerId,
isContaining = true,
isEscapeInScope,
} = options;
const getRestoreTargetRef = useRef(getRestoreTarget);
getRestoreTargetRef.current = getRestoreTarget;
Expand All @@ -623,8 +637,9 @@ export function useFocusTrapInternal<T extends HTMLElement = HTMLElement>(

const containerRef = useRef<T | null>(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.
Comment on lines +640 to +641

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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
Expand All @@ -646,6 +661,7 @@ export function useFocusTrapInternal<T extends HTMLElement = HTMLElement>(
// 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);
Expand Down