From 0c376dd0e45c8d19c82e5e93a3202244b38241cc Mon Sep 17 00:00:00 2001 From: dealerweb Date: Fri, 2 Oct 2026 12:42:40 +0200 Subject: [PATCH 1/2] fix: keep the focus in a dialog's field while the page behind renders again useFocusTrap started over whenever its onEscape changed, and callers pass a new function on every render. Each start moved the focus back to the opener and then to the dialog's first element, so a field lost the focus whenever the page behind rendered again: every 30 seconds through the connection check, or when new mail updated the folder counts. The trap now reads the latest onEscape from a ref and starts once per opening. A view change in a dialog (a list giving way to a form) can remove the element that had the focus. The restart used to pull it back by accident; the trap now does that on purpose, and only when the focused element was removed. --- .../__tests__/filter-settings-focus.test.tsx | 84 +++++++++++++++++ hooks/__tests__/use-focus-trap.test.tsx | 90 +++++++++++++++++++ hooks/use-focus-trap.ts | 35 +++++++- 3 files changed, 206 insertions(+), 3 deletions(-) create mode 100644 components/settings/__tests__/filter-settings-focus.test.tsx create mode 100644 hooks/__tests__/use-focus-trap.test.tsx diff --git a/components/settings/__tests__/filter-settings-focus.test.tsx b/components/settings/__tests__/filter-settings-focus.test.tsx new file mode 100644 index 000000000..a5372fc9e --- /dev/null +++ b/components/settings/__tests__/filter-settings-focus.test.tsx @@ -0,0 +1,84 @@ +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 +// and a mailbox refresh. + +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(); + 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(); + fireEvent.click(screen.getByText('raw_editor')); + const script = screen.getByLabelText('script_content'); + script.focus(); + act(() => useAuthStore.setState({ connectionLost: false })); + expect(document.activeElement).toBe(script); + }); +}); diff --git a/hooks/__tests__/use-focus-trap.test.tsx b/hooks/__tests__/use-focus-trap.test.tsx new file mode 100644 index 000000000..4492caf62 --- /dev/null +++ b/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 ( +
+ + + {note &&

{note}

} +
+ ); +} + +function Page({ open, onEscape, note }: { open: boolean; onEscape: () => void; note?: string }) { + return ( + <> + + {open && } + + ); +} + +// 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 ( +
+ + {editing + ? + : } +
+ ); +} + +describe('useFocusTrap', () => { + it('moves the focus in when it opens and back to the opener when it closes', () => { + const { rerender } = render( {}} />); + screen.getByText('opener').focus(); + rerender( {}} />); + expect(document.activeElement).toBe(screen.getByText('close')); + rerender( {}} />); + 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( {}} />); + const field = screen.getByLabelText('name'); + field.focus(); + rerender( {}} />); + rerender( {}} />); + 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(); + rerender(); + 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(); + 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( {}} />); + const field = screen.getByLabelText('name'); + field.focus(); + field.blur(); + rerender( {}} note="saved" />); + await screen.findByText('saved'); + await Promise.resolve(); + expect(document.activeElement).toBe(document.body); + }); +}); diff --git a/hooks/use-focus-trap.ts b/hooks/use-focus-trap.ts index 0bec6891c..60dfe32a9 100644 --- a/hooks/use-focus-trap.ts +++ b/hooks/use-focus-trap.ts @@ -13,6 +13,14 @@ export function useFocusTrap({ }: UseFocusTrapOptions) { const containerRef = useRef(null); const previousActiveElement = useRef(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; } From 0f7e6d0c16ee750ae4cd085d63575f62861d3107 Mon Sep 17 00:00:00 2001 From: dealerweb Date: Fri, 2 Oct 2026 12:42:40 +0200 Subject: [PATCH 2/2] fix: keep an open filter dialog through a reload in the background A change to the Sieve script from another device or tab reloads the filters, and the page showed its loading or error state instead of its content. That took an open rule dialog or the Sieve editor with it, together with whatever was typed there. While a dialog is open, the page keeps what it has. --- .../__tests__/filter-settings-focus.test.tsx | 34 +++++++++++++++++-- components/settings/filter-settings.tsx | 9 +++-- 2 files changed, 39 insertions(+), 4 deletions(-) diff --git a/components/settings/__tests__/filter-settings-focus.test.tsx b/components/settings/__tests__/filter-settings-focus.test.tsx index a5372fc9e..ebe473313 100644 --- a/components/settings/__tests__/filter-settings-focus.test.tsx +++ b/components/settings/__tests__/filter-settings-focus.test.tsx @@ -8,8 +8,9 @@ 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 -// and a mailbox refresh. +// 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() } })); @@ -81,4 +82,33 @@ describe('typing in a dialog on the filter page while things happen in the backg 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(); + 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(); + act(() => useFilterStore.setState({ isLoading: true })); + expect(screen.getByText('loading')).toBeInTheDocument(); + }); }); diff --git a/components/settings/filter-settings.tsx b/components/settings/filter-settings.tsx index 855515c61..5826234d6 100644 --- a/components/settings/filter-settings.tsx +++ b/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 (
@@ -443,7 +448,7 @@ export function FilterSettings() { ); } - if (error) { + if (error && !editing) { return (