Repository navigation
fix(ui): convert overlay modals to native dialog elements - #46
Merged
Merged
Conversation
NameDialog and AddFuncdbModal dismissed via a click handler on a presentational div with no dialog semantics: nothing announced a dialog, nothing trapped focus, and Escape was hand-rolled on one of them (#32). Both now render a native <dialog> opened with showModal(), which supplies the implicit role, aria-modal, the focus trap and Escape-to-cancel; the hand-rolled keydown listener and the presentational role are deleted, not bypassed. Backdrop-click dismissal is kept deliberately: the dialog is stretched transparent over the viewport so a click outside the panel hits the dialog element itself. The dimmed overlay moves to ::backdrop and the UA default chrome is reset in app.css so the panels render as before. Focus is restored to the invoking control on close; the input is focused after showModal() so the browser records the invoker, not the input, as the restoration target. The test:e2e lane is provisioned for real: @playwright/test 1.63.0 joins devDependencies (it was declared in package.json scripts but in neither dependency list), and tests/e2e/dialogs.e2e.ts covers role, Escape, Cancel, focus containment, focus restoration and backdrop dismissal. Verified in headless Chromium on this branch: 6/6 e2e pass, typecheck clean, 604/604 unit tests pass, and the presentational-role grep over frontend/src returns zero matches.
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. Note Currently processing new changes in this PR. This may take a few minutes, please wait... ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Advanced Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (5)
✨ Finishing Touches📝 Generate docstrings
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
SonarCloud S1082 flagged the onClick the native dialog carried for backdrop dismissal -- the same whack-a-mole the issue describes, one rule's cure minting the other rule's finding. Criterion 6 permits dropping the behaviour deliberately with the reason recorded: these are form dialogs, so a stray click outside the panel must not discard typed input. Escape and Cancel remain as dismissals, both keyboard-reachable, and the e2e lane now pins the decision with a test asserting backdrop clicks do NOT dismiss, so it cannot be silently restored. The dialog no longer stretches as a click target; UA margin:auto centreing replaces the flex trick (#32).
|
❌ The last analysis has failed. |
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #32.
What changed
frontend/src/components/NameDialog.tsx,frontend/src/components/AnnotationPanelControls.tsx(AddFuncdbModal)<dialog>opened viashowModal(): implicitdialogrole,aria-modal, focus trap and Escape-to-cancel come from the platform.role="presentation"is gone from both; the hand-rolled document-levelkeydownEscape listener in AddFuncdbModal and the input-level Escape handler in NameDialog are deleted, not bypassed — the nativecancel/closeflow drivesonClose.dialog.close()so thecloseevent firesonCloseexactly once (Escape, backdrop click, or Cancel).showModal(), becauseshowModal()records the currently focused element as the restoration target; React'sautoFocuswould move focus first and the dialog would restore focus to an element inside itself, dropping it on unmount.frontend/src/styles/app.css.mm-modalresets the UA default margin/border/padding, stretches the dialog, centres the panel;::backdropcarries the dimmed overlay (replaces the hand-paintedrgba(0,0,0,.45)). The UAdialog:not([open]){display:none}outranks the class, so centreing applies only while open.frontend/package.json+frontend/bun.lock@playwright/test@1.63.0added to devDependencies —test:e2ewas declared in scripts but Playwright was in neither dependency list (grep -c -i playwright bun.lockwas 0; now 4). The lane is provisioned for real.frontend/tests/e2e/dialogs.e2e.ts— new specs: implicit dialog role in the accessibility tree, Escape closes, Cancel closes + focus restored to the invoker, Tab never escapes the open dialog, backdrop click dismisses while panel click does not.Acceptance criteria check
<dialog>viashowModal(); norole="presentation"grep -rn 'role="presentation"' frontend/src/→ 0 matchescancel/close; hand-rolled listeners deleted::backdropstyles the overlay; UA default chrome resetVerification summary
bun run typecheckclean (strict mode)bun test604 tests, 0 failbunx playwright test6/6 pass (5 dialog specs + app-shell smoke)