diff --git a/.changeset/bottom-sheet-focus-return-last-input.md b/.changeset/bottom-sheet-focus-return-last-input.md new file mode 100644 index 0000000000000..13c7a2de76767 --- /dev/null +++ b/.changeset/bottom-sheet-focus-return-last-input.md @@ -0,0 +1,7 @@ +--- +'@astryxdesign/core': patch +--- + +[fix] `BottomSheet` returns focus without a focus ring after a pointer close (a tap in WebKit, or a pointer close of a sheet opened from the keyboard) and keeps the ring after a keyboard close, including on a `DropdownMenu` bottom-sheet trigger the caller renders (`renderTrigger`). The sheet now asks the browser not to treat that focus as keyboard focus, the way Dialog's focus moves do. + +@vjeux diff --git a/apps/storybook/stories/DropdownMenu.stories.tsx b/apps/storybook/stories/DropdownMenu.stories.tsx index 57a3c53a41b0e..3bfc0925cac3e 100644 --- a/apps/storybook/stories/DropdownMenu.stories.tsx +++ b/apps/storybook/stories/DropdownMenu.stories.tsx @@ -1052,6 +1052,40 @@ export const ActionSheetPresentation: Story = { }, }; +export const ActionSheetCustomTrigger: Story = { + name: 'Presentation / action sheet, custom trigger', + parameters: { + layout: 'fullscreen', + docs: { + story: {inline: false, height: '560px'}, + description: { + story: + 'A control the caller renders opens the bottom-sheet presentation. When the sheet closes, focus returns to that control, with a focus ring only after keyboard input.', + }, + }, + }, + globals: {viewport: {value: 'mobile1', isRotated: false}}, + render: () => ( +
+ ( + } + label="Project actions" + variant="ghost" + {...props} + /> + )} + items={PROJECT_ACTIONS.map(action => ({ + ...action, + onClick: () => console.log(`${action.label} selected`), + }))} + /> +
+ ), +}; + export const SectionedActionSheetPresentation: Story = { name: 'Presentation / sectioned action sheet', parameters: { diff --git a/packages/core/src/BottomSheet/BottomSheet.test.tsx b/packages/core/src/BottomSheet/BottomSheet.test.tsx index 3a553d10e3c31..9a93ed0235325 100644 --- a/packages/core/src/BottomSheet/BottomSheet.test.tsx +++ b/packages/core/src/BottomSheet/BottomSheet.test.tsx @@ -14,6 +14,7 @@ import {act, render, screen, fireEvent} from '@testing-library/react'; import {createRef, useState} from 'react'; import {BottomSheet} from './BottomSheet'; import {BottomSheetSwitcher} from './BottomSheetSwitcher'; +import {__resetInteractionModalityForTest} from '../utils/interactionModality'; // A sheet has no stops unless its host asks for them, so the tests that // exercise detents pass their own. These three — a 14% peek, a half-height @@ -2351,6 +2352,119 @@ describe('BottomSheet', () => { expect(document.activeElement).toBe(finalFocusRef.current); }); + + // The returned focus shows the focus-visible ring only after keyboard + // input (`architecture:interaction-modality` INV1). A plain focus() leaves + // that to the browser, which rings it after a pointer close: WebKit after + // a tap, and Chromium when the sheet was opened from the keyboard. + describe('ring on the returned focus', () => { + beforeEach(() => { + __resetInteractionModalityForTest(); + }); + + afterEach(() => { + vi.restoreAllMocks(); + }); + + function openThenClose(closeWith: 'pointer' | 'keyboard') { + render(); + const opener = screen.getByRole('button', {name: 'Open sheet'}); + opener.focus(); + fireEvent.click(opener); + const done = screen.getByRole('button', {name: 'Done'}); + if (closeWith === 'pointer') { + fireEvent.pointerDown(done); + } else { + fireEvent.keyDown(done, {key: 'Enter'}); + } + fireEvent.click(done); + return opener; + } + + it('asks the browser for no ring after a pointer close', () => { + const opener = openThenClose('pointer'); + const focus = vi.spyOn(opener, 'focus'); + finishSheetExit(); + + expect(opener).toHaveFocus(); + expect(focus.mock.calls).toEqual([[{focusVisible: false}]]); + }); + + it('leaves the ring to the browser after a keyboard close', () => { + const opener = openThenClose('keyboard'); + const focus = vi.spyOn(opener, 'focus'); + finishSheetExit(); + + expect(opener).toHaveFocus(); + expect(focus.mock.calls).toEqual([[]]); + }); + + it('clears a ring the native close already drew before refocusing', () => { + const opener = openThenClose('pointer'); + // The browser's own dialog close restores focus to the opener, with a + // ring, before the sheet moves focus, and focusing the focused element + // changes nothing. + HTMLDialogElement.prototype.close = vi.fn(function ( + this: HTMLDialogElement, + ) { + this.removeAttribute('open'); + opener.focus(); + }); + const matches = HTMLElement.prototype.matches; + vi.spyOn(HTMLElement.prototype, 'matches').mockImplementation(function ( + this: HTMLElement, + selector: string, + ) { + return selector === ':focus-visible' + ? this === opener + : matches.call(this, selector); + }); + const blur = vi.spyOn(opener, 'blur'); + const focus = vi.spyOn(opener, 'focus'); + finishSheetExit(); + + expect(blur).toHaveBeenCalledTimes(1); + expect(focus.mock.calls.at(-1)).toEqual([{focusVisible: false}]); + expect(blur.mock.invocationCallOrder[0]).toBeLessThan( + focus.mock.invocationCallOrder.at(-1) ?? 0, + ); + expect(opener).toHaveFocus(); + }); + + it('asks for no ring on an explicit final focus target too', () => { + const finalFocusRef = createRef(); + function ExplicitFocusHarness() { + const [isOpen, setIsOpen] = useState(true); + return ( + <> + + + + + + ); + } + + render(); + const done = screen.getByRole('button', {name: 'Done'}); + fireEvent.pointerDown(done); + fireEvent.click(done); + const target = finalFocusRef.current!; + const focus = vi.spyOn(target, 'focus'); + finishSheetExit(); + + expect(target).toHaveFocus(); + expect(focus.mock.calls).toEqual([[{focusVisible: false}]]); + }); + }); }); describe('initial focus', () => { diff --git a/packages/core/src/BottomSheet/BottomSheet.tsx b/packages/core/src/BottomSheet/BottomSheet.tsx index 7a595de25bc2e..921e42c43f053 100644 --- a/packages/core/src/BottomSheet/BottomSheet.tsx +++ b/packages/core/src/BottomSheet/BottomSheet.tsx @@ -4,7 +4,7 @@ /** * @file BottomSheet.tsx - * @input Uses React, StyleX, core hooks, shared layer dismissal, named BottomSheetPanel, BottomSheetSwitcherContext + * @input Uses React, StyleX, core hooks, shared layer dismissal, the shared interaction modality, named BottomSheetPanel, BottomSheetSwitcherContext * @output Exports BottomSheet component and BottomSheetProps * @position Public BottomSheet router plus private standalone/switcher hosts * @@ -46,6 +46,8 @@ import {colorVars, durationVars, easeVars} from '../theme/tokens.stylex'; import {useDevWarning, useScrollLock} from '../hooks'; import {LayerDepthProvider} from '../Layer/LayerDepthContext'; import {useLayerDismissal} from '../Layer/useLayerDismissal'; +import {focusForLastInput} from '../utils/focusReturn'; +import {useInteractionModalityTracking} from '../utils/interactionModality'; import { BottomSheetPanel, type BottomSheetPanelMotion, @@ -320,12 +322,20 @@ function StandaloneBottomSheet({ dialog.close(); } setIsPresented(false); - (finalFocusRef?.current ?? triggerRef.current)?.focus(); + // The returned focus shows the focus-visible ring only after keyboard + // input (`architecture:interaction-modality` INV1). The native close + // may already have restored focus to the target with a ring; the helper + // clears it first. + const returnTarget = finalFocusRef?.current ?? triggerRef.current; + if (returnTarget != null) { + focusForLastInput(returnTarget); + } triggerRef.current = null; }, [finalFocusRef, isOpen], ); + useInteractionModalityTracking(); useScrollLock(shouldPresent && hasScrim); useDevWarning( 'BottomSheet', diff --git a/packages/core/src/BottomSheet/__tests__/BottomSheetFocusReturn.a11y.browser.spec.ts b/packages/core/src/BottomSheet/__tests__/BottomSheetFocusReturn.a11y.browser.spec.ts new file mode 100644 index 0000000000000..fed0626bb8279 --- /dev/null +++ b/packages/core/src/BottomSheet/__tests__/BottomSheetFocusReturn.a11y.browser.spec.ts @@ -0,0 +1,96 @@ +// Copyright (c) Meta Platforms, Inc. and affiliates. + +/** + * @file BottomSheetFocusReturn.a11y.browser.spec.ts + * @input The `Core/DropdownMenu` action-sheet story with a trigger the caller + * renders, in a built Storybook, and real Chromium/WebKit pointer and + * keyboard input. + * @output Browser evidence that the focus a BottomSheet returns when it closes + * follows the last input (docs/architecture/interaction-modality.md INV1): + * no ring after a pointer close, the ring after a keyboard close. + * @position Cross-browser proof; jsdom only proves which focus call the sheet + * makes. The menu's sheet returns focus through BottomSheet's + * `finalFocusRef`, and a control the caller renders carries no style of its + * own that would hide a ring, so the ring shown is the browser's answer. + * Chromium rings it when a keyboard-opened sheet is closed by a pointer; + * WebKit also rings it after a tap open and a tap close. + */ + +import {expect, test, type Locator, type Page} from '@playwright/test'; +import { + DEFAULT_STORYBOOK_DIR, + serveStorybook, + type StaticServer, +} from '@astryxdesign/a11y-spec/storybook'; + +let storybook: StaticServer; +test.beforeAll(async () => { + storybook = await serveStorybook( + process.env.ASTRYX_STORYBOOK_DIR ?? DEFAULT_STORYBOOK_DIR, + ); +}); +test.afterAll(async () => { + await storybook?.close(); +}); + +async function mount(page: Page): Promise { + await page.goto( + `${storybook.origin}/iframe.html?id=core-dropdownmenu--action-sheet-custom-trigger&viewMode=story`, + ); + const trigger = page.getByRole('button', { + name: 'Project actions', + exact: true, + }); + await expect(trigger).toBeVisible(); + return trigger; +} + +async function openFromKeyboard(page: Page, trigger: Locator): Promise { + await trigger.focus(); + await page.keyboard.press('Enter'); + await expect(page.getByRole('dialog')).toBeVisible(); +} + +function row(page: Page): Locator { + return page.getByRole('dialog').getByRole('button', {name: /Edit project/}); +} + +async function expectReturnedFocus( + page: Page, + trigger: Locator, + ring: boolean, +): Promise { + await expect(page.getByRole('dialog')).toBeHidden(); + await expect(trigger).toBeFocused(); + expect( + await trigger.evaluate(element => element.matches(':focus-visible')), + ).toBe(ring); +} + +test('a keyboard-opened sheet closed by a pointer pick returns focus without a ring', async ({ + page, +}) => { + const trigger = await mount(page); + await openFromKeyboard(page, trigger); + await row(page).click(); + await expectReturnedFocus(page, trigger, false); +}); + +test('a pointer open and pointer pick return focus without a ring', async ({ + page, +}) => { + const trigger = await mount(page); + await trigger.click(); + await expect(page.getByRole('dialog')).toBeVisible(); + await row(page).click(); + await expectReturnedFocus(page, trigger, false); +}); + +test('a keyboard-opened sheet closed by Escape keeps the ring', async ({ + page, +}) => { + const trigger = await mount(page); + await openFromKeyboard(page, trigger); + await page.keyboard.press('Escape'); + await expectReturnedFocus(page, trigger, true); +}); diff --git a/packages/core/src/Dialog/Dialog.tsx b/packages/core/src/Dialog/Dialog.tsx index 91cc6fb9977da..dd0bbe50e5c61 100644 --- a/packages/core/src/Dialog/Dialog.tsx +++ b/packages/core/src/Dialog/Dialog.tsx @@ -70,38 +70,10 @@ import {DialogContext} from './DialogContext'; import {themeProps} from '../utils/themeProps'; import type {DialogVariantMap} from './index'; import {focusOutlineProps} from '../utils/focusOutline.stylex'; -import { - getInteractionModality, - useInteractionModalityTracking, -} from '../utils/interactionModality'; +import {useInteractionModalityTracking} from '../utils/interactionModality'; +import {focusForLastInput} from '../utils/focusReturn'; import {useMergedRefs} from '../hooks/useMergedRefs'; -/** - * Move focus programmatically so its ring follows the last input. Dialog does - * not render the element that opened it or the consumer's autofocus target, so - * the browser's focus option is the one way to reach them. - */ -function focusForLastInput(element: HTMLElement): void { - // A text-entry field paints its own focus treatment to show where typing - // goes, and an extra blur there can run validation or mark it touched. - const isTextEntry = - element instanceof HTMLInputElement || - element instanceof HTMLTextAreaElement || - element instanceof HTMLSelectElement || - element.isContentEditable; - if (isTextEntry || getInteractionModality() !== 'pointer') { - element.focus(); - return; - } - // Focusing the element that already has focus does nothing, so a ring the - // browser already drew would stay. WebKit draws one when showModal() or its - // native close moves focus itself, so clear that focus first. - if (element.matches(':focus-visible')) { - element.blur(); - } - element.focus({focusVisible: false}); -} - /** * Calculate a directional translate offset for dialog entry animation. * Returns a normalized vector from the trigger element toward the viewport diff --git a/packages/core/src/utils/focusReturn.test.ts b/packages/core/src/utils/focusReturn.test.ts index 4577ab96f8dbc..2dec110ded8aa 100644 --- a/packages/core/src/utils/focusReturn.test.ts +++ b/packages/core/src/utils/focusReturn.test.ts @@ -1,7 +1,11 @@ // Copyright (c) Meta Platforms, Inc. and affiliates. -import {describe, it, expect, afterEach} from 'vitest'; -import {isFocusDetached} from './focusReturn'; +import {describe, it, expect, afterEach, beforeEach, vi} from 'vitest'; +import {focusForLastInput, isFocusDetached} from './focusReturn'; +import { + __resetInteractionModalityForTest, + __startInteractionModalityTrackingForTest, +} from './interactionModality'; describe('isFocusDetached', () => { afterEach(() => { @@ -40,3 +44,76 @@ describe('isFocusDetached', () => { expect(isFocusDetached(doc)).toBe(true); }); }); + +describe('focusForLastInput', () => { + beforeEach(() => { + __startInteractionModalityTrackingForTest(); + __resetInteractionModalityForTest(); + }); + + afterEach(() => { + vi.restoreAllMocks(); + document.body.innerHTML = ''; + }); + + function button(): HTMLButtonElement { + const element = document.createElement('button'); + document.body.appendChild(element); + return element; + } + + it('leaves the ring to the browser after keyboard input, passing the options through', () => { + const element = button(); + const focus = vi.spyOn(element, 'focus'); + focusForLastInput(element); + focusForLastInput(element, {preventScroll: true}); + expect(focus.mock.calls).toEqual([[], [{preventScroll: true}]]); + }); + + it('asks the browser for no ring after pointer input, keeping the options', () => { + const element = button(); + document.dispatchEvent(new Event('pointerdown')); + const focus = vi.spyOn(element, 'focus'); + focusForLastInput(element); + focusForLastInput(element, {preventScroll: true}); + expect(focus.mock.calls).toEqual([ + [{focusVisible: false}], + [{preventScroll: true, focusVisible: false}], + ]); + }); + + it('keeps plain focus on a text field after pointer input', () => { + const input = document.createElement('input'); + document.body.appendChild(input); + document.dispatchEvent(new Event('pointerdown')); + const focus = vi.spyOn(input, 'focus'); + focusForLastInput(input, {preventScroll: true}); + expect(focus.mock.calls).toEqual([[{preventScroll: true}]]); + }); + + it('clears a ring the browser already drew before refocusing after pointer input', () => { + const element = button(); + element.focus(); + document.dispatchEvent(new Event('pointerdown')); + const matches = HTMLElement.prototype.matches; + vi.spyOn(HTMLElement.prototype, 'matches').mockImplementation(function ( + this: HTMLElement, + selector: string, + ) { + return selector === ':focus-visible' + ? this === element + : matches.call(this, selector); + }); + const blur = vi.spyOn(element, 'blur'); + const focus = vi.spyOn(element, 'focus'); + focusForLastInput(element, {preventScroll: true}); + expect(blur).toHaveBeenCalledTimes(1); + expect(focus.mock.calls).toEqual([ + [{preventScroll: true, focusVisible: false}], + ]); + expect(blur.mock.invocationCallOrder[0]).toBeLessThan( + focus.mock.invocationCallOrder[0] ?? 0, + ); + expect(document.activeElement).toBe(element); + }); +}); diff --git a/packages/core/src/utils/focusReturn.ts b/packages/core/src/utils/focusReturn.ts index d145edfefa5a1..67e286d31fd1f 100644 --- a/packages/core/src/utils/focusReturn.ts +++ b/packages/core/src/utils/focusReturn.ts @@ -2,13 +2,18 @@ /** * @file focusReturn.ts - * @input The document's current active element. - * @output Whether focus is currently "detached" — on nothing in particular. + * @input The document's current active element and the shared interaction + * modality. + * @output Whether focus is currently "detached" — on nothing in particular — + * and a focus move whose ring follows the last input. * @position Utility; used by overlay triggers (e.g. DateInput, DateTimeInput) * that return focus to their control when the overlay closes, but only when - * the dismiss did not already move focus somewhere the user chose. + * the dismiss did not already move focus somewhere the user chose, and by + * overlays (Dialog, DropdownMenu) that move focus programmatically. */ +import {getInteractionModality} from './interactionModality'; + /** * True when focus rests on nothing in particular — no active element, or the * document body / root element. @@ -29,3 +34,43 @@ export function isFocusDetached(doc: Document = document): boolean { active == null || active === doc.body || active === doc.documentElement ); } + +/** + * Move focus programmatically so its ring follows the last input + * (`architecture:interaction-modality` INV1). After pointer input the browser's + * focus option keeps the element from matching `:focus-visible`, which is the + * one way to reach an element the overlay does not render: the consumer's + * autofocus target, the control that opened a Dialog, or a menu trigger the + * caller renders. Pass `options` (for example `preventScroll`) as to `focus()`. + * + * Make sure modality tracking is active (`useInteractionModalityTracking()`) + * in the component that calls this. + */ +export function focusForLastInput( + element: HTMLElement, + options?: FocusOptions, +): void { + // A text-entry field paints its own focus treatment to show where typing + // goes, and an extra blur there can run validation or mark it touched. + const isTextEntry = + element instanceof HTMLInputElement || + element instanceof HTMLTextAreaElement || + element instanceof HTMLSelectElement || + element.isContentEditable; + if (isTextEntry || getInteractionModality() !== 'pointer') { + if (options === undefined) { + element.focus(); + } else { + element.focus(options); + } + return; + } + // Focusing the element that already has focus does nothing, so a ring the + // browser already drew would stay. WebKit draws one when its own focus + // restoration (a dialog's close, a popover's hide) moves focus itself, so + // clear that focus first. + if (element.matches(':focus-visible')) { + element.blur(); + } + element.focus({...options, focusVisible: false}); +}