Repository navigation
fix(BottomSheet): a sheet removed while open returns focus where a close would - #10061
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
meta-cla
Bot
added
the
CLA Signed
|
AI review status for this pull request.
|
|
/approve-spec 156b994 |
PR Analysis Report🔎 Preview ·
|
| Component | Size | LoC | Visual · A11y · RTL |
|---|---|---|---|
| BottomSheet | 56.6 kB (+246 B) · 7.5 kB CSS (0) | — | ✓ · ✓ · — |
| TimeInput | 140.8 kB (+230 B) · 14.5 kB CSS (0) | — | — · — · — |
| DateTimeInput | 185.6 kB (+194 B) · 17.2 kB CSS (0) | — | — · — · — |
62 more components
| Component | Size | LoC | Visual · A11y · RTL |
|---|---|---|---|
| DateInput | 163.7 kB (+188 B) · 15.5 kB CSS (0) | — | — · — · — |
| ContextMenu | 83.0 kB (+174 B) · 16.3 kB CSS (0) | — | — · — · — |
| PowerSearch | 260.2 kB (+168 B) · 22.6 kB CSS (0) | — | — · — · — |
| lab/Schedule | 63.9 kB (+166 B) · 3.9 kB CSS (0) | — | — · — · — |
| TableCell | 84.2 kB (+155 B) · 16.7 kB CSS (0) | — | — · — · — |
| TableHeaderCell | 83.8 kB (+155 B) · 16.5 kB CSS (0) | — | — · — · — |
| Markdown | 153.7 kB (+149 B) · 19.2 kB CSS (0) | — | — · — · — |
| Table | 92.4 kB (+147 B) · 16.9 kB CSS (0) | — | — · — · — |
| DropdownMenu | 77.9 kB (+132 B) · 16.4 kB CSS (0) | — | — · — · — |
| MoreMenu | 78.2 kB (+129 B) · 16.4 kB CSS (0) | — | — · — · — |
| lab/CommentComposer | 109.0 kB (+120 B) · 1.9 kB CSS (0) | — | — · — · — |
| lab/TourStep | 61.3 kB (+116 B) · 590 B CSS (0) | — | — · — · — |
| MultiSelector | 80.3 kB (+111 B) · 15.6 kB CSS (0) | — | — · — · — |
| Selector | 83.2 kB (+111 B) · 16.2 kB CSS (0) | — | — · — · — |
| Pagination | 93.2 kB (+110 B) · 16.6 kB CSS (0) | — | — · — · — |
| lab/ChatEmojiPicker | 69.1 kB (+101 B) · 773 B CSS (0) | — | — · — · — |
| lab/ChatReactionBar | 69.6 kB (+101 B) · 1.1 kB CSS (0) | — | — · — · — |
| Popover | 55.4 kB (+99 B) · 12.8 kB CSS (0) | — | — · — · — |
| Tokenizer | 74.8 kB (+98 B) · 17.1 kB CSS (0) | — | — · — · — |
| CommandPalette | 58.7 kB (+96 B) · 12.3 kB CSS (0) | — | — · — · — |
| DateRangeInput | 75.9 kB (+93 B) · 11.2 kB CSS (0) | — | — · — · — |
| lab/TransferListSelector | 87.1 kB (+91 B) · 1.9 kB CSS (0) | — | — · — · — |
| Dialog | 22.3 kB (+83 B) · 6.7 kB CSS (0) | — | — · — · — |
| Drawer | 41.4 kB (+82 B) · 8.2 kB CSS (0) | — | — · — · — |
| AlertDialog | 90.2 kB (+73 B) · 13.1 kB CSS (0) | — | — · — · — |
| Lightbox | 55.0 kB (+73 B) · 9.1 kB CSS (0) | — | — · — · — |
| lab/Comments | 273.6 kB (+71 B) · 2.5 kB CSS (0) | — | — · — · — |
| lab/CommentThread | 268.5 kB (+69 B) · 2.4 kB CSS (0) | — | — · — · — |
| ContextMenuItem | 25.9 kB (+60 B) · 6.1 kB CSS (0) | — | — · — · — |
| richtext/RichTextEditorToolbar | 234.0 kB (+55 B) · 454 B CSS (0) | — | — · — · — |
| DropdownMenuRadioItem | 21.1 kB (+5 B) · 6.4 kB CSS (0) | — | — · — · — |
| TopNavMenu | 61.0 kB (+5 B) · 9.9 kB CSS (0) | — | — · — · — |
| SideNavHeading | 61.7 kB (+4 B) · 10.0 kB CSS (0) | — | — · — · — |
| Collapsible | 24.6 kB (+1 B) · 4.5 kB CSS (0) | — | — · — · — |
| lab/CommentPin | 48.5 kB (+1 B) · 1.2 kB CSS (0) | — | — · — · — |
| AppShell | 76.3 kB (0) · 12.2 kB CSS (0) | — | — · — · — |
| BottomSheetSwitcher | 17.4 kB (0) · 3.4 kB CSS (0) | — | ✓ · ✓ · — |
| BreadcrumbItem | 71.5 kB (0) · 11.3 kB CSS (0) | — | — · — · — |
| ChatComposer | 63.3 kB (0) · 10.5 kB CSS (0) | — | — · — · — |
| ChatComposerInput | 61.3 kB (0) · 10.0 kB CSS (0) | — | — · — · — |
| ChatComposerTokenElement | 61.3 kB (0) · 10.0 kB CSS (0) | — | — · — · — |
| ComplexSelector | 57.6 kB (0) · 10.4 kB CSS (0) | — | — · — · — |
| DropdownMenuCheckboxItem | 21.0 kB (0) · 6.4 kB CSS (0) | — | — · — · — |
| DropdownMenuItem | 20.3 kB (0) · 6.0 kB CSS (0) | — | — · — · — |
| DropdownMenuSubMenu | 59.7 kB (0) · 9.3 kB CSS (0) | — | — · — · — |
| LayerProvider | 64.5 kB (0) · 9.4 kB CSS (0) | — | — · — · — |
| LayoutPanel | 20.5 kB (0) · 4.9 kB CSS (0) | — | — · — · — |
| SideNav | 76.3 kB (0) · 10.1 kB CSS (0) | — | — · — · — |
| SideNavItem | 56.7 kB (0) · 9.8 kB CSS (0) | — | — · — · — |
| TableSelectionToolbar | 56.4 kB (0) · 11.8 kB CSS (0) | — | — · — · — |
| TabList | 35.8 kB (0) · 6.3 kB CSS (0) | — | — · — · — |
| Toolbar | 35.2 kB (0) · 7.8 kB CSS (0) | — | — · — · — |
| TopNavHeading | 61.1 kB (0) · 9.8 kB CSS (0) | — | — · — · — |
| TopNavMegaMenu | 56.2 kB (0) · 10.6 kB CSS (0) | — | — · — · — |
| TreeList | 40.8 kB (0) · 5.4 kB CSS (0) | — | — · — · — |
| Typeahead | 65.1 kB (0) · 11.2 kB CSS (0) | — | — · — · — |
| AvatarGroup | 37.8 kB (−1 B) · 5.5 kB CSS (0) | — | — · — · — |
| BaseTypeahead | 57.2 kB (−1 B) · 9.6 kB CSS (0) | — | — · — · — |
| ButtonGroup | 9.1 kB (−1 B) · 3.4 kB CSS (0) | — | — · — · — |
| NavHeadingMenu | 15.4 kB (−1 B) · 3.5 kB CSS (0) | — | — · — · — |
| SegmentedControl | 83.9 kB (−2 B) · 6.7 kB CSS (0) | — | — · — · — |
| TabMenu | 58.3 kB (−7 B) · 9.8 kB CSS (0) | — | — · — · — |
Package size: @astryxdesign/core 6.57 MB packed (+1.3 kB); no new dependencies.
API changes: none in this pull request
Committed API report behind this build: cli 9 rows, core 4 rows; run pnpm api:report.
Touched components checked; add ci:run-visual, ci:run-a11y or ci:run-rtl to run the full suites.
Generated by PR Enrichment workflow | View full report
![astracat-bot[bot]](https://avatars.githubusercontent.com/in/4796231?s=60&v=4){kind=small}
There was a problem hiding this comment.
Changes requested. Removing a single open sheet now returns focus to finalFocusRef, the opener or the sheet beneath, and the new tests fail on main and pass here. Three things need to change before approval:
- A stack removed in one update (Filters with Sort opened from it, removed by a button in Sort) still leaves focus on the page body, because the cleanup returns early for the covered sheet at
BottomSheet.tsx:563; dropping|| stackViewRef.current.depth > 0lets the focus-lost check return focus to the first opener, as FR9 does for a stack closed together, and the BottomSheet suite still passes with that change. - The changeset's "Focus no longer drops to the page" claims more than the change does, since the new "moves no focus when the opener was removed too" test expects focus on the body, so please say that focus returns while its target is still on the page.
- The removal repair is a second layout Effect that works only while declared above the registration Effect, so please prepare it at registration, choose the target in that cleanup before the registry's leave runs, and remove the separate Effect.
Please also add a whole-stack case to the suite and to FR19's verification row, and limit FR19's covered-sheet sentence to a top sheet that stays. A new head needs a fresh /approve-spec from an engineering owner.
[Automated review]
| const key = stackKeyRef.current; | ||
| const repairOnRemoval = prepareFocusRepairOnRemoval(dialog); | ||
| return () => { | ||
| if (!isSheetHostPresented(dialog) || stackViewRef.current.depth > 0) { |
There was a problem hiding this comment.
When a whole stack is removed in one update, this early return skips the covered (bottom) sheet. The top sheet's opener was inside that sheet, so focus stays on the body. Dropping || stackViewRef.current.depth > 0 lets prepareFocusRepairOnRemoval's focus-lost check decide. A covered sheet removed under a top sheet that stays still moves nothing, because focus is in the top sheet. A removed stack returns focus to the bottom sheet's finalFocusRef or opener, as FR9 does for a stack closed together.
| // that took focus in the same commit (a replace, a switcher step) keeps it. | ||
| // Declared before the registration below, so its cleanup still sees this | ||
| // sheet in the stack. | ||
| useLayoutEffect(() => { |
There was a problem hiding this comment.
This Effect is correct only while it sits above the registration Effect. Below it, the registry's leave would already have closed the dialog and dropped the entry, so the repair would never run. Prepare the repair when the sheet registers, and choose the target in that cleanup before calling the leave function. That removes the extra Effect and its order dependency.
| '@astryxdesign/core': patch | ||
| --- | ||
|
|
||
| [fix] BottomSheet: a sheet removed while it is open (rendered only while open, or removed with the rest of a view) returns focus where a close would send it: to `finalFocusRef`, else the control that opened it, or into the sheet beneath. Focus no longer drops to the page, so keyboard and screen-reader users keep their place. A sheet that opens in the same update keeps focus, and removing a covered sheet moves no focus. |
There was a problem hiding this comment.
"Focus no longer drops to the page" claims more than the change does: the new "moves no focus when the opener was removed too" test expects focus on the body. Suggested wording: focus returns where a close would send it while that target is still on the page. When none remains, as when the opener is removed with the sheet, focus stays on the page.
| // close for focus: focus goes where an ordinary close sends it. A covered | ||
| // sheet moves no focus, because focus is in the sheet on top, and a sheet | ||
| // that took focus in the same commit (a replace, a switcher step) keeps it. |
There was a problem hiding this comment.
Non-blocking (comment style): this sentence has 31 words and two topics. Suggested: "A covered sheet moves no focus. Focus is in the sheet on top. A sheet that took focus in the same commit keeps it."
…a whole stack removed at once returns focus to its first opener
|
/approve-spec 5f21650 |
There was a problem hiding this comment.
Changes requested. Removing an open sheet now returns focus to finalFocusRef, the opener or the sheet beneath, the changes requested earlier are in, and the new cases fail on main and pass here. One thing needs to change before approval:
- When sibling sheets are declared top sheet first and the top sheet has a
finalFocusRefoutside the bottom sheet, removing both in one update sends focus to thatfinalFocusRefinstead of the bottom sheet's opener as FR19 states, because the top sheet's repair runs first and falls through when the sheet beneath has also left (BottomSheet.tsx:629-639); return early whenrevealedis no longer connected, and add a sibling-order whole-stack case to the suite.
Optionally, split the 26-word comment at BottomSheet.tsx:561-562. A new head needs a fresh /approve-spec from an engineering owner.
[Automated review]
| repairOnRemoval(() => { | ||
| if ( | ||
| revealed != null && | ||
| revealed.element.isConnected && | ||
| (returnTarget == null || !revealed.element.contains(returnTarget)) | ||
| ) { | ||
| revealed.focus(); | ||
| } else if (returnTarget != null && returnTarget.isConnected) { | ||
| returnFocusForLastInput(returnTarget); | ||
| } | ||
| }); |
There was a problem hiding this comment.
When a whole stack is removed and the top sheet is declared before the bottom sheet (siblings), this repair runs first: revealed is the bottom sheet, which has also left, so it falls through to this sheet's own finalFocusRef, and the bottom sheet's repair then finds focus no longer lost. FR19 says the whole stack returns to the bottom sheet's finalFocusRef or opener. Returning early when revealed != null && !revealed.element.isConnected leaves the return to the bottom sheet's repair; the BottomSheet suite still passes with it. Please add a sibling, top-declared-first case with a finalFocusRef on the top sheet.
| // A sheet removed while open is closed for focus: once the commit has | ||
| // finished, focus goes where a close sends it, if the removal lost focus. |
There was a problem hiding this comment.
Non-blocking (comment style): this sentence has 26 words and two topics. Suggested: "After the commit, the repair sends focus where a close would, if the removal lost focus."
… focus for the stack
|
/approve-spec 3486879 |
There was a problem hiding this comment.
Changes requested. Removing an open standalone sheet now returns focus to finalFocusRef, the opener or the sheet beneath, the sibling-stack case from the last round is fixed, and the new cases fail on main and pass here. One thing needs to change before approval:
- Miss: FR19 (
BottomSheet.spec.md:170) and the changeset describe every removed sheet and stack, but aBottomSheetSwitcherflow removed while open, alone or as the bottom level of a stack (FR12), still leaves focus onbodybecause onlyStandaloneBottomSheetarms the repair, so either scope FR19 and the changeset to standalone sheets with a gap note for the switcher flow, or give the switcher's registration cleanup (BottomSheetSwitcher.tsx:690-699) the same repair and a test.
A new head needs a fresh /approve-spec from an engineering owner.
[Automated review]
| | FR16 | An open modal sheet is a focus region through Core's shared focus-region module (`component:Dialog/DEC-2`): in the top sheet, Tab from the last tabbable control moves focus to the first and Shift+Tab from the first moves focus to the last, so focus never leaves the sheet for the browser's own interface. A covered or closing sheet does not answer Tab, and a non-modal sheet contains nothing (FR11). A switcher step (the deprecated BottomSheetSwitcher) is the exception to DEC-2, as `component:Dialog` DEC-2 states. | `component:Dialog/DEC-2` | | ||
| | FR17 | A BottomSheet open on the server, by its own `isOpen` or as a `BottomSheetSwitcher`'s active sheet, renders closed until hydration (no open styles, no `aria-modal`, the markup of a closed sheet) and opens in the render after hydration, the first that can enter its native host; it never claims a modality it does not have (`spec:AST-020` FR1, WCAG 2.2 SC 4.1.2). The server and hydrating renders produce the same markup (`spec:AST-013` FR9), and the open after hydration is settled, with no entry motion; later opens and closes animate as usual (`spec:AST-013` FR13). | `spec:AST-020` FR1 | | ||
| | FR18 | A finger's pull up past the tallest detent stretches the sheet on `spec:AST-094` FR3's rubber band, the same on every pull, with no hold. The sheet's surface reaches below the screen far enough for the whole stretch and the release, so the page under it never shows. A pull on the sheet's body stretches it as a pull on the grabber does, once the content has no room left to scroll that way: content that fits stretches at once, and a long list scrolls to its end first, then holds still while the sheet stretches. At every detent, a list that reaches its end mid-pull holds still while the sheet takes the rest of the pull. A sheet the caller sizes or styles keeps 48 px below the screen, and its stretch stops there. An inline block size, any `xstyle` and any class name count, because the sheet cannot read their rules. A mouse or a pen keeps its own resisted stretch, at most 48 px (`spec:AST-017` FR59). | DEC-3; `spec:AST-094` FR3 | | ||
| | FR19 | A sheet removed while open is closed for focus: when it was the standalone or top sheet and focus was inside it, or fell to the body with its removal, focus returns as FR9 (modal) or FR11 (non-modal) state, to `finalFocusRef`, else the opener while it is still connected; with a sheet open beneath, focus goes into that revealed sheet by FR9. An FR10 replace keeps its handoff to the entering sheet, and a switcher step's change keeps FR12's handoff. Removing a whole stack in one update returns focus once, as FR9 closes it: to the bottom sheet's `finalFocusRef` or opener. Removing a covered sheet under a top sheet that stays moves no focus, because focus is in the top sheet. A removed opener moves no focus and raises no error. | `spec:AST-020/FR1` (WCAG 2.2 SC 2.4.3); FR9; FR11 | |
There was a problem hiding this comment.
Miss: this row covers every sheet removed while open and every stack, and it names switcher steps, but only StandaloneBottomSheet arms the repair. A BottomSheetSwitcher flow removed while open, alone or as the bottom level of a stack (FR12), still leaves focus on body. Scope the row to standalone sheets, for example "A standalone sheet (not a switcher step) removed while open …", with an implementation gap note for the switcher flow, or give BottomSheetSwitcher's registration cleanup the same repair and a test.
| '@astryxdesign/core': patch | ||
| --- | ||
|
|
||
| [fix] BottomSheet: a sheet removed while it is open (rendered only while open, or removed with the rest of a view) returns focus where a close would send it, while that place is still on the page: to `finalFocusRef`, else the control that opened it, or into the sheet beneath. A whole stack removed at once returns focus to the first sheet's `finalFocusRef` or opener. When none of these is left on the page, for example when the opener is removed with the sheet, focus stays where the browser leaves it. A sheet that opens in the same update keeps focus, and removing a covered sheet under a top sheet that stays moves no focus. |
There was a problem hiding this comment.
Miss: a <BottomSheet sheetId> step in a BottomSheetSwitcher that is removed while open still leaves focus on body. Suggested wording, unless the switcher gets the same repair: "BottomSheet: a standalone sheet removed while it is open (rendered only while open, or removed with the rest of a view) returns focus …".
…flow is a recorded gap
|
/approve-spec f951384 |
There was a problem hiding this comment.
Changes requested. Removing an open standalone sheet now returns focus to finalFocusRef, the opener or the sheet beneath, FR19 and the changeset now name standalone sheets, and six of the nine new cases fail on main and pass here. One thing needs to change before approval:
- Miss: a non-modal sheet (
hasScrim={false}) rendered only while open, removed after the person clicked the page behind it and pressed Escape, now moves focus frombodyto its opener, which a close of the same sheet does not do and which FR19 ("focus was inside it, or fell to the body with its removal") and FR11 ("only while focus is still inside the sheet") rule out, so skip the opener return when focus already rests on the body before the removal (BottomSheet.tsx:626-627) and add that case to the suite, or state that behavior in FR19 and the changeset.
A new head needs a fresh /approve-spec from an engineering owner.
[Automated review]
| if (repairOnRemoval != null && isSheetHostPresented(dialog)) { | ||
| const returnTarget = | ||
| finalFocusRefRef.current?.current ?? triggerRef.current; |
There was a problem hiding this comment.
Miss: for a non-modal sheet this returns focus to the opener even when focus already rested on the page body before the removal (the person clicked the live page, then pressed Escape). isFocusLost is also true for that body focus, so the repair runs, while a close of the same sheet leaves focus alone (FR11, and the close path's hasScrim || hadFocusAtCloseRef.current at line 754). Suggested: read here, before leave(), whether focus already rests on the body, and skip the opener return for a non-modal sheet when it does, with a test; or state this behavior in FR19 and the changeset.
|
/approve-spec ca21aa3 |
There was a problem hiding this comment.
Changes requested. Removing an open standalone sheet now returns focus to finalFocusRef, the opener or the sheet beneath, and the non-modal case from the last round is fixed: focus that already rested on the page body stays there. Two things in the new commit need to change before approval:
BottomSheet.tsx:308-323addsisDetachedFocus, a copy ofisFocusDetached, and module-level state to decide whether a removal lost focus, whichprepareFocusRepairOnRemovalinutils/focusReturn.tsalready decides, so move that before-removal read intofocusReturn.ts(for example an option that recordsisFocusDetached(doc)at the commit's first removal) and let the cleanup make one call.- A non-modal sheet with
finalFocusRefthat is removed after the person clicked the page and pressed Escape now leaves focus onbody, while closing the same sheet focusesfinalFocusRef(FR11), so the changeset's "returns focus where a close would send it" and the comment atBottomSheet.tsx:646-648overstate the change; add FR19's condition to both (for example "Focus the person had already moved out of a non-modal sheet stays where it is") or keepfinalFocusRefin that case and add a test.
CI has not run on this draft yet, and a new head needs a fresh /approve-spec from an engineering owner.
[Automated review]
| const focusAtRemovalStart = new WeakMap<Document, Element | null>(); | ||
| function focusBeforeRemoval(doc: Document): Element | null { | ||
| if (!focusAtRemovalStart.has(doc)) { | ||
| focusAtRemovalStart.set(doc, doc.activeElement); | ||
| queueMicrotask(() => focusAtRemovalStart.delete(doc)); | ||
| } | ||
| return focusAtRemovalStart.get(doc) ?? null; | ||
| } | ||
|
|
||
| function isDetachedFocus(element: Element | null): boolean { | ||
| return ( | ||
| element == null || | ||
| element === element.ownerDocument.body || | ||
| element === element.ownerDocument.documentElement | ||
| ); | ||
| } |
There was a problem hiding this comment.
isDetachedFocus repeats the three checks of isFocusDetached (utils/isFocusDetached.ts) on a stored element, and focusAtRemovalStart keeps module-level state for whether the removal lost focus, which prepareFocusRepairOnRemoval decides. Suggested: record isFocusDetached(doc) once per document at the commit's first removal inside focusReturn.ts (an option Dialog and Lightbox leave off), so this cleanup makes one call and these helpers go.
| // Read before the registry lets go of the sheet beneath. Focus that | ||
| // already rested on the page body did not fall with the removal, so it | ||
| // stays, as a close leaves it. |
There was a problem hiding this comment.
"As a close leaves it" holds only for a non-modal sheet without finalFocusRef: a non-modal close focuses finalFocusRef wherever focus is (FR11), and for a modal sheet the native close in leave() returns focus to the opener. Suggested: "Focus already on the page body did not fall with the removal. The repair leaves it."
| '@astryxdesign/core': patch | ||
| --- | ||
|
|
||
| [fix] BottomSheet: a standalone sheet (not a `BottomSheetSwitcher` step) removed while it is open (rendered only while open, or removed with the rest of a view) returns focus where a close would send it, while that place is still on the page: to `finalFocusRef`, else the control that opened it, or into the sheet beneath. A whole stack of standalone sheets removed at once returns focus to the first sheet's `finalFocusRef` or opener. When none of these is left on the page, for example when the opener is removed with the sheet, focus stays where the browser leaves it. A sheet that opens in the same update keeps focus, and removing a covered sheet under a top sheet that stays moves no focus. |
There was a problem hiding this comment.
With a non-modal sheet and finalFocusRef, focus that rested on the page body before the removal now stays there, while closing the same sheet focuses finalFocusRef (FR11). Suggested addition: "Focus the person had already moved out of a non-modal sheet stays where it is."
…s rested on the page
|
/approve-spec d3f7d67 |
There was a problem hiding this comment.
Approved pending CI: CI has not run on this head yet. Every CI job is skipped while the pull request is a draft, so CI runs once it is marked ready for review. Removing an open standalone sheet now returns focus to finalFocusRef, the opener or the sheet beneath, and both points from the last round are fixed: the shared prepareFocusRepairOnRemoval now owns the check for focus already resting on the page body, and the changeset says such focus stays where it is. The new tests fail on main and pass here.
[Automated review]
|
/approve-spec 6876fbe |
There was a problem hiding this comment.
Approved pending CI: the required build, test and docsite-test checks report the heavy-CI deferral on this head, so those suites have not run yet; they run on the pre-merge re-run.
A standalone BottomSheet removed while open now returns focus to finalFocusRef, the opener or the sheet beneath, as FR20 states, and the new removed-while-open cases fail on main and pass here. Numbering the rule FR20 keeps it clear of the FR19 proposed in #10055.
[Automated review]
There was a problem hiding this comment.
Approved pending CI: the heavy test, build, interaction, visual-regression and accessibility jobs are still running on this head, so this approval does not cover their results.
A standalone BottomSheet removed while open now returns focus to finalFocusRef, the opener or the sheet beneath, as FR20 states, and the new removed-while-open cases fail on main and pass with this change. The merge from main leaves this pull request's own changes untouched.
[Automated review]
|
/approve-spec 2de4225 |
Refs #8963
User impact
A BottomSheet that is removed while it is open drops focus to
<body>. That happens when the app renders the sheet only while open ({isEditing && <BottomSheet isOpen …>}), or removes it with the rest of a view. Keyboard and screen-reader users then lose their place: the next Tab starts from the top of the page. After this change, focus goes where a close would send it: tofinalFocusRef, else the control that opened the sheet, or into the sheet beneath.Problem and solution fit
<dialog>runs no close and no browser focus return. BottomSheet's own focus return runs only when a close's exit ends. On unmount, the registry only takes the dialog out of the top layer (closeDialogKeepingFocus).<body>means the person must find their place again.spec:AST-020FR1. Dialog already restores focus in this case, as does Lightbox.Fix: a removal while open is a close for focus. The stack registration prepares the same repair Dialog uses. Its cleanup picks the target before the registry lets the sheet go. After the commit, the repair acts only if the sheet really left the document and the removal lost focus. Then:
finalFocusRef, else the opener, while it is still connected.A sheet that took focus in the same commit (an opening sheet, a switcher step) keeps it, because focus is not lost. A covered sheet removed under a top sheet that stays moves nothing, because focus is in the top sheet. A whole stack removed at once returns focus to the first sheet's
finalFocusRefor opener, as FR9 does for a stack closed together. When no target is left on the page (the opener removed with the sheet), focus stays where the browser leaves it. Focus the person had already moved out of the sheet to the page body (they clicked the page behind a non-modal sheet) stays where it is, even with afinalFocusRef: the removal did not lose that focus (FR20's condition). The shared repair decides this through a newkeepRestingFocusoption onprepareFocusRepairOnRemoval. It reads whether focus rested on the body at the commit's first removal, because one sheet's removal can drop focus before a later sibling's cleanup runs. Dialog and Lightbox do not set it.utils/focusReturn.tssplitsprepareFocusReturnOnRemovalinto a generalprepareFocusRepairOnRemoval(surface)(repair)plus the opener return Dialog and Lightbox already use. Their behavior is unchanged.Expected behavior and authority
component:BottomSheetFR20 (new, from the specification owner's ruling): "A sheet removed while open is closed for focus…", with FR9 and FR11 for where focus goes.spec:AST-020FR1 (WCAG 2.2 SC 2.4.3).Smallest restoration
BottomSheet.tsx: the removal repair, prepared in the existing stack registration. No new Effect.utils/focusReturn.ts: the shared repair split out, plus the opt-inkeepRestingFocusread. No behavior change for Dialog or Lightbox.BottomSheetCloseFocus.test.tsx: a "removed while open" suite.BottomSheet.spec.md: FR20, its verification row, and the AR4 cross-reference.RenderedWhileOpen, for the reproduction.Evidence
Real Chromium (headless), story
core-bottomsheet--rendered-while-open, keyboard only: Tab to "Edit filters", Enter, Tab to "Done", Enter (which removes the open sheet).main(same story)BODYBODYBODY), then Escape removes itBODYBODY(unchanged)Scope
This fixes standalone BottomSheets only. A
BottomSheetSwitcherflow removed while open still returns no focus; FR20 records that as an implementation gap, and it is a separate follow-up. This fixes only the BottomSheet part of #8963. Dialog (with AlertDialog and CommandPalette) and Lightbox are already fixed, and Drawer has its own pull request. No gesture or motion changes.Testing
main, 6 fail: a modal and a non-modal removed from inside,finalFocusRef, the top sheet of a stack, and a whole stack nested or as siblings declared top first. The other 4 guard cases pass onmain: a covered sheet removed under a top sheet that stays, a sheet opening in the same update, an opener removed with the sheet, and focus that rested on the body before a non-modal sheet was removed (that one fails on this PR's previous head). The nested whole-stack case also fails on this PR's first head, and the sibling case on its second.vitest run packages/core/src/BottomSheet: BottomSheet, Dialog, Lightbox and utils: 77 files, 1792 tests passed.pnpm -F @astryxdesign/core typecheck, eslint, Prettier,pnpm check:changesets,pnpm check:knowledge.Writing rubric results
Run over the
BottomSheet.spec.mdchanges (FR20, including its whole-stack and covered-sheet sentences, its verification row, the AR4 sentence):