fix: report focus honestly when something else holds the focus - #3206
Open
christianhg wants to merge 1 commit into
Open
fix: report focus honestly when something else holds the focus#3206christianhg wants to merge 1 commit into
focus honestly when something else holds the focus#3206christianhg wants to merge 1 commit into
Conversation
🦋 Changeset detectedLatest commit: 893ee6e The changes in this PR will be included in the next version bump. This PR includes changesets to release 14 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
Bundle Stats✅ No significant changes. All scenario measurements (7)🗺️
Significant means at least 1.0 KB and 1% gzip, or at least 5 ms and 10% import time. |
christianhg
marked this pull request as ready for review
September 1, 2026 08:04
`DOMEditor.focus` set `editor.focused = true` before `el.focus()` and never verified the focus took. When a focus-containing overlay held focus (a react-aria modal's containment, or `inert` on surrounding content, where `el.focus()` is a complete no-op), the flag was left claiming focus the editor did not have, and only incidental paths (a real blur firing `onBlur`, the layout effect's `selectionchange`) corrected it. A `focus` sent while such an overlay was open was silently swallowed, and the throw on exhausted operation retries fired inside a `setTimeout` callback: uncatchable, unpinned, unreachable through the only caller. `focus` now makes one verified attempt: after `el.focus()`, it checks `root.activeElement === el`, and a failed take resets `editor.focused` and gives up with a `debug` diagnostic. There is deliberately no retry against a focus holder: no editor in the studied prior art (Wordgard, CodeMirror, ProseMirror, Lexical, slate-react) competes with an external focus owner, and the DOM cannot distinguish a closing overlay's restore from a user's deliberate focus move. Recovery is the consumer's explicit re-request, which now works because the flag is honest: the `if (editor.focused)` early-return also verifies reality, and an already-DOM-focused editor gets the flag corrected without rewriting the DOM selection. The retry loop that waits out the editor's own pending operations stays (renderer readiness, slate-react ancestry, budget 5); its exhaustion now gives up silently through the same diagnostic, its callbacks are wrapped so a selection invalidated between ticks cannot surface as an unhandled window error, and a pending retry is cancelled by a newer `focus` request and by `blur` (kept unpinned: the window needs pending operations, which the public test surface cannot hold open). Pinned by the `focus trap recovery` suite: a trap steals exactly once and the editor reports itself unfocused (red on a retry-loop implementation: `expected 19 to be 1`; red on the pre-fix code's unverified flag), a later send after the trap releases recovers, the `inert` case fires no events and recovers on re-request, and a `focus` send never rewrites an already-correct DOM selection. Focus hand-off from closing dialogs is the integration layer's job (the pattern Radix ships as `onCloseAutoFocus` and Floating UI as `returnFocus`; react-aria lacks the seam, tracked upstream in react-spectrum#9876) and lives with the dialog owner, not here.
christianhg
force-pushed
the
fix/verify-dom-focus
branch
from
September 1, 2026 09:02
ed19c8a to
893ee6e
Compare
christianhg
changed the title
focus honestly when something else holds the focus
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closing a toolbar dialog and handing focus back to the editor silently fails:
DOMEditor.focussetseditor.focused = truebeforeel.focus()and never verifies the focus took. A focus-containing overlay (a react-aria modal's containment, orinertoutside content, whereel.focus()is a complete no-op) wins silently, and the editor is left claiming focus it does not have; only incidental side effects (a real blur, aselectionchange) corrected the flag. In the playground, adding a comment through a dialog left focus parked on the toolbar button, while an identical annotation dialog appeared to work purely through timing luck (its document edit keeps the machine's busy loop alive past the modal's teardown).The fix makes
focusone honest, verified attempt: afterel.focus(), checkroot.activeElement === el; a failed take resets the flag and gives up with a debug diagnostic. Deliberately no retry against a focus holder: none of the studied editors (Wordgard, CodeMirror, ProseMirror, Lexical, slate-react) competes with an external focus owner, and the DOM cannot distinguish a closing overlay's restore from a user's deliberate focus move. Recovery is an explicit re-request, which now works because the flag is honest; the early-return guard verifies reality too, and an already-focused editor gets the flag corrected without rewriting the DOM selection. The pre-existing retry that waits out the editor's own pending operations stays (renderer readiness, slate-react ancestry); its exhaustion gives up silently instead of throwing inside asetTimeoutcallback where nothing could catch it, and a pending retry is cancelled by a newerfocusor ablur.The
focus trap recoverysuite pins the contract: a trap steals exactly once with the editor reporting itself unfocused (red both on the pre-fix unverified flag and on a retry-loop implementation,expected 19 to be 1), a later send after release recovers, theinertcase fires no events and recovers on re-request, andfocusnever rewrites an already-correct DOM selection. The blur-cancellation of a pending operations-retry is kept but unpinned; the window requires pending operations the public test surface cannot hold open. WebKit could not be launched in this environment; Chromium and Firefox are green.Dialog-close focus hand-off is the integration layer's job (Radix ships it as
onCloseAutoFocus, Floating UI asreturnFocus; react-aria lacks the seam, tracked in react-spectrum#9876) and lands with the dialog owner separately.