fix(react): inbox a11y and robustness polish#72
Conversation
Deferred items from the hostile-frontend review: - move focus into the popover dialog on open (APG dialog pattern) - arrow-key/Home/End navigation for the more-actions menu - cap popover width at min(360px, calc(100vw - 16px)) - ensureStyles re-probes the DOM so head-replacing nav re-injects styles Refs dodopayments#61
Greptile SummaryThis PR polishes the React inbox accessibility and styling behavior. The main changes are:
Confidence Score: 5/5This looks safe to merge.
Important Files Changed
Reviews (1): Last reviewed commit: "fix(react): inbox a11y and robustness po..." | Re-trigger Greptile |
|
I independently implemented #61 against the same review items and hit two keyboard edge cases in testing that look like they apply to this branch too, so sharing in case they save you a round trip.
The open effect moves focus in: useEffect(() => {
if (!isOpen) {
return;
}
popoverRef.current?.focus();
}, [isOpen]);but nothing contains it afterwards, and the close triggers are outside pointerdown and Escape only. A portaled popover is the last child of document.body, so once you Tab past its last control (or Shift+Tab from the freshly focused dialog container), focus lands back in the host page with the popover still open and no obvious way back except re-finding the bell. Two coherent fixes here: trap Tab at the dialog edges and declare aria-modal="true" (trapping without declaring modality leaves screen readers treating the page behind as interactive), or keep it non-modal and close when focus leaves. I went with the trap. Closing on focusout interacts badly with archiving: the archive button that has focus gets removed, focus drops to body, and the popover shuts mid-action. If you go the trap route, one edge that cost me a test cycle: a focusable selector like
// Roving focus across the more-actions menu items. Down and Up wrap, Home
// and End jump. Escape and Tab are left to their default handlers.Tab is neither Escape nor an outside pointerdown, so it moves focus on (the menu items are natural tab stops, so it first walks through them) while the menu stays rendered over the list with aria-expanded still true. There is a second-order effect: the menu's capture-phase Escape listener stays armed, so pressing Escape later from, say, the preferences button closes the stale menu and pulls focus back to the more-actions trigger instead of closing the popover. The APG menu pattern closes the menu when Tab moves focus out. My version closes it, parks focus on the trigger, and leaves the keydown uncancelled so the browser continues Tab from the menu's place in the sequence. Happy to share the regression tests for both if useful (Tab wrap at the dialog edges, the roving-tab false edge, and Tab-out closing the menu). They are plain testing-library keyDown tests, no new dependencies. |
3 participants
Closes #61.
Addresses 4 of the 6 deferred hostile-frontend review items; the remaining two are called out as follow-ups below.
Done
role=dialog) now takes focus on open (APG dialog pattern) viatabIndex={-1}+.focus(), so keyboard users reach adocument.body-portaled popover directly instead of tabbing through the host page first. Escape still returns focus to the bell.role=menu/menuitemmarkup now has real behavior: focus the first item on open, ArrowDown/ArrowUp (wrapping), Home/End. Escape and outside-click close it unchanged, restoring trigger focus.width: min(360px, calc(100vw - 16px)), so it no longer overflows sub-376px viewports that floating-uishift()cannot compress.ensureStyles()re-probesdocument.querySelector('style[data-chimely]')on each call instead of trusting a module-level flag, so head-replacing navigation (Turbo/PJAX) cannot leave the inbox unstyled until a full reload. Injection stays idempotent.Deferred (follow-ups, as the issue frames them)
ChimelyProviderfor pages with many inboxes (item 5) -- pure docs.aria-liveannouncements for the new-arrival pill/badge (item 6) -- the issue itself calls this "a further step"; it carries UX judgment (polite vs assertive, throttling) better settled with maintainer input.If you'd rather keep #61 open to track those two, happy to drop the
Closeskeyword or open a follow-up issue -- your call.Verification
biome,pnpm --filter "./packages/*" build,@chimely/reacttypecheck, and vitest are all green -- 108 react tests pass, including 3 new ones: focus-into-dialog on open, menu roving focus (arrows/Home/End/wrap), and style re-injection after a simulated head swap. Only@chimely/reactis touched; a patch changeset is included.