Skip to content

Commit 9daba00

Browse files
lidge-junlidge-jun
committed
fix(gui): retain restore focus through history refresh
1 parent 58c616d commit 9daba00

2 files changed

Lines changed: 58 additions & 28 deletions

File tree

‎gui/src/pages/integrations/RestoreDialog.tsx‎

Lines changed: 23 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -33,6 +33,7 @@ export default function RestoreDialog({
3333
const dialogRef = useRef<HTMLDialogElement>(null);
3434
const restoreFocusRef = useRef<HTMLElement | null>(null);
3535
const restoreFallbackRef = useRef<HTMLElement | null>(null);
36+
const restoredRef = useRef(false);
3637
const [drift, setDrift] = useState(false);
3738
const [pending, setPending] = useState(false);
3839
const [failure, setFailure] = useState<string | null>(null);
@@ -44,29 +45,38 @@ export default function RestoreDialog({
4445
// focus-restore uses the same tagName check.
4546
const active = document.activeElement;
4647
restoreFocusRef.current = active?.tagName === "BUTTON" ? active as HTMLElement : null;
47-
// The trigger may not survive the restore. A row whose snapshot is consumed
48-
// re-renders as an `expired` badge with no button (RollbackHistory.tsx:44-46),
49-
// so the element captured above is detached by the time the cleanup runs and
50-
// focus lands on <body> — a keyboard user dropped at the top of the document
51-
// with nothing announced (#3059). Remember the enclosing region too, so there
52-
// is somewhere to return to that cannot disappear.
48+
// The trigger may not survive the asynchronous history refresh. A row whose
49+
// snapshot is consumed re-renders as an `expired` badge with no button
50+
// (RollbackHistory.tsx:44-46), so remember the enclosing region too: it
51+
// remains a focus target after the trigger disappears (#3059).
5352
restoreFallbackRef.current =
5453
(active?.closest?.("section, [role='region'], main") as HTMLElement | null) ?? null;
5554
if (dialog && !dialog.open) dialog.showModal();
5655
return () => {
5756
if (dialog?.open) dialog.close();
58-
// Prefer the trigger; fall back to its region when the restore removed it.
59-
// `isConnected` is the check that matters: a detached node accepts .focus()
60-
// silently and focus stays on <body>, which is the reported symptom.
57+
const fallback = restoreFallbackRef.current;
58+
// A successful restore starts the history refresh before it closes the
59+
// dialog. The trigger is therefore still connected during this cleanup,
60+
// but can disappear when the asynchronous refresh consumes its snapshot.
61+
// Put successful restores on the stable region now; cancellation keeps
62+
// the usual trigger restoration below.
63+
if (restoredRef.current && fallback?.isConnected) {
64+
// A region is not focusable by default; -1 makes it programmatically
65+
// focusable without adding it to the Tab order.
66+
if (!fallback.hasAttribute("tabindex")) fallback.setAttribute("tabindex", "-1");
67+
fallback.focus?.();
68+
return;
69+
}
70+
// Prefer the trigger when the user cancelled; fall back to its region
71+
// only if it was already removed. `isConnected` is the check that
72+
// matters: a detached node accepts .focus() silently and focus stays on
73+
// <body>, which is the reported symptom.
6174
const trigger = restoreFocusRef.current;
6275
if (trigger?.isConnected) {
6376
trigger.focus?.();
6477
return;
6578
}
66-
const fallback = restoreFallbackRef.current;
6779
if (!fallback?.isConnected) return;
68-
// A region is not focusable by default; -1 makes it programmatically
69-
// focusable without adding it to the Tab order.
7080
if (!fallback.hasAttribute("tabindex")) fallback.setAttribute("tabindex", "-1");
7181
fallback.focus?.();
7282
};
@@ -83,6 +93,7 @@ export default function RestoreDialog({
8393
setFailure(null);
8494
try {
8595
await restoreIntegration(apiBase, row.opId, drift);
96+
restoredRef.current = true;
8697
onRestored();
8798
onClose();
8899
} catch (error) {

‎gui/tests/integrations-surfaces.test.tsx‎

Lines changed: 35 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -674,25 +674,20 @@ test("a drifted restore asks a second time instead of failing", async () => {
674674
});
675675

676676
/**
677-
* #3059: focus had nowhere to go when the restore consumed the row that owned the
678-
* trigger. A consumed snapshot re-renders as an `expired` badge with no button, so
679-
* the remembered element is detached by the time the dialog closes — and calling
680-
* .focus() on a detached node succeeds silently while focus stays on <body>. A
681-
* keyboard user ends up at the top of the document with nothing announced.
682-
*
683-
* The reporter blamed a page unmount, which the tree does not do: refresh() keeps
684-
* stale data (client-resource.ts:339-341), so `if (!status)` is cold-load only. The
685-
* mechanism is wrong and the experience is real, which is why this is a fix rather
686-
* than a close.
677+
* #3059: a successful restore starts an asynchronous history refresh before closing
678+
* the dialog. That means normal focus restoration first finds the trigger still in
679+
* the tree, and only later does the refresh consume its snapshot and remove the
680+
* trigger. The region must receive focus on the successful close, before that later
681+
* removal can send focus to <body>.
687682
*/
688-
test("focus returns to a stable region when the restore removed its trigger", async () => {
683+
test("a successful restore keeps focus on the stable region after refresh removes its trigger", async () => {
689684
const [{ createRoot }, { LanguageProvider }, { default: RestoreDialog }] = await Promise.all([
690685
import("react-dom/client"),
691686
import("../src/i18n/provider"),
692687
import("../src/pages/integrations/RestoreDialog"),
693688
]);
694689

695-
// The shape RollbackHistory renders: a region holding the row's trigger.
690+
// The shape RollbackHistory renders: a stable region holding the row trigger.
696691
const region = testWindow.document.createElement("section");
697692
const trigger = testWindow.document.createElement("button");
698693
region.appendChild(trigger);
@@ -709,19 +704,43 @@ test("focus returns to a stable region when the restore removed its trigger", as
709704
snapshot: "stored" as const,
710705
undoable: false,
711706
};
707+
let resolveRestore: ((response: Response) => void) | undefined;
708+
const restoreFetch = ((input: RequestInfo | URL) => {
709+
if (String(input).includes("/restore")) {
710+
return new Promise<Response>(resolve => { resolveRestore = resolve; });
711+
}
712+
return Promise.resolve(json({ operations: [] }));
713+
}) as typeof fetch;
714+
Object.defineProperty(globalThis, "fetch", { configurable: true, value: restoreFetch });
715+
Object.defineProperty(testWindow, "fetch", { configurable: true, value: restoreFetch });
716+
712717
await act(async () => {
713718
root = createRoot(container);
714719
root.render(
715720
<LanguageProvider>
716-
<RestoreDialog apiBase={apiBase} row={row} onClose={() => {}} onRestored={() => {}} />
721+
<RestoreDialog
722+
apiBase={apiBase}
723+
row={row}
724+
onRestored={() => {}}
725+
onClose={() => {
726+
root!.unmount();
727+
root = null;
728+
}}
729+
/>
717730
</LanguageProvider>,
718731
);
719732
});
720733

721-
// The restore consumes the snapshot, so the row re-renders without its button.
722-
trigger.remove();
723-
await act(async () => { root!.unmount(); root = null; });
734+
await act(async () => { buttonByText("Restore")!.click(); });
735+
expect(resolveRestore).toBeDefined();
724736

737+
// Restore succeeds and closes while the trigger is still connected.
738+
await act(async () => { resolveRestore!(json({ ok: true })); });
739+
expect(trigger.isConnected).toBe(true);
740+
expect(testWindow.document.activeElement).toBe(region);
741+
742+
// The asynchronous history refresh then consumes the snapshot and its trigger.
743+
trigger.remove();
725744
expect(testWindow.document.activeElement).toBe(region);
726745
expect(testWindow.document.activeElement).not.toBe(testWindow.document.body);
727746
region.remove();

0 commit comments

Comments
 (0)