From 9d1bf8cb46064bfae18a615b987597a1c6966ec4 Mon Sep 17 00:00:00 2001 From: Alex Rawlings Date: Tue, 28 Jul 2026 10:16:28 -0600 Subject: [PATCH] Fix focus handling when opening the morpheme editor Opening the editor now focuses its token and its own input: the morpheme cells no longer let the chip label steal focus, and the panel relies on the popover's open auto-focus, which a mount effect could never do since the portal has no children on that commit. --- __mocks__/platform-bible-react.tsx | 54 ++++++++++---- src/__tests__/components/MorphemeBox.test.tsx | 55 +++++++++++++++ .../components/MorphemeEditor.test.tsx | 39 +++++++++-- src/__tests__/components/TokenChip.test.tsx | 70 ++++++++++++++++++- src/components/MorphemeBox.tsx | 23 +++++- src/components/MorphemeEditor.tsx | 51 ++++++++------ src/components/PhraseBox.tsx | 3 +- src/components/TokenChip.tsx | 23 ++++-- 8 files changed, 271 insertions(+), 47 deletions(-) diff --git a/__mocks__/platform-bible-react.tsx b/__mocks__/platform-bible-react.tsx index a14d7e7c..612fd4d1 100644 --- a/__mocks__/platform-bible-react.tsx +++ b/__mocks__/platform-bible-react.tsx @@ -11,8 +11,10 @@ import { isValidElement, useContext, useEffect, + useLayoutEffect, useMemo, useRef, + useState, } from 'react'; import type { ChangeEventHandler, @@ -702,10 +704,16 @@ export function PopoverAnchor({ * component implements positioning, portaling, and dismissal internally; this stub exposes the * dismissal callbacks so tests can simulate them: * - * - `onOpenAutoFocus` is invoked once on mount (mirroring Radix's open auto-focus event). + * - The panel's children render only from the second commit, mirroring Radix's portal (which renders + * nothing until its own layout effect flips its `mounted` state). Consumers must therefore not + * reach for a child element from their own mount effect — in the real app that element does not + * exist yet — and this stub is deliberately faithful about that rather than making such code + * appear to work. + * - Once the children exist, `onOpenAutoFocus` is invoked; unless it is prevented, the panel's first + * enabled tabbable child is focused (and selected, if a text input), as Radix's focus scope does. * - An Escape keydown anywhere inside the content invokes `onEscapeKeyDown`. - * - A sentinel `data-testid="popover-outside"` button invokes `onInteractOutside` on click, - * simulating a pointer interaction outside the popover. + * - A sentinel `data-testid="popover-outside"` button invokes `onPointerDownOutside` on click, + * simulating a pointer press outside the popover. * - A sentinel `data-testid="popover-close"` button invokes `onCloseAutoFocus` on click, * simulating Radix's focus-restoration event fired as the popover closes. * @@ -714,10 +722,11 @@ export function PopoverAnchor({ * @param props.className - CSS class names forwarded to the div. * @param props.onEscapeKeyDown - Called with the native `KeyboardEvent` when Escape is pressed * inside the content, matching Radix's signature. - * @param props.onInteractOutside - Called with a `CustomEvent` carrying the original pointer event - * in `detail.originalEvent` when the sentinel outside button is clicked, matching the shape of - * Radix's `PointerDownOutsideEvent`. - * @param props.onOpenAutoFocus - Called once on mount with a plain `Event`. + * @param props.onPointerDownOutside - Called with a `CustomEvent` carrying the original pointer + * event in `detail.originalEvent` when the sentinel outside button is clicked, matching the shape + * of Radix's `PointerDownOutsideEvent`. + * @param props.onOpenAutoFocus - Called once, on the commit that first renders the children, with a + * cancelable `Event`; preventing it suppresses the stub's own focusing, as in Radix. * @param props.onCloseAutoFocus - Called with a plain `Event` when the sentinel close button is * clicked, mirroring Radix's close-time focus-restoration event. * @param props.onClick - Click handler forwarded to the div. @@ -728,7 +737,7 @@ export function PopoverContent({ children, className, onEscapeKeyDown, - onInteractOutside, + onPointerDownOutside, onOpenAutoFocus, onCloseAutoFocus, onClick, @@ -739,20 +748,39 @@ export function PopoverContent({ align?: 'start' | 'center' | 'end'; sideOffset?: number; onEscapeKeyDown?: (event: KeyboardEvent) => void; - onInteractOutside?: (event: CustomEvent) => void; + onPointerDownOutside?: (event: CustomEvent) => void; onOpenAutoFocus?: (event: Event) => void; onCloseAutoFocus?: (event: Event) => void; onClick?: MouseEventHandler; onMouseDown?: MouseEventHandler; }>): ReactElement { + // Radix's portal renders no children until its own layout effect has run, so the panel's content + // is absent for the first commit. Mirrored here so a consumer that reaches for a child from its + // mount effect fails the same way it does in the app instead of silently passing. + const [portalMounted, setPortalMounted] = useState(false); + useLayoutEffect(() => setPortalMounted(true), []); + // eslint-disable-next-line no-null/no-null + const contentRef = useRef(null); // Capture the mount-time callback so the simulation fires exactly once, like the real event. const openAutoFocusRef = useRef(onOpenAutoFocus); useEffect(() => { - openAutoFocusRef.current?.(new Event('openAutoFocus')); - }, []); + if (!portalMounted) return; + const event = new Event('openAutoFocus', { cancelable: true }); + openAutoFocusRef.current?.(event); + if (event.defaultPrevented) return; + const candidates = contentRef.current?.querySelectorAll('input, button') ?? []; + // The sentinels below are test scaffolding, not panel content, so they are never focus targets. + const first = Array.from(candidates).find( + (el) => !el.hasAttribute('disabled') && !el.dataset.testid?.startsWith('popover-'), + ); + first?.focus(); + if (first instanceof HTMLInputElement) first.select(); + }, [portalMounted]); + if (!portalMounted) return
; return ( // eslint-disable-next-line jsx-a11y/no-static-element-interactions
{children} - {onInteractOutside && ( + {onPointerDownOutside && ( +
); }, @@ -439,6 +446,35 @@ describe('TokenChip', () => { expect(screen.queryByTestId('morpheme-popover')).not.toBeInTheDocument(); }); + it('reports the token as focused when the define button opens the popover', async () => { + // The trigger is a button, so neither it nor the label's mouse-down handler moves focus into + // the chip; without an explicit report the editor would open over a token the view still + // treats as unfocused. + const handleFocus = jest.fn(); + render( + + + , + ); + await userEvent.click( + screen.getByRole('button', { name: 'Define morpheme breakdown for hello' }), + ); + expect(handleFocus).toHaveBeenCalledTimes(1); + }); + + it('does not report focus from the define button when disabled', async () => { + const handleFocus = jest.fn(); + render( + + + , + ); + await userEvent.click( + screen.getByRole('button', { name: 'Define morpheme breakdown for hello' }), + ); + expect(handleFocus).not.toHaveBeenCalled(); + }); + it('renders the morpheme box instead of the define button when morphemes exist', () => { // AnalysisStore imported at top level jest.spyOn(AnalysisStore, 'useMorphemes').mockReturnValue([ @@ -485,6 +521,38 @@ describe('TokenChip', () => { expect(screen.getByTestId('morpheme-popover')).toBeInTheDocument(); }); + it('reports the token as focused when the box opens the editor on an analyzed token', async () => { + // AnalysisStore imported at top level + jest + .spyOn(AnalysisStore, 'useMorphemes') + .mockReturnValue([{ id: 'm-1', form: 'hel', writingSystem: 'und' }]); + const handleFocus = jest.fn(); + render( + + + , + ); + await userEvent.click(screen.getByRole('button', { name: 'mock-edit-breakdown' })); + expect(handleFocus).toHaveBeenCalledTimes(1); + }); + + it('reports the token as focused when a morpheme gloss input is focused', async () => { + // The morpheme glosses are gloss fields of this same token, so focusing one must move the + // view's focus exactly as focusing the chip's own gloss input does. + // AnalysisStore imported at top level + jest + .spyOn(AnalysisStore, 'useMorphemes') + .mockReturnValue([{ id: 'm-1', form: 'hel', writingSystem: 'und' }]); + const handleFocus = jest.fn(); + render( + + + , + ); + await userEvent.click(screen.getByRole('textbox', { name: 'mock-morpheme-gloss' })); + expect(handleFocus).toHaveBeenCalledTimes(1); + }); + it('dispatches morpheme breakdown when saving from the popover', async () => { const mockDispatch = jest.fn(); // AnalysisStore imported at top level diff --git a/src/components/MorphemeBox.tsx b/src/components/MorphemeBox.tsx index 3c22b4af..21191a83 100644 --- a/src/components/MorphemeBox.tsx +++ b/src/components/MorphemeBox.tsx @@ -47,6 +47,9 @@ const MORPHEME_BOX_STRING_KEYS = [ * look. * @param props.onEditBreakdown - Called when a form cell is clicked (while enabled) to open the * whole-breakdown editor. + * @param props.onGlossFocus - Called when any morpheme gloss input receives focus, so the chip can + * report the token as focused; these fields are gloss fields of the same token as the chip's own + * gloss input, so focusing one must move the view's focus just as focusing that input does. * @returns A boxed grid of morpheme forms and their gloss fields, wrapped in a popover anchor. */ export function MorphemeBox({ @@ -56,6 +59,7 @@ export function MorphemeBox({ disabled, popoverOpen, onEditBreakdown, + onGlossFocus, }: Readonly<{ token: Token & { type: 'word' }; morphemes: readonly MorphemeAnalysis[]; @@ -63,6 +67,7 @@ export function MorphemeBox({ disabled: boolean; popoverOpen: boolean; onEditBreakdown: () => void; + onGlossFocus: () => void; }>) { const [localizedStrings] = useLocalizedStrings(MORPHEME_BOX_STRING_KEYS); // Hovering anywhere in the box tints the whole forms row: clicking any cell opens the same @@ -121,9 +126,15 @@ export function MorphemeBox({ aria-hidden="true" className={formClassName} onClick={handleClick} - // Not a button, so stop mousedown from bubbling to TokenChip's label handler (which - // would otherwise focus the gloss input). - onMouseDown={(e) => e.stopPropagation()} + // Not a button, so this cell is subject to focus moves the first cell is exempt from, + // in two ways that both have to be shut off or the editor loses focus the moment it + // opens: the browser forwards mouse-down on a label to the labeled control (canceled + // with preventDefault), and TokenChip's own label handler does the same deliberately + // (kept away by not letting the event reach it). + onMouseDown={(e) => { + e.preventDefault(); + e.stopPropagation(); + }} style={formStyle} > {m.form} @@ -138,6 +149,7 @@ export function MorphemeBox({ column={i + 1} disabled={disabled} morpheme={m} + onFocus={onGlossFocus} tokenRef={token.ref} /> ))} @@ -159,6 +171,8 @@ export function MorphemeBox({ * @param props.analysisLanguage - BCP 47 tag for reading/writing the gloss. * @param props.disabled - When true, the input is read-only. * @param props.column - 1-based grid column the input occupies (shared with the morpheme's form). + * @param props.onFocus - Called when the input receives focus, so the containing chip can report + * its token as focused. * @returns A cell-filling text input for the morpheme gloss, placed in the gloss row. */ export function MorphemeGlossInput({ @@ -167,12 +181,14 @@ export function MorphemeGlossInput({ analysisLanguage, disabled, column, + onFocus, }: Readonly<{ morpheme: MorphemeAnalysis; tokenRef: string; analysisLanguage: string; disabled: boolean; column: number; + onFocus: () => void; }>) { const committed = morpheme.gloss?.[analysisLanguage] ?? ''; const dispatchMorphemeGloss = useMorphemeGlossDispatch(); @@ -202,6 +218,7 @@ export function MorphemeGlossInput({ style={{ gridColumn: column, gridRow: 2, fieldSizing: 'content', minWidth: '2ch' }} value={draft} onChange={(e) => setDraft(e.target.value)} + onFocus={onFocus} onBlur={() => { if (!disabled && draft !== committed) dispatchMorphemeGloss(tokenRef, morpheme.id, draft); }} diff --git a/src/components/MorphemeEditor.tsx b/src/components/MorphemeEditor.tsx index 2103a2eb..7e3bc995 100644 --- a/src/components/MorphemeEditor.tsx +++ b/src/components/MorphemeEditor.tsx @@ -6,7 +6,7 @@ */ import { useLocalizedStrings } from '@papi/frontend/react'; import { Button, Input, Label, PopoverContent } from 'platform-bible-react'; -import { useEffect, useId, useRef, useState } from 'react'; +import { useId, useRef, useState } from 'react'; import type { KeyboardEvent, MouseEvent } from 'react'; const POPOVER_STRING_KEYS = [ @@ -96,15 +96,15 @@ export function MorphemeBreakdownPopover({ const inputId = useId(); const [draft, setDraft] = useState(initialValue); const [confirmingReset, setConfirmingReset] = useState(false); - // eslint-disable-next-line no-null/no-null - const inputRef = useRef(null); + // Whether the panel is closing because the user pressed the pointer outside it, which + // {@link handleCloseAutoFocus} needs to know so it can leave focus where that press put it. + const dismissedByOutsidePointerRef = useRef(false); - // Focus and select the input on open. The popover's own auto-focus is suppressed (see - // onOpenAutoFocus below) so this effect, which runs after it, is the only focus on open. - useEffect(() => { - inputRef.current?.focus(); - inputRef.current?.select(); - }, []); + // The popover's own open auto-focus is left in place: it focuses and selects the panel's first + // tabbable element (this input — the label above it is not tabbable) with `preventScroll`, which + // is exactly what this panel wants on open. Focusing the input from a mount effect here instead + // would silently do nothing: the panel is portaled, and Radix's portal renders no children until + // its own layout effect has run, so on the commit this component mounts there is no input yet. /** * Collapses leading/trailing and repeated internal whitespace to a single space. @@ -179,15 +179,22 @@ export function MorphemeBreakdownPopover({ }; /** - * Commits the draft when the user interacts outside the popover, except when the text was not - * edited — then the interaction acts like Cancel, because an accidental outside click is not a - * deliberate commit. An empty draft is likewise dismissed without writing: {@link handleSave} - * refuses to interpret it and would otherwise leave the panel open, but an outside click on a - * modal popover must always dismiss. While the reset confirmation is showing, an outside click - * dismisses it without resetting: the confirmation exists precisely because the loss is - * irreversible, so it must not be answered by a stray click. + * Commits the draft when the user presses the pointer outside the popover, except when the text + * was not edited — then the interaction acts like Cancel, because an accidental outside click is + * not a deliberate commit. An empty draft is likewise dismissed without writing: + * {@link handleSave} refuses to interpret it and would otherwise leave the panel open, but an + * outside click on a modal popover must always dismiss. While the reset confirmation is showing, + * an outside click dismisses it without resetting: the confirmation exists precisely because the + * loss is irreversible, so it must not be answered by a stray click. + * + * Wired to `onPointerDownOutside` rather than the broader `onInteractOutside`, which also fires + * when focus merely moves outside the panel. A modal popover is not supposed to dismiss on that + * (Radix's modal content cancels its own focus-outside dismissal), but `onInteractOutside` runs + * regardless of that cancellation — so listening to it would let any stray focus move commit the + * user's draft and close the panel. */ - const handleInteractOutside = () => { + const handlePointerDownOutside = () => { + dismissedByOutsidePointerRef.current = true; if (confirmingReset || isUnedited || isEmpty) { onClose(); return; @@ -216,10 +223,16 @@ export function MorphemeBreakdownPopover({ * query could match another token's field. Falls back to the token gloss input when no morpheme * field exists (dismissed with no breakdown, or deleted). * + * A dismissal by pointer press outside the panel is exempt: that press has already put focus + * where the user aimed it (typically another token), so pulling focus back to this chip would + * yank it out of whatever they just clicked. The default is still prevented, so Radix does not + * restore focus to the pre-open element either — the click's own focus stands. + * * @param e - The Radix close auto-focus event. */ const handleCloseAutoFocus = (e: Event) => { e.preventDefault(); + if (dismissedByOutsidePointerRef.current) return; const glossInput = document.getElementById(glossInputId); const firstMorphemeGloss = glossInput ?.closest('label') @@ -235,9 +248,8 @@ export function MorphemeBreakdownPopover({ onClick={stopMouseEvents} onCloseAutoFocus={handleCloseAutoFocus} onEscapeKeyDown={onClose} - onInteractOutside={handleInteractOutside} + onPointerDownOutside={handlePointerDownOutside} onMouseDown={stopMouseEvents} - onOpenAutoFocus={(e) => e.preventDefault()} > {confirmingReset ? ( <> @@ -274,7 +286,6 @@ export function MorphemeBreakdownPopover({ {localizedStrings['%interlinearizer_morphemeEditor_splitLabel%']} { + onFocus(); + setPopoverOpen(true); + }; + /** * Commits the morpheme breakdown from the popover input, splitting on whitespace. * @@ -391,7 +405,7 @@ export function TokenChip({ // current forms on every open. // // `onOpenChange` is intentionally omitted: this consumer owns every dismissal path - // (onEscapeKeyDown, onInteractOutside, explicit button clicks). Don't wire onOpenChange + // (onEscapeKeyDown, onPointerDownOutside, explicit button clicks). Don't wire onOpenChange // without also removing those, or closes would double-fire. {hasMorphemes ? ( @@ -399,7 +413,8 @@ export function TokenChip({ analysisLanguage={analysisLanguage} disabled={disabled} morphemes={morphemes} - onEditBreakdown={() => setPopoverOpen(true)} + onEditBreakdown={openMorphemeEditor} + onGlossFocus={onFocus} popoverOpen={popoverOpen} token={token} /> @@ -415,7 +430,7 @@ export function TokenChip({ variant="ghost" onClick={(e) => { e.preventDefault(); - if (!disabled) setPopoverOpen(true); + if (!disabled) openMorphemeEditor(); }} > {token.surfaceText}