FIX Restore keyboard focus around Converter Registry dialogs - #2717
Dmitry Voropaev (v0ropaev) wants to merge 7 commits into
Conversation
Both registry dialogs open from component state rather than from a DialogTrigger, so nothing in the tree was a Tabster restore target and every dismissal left document.activeElement on <body>. Mark the three opening controls the way FeedbackDialog does, which covers Cancel, Escape and backdrop dismissals, and restore focus explicitly on the two paths where the control that opened the dialog unmounts with the list: a removed row, and the empty-state button once the registry holds a converter. A restore that is still queued when the next dialog opens is dropped, so a slow refresh cannot pull focus out of that dialog. On a failed creation the primary action is disabled while the request is in flight. A browser runs the unfocusing steps for it a tick later, which drops focus to <body> and out of the open dialog, and Escape then stops dismissing it because Tabster handles that key on the dialog surface. Keep the action focusable while disabled, which preserves the double-submit guard through aria-disabled, and move focus onto the error once it arrives, exposed as an alert so it is announced.
A create request can outlive the dialog it was submitted from, and onCreated ran unconditionally when it landed. It cleared the single boolean that said a dialog was open and restored focus to New Converter even when the user had since opened a converter's Remove dialog: Tabster then marked that still-visible dialog aria-hidden, and Escape stopped dismissing it. Mint a token for each dialog opening and keep the token of the dialog on screen, so a response is matched against the opening it belongs to rather than against "some dialog is open". A response that outlived its own dialog still refreshes the list, but only its own opening may clear the dialog state and restore focus. That covers a normal success, where the dialog is still open and the trigger may have unmounted with the list, which is what the explicit restore exists for; a success after the dialog was dismissed with nothing else open; a success while the Remove dialog is open; and a success after the create dialog was reopened. Removal needs no token of its own, because Cancel is disabled and the Escape handler ignored while its request is in flight. jsdom has no exit animation, so the mock Playwright project covers the aria-hidden and Escape consequences in a real browser.
The focus call for a failed creation ran from requestAnimationFrame, which can run before React has rendered the setError update. errorRef.current was still null in that case and the call silently did nothing, so the message appeared while the keyboard stayed on Add Converter. Focus it from an effect instead, which runs after the commit that adds the message bar. Each message now records whether it came from a submission, so a metadata-loading failure, which arrives without the user asking for anything, cannot take focus. A submission that outlived its own opening of the dialog drops its failure rather than showing it, and taking focus, in a later one. A synchronous frame pins the render ordering in jsdom, where the real requestAnimationFrame lands late enough to hide it. The mock Playwright project covers the reopened dialog in a real browser.
The success path reset the form unconditionally, so a create response that outlived its opening wiped the opening the user had moved on to: the typed registry name, the selected type and its parameter rows all disappeared, and reset() also cleared a submission error they were reading. Gate the reset on the opening the request was submitted from, the same way the failure path already gates setError, and leave onCreated ungated so a late success still refreshes the list. The submitting flag needed the same treatment, and clearing it when the dialog opens rather than when a stale response lands, so a request from a previous opening no longer re-enables the primary action of an opening that has its own request in flight.
|
Thanks — both are real, and reproducing your first scenario in Chromium showed exactly the chain you described: focus on New Converter, the still-visible "Remove converter?" surface carrying 1. Late create response ( Cases now covered, with the test that pins each:
The removal path deliberately keeps no token, and there is now a comment saying why: Cancel is Measured with 2. Error focus timing ( Measured with that file reverted: 2 failed / 9 passed, including a test that pins the exact failure mode you named (a frame callback stubbed to run synchronously, so the pre-fix code focuses nothing). 3. One more thing your first comment led me to, fixed in Checks on One unrelated thing I ran into while testing, not touched here: reopening the Add Converter dialog leaves it unfocused — first open focuses Cancel, but after Escape and a second click |
…y-focus # Conflicts: # frontend/src/components/Registry/CreateConverterDialog.test.tsx # frontend/src/components/Registry/CreateConverterDialog.tsx
|
Rebased on
Checks on the merge: |
The restore ran in a requestAnimationFrame callback, which React does not wait for. Creating the first converter closes the dialog and starts the refresh in the same handler, so the callback could land before React committed the new list: the empty-state button was still connected, took focus, and lost it to <body> when that same commit unmounted it. The removal path had the identical defect, and the comment claiming its pre-refresh restore kept focus off <body> was wrong for the same reason. Requesting the restore as state and performing it in an effect removes the race rather than narrowing it: an effect cannot run before the commit that scheduled it, so the tree it inspects is the one the user is about to see, and the existing isConnected fallback then picks New Converter. Nothing clears the state from inside the effect, which react-hooks/set-state-in-effect would reject. Both paths get a regression test that runs frame callbacks where they are registered, the lever this PR already uses for the submission-error fix.
|
Reproduced your sequence and fixed it in Why the frame was wrong, not just early. The removal path had the same defect, so it gets the same treatment and its own regression. The comment at the old Regression tests. Both run frame callbacks where they are registered, which is the lever this PR already uses at One thing I want to flag rather than let you find it. I wrote a third test for the Verification (Node 24.8,
I also considered passing an explicit |
This branch has not been deployed
2 participants
Description
Fixes #2701. Frontend only.
Two keyboard-accessibility defects in the Converter Registry, and the fix for each turned out to need something the other does not:
DialogTrigger, so Fluent has nothing to restore to and focus lands on<body>. The three triggers (New Converter, Create First Converter, per-row Remove) now carryuseRestoreFocusTarget(), the convention this repo already uses for state-controlled dialogs (FeedbackDialog.tsx:180-182). That covers dismissal. It does not cover the two paths where the trigger unmounts — a successful removal and the first creation — so those restore explicitly with therequestAnimationFrame(() => ref.current?.focus())idiom already used atScenarioRunPage.tsx:169.disabled+disabledFocusable, so it stays focusable while inert, and the errorMessageBargetsrole="alert"and takes focus when an error appears.What I measured, because the diff does not show it
My claim comment on the issue said
useRestoreFocusTarget, and an earlier draft of this analysis — done against jsdom — concluded the hook only helps keyboard dismissal. A real browser disagrees, so for the record, in Chromium via--project=mock:<body>;Hence both. In jsdom the hook contributes nothing measurable (all 13
ConverterRegistrytests pass with the three spreads deleted), which is exactly how the first analysis went wrong.On the disabled button: Chromium runs the unfocusing steps asynchronously. On
main, with the POST held open,document.activeElementis the disabled button at t=0 but<body>at t=100ms and t=1000ms, and Escape after that does nothing — so "Escape stops working" lasts the whole request, and moving focus only once the error arrives would repair the end state, not the window.disabledFocusablekeeps the double-submit guard intact (useARIAButtonPropsdrops the native attribute, setsaria-disabledand replaces the handlers withundefined) and the styling is unchanged. One semantic consequence worth your call: the button is nowaria-disabledrather thandisabledin the "no converter types" case too, so it stays in the tab order.Tests and Documentation
10 new jest tests, 4 new Playwright tests (
frontend/e2e/registry.spec.ts, mock project — no backend).npx jest src/components/Registry— 3 suites, 24 tests pass. 9 of the 10 new jest tests are red on unmodifiedmain(Received element with focus: <body>).mainand red only against my own first attempt, where the deferred restore could steal focus into the background while a dialog was open. It is a guard against a defect this design could introduce, not a regression test for BUG Converter Registry dialogs lose keyboard focus after dismissal and duplicate errors #2701 — flagging it rather than counting it.npx playwright test --project=mock e2e/registry.spec.ts— 6/6 pass, all 4 new tests red on unmodifiedmainin Chromium (two ontoBeFocused, one on focus being outside the dialog mid-request, one on the missingrole="alert"). jsdom does not blur a button that becomes disabled, which is why the duplicate-name half needed a browser.npx jest— 2221 of 2222 pass. The one failure (LabelsBar > should dismiss the picker when the user clicks away) reproduces on this machine with my change stashed, andmain's Frontend Tests run is green, so it is environmental here rather than pre-existing upstream.npx jest --coverage— 93.30% statements / 86.39% branches / 91.44% functions / 94.59% lines against the configured 85/85/90/90; no new uncovered branch.npm run lint(0 warnings),npm run type-check,npx tsc --noEmit -p tsconfig.test.json,pre-commit run --files …— all clean.The existing mock of
CreateConverterDialoginConverterRegistry.test.tsxwas upgraded from a bare<div role="dialog">to a real FluentDialog(still a mock, still no API calls), because a plain div has no Tabster restore source. Existing assertions on it are unchanged.No documentation or JupyText changes: frontend-only, no API surface.
Notes for review
CreateConverterDialog.tsxmerges clean — I reconstructed your diff ontomainand rangit merge-file --diff3; your nearest hunks end at old lines 315 and 455, mine sit at 316 and ~462.CreateConverterDialog.test.tsxconflicts, because both PRs append tests at the same@@ -245,4 @@anchor; resolution is "keep both". Also: your client-side validation sets the error beforesetSubmitting(true), so post-merge it renders asrole="alert"without stealing focus, which is right — that path never disables the button.ScenarioDetail.tsx, solved there with a manual ref helper and no Fluent hook. On the evidence above, the manual restore is only necessary where the trigger unmounts, and that Launch button does not, so the hook alone would cover it. Two offers: once both land, factor the rAF restore into a small shared helper (ScenarioRunPage.tsx:169is a third copy of those three lines), and unify on guarded vs unguardedrequestAnimationFrameeither way — I followed the unguarded in-repo idiom.ConverterRegistry.tsxhalf stands alone if you would rather keep FEAT Add focused enum and word-selection converter inputs #2708's file untouched; the two halves fix different defects.AnnounceProvider, soMessageBar's built-inuseAnnounceis a no-op and this error was not announced at all before.role="alert"fixes it for this one dialog; app-wide announcements look like their own issue, happy to file it.