Skip to content

Fix focus handling when opening the morpheme editor #178

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
alex-rawlings-yyc merged 1 commit into from
Jul 28, 2026
Merged

Fix focus handling when opening the morpheme editor #178

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
54 changes: 41 additions & 13 deletions __mocks__/platform-bible-react.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,8 +11,10 @@ import {
isValidElement,
useContext,
useEffect,
useLayoutEffect,
useMemo,
useRef,
useState,
} from 'react';
import type {
ChangeEventHandler,
Expand Down Expand Up @@ -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.
*
Expand All @@ -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.
Expand All @@ -728,7 +737,7 @@ export function PopoverContent({
children,
className,
onEscapeKeyDown,
onInteractOutside,
onPointerDownOutside,
onOpenAutoFocus,
onCloseAutoFocus,
onClick,
Expand All @@ -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<HTMLDivElement>;
onMouseDown?: MouseEventHandler<HTMLDivElement>;
}>): 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<HTMLDivElement | null>(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<HTMLElement>('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 <div data-testid="popover-content" />;
return (
// eslint-disable-next-line jsx-a11y/no-static-element-interactions
<div
ref={contentRef}
className={className}
data-testid="popover-content"
onClick={onClick}
Expand All @@ -762,12 +790,12 @@ export function PopoverContent({
onMouseDown={onMouseDown}
>
{children}
{onInteractOutside && (
{onPointerDownOutside && (
<button
data-testid="popover-outside"
type="button"
onClick={(e) =>
onInteractOutside(
onPointerDownOutside(
new CustomEvent('dismissableLayer.pointerDownOutside', {
detail: { originalEvent: e.nativeEvent },
}),
Expand Down
55 changes: 55 additions & 0 deletions src/__tests__/components/MorphemeBox.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,7 @@ function renderBox(props: Partial<Parameters<typeof MorphemeBox>[0]> = {}) {
disabled={false}
morphemes={MORPHEMES}
onEditBreakdown={jest.fn()}
onGlossFocus={jest.fn()}
popoverOpen={false}
token={WORD_TOKEN}
{...props}
Expand Down Expand Up @@ -150,6 +151,7 @@ describe('MorphemeBox', () => {
disabled={false}
morphemes={MORPHEMES}
onEditBreakdown={jest.fn()}
onGlossFocus={jest.fn()}
popoverOpen={false}
token={WORD_TOKEN}
/>
Expand All @@ -159,13 +161,45 @@ describe('MorphemeBox', () => {
expect(onAncestorMouseDown).not.toHaveBeenCalled();
});

it('keeps a form-cell click from focusing the token gloss input the chip label points at', async () => {
// Regression test: the box renders inside TokenChip's <label htmlFor={glossInput}>, whose
// activation behavior forwards a click on a plain span to that input. Focus landing there is
// outside the editor the same click is opening, and the editor closes on outside interactions —
// so the panel would open and vanish on the same click, leaving focus in the chip.
render(
// Stand-in for TokenChip's label, not real UI.
// eslint-disable-next-line jsx-a11y/label-has-associated-control
<label htmlFor="token-gloss">
<MorphemeBox
analysisLanguage="en"
disabled={false}
morphemes={MORPHEMES}
onEditBreakdown={jest.fn()}
onGlossFocus={jest.fn()}
popoverOpen={false}
token={WORD_TOKEN}
/>
<input aria-label="Gloss for hello" id="token-gloss" />
</label>,
);
await userEvent.click(screen.getByText('-lo'));
expect(screen.getByRole('textbox', { name: 'Gloss for hello' })).not.toHaveFocus();
});

it('does not call onEditBreakdown when disabled', async () => {
const onEditBreakdown = jest.fn();
renderBox({ disabled: true, onEditBreakdown });
await userEvent.click(screen.getByText('hel'));
expect(onEditBreakdown).not.toHaveBeenCalled();
});

it('calls onGlossFocus when a morpheme gloss input receives focus', async () => {
const onGlossFocus = jest.fn();
renderBox({ onGlossFocus });
await userEvent.click(screen.getByRole('textbox', { name: 'Gloss for morpheme -lo' }));
expect(onGlossFocus).toHaveBeenCalledTimes(1);
});

it('disables the gloss inputs when disabled', () => {
renderBox({ disabled: true });
expect(screen.getByRole('textbox', { name: 'Gloss for morpheme hel' })).toBeDisabled();
Expand Down Expand Up @@ -200,6 +234,7 @@ describe('MorphemeGlossInput', () => {
column={1}
disabled={false}
morpheme={{ id: 'm-1', form: 'un-', writingSystem: 'und' }}
onFocus={jest.fn()}
tokenRef="tok-1"
/>,
);
Expand All @@ -213,6 +248,7 @@ describe('MorphemeGlossInput', () => {
column={1}
disabled={false}
morpheme={{ id: 'm-1', form: 'un-', writingSystem: 'und', gloss: { und: 'not' } }}
onFocus={jest.fn()}
tokenRef="tok-1"
/>,
);
Expand All @@ -229,6 +265,7 @@ describe('MorphemeGlossInput', () => {
column={1}
disabled={false}
morpheme={{ id: 'm-1', form: 'un-', writingSystem: 'und', gloss: { und: 'not' } }}
onFocus={jest.fn()}
tokenRef="tok-1"
/>,
);
Expand All @@ -247,6 +284,7 @@ describe('MorphemeGlossInput', () => {
column={1}
disabled={false}
morpheme={{ id: 'm-1', form: 'un-', writingSystem: 'und' }}
onFocus={jest.fn()}
tokenRef="tok-1"
/>,
);
Expand All @@ -255,6 +293,22 @@ describe('MorphemeGlossInput', () => {
expect(dispatchMock).toHaveBeenCalledWith('tok-1', 'm-1', 'not');
});

it('reports focus so the containing chip can focus its token', async () => {
const onFocus = jest.fn();
render(
<MorphemeGlossInput
analysisLanguage="und"
column={1}
disabled={false}
morpheme={{ id: 'm-1', form: 'un-', writingSystem: 'und' }}
onFocus={onFocus}
tokenRef="tok-1"
/>,
);
await userEvent.click(screen.getByRole('textbox', { name: 'Gloss for morpheme un-' }));
expect(onFocus).toHaveBeenCalledTimes(1);
});

it('does not dispatch when disabled', () => {
const dispatchMock = jest.fn();
jest.spyOn(AnalysisStore, 'useMorphemeGlossDispatch').mockReturnValue(dispatchMock);
Expand All @@ -265,6 +319,7 @@ describe('MorphemeGlossInput', () => {
column={1}
disabled
morpheme={{ id: 'm-1', form: 'un-', writingSystem: 'und' }}
onFocus={jest.fn()}
tokenRef="tok-1"
/>,
);
Expand Down
39 changes: 34 additions & 5 deletions src/__tests__/components/MorphemeEditor.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,12 +53,12 @@ describe('MorphemeBreakdownPopover', () => {
expect(input).toHaveValue('un- believe -able');
});

it('auto-focuses and selects the input on mount', () => {
it('auto-focuses and selects the input on open', () => {
renderPopover({ initialValue: 'word' });
const input = screen.getByRole('textbox');
expect(input).toHaveFocus();
// The mount effect calls .select(), so the whole value is selected and a fresh keystroke
// replaces it. Asserting the selection range catches a regression that drops the .select() call.
// The popover's open auto-focus selects the value too, so a fresh keystroke replaces it.
// Asserting the selection range catches a regression that suppresses that auto-focus.
expect(input).toHaveProperty('selectionStart', 0);
expect(input).toHaveProperty('selectionEnd', 'word'.length);
});
Expand Down Expand Up @@ -140,8 +140,8 @@ describe('MorphemeBreakdownPopover', () => {
const onSave = jest.fn();
const onClose = jest.fn();
renderPopover({ initialValue: 'te -st', onSave, onClose, surfaceText: 'test' });
// The platform-bible-react mock exposes a sentinel button that fires onInteractOutside,
// simulating a pointer interaction outside the popover.
// The platform-bible-react mock exposes a sentinel button that fires onPointerDownOutside,
// simulating a pointer press outside the popover.
await userEvent.click(screen.getByTestId('popover-outside'));
expect(onSave).not.toHaveBeenCalled();
expect(onClose).toHaveBeenCalledTimes(1);
Expand Down Expand Up @@ -346,6 +346,35 @@ describe('MorphemeBreakdownPopover', () => {
expect(screen.getByRole('textbox', { name: 'morpheme gloss' })).toHaveFocus();
});

it('leaves focus alone when the popover was dismissed by a press outside it', async () => {
// A press outside has already put focus where the user aimed it — typically another token they
// clicked. Pulling focus back to this chip's first morpheme gloss would yank it out of that
// token, so the close-focus redirect is skipped for this dismissal route.
render(
<>
<label>
<input aria-label="morpheme gloss" data-morpheme-gloss="true" />
<input aria-label="token gloss" id="gloss-1" />
<MorphemeBreakdownPopover
glossInputId="gloss-1"
initialValue="word"
onClose={jest.fn()}
onSave={jest.fn()}
surfaceText="word"
/>
</label>
<input aria-label="another token gloss" />
</>,
);
// fireEvent so the sentinels themselves never take focus: what matters is where the outside
// press left focus, which is stood in for by focusing the other token's gloss directly.
fireEvent.click(screen.getByTestId('popover-outside'));
const elsewhere = screen.getByRole('textbox', { name: 'another token gloss' });
elsewhere.focus();
fireEvent.click(screen.getByTestId('popover-close'));
expect(elsewhere).toHaveFocus();
});

describe('reset confirmation', () => {
/**
* Renders the popover on a glossed, solely-linked breakdown — the state in which a reset is
Expand Down
70 changes: 69 additions & 1 deletion src/__tests__/components/TokenChip.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,14 +69,21 @@ jest.mock('../../components/MorphemeBox', () => ({
*/
MorphemeBox({
onEditBreakdown,
onGlossFocus,
disabled,
popoverOpen,
}: Readonly<{ onEditBreakdown: () => void; disabled: boolean; popoverOpen: boolean }>) {
}: Readonly<{
onEditBreakdown: () => void;
onGlossFocus: () => void;
disabled: boolean;
popoverOpen: boolean;
}>) {
return (
<div data-morpheme-box-open={popoverOpen} data-testid="morpheme-box">
<button disabled={disabled} onClick={onEditBreakdown} type="button">
mock-edit-breakdown
</button>
<input aria-label="mock-morpheme-gloss" onFocus={onGlossFocus} />
</div>
);
},
Expand Down Expand Up @@ -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(
<AnalysisStoreProvider analysisLanguage="und">
<TokenChip {...requiredProps()} showMorphology onFocus={handleFocus} />
</AnalysisStoreProvider>,
);
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(
<AnalysisStoreProvider analysisLanguage="und">
<TokenChip {...requiredProps()} showMorphology disabled onFocus={handleFocus} />
</AnalysisStoreProvider>,
);
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([
Expand Down Expand Up @@ -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(
<AnalysisStoreProvider analysisLanguage="und">
<TokenChip {...requiredProps()} showMorphology onFocus={handleFocus} />
</AnalysisStoreProvider>,
);
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(
<AnalysisStoreProvider analysisLanguage="und">
<TokenChip {...requiredProps()} showMorphology onFocus={handleFocus} />
</AnalysisStoreProvider>,
);
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
Expand Down
Loading
Loading