Skip to content

Two overlay modals have no dialog semantics: role="presentation" stands in for a native <dialog> #32

Description

@hyperpolymath

Two overlay modals dismiss on backdrop click via a click handler on a plain
<div>. To satisfy typescript:S1082 (click handler on a non-interactive
element) that div was given role="presentation", and SonarCloud then flagged
the role itself under typescript:S6819, which asks for the native element
instead. One rule's cure minted the other rule's finding, which is the tell that
neither attribute is the real answer.

Sites (both identical in shape — a position: fixed; inset: 0 backdrop
whose onClick closes when event.target === event.currentTarget):

  • frontend/src/components/AnnotationPanelControls.tsx:100-108
  • frontend/src/components/NameDialog.tsx:37-45

The larger gap. Measured across frontend/src/:

grep -rn '<dialog'                    -> 0 matches
grep -rn 'aria-modal|role="dialog"'   -> 0 matches

So these are not merely mis-roled — they carry no dialog semantics at all.
A screen reader is given an unlabelled presentational box; nothing announces a
dialog, nothing constrains focus to it, and background content stays reachable
behind the overlay. Both components hand-roll an Escape-key listener
(AnnotationPanelControls.tsx:95-97) that a native <dialog> provides for free.

Proposed fix: convert both to a native <dialog> opened with showModal().
That supplies the implicit dialog role, aria-modal, the focus trap, the
Escape handling and a real ::backdrop, and removes role="presentation", the
backdrop click handler and the manual keydown effect together.

Why this is an issue rather than a commit on #29. The conversion changes
focus behaviour, Escape handling and the overlay's painted chrome (<dialog>
carries default margin/border/padding, and ::backdrop replaces the current
rgba(0,0,0,.45) background). None of that is observable from tsc or the bun
unit suite, and this repo has no working browser lane to check it in —
frontend/package.json declares "test:e2e": "bunx playwright test" but
grep -c -i playwright frontend/bun.lock is 0, so Playwright is in neither
dependencies nor devDependencies. Landing an unverifiable focus-management
change is a worse outcome than leaving a scanner finding open, so per the
standing rule a new scanner finding becomes an issue with acceptance criteria
rather than a merge blocker.

Acceptance criteria

  1. AnnotationPanelControls and NameDialog each render a native <dialog>
    opened via showModal(); role="presentation" is gone from both.
  2. grep -rn 'role="presentation"' frontend/src/ returns 0 matches.
  3. Escape closes each modal with the hand-rolled keydown listener deleted,
    not merely bypassed — the native cancel/close event drives onClose.
  4. Focus moves into the dialog on open and is restored to the invoking control
    on close; Tab does not reach content behind the overlay.
  5. The backdrop is styled through ::backdrop, and the dialog's default
    margin/border/padding are reset so the panel renders as it does today.
  6. Backdrop-click-to-dismiss either still works, or is dropped deliberately with
    the reason recorded in the PR — it must not break silently.
  7. SonarCloud reports 0 typescript:S6819 and 0 typescript:S1082
    findings in these two files.
  8. Verified in a real browser, since none of 3–6 is observable from tsc or
    bun test. If that means provisioning the test:e2e lane first, say so in
    the PR rather than asserting behaviour that was not run.

Raised while clearing the SonarCloud gate on #29; the other five of that branch's
seven findings are fixed there (plus one further finding its own cure minted, also
fixed). Related: #30, #31.

🤖 Generated with Claude Code

https://claude.ai/code/session_01X3hgXxWm6umMgZkjYyHnnm

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions