fix(ui-ux): make analysis-run digest verification keyboard/touch accessible - #557
fix(ui-ux): make analysis-run digest verification keyboard/touch accessible#557seonghobae wants to merge 1 commit into
Conversation
…ssible The reproducibility digest (Code/Config/Result SHA) in the analysis-run detail popup exposed its full hash only via the `title` attribute on a bare `<span>`, which has no keyboard or touch equivalent -- a WCAG 2.1.1 failure with no way to copy/compare the full digest without a mouse. Swap the `<span title=...>` for the same native `<details>/<summary>` disclosure pattern already used elsewhere in App.tsx for "compact summary, full detail on demand". `<details>/<summary>` is natively focusable and toggled by Enter/Space/click/tap with zero JS. The wrapping `<p>` becomes a `<div>` since `<details>` is not valid phrasing content inside `<p>`; minor CSS keeps the disclosures reading inline in the '·'-separated post-meta line (the other `<details>` usages in this file are block-level list items). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011jWJzKUd82yy97esEBfJbt
|
Warning Review limit reachedNext included review available in 4 minutes. View limit detailsLimit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
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 |
| <span key={part.label}> | ||
| {index > 0 ? " · " : null} | ||
| <span title={part.digest}>{`${part.label} ${analysisRunDigestPrefix(part.digest)}`}</span> | ||
| <details className="analysis-run-digest"> | ||
| <summary>{`${part.label} ${analysisRunDigestPrefix(part.digest)}`}</summary> | ||
| {part.digest} | ||
| </details> | ||
| </span> |
There was a problem hiding this comment.
🟡 Disclosure nested inside inline span
Each <details> disclosure sits directly inside a <span>, but a span accepts only phrasing content while <details> is flow content, so the markup is invalid. This is the same validity problem that prompted changing the wrapping element from <p> to <div>, left unfixed for the inner span.
| <span key={part.label}> | |
| {index > 0 ? " · " : null} | |
| <span title={part.digest}>{`${part.label} ${analysisRunDigestPrefix(part.digest)}`}</span> | |
| <details className="analysis-run-digest"> | |
| <summary>{`${part.label} ${analysisRunDigestPrefix(part.digest)}`}</summary> | |
| {part.digest} | |
| </details> | |
| </span> | |
| <Fragment key={part.label}> | |
| {index > 0 ? " · " : null} | |
| <details className="analysis-run-digest"> | |
| <summary>{`${part.label} ${analysisRunDigestPrefix(part.digest)}`}</summary> | |
| {part.digest} | |
| </details> | |
| </Fragment> |
Was this helpful? React with 👍 or 👎 to provide feedback.
| <details className="analysis-run-digest"> | ||
| <summary>{`${part.label} ${analysisRunDigestPrefix(part.digest)}`}</summary> | ||
| {part.digest} | ||
| </details> |
There was a problem hiding this comment.
📝 Info: Full digest now always present in DOM text
The full digest was previously only in the title attribute; it is now text inside the closed <details>, so it is always in textContent. The component is only mounted in the detail popup (AnalysisRunReproducibilityDigests at frontend/src/App.tsx:2913), so the home-list negative assertion still holds. Updated tests click to reveal it.
Was this helpful? React with 👍 or 👎 to provide feedback.
Pull request was closed
Gap
Dimension: touch_interaction
Severity: medium
The Analysis-run reproducibility digest (Code/Config/Result SHA) shown in the
analysis-run detail popup exposed its full-precision hash only through the
titleattribute of a bare<span>:That
<span>carries notabIndex,role, oronClick, so the full digestwas reachable only via mouse hover -- a WCAG 2.1.1 (keyboard) failure with no
touch equivalent.
App.test.tsxpreviously asserted the full digest wasretrievable only via
screen.getByTitle(...), confirming there was noother path to it.
Severity is medium, not high: the truncated prefix stays visible and
screen-reader-audible to everyone, so no one is blocked from the core
analysis-run-detail workflow -- only from copying/comparing the full digest
without a mouse.
Fix
Swapped the bare
<span title=...>for the same native<details>/<summary>disclosure pattern this codebase already uses elsewhere for "compact summary,
full detail on demand" (e.g. the
Evidence operations/Related postsdisclosures in the same file):
<details>/<summary>is natively focusable and toggled by Enter/Space/click/tap with zero JS, so this closes the keyboard and touch gap without a
new button+state pattern.
Two small adaptations:
<p className="post-meta">to<div className="post-meta">, since<details>is flow content, notphrasing content, and is not valid inside
<p>..analysis-run-digest) keeps the disclosures reading inlinewithin the
·-separated post-meta line -- the other<details>usages inthis file are block-level list items, so this is the one visual adaptation
needed.
"Open a prefix..." to match the new interaction.
Tests
Updated the two existing tests that exercised this component
(
App.test.tsx) to replace thegetByTitle-only assertions with assertionsthat:
not.toHaveAttribute("open")),<summary>carries notabIndex/role(proving it relies on nativesemantics, not a hand-rolled focus hack),
disclosure and reveals the full digest text.
These tests fail against the pre-fix
<span title>markup (no<details>toquery,
getByTitleassertions gone) and pass after the fix.Checks
From
frontend/:corepack pnpm exec tsc -b-- 2 pre-existing errors, both unrelated tothis change and present identically on
main(unusedoidcReturnUrlimport,
AdminPanel accessTokenoptional-vs-required type) -- verified viagit stashbefore/after comparison.corepack pnpm exec oxlint src/App.tsx src/App.test.tsx src/App.css-- exit0; only the same 2 pre-existing unused-import warnings noted above.
corepack pnpm exec vitest run src/App.test.tsx-- 78 passed (78).corepack pnpm exec vitest run src/i18n.test.ts-- 18 passed (18)(sanity check; i18n.ts itself is untouched).
Scope note
This region of
App.tsx(analysis-run detail: labels like "Code"/"Config"/"Result", "Cutoff"/"Requested", etc.) is not yet wrapped in
t()anywhere,including in the sibling helper functions right next to this component. To
match the existing local convention and avoid unrelated scope creep, the
changed/added instructional text follows that same (currently un-i18n'd)
convention rather than introducing a lone
t()-wrapped string in anotherwise plain-English panel.
🤖 Generated with Claude Code
https://claude.ai/code/session_011jWJzKUd82yy97esEBfJbt