Repository navigation
fix(Tokenizer): focus hand-offs live in the controls' callback refs and the confirmation's close handler - #9162
Conversation
…nd the confirmation's close handler From the raw-effects audit (architecture:react-component-runtime INV2, INV6). The presentation-swap hand-off was an effect fed by a read of document.activeElement during render; both controls' callback refs now own it: the detaching control notes it held focus (still in the document then, which also covers a control focused before hydration), and the attaching control takes it. The Clear all confirmation's close handler commits the unmount with flushSync and then moves focus and announces, instead of an effect waiting for the next render. Existing FR16 and FR24 focus tests fail with either move removed; a new test shows a re-attach without a swap moves no focus. Co-authored-by: imdreamrunner <imdreamrunner@users.noreply.github.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
AI review status for this pull request.
|
meta-cla
Bot
added
the
CLA Signed
PR Analysis Report
📚 Storybook Preview🧪 Sandbox PreviewModified ComponentsTokenizer (@astryxdesign/core) · View in Storybook
Component Size
Sizes are gzip. Import counts the JavaScript the import loads eagerly; CSS counts the package stylesheet rules referenced by the modules it can load from its own package, lazily loaded ones included. Bundle Size SummaryGzip figures measure each package's entry file, not the code an import pulls in.
Published Package Size
10 other published packages are unchanged. Download is the gzip tarball npm fetches; unpacked is what lands in Accessibility AuditStatus: Accessibility did not run on this pull request; add Generated by PR Enrichment workflow | Storybook | Sandbox | 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: moving both hand-offs out of Effects is the right direction, and the FR16 and FR24 tests prove the new homes carry them, but the swap hand-off now covers only the input and the trigger. With presentation: 'adaptive', focus on a token's Remove button or the inline Clear all button drops to the page body when the screen turns compact-touch, where the base moved it to the trigger.
- Note the pending hand-off whenever focus is inside the detaching field, and add swap tests that start on Remove and on Clear all.
- Reword the changeset and the PR's "Unchanged" line: the public
Tokenizerhas no presentation swap or sheet, so the release note should not announce a consumer-facing fix. - Call
isFocusDetachedfromutils/focusReturn.tsfor the body check inhandOffFocusRef.
[Automated review]
| return () => { | ||
| if (node.ownerDocument.activeElement === node) { | ||
| isFocusPendingHandOffRef.current = true; | ||
| } | ||
| }; |
There was a problem hiding this comment.
Only the input or the trigger notes a pending hand-off here. With presentation: 'adaptive', a keyboard user on a token's Remove button or the inline Clear all button loses focus to the page body when the screen turns compact-touch; the base moved it to the trigger, because its flag was set on any focus entry into the field. Noting the hand-off when focus is anywhere inside the detaching field (for example from a cleanup-returning callback ref on the inline wrapper that checks el.contains(el.ownerDocument.activeElement)) would restore that, with swap tests that start on Remove and on Clear all.
| const takesFocus = isFocusPendingHandOffRef.current; | ||
| isFocusPendingHandOffRef.current = false; | ||
| const active = node.ownerDocument.activeElement; | ||
| if (takesFocus && (active == null || active === node.ownerDocument.body)) { |
There was a problem hiding this comment.
This is isFocusDetached(node.ownerDocument) from utils/focusReturn.ts, which also treats documentElement as detached. Calling it keeps one definition of "focus is on nothing".
| '@astryxdesign/core': patch | ||
| --- | ||
|
|
||
| [fix] `Tokenizer` hands focus to the new control when a presentation swap removes the focused one, through the controls' callback refs instead of a read of the focused element during render. A render React discards, as concurrent rendering can, no longer drops a keyboard user's focus to the page. After the sheet's Clear all confirmation closes, focus moves, and the clear is announced, in the confirm or cancel handler itself. Thanks @imdreamrunner (#7977). |
There was a problem hiding this comment.
The public Tokenizer offers no presentation option and renders only the popover, so it has no presentation swap or sheet, and on the base a discarded render only affected the hasAutoFocus hydration focus. As written, the release note tells consumers a public focus bug was fixed. Suggest wording that says the change is inside the internal sheet presentation (not yet public) and that public fields are unchanged.
…n or Clear all; changeset says the change is internal Review follow-up: only the input and the trigger noted a pending hand-off, so with adaptive presentation, focus on a token's Remove button or the inline Clear all fell to the body on a swap to the sheet, where the base moved it to the trigger. The inline wrapper's callback ref (now stable) notes focus anywhere inside it on detach, with a swap test from each. The body check uses isFocusDetached. The changeset is a [chore] that says the change sits inside the not-yet-public sheet presentation.
There was a problem hiding this comment.
Changes requested: the follow-up fixes the Remove and Clear all hand-off, but a hand-off left pending by the inline wrapper's ref cleanup can now pull focus back into a field the user has left.
- Toggling
isDisabledon a focused field withdisabledMessagere-attaches the wrapper and leaves the flag set, so after the user blurs to the page a new inlinerefrefocuses the input withoutonFocus, and an adaptive swap focuses the trigger; clearing the flag inhandleBlurCapture's leave branch and the trigger'sonBlur, with a regression test, fixes both. - A discarded render did not drop focus on the base (its ref writes persist), so the PR description's Before and the changeset's "Public fields … are unchanged" should describe a conformance change instead.
- The new test's Remove step focuses Remove from the page, which the field redirects to the input; focus the input first and assert Remove holds focus before the swap.
[Automated review]
| } | ||
| return () => { | ||
| if (el.contains(el.ownerDocument.activeElement)) { | ||
| isFocusPendingHandOffRef.current = true; |
There was a problem hiding this comment.
This cleanup also runs when the same wrapper re-attaches through a new wrapperCallbackRef, which happens whenever disabledMessageTooltip.ref changes identity (for example when isDisabled toggles on a field with disabledMessage). Nothing clears the flag on leave, so after the user blurs to the page, the next attach (a new inline ref on the input, or an adaptive swap) takes focus back, with onFocus suppressed. Clearing isFocusPendingHandOffRef in handleBlurCapture's leave branch and the trigger's onBlur keeps every existing test green and stops it.
| render(<Harness presentation="adaptive" hasClear />); | ||
| // Inline: a token's Remove button holds focus, then the screen turns | ||
| // compact-touch. | ||
| screen.getByRole('button', {name: 'Remove Design'}).focus(); |
There was a problem hiding this comment.
Focusing Remove from the page counts as entering the field, and handleFocusCapture redirects it to the input, so this step swaps from the combobox and passes without the wrapper's note. Focusing the combobox first and asserting expect(remove).toHaveFocus() before turn(true) makes it test the Remove path.
| '@astryxdesign/core': patch | ||
| --- | ||
|
|
||
| [chore] Inside `Tokenizer`'s internal sheet presentation (not yet public), focus hand-offs move from effects into callback refs and the Clear all confirmation's close handler. Public fields, which render only the popover, are unchanged. Thanks @imdreamrunner (#7977). |
There was a problem hiding this comment.
"Public fields … are unchanged" holds only once the pending hand-off is cleared on leave: today a public field can get focus pulled back into its input after a disabled toggle.
…geset calls this a conformance change Review follow-up: a wrapper re-attached while focused (an isDisabled toggle with disabledMessage) left the hand-off pending, so after the user left the field a new inline ref or a swap pulled focus back. The blur-capture leave branch and the trigger's onBlur now clear it, with a regression test that fails without the clear. The Remove/Clear all swap test enters the field first, so it tests the Remove path. The changeset is a [chore] conformance note: where focus lands is unchanged.
There was a problem hiding this comment.
Looks good: the follow-up clears a pending hand-off when focus leaves the field, so a re-attached wrapper no longer pulls focus back, and the new test fails without that clear. Every focus destination (a swap from the input, a Remove button or Clear all, and the Clear all confirmation) matches main, and CI is green. One optional addition: a test where a hand-off is pending and focus sits on an in-field Remove button when the input re-attaches would pin the isFocusDetached guard, which no current test exercises.
[Automated review]
User impact
onFocus/onBlurreports are all unchanged.adaptivechanging while the sheet is closed) moves from an effect, fed by adocument.activeElementread during render, into the controls' callback refs (INV6).experimental.presentation, not yet public), so public fields, which render only the popover, behave as before.Spec
architecture:react-component-runtimeINV6: the callback ref owns attach, detach and replacement. INV2: a discrete action stays in its handler. These come from the raw-effects audit's findings forTokenizer.tsx:864andTokenizerBottomSheet.tsx:350.component:TokenizerFR16 (focus follows a presentation swap) and FR24 (focus after the confirmation) are unchanged and still hold.Changes
Tokenizer.tsx: onehandOffFocusRefcallback is merged into the input's and the trigger's refs.hasAutoFocus) that no focus event reported.onBlur) clears any pending hand-off. A later re-attach (a new inline ref, or a swap) then never pulls focus back into a field the user has left.isFocusDetached).isControlFocusedRefare removed.TokenizerBottomSheet.tsx:closeClearConfirmationrunsflushSyncto unmount the confirmation, so the sheet is the modal on top again. It then focuses search and announces (confirm), or focuses Clear all (cancel). The effect andafterConfirmationRefare removed.isDisabledtoggle withdisabledMessage) and the user leaves, a new inline ref pulls no focus back, and this fails without the leave clear. A control that re-attaches without a swap (a new inline ref callback) moves no focus and reports no secondonFocus. It fails if the hand-off ignores where focus is.Evidence
hasAutoFocus) fail with the refs' hand-off removed;flushSynccarries an eslint exception that gives the reason.Credit: @imdreamrunner (#7977), who built the sheet and these hand-offs.