Refactor/a11y modals and icon buttons#34
Conversation
Prep for upcoming a11y pass: icon-only buttons currently use hardcoded English title strings (Expand/Collapse/Undo/Redo). Localizing them gives us a proper aria-label source in every locale.
…tReport Each modal container now exposes role=dialog, aria-modal and aria-labelledby pointing at its title. Close buttons get aria-label via t.app.close so screen readers announce the icon-only control. Hardcoded English titles (Import ZPL, Import Report) are kept as-is for now; they're a separate i18n concern.
Collapse/Expand toggle (ZPLOutput), Undo/Redo (AppShell) and FontManager delete button now expose accessible names. The hardcoded English titles on Undo/Redo and Collapse/Expand are also moved to t.app keys.
Previously hardcoded as ⌘Z / ⌘⇧Z which only matches Mac. The new kbd() helper renders ⌘Z on Mac and Ctrl+Z elsewhere, mirroring what useGlobalShortcuts already accepts (metaKey OR ctrlKey).
All five modals now trap Tab cycling, close on Escape, focus the first focusable on mount and restore focus to the previously focused element on unmount. Latest onEscape is captured via ref so callers can pass an inline arrow without re-mounting the trap on every render.
ConfirmDialog already had role=alertdialog and aria-modal but lacked focus trapping and focus restoration. Replace the inline Escape handler with the shared useFocusTrap hook; keep the body scroll lock as a separate effect since it's an unrelated concern. autoFocus on the safer button (cancel for destructive ops) is preserved because the trap only moves focus when nothing inside the dialog is already focused.
…ication The five output modals plus ConfirmDialog had grown ~6 lines each of identical setup (containerRef + useFocusTrap + role/aria-modal/ aria-labelledby + click-out close). DialogShell wraps that, plus a new useBodyScrollLock hook so all modals lock background scroll consistently (only ConfirmDialog had it before). Backdrop close uses onMouseDown with target===currentTarget across the board, preventing accidental close when dragging text out of an input.
…pile time The Props were both optional, so a future caller could mount a dialog with no accessible name and only fail at runtime. Replace the two optional fields with a discriminated union — at least one must be present — turning a runtime a11y bug into a TypeScript error.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a standardized DialogShell component to handle modal dialogs, incorporating focus trapping and body scroll locking. It also adds a platform-aware keyboard shortcut utility and updates various components with localized strings and accessibility labels. Review feedback identifies a discrepancy in the backdrop click implementation, potential issues with stacked modals in the focus trap hook, and several hardcoded strings that require localization.
| <div | ||
| ref={containerRef} | ||
| className="fixed inset-0 z-50 flex items-center justify-center bg-black/50" | ||
| onMouseDown={(e) => { if (e.target === e.currentTarget) onClose(); }} |
There was a problem hiding this comment.
The backdrop close logic uses onMouseDown, which triggers immediately on press. However, the documentation on lines 32-34 states that it should only fire when both press and release happen on the backdrop. Using onClick would align the implementation with the documentation and provide a more standard user experience. Additionally, the container needs tabIndex={-1} to support the focus fallback logic in useFocusTrap (when no focusable elements are present).
| <div | |
| ref={containerRef} | |
| className="fixed inset-0 z-50 flex items-center justify-center bg-black/50" | |
| onMouseDown={(e) => { if (e.target === e.currentTarget) onClose(); }} | |
| <div | |
| ref={containerRef} | |
| tabIndex={-1} | |
| className="fixed inset-0 z-50 flex items-center justify-center bg-black/50 focus:outline-none" | |
| onClick={(e) => { if (e.target === e.currentTarget) onClose(); }} | |
| } | ||
| }; | ||
|
|
||
| document.addEventListener('keydown', handleKey); |
There was a problem hiding this comment.
Adding a keydown listener to the document means that if multiple modals are open (e.g., a confirmation dialog on top of an import modal), pressing Escape will trigger the onEscape callback for all of them, closing the entire stack. Consider implementing a mechanism to ensure only the top-most modal handles the event, or check if the event has already been handled by another listener (e.g., by checking e.defaultPrevented).
| </button> | ||
| </div> | ||
| <div className="flex items-center justify-between px-4 py-3 border-b border-border shrink-0"> | ||
| <span id="zpl-import-title" className="font-mono text-xs text-muted uppercase tracking-widest">Import ZPL</span> |
There was a problem hiding this comment.
The title "Import ZPL" is hardcoded. Since useT() is available in this component, it should use the localized string t.app.importZpl for consistency with the rest of the application.
| <span id="zpl-import-title" className="font-mono text-xs text-muted uppercase tracking-widest">Import ZPL</span> | |
| <span id="zpl-import-title" className="font-mono text-xs text-muted uppercase tracking-widest">{t.app.importZpl}</span> |
The onMouseDown handler fired on press alone, while the docstring
claimed close happens only when press and release land on the
backdrop. Switch to onClick which natively enforces that semantic
(the click event only fires when both events hit the same element),
and add tabIndex={-1} + focus:outline-none so the container-as-focus
fallback in useFocusTrap actually receives focus without a stray
focus ring.
The previous document-level listener meant stacked modals (e.g. a confirm dialog on top of an import dialog) would all close on a single Escape press. Listen on the container instead — keydown bubbles up from the focused element, which is always inside the trap, so only the active modal handles the key.
Title/Close/Cancel in ZplImportModal and Close in ImportReportModal now go through t.app.* keys that already exist. The remaining English strings in those two modals (paragraph copy, Import button, Choose file, Copy report, error messages) are tracked as separate i18n debt — out of scope for this branch.
1 participant
No description provided.