Merge pull request #1162 from dealerweb/fix/focus-trap-rerender

fix: keep the focus in dialogs while the page behind renders again

components/settings/__tests__/filter-settings-focus.test.tsx

@@ -0,0 +1,114 @@
+import { act, fireEvent, render, screen } from '@testing-library/react';
+import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest';
+import React from 'react';
+import type { SieveCapabilities } from '@/lib/jmap/sieve-types';
+import { useAuthStore } from '@/stores/auth-store';
+import { useEmailStore } from '@/stores/email-store';
+import { useFilterStore } from '@/stores/filter-store';
+import { FilterSettings } from '../filter-settings';
+
+// The real settings page and its dialogs, against what happens in the
+// background while the user types: the connection check every 30 seconds,
+// a mailbox refresh, and a reload of the filters (another device changed
+// the script).
+
+vi.mock('@/stores/toast-store', () => ({ toast: { error: vi.fn(), success: vi.fn(), info: vi.fn() } }));
+
+const CAPABILITIES: SieveCapabilities = {
+  implementation: 'Stalwart',
+  maxSizeScript: 102400,
+  sieveExtensions: ['fileinto', 'mailbox', 'imap4flags', 'copy'],
+  notificationMethods: [],
+  externalLists: [],
+  maxNumberRedirects: 1,
+};
+
+const initial = {
+  auth: useAuthStore.getState(),
+  email: useEmailStore.getState(),
+  filter: useFilterStore.getState(),
+};
+
+beforeEach(() => {
+  useAuthStore.setState({ client: { updateSieveScript: vi.fn(async () => {}), getSieveAccountId: () => 'primary' } as never });
+  useEmailStore.setState({ fetchMailboxes: vi.fn(async () => {}) as never });
+  useFilterStore.setState({
+    rules: [],
+    isSupported: true,
+    isLoading: false,
+    error: null,
+    isOpaque: false,
+    activeScriptId: 'script-1',
+    selectedAccountId: 'primary',
+    sieveCapabilities: CAPABILITIES,
+    selectAccount: vi.fn(async () => {}),
+  });
+});
+
+afterEach(() => {
+  useAuthStore.setState(initial.auth, true);
+  useEmailStore.setState(initial.email, true);
+  useFilterStore.setState(initial.filter, true);
+});
+
+function typeNewRule() {
+  render(<FilterSettings />);
+  fireEvent.click(screen.getByText('add_rule'));
+  const name = screen.getByPlaceholderText('rule_name_placeholder');
+  name.focus();
+  fireEvent.change(name, { target: { value: 'News' } });
+  return name;
+}
+
+describe('typing in a dialog on the filter page while things happen in the background', () => {
+  it('keeps the focus in the rule name through the connection check', () => {
+    const name = typeNewRule();
+    // What the ping every 30 seconds does: it sets the connection state again.
+    act(() => useAuthStore.setState({ connectionLost: false }));
+    expect(document.activeElement).toBe(name);
+  });
+
+  it('keeps the focus in the rule name through a mailbox refresh', () => {
+    const name = typeNewRule();
+    act(() => useEmailStore.setState({ mailboxes: [] }));
+    expect(document.activeElement).toBe(name);
+  });
+
+  it('keeps the focus in the Sieve editor through the connection check', () => {
+    render(<FilterSettings />);
+    fireEvent.click(screen.getByText('raw_editor'));
+    const script = screen.getByLabelText('script_content');
+    script.focus();
+    act(() => useAuthStore.setState({ connectionLost: false }));
+    expect(document.activeElement).toBe(script);
+  });
+
+  it('keeps the rule dialog and what was typed through a reload of the filters', () => {
+    typeNewRule();
+    act(() => useFilterStore.setState({ isLoading: true }));
+    expect(screen.getByRole('dialog')).toBeInTheDocument();
+    act(() => useFilterStore.setState({ isLoading: false }));
+    expect(screen.getByPlaceholderText('rule_name_placeholder')).toHaveValue('News');
+  });
+
+  it('keeps the Sieve editor and what was typed through a reload of the filters', () => {
+    render(<FilterSettings />);
+    fireEvent.click(screen.getByText('raw_editor'));
+    fireEvent.change(screen.getByLabelText('script_content'), { target: { value: 'keep;' } });
+    act(() => useFilterStore.setState({ isLoading: true }));
+    act(() => useFilterStore.setState({ isLoading: false }));
+    expect(screen.getByLabelText('script_content')).toHaveValue('keep;');
+  });
+
+  it('keeps the rule dialog through a reload that fails', () => {
+    typeNewRule();
+    act(() => useFilterStore.setState({ error: 'offline' }));
+    expect(screen.getByPlaceholderText('rule_name_placeholder')).toHaveValue('News');
+  });
+
+  it('still shows the loading state when no dialog is open', () => {
+    render(<FilterSettings />);
+    act(() => useFilterStore.setState({ isLoading: true }));
+    expect(screen.getByText('loading')).toBeInTheDocument();
+  });
+});

components/settings/filter-settings.tsx

@@ -432,7 +432,12 @@ export function FilterSettings() {
     );
   }
 
-  if (isLoading) {
+  // A reload in the background (another device or tab changed the script)
+  // must not take away a dialog the user is typing in: while one is open,
+  // the page keeps what it has.
+  const editing = showRuleModal || showSieveEditor;
+
+  if (isLoading && !editing) {
     return (
       <SettingsSection title={t("title")} description={t("description")}>
         <div className="flex items-center gap-2 py-4 text-sm text-muted-foreground">
@@ -443,7 +448,7 @@ export function FilterSettings() {
     );
   }
 
-  if (error) {
+  if (error && !editing) {
     return (
       <SettingsSection title={t("title")} description={t("description")}>
         <div className="text-sm text-red-600 dark:text-red-400 py-4">

hooks/__tests__/use-focus-trap.test.tsx

@@ -0,0 +1,90 @@
+import { fireEvent, render, screen, waitFor } from '@testing-library/react';
+import { describe, expect, it, vi } from 'vitest';
+import React, { useState } from 'react';
+import { useFocusTrap } from '../use-focus-trap';
+
+function Dialog({ onEscape, note }: { onEscape: () => void; note?: string }) {
+  const ref = useFocusTrap({ isActive: true, onEscape });
+  return (
+    <div ref={ref} role="dialog">
+      <button type="button">close</button>
+      <input aria-label="name" />
+      {note && <p>{note}</p>}
+    </div>
+  );
+}
+
+function Page({ open, onEscape, note }: { open: boolean; onEscape: () => void; note?: string }) {
+  return (
+    <>
+      <button type="button">opener</button>
+      {open && <Dialog onEscape={onEscape} note={note} />}
+    </>
+  );
+}
+
+// Like the identity and template managers: a list that gives way to a form,
+// taking the button that had the focus with it.
+function Manager() {
+  const [editing, setEditing] = useState(false);
+  const ref = useFocusTrap({ isActive: true, onEscape: () => setEditing(false) });
+  return (
+    <div ref={ref} role="dialog">
+      <button type="button">close</button>
+      {editing
+        ? <input aria-label="field" />
+        : <button type="button" onClick={() => setEditing(true)}>edit</button>}
+    </div>
+  );
+}
+
+describe('useFocusTrap', () => {
+  it('moves the focus in when it opens and back to the opener when it closes', () => {
+    const { rerender } = render(<Page open={false} onEscape={() => {}} />);
+    screen.getByText('opener').focus();
+    rerender(<Page open onEscape={() => {}} />);
+    expect(document.activeElement).toBe(screen.getByText('close'));
+    rerender(<Page open={false} onEscape={() => {}} />);
+    expect(document.activeElement).toBe(screen.getByText('opener'));
+  });
+
+  it('leaves the focus in the field while the page behind renders again', () => {
+    // A background request renders the page again, and the dialog gets a
+    // new onEscape: the field the user is typing in keeps the focus.
+    const { rerender } = render(<Page open onEscape={() => {}} />);
+    const field = screen.getByLabelText('name');
+    field.focus();
+    rerender(<Page open onEscape={() => {}} />);
+    rerender(<Page open onEscape={() => {}} />);
+    expect(document.activeElement).toBe(field);
+  });
+
+  it('answers Escape with the onEscape of the latest render', () => {
+    const first = vi.fn();
+    const latest = vi.fn();
+    const { rerender } = render(<Page open onEscape={first} />);
+    rerender(<Page open onEscape={latest} />);
+    fireEvent.keyDown(screen.getByRole('dialog'), { key: 'Escape' });
+    expect(latest).toHaveBeenCalledTimes(1);
+    expect(first).not.toHaveBeenCalled();
+  });
+
+  it('keeps the focus in the dialog when a view change removes the element that had it', async () => {
+    render(<Manager />);
+    const edit = screen.getByText('edit');
+    edit.focus();
+    fireEvent.click(edit);
+    await waitFor(() => expect(document.activeElement).toBe(screen.getByText('close')));
+  });
+
+  it('leaves a focus the user moved away alone when the dialog changes', async () => {
+    const { rerender } = render(<Page open onEscape={() => {}} />);
+    const field = screen.getByLabelText('name');
+    field.focus();
+    field.blur();
+    rerender(<Page open onEscape={() => {}} note="saved" />);
+    await screen.findByText('saved');
+    await Promise.resolve();
+    expect(document.activeElement).toBe(document.body);
+  });
+});

hooks/use-focus-trap.ts

@@ -13,6 +13,14 @@ export function useFocusTrap({
 }: UseFocusTrapOptions) {
   const containerRef = useRef<HTMLDivElement>(null);
   const previousActiveElement = useRef<HTMLElement | null>(null);
+  // Callers pass a new onEscape on every render. The trap reads the latest
+  // one from here instead of starting over for it: each new start moved the
+  // focus back to the opener and then to the first element, out of the field
+  // the user was typing in whenever the page behind rendered again.
+  const onEscapeRef = useRef(onEscape);
+  useEffect(() => {
+    onEscapeRef.current = onEscape;
+  });
 
   useEffect(() => {
     if (!isActive || !containerRef.current) return;
@@ -29,6 +37,13 @@ export function useFocusTrap({
       );
     };
 
+    // The element in the dialog that had the focus last.
+    let lastFocused: HTMLElement | null = null;
+    const handleFocusIn = (e: FocusEvent) => {
+      lastFocused = e.target as HTMLElement;
+    };
+    container.addEventListener('focusin', handleFocusIn);
+
     // Focus first element
     const focusableElements = getFocusableElements();
     const firstElement = focusableElements[0];
@@ -39,8 +54,8 @@ export function useFocusTrap({
     // Handle Tab key to trap focus
     const handleKeyDown = (e: KeyboardEvent) => {
       // Handle Escape
-      if (e.key === 'Escape' && onEscape) {
-        onEscape();
+      if (e.key === 'Escape' && onEscapeRef.current) {
+        onEscapeRef.current();
         return;
       }
 
@@ -68,8 +83,22 @@ export function useFocusTrap({
 
     container.addEventListener('keydown', handleKeyDown);
 
+    // A view change in the dialog (a list giving way to a form) can remove
+    // the element that had the focus, which then drops to the page behind,
+    // out of the trap. Bring it back to the first element - only then, so a
+    // focus the user moved on purpose stays where it is.
+    const observer = new MutationObserver(() => {
+      const active = document.activeElement;
+      if (lastFocused && !lastFocused.isConnected && (!active || active === document.body)) {
+        getFocusableElements()[0]?.focus();
+      }
+    });
+    observer.observe(container, { childList: true, subtree: true });
+
     // Cleanup
     return () => {
+      observer.disconnect();
+      container.removeEventListener('focusin', handleFocusIn);
       container.removeEventListener('keydown', handleKeyDown);
 
       // Restore focus to previous element
@@ -77,7 +106,7 @@ export function useFocusTrap({
         previousActiveElement.current.focus();
       }
     };
-  }, [isActive, onEscape, restoreFocus]);
+  }, [isActive, restoreFocus]);
 
   return containerRef;
 }