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..ebe473313 --- /dev/null +++ b/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(); + 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); + }); + + 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 (
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; }