Skip to content

fix(ui): make activity-heatmap day cells keyboard-focusable#4957

Closed
philluiz2323 wants to merge 1 commit into
JSONbored:mainfrom
philluiz2323:fix/activity-heatmap-keyboard-focusable
Closed

fix(ui): make activity-heatmap day cells keyboard-focusable#4957
philluiz2323 wants to merge 1 commit into
JSONbored:mainfrom
philluiz2323:fix/activity-heatmap-keyboard-focusable

Conversation

@philluiz2323

Copy link
Copy Markdown
Contributor

Closes #3955

What

activity-heatmap.tsx's day cells were bare <div>s wrapped in a Radix TooltipTrigger asChild, with onPointerMove/onPointerLeave-driven hover as the only way to reveal the per-day probe/incident tooltip. There was no tabIndex/focus handling, so keyboard-only and screen-reader users got only the parent grid's single static aria-label summary and never the per-day detail mouse users see on hover.

Building on a prior, correctly-reviewed attempt

An earlier PR (#4652) took the straightforward fix — swap the <div> for a <button> — and was reviewed favorably (readiness 95/100, no blockers) before being closed. That review flagged a real, unaddressed wrinkle: the day cells sit inside a parent <div role="img" aria-label="Registry activity heatmap...">. role="img" tells assistive tech the element is a single atomic graphic with no interactive descendants — most screen readers flatten/hide a role="img" subtree, so nesting real <button>s inside it would restore keyboard tab focus but likely still fail to expose them to screen-reader users navigating by AT, defeating part of the point.

This PR fixes both pieces:

  1. Each day cell (activity-heatmap.tsx:127) becomes a <button type="button">, keeping the same className/style/aria-label, plus a focus-visible:ring-1 focus-visible:ring-ring ring consistent with other focusable tiles in this codebase (e.g. stat-with-spark.tsx). Since Radix's TooltipTrigger asChild already wires its child's focus/blur to open/close the tooltip, this alone makes the existing tooltip reveal on keyboard focus — no new tooltip logic needed.
  2. The parent grid (activity-heatmap.tsx:116-120) changes from role="img" to role="group" with the same aria-label. group is a container role that does not flatten or hide interactive descendants, so it keeps a labeled-region summary for the grid as a whole while letting each day cell be independently focusable and exposed to assistive tech — resolving the exact nested-interactive-in-atomic-image conflict the prior PR's review identified, rather than leaving it unaddressed.

Screenshots

Viewport · Theme Before After
Desktop · Light
before

after
Desktop · Dark
before

after
Tablet · Light
before

after
Tablet · Dark
before

after
Mobile · Light
before

after
Mobile · Dark
before

after

At rest the heatmap is pixel-identical before/after (confirmed above) — Tailwind's base reset already neutralizes native <button> default styling, so no visual regression from the div→button swap.

Keyboard interaction (animated)

Target Before After
Day-cell focus + tooltip reveal, /subnets/1 before-heatmap-kbd.webm after-heatmap-kbd.webm

(GitHub doesn't inline-render .webm from a raw.githubusercontent.com link — click through to view. Before: repeatedly pressing Tab near the grid never lands on or reveals a day cell's tooltip. After: Tab reaches the first cell directly, and the Radix tooltip opens on focus exactly as it does on hover; continuing to Tab moves cleanly through subsequent cells.)

Verification

  • Focus lands on a day-cell <button> (confirmed via document.activeElement), and the Radix TooltipContent becomes visible with real per-day text (e.g. "2026-04-19", "0 probes") — no separate tooltip-reveal code needed, since TooltipTrigger asChild already wires this.
  • Confirmed no ancestor of a day-cell button carries role="img" anymore (walked the DOM tree in-browser) — the exact nested-interactive-in-atomic-image conflict the prior PR's review flagged is resolved.
  • Tab from the first cell lands on the second cell — clean sequential order, no trapping.

Acceptance criteria

  • activity-heatmap.tsx appears in the diff
  • Keyboard focus reaches each day cell and reveals the same per-day tooltip mouse hover does
  • No invalid nested-interactive/atomic-image ARIA conflict — role="img" replaced with role="group" on the parent, which doesn't hide interactive descendants
  • Mouse-hover behavior unchanged; at-rest visual rendering unchanged (screenshots above)
  • Full 3-viewport × 2-theme before/after matrix, plus animated evidence for the focus-triggered tooltip reveal

Testing

  • npx tsc --noEmit: no new errors (2 pre-existing, unrelated errors reproduce identically on a clean main checkout).
  • npx prettier --check and npx eslint on the changed file: clean aside from pre-existing CRLF noise unrelated to this change.
  • Diff is 4 insertions / 3 deletions in a single file.

@philluiz2323 philluiz2323 requested a review from JSONbored as a code owner July 12, 2026 14:45
@superagent-security

Copy link
Copy Markdown

Superagent didn't find any vulnerabilities or security issues in this PR.

@codecov

codecov Bot commented Jul 12, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 97.94%. Comparing base (970ded1) to head (61340d0).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #4957   +/-   ##
=======================================
  Coverage   97.94%   97.94%           
=======================================
  Files         163      163           
  Lines       19418    19418           
  Branches     7376     7376           
=======================================
  Hits        19018    19018           
  Misses         53       53           
  Partials      347      347           
🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@gittensory-orb gittensory-orb Bot added the gittensor:bug Gittensor-scored bug fix or unsolicited PR — scores a 0.05x multiplier. label Jul 12, 2026
@gittensory-orb

gittensory-orb Bot commented Jul 12, 2026

Copy link
Copy Markdown
Contributor

Warning

🟨🟨🟨🟨🟨🟨🟨🟨🟨🟨🟨🟨

⏸️ Gittensory review result - manual review recommended

Review updated: 2026-07-13 00:44:10 UTC

1 file · 1 AI reviewer · no blockers · readiness 100/100 · CI green · clean

⏸️ Suggested Action - Manual Review

  • Touches a guarded path — held for manual review: This PR changes guardrail-protected path(s): apps/ui/src/components/metagraphed/charts/activity-heatmap.tsx (matched apps/ui/**).

Review summary
This is a straightforward, correct fix for a real a11y gap: day cells become native `<button type="button">` elements with a focus-visible ring, and the parent grid's role changes from `img` (which flattens/hides interactive descendants for AT) to `group` (which does not), so keyboard and screen-reader users now get the per-day tooltip that mouse users already had. Radix's `TooltipTrigger asChild` already wires focus/blur to tooltip open/close, so no new tooltip logic was needed, and the change is scoped to exactly the two lines the issue implicates. The pattern mirrors existing focusable-tile conventions elsewhere in the codebase (e.g. `latency-heatmap.tsx`'s button + focus-visible ring), and no other subnet-pulse-grid-style behavior was disturbed.

Nits — 7 non-blocking
  • No test-path changes accompany this fix; a simple test asserting the day cell renders as a `<button>` with `tabIndex` reachable via keyboard (or an axe/role assertion on the parent `role="group"`) would guard against regression.
  • The button has no `type` conflict but also no explicit `:focus` reset beyond `focus:outline-none focus-visible:ring-1` — worth confirming this matches the codebase's broader focus-ring convention (other tiles use `focus-visible:ring-2 ... ring-offset-1`), for visual consistency across heatmap-style components.
  • Consider adding a lightweight RTL/vitest test asserting the day cells render as `<button>` elements and the grid uses `role="group"`, since this file currently has no test coverage.
  • For visual consistency with `latency-heatmap.tsx`'s cell buttons (`focus-visible:ring-2 focus-visible:ring-accent/70 focus-visible:ring-offset-1`), consider aligning the ring style/offset here.
  • apps/ui/src/components/metagraphed/charts/activity-heatmap.tsx:127 creates up to `weeks * 7` tab stops for read-only cells; that satisfies the issue, but it may be worth confirming this is the intended keyboard experience rather than using a roving-focus grid pattern later.
  • Code changes lack test evidence — Add focused regression tests or explain why existing coverage is sufficient.
  • Touches a guarded path — held for manual review — A maintainer must review and merge this change.
Signal Result Evidence
Code review ✅ No blockers 1 reviewer
Linked issue ✅ Linked #3955
Related work ✅ No active overlap found No same-issue or scoped active PR overlap found.
Change scope ✅ 20/20 Low review scope from cached public metadata (1 linked issue).
Validation posture ✅ 25/25 PR body includes validation/test evidence.
Contributor workload ✅ 10/10 Author activity: 1092 registered-repo PR(s), 666 merged, 123 issue(s).
Contributor context ✅ Confirmed Gittensor contributor philluiz2323; Gittensor profile; 1092 PR(s), 123 issue(s).
Gate result ⚠️ Not blocking Advisory; not blocking this PR.
Review context
  • Author: philluiz2323
  • Role context: outside_contributor
  • Public audience mode: oss maintainer
  • Lane context: Repository is configured for direct PR review.
  • Public profile languages: not available
  • Official Gittensor activity: 1092 PR(s), 123 issue(s).
  • PR-specific overlap: none found.
Contributor next steps
  • Keep the PR focused and include validation evidence before maintainer review.
Signal definitions
  • Related work = same linked issue, overlapping active PRs, or title/path similarity.
  • Change scope = cached public metadata such as size labels, draft state, and review-burden hints.
  • Validation posture = whether the PR provides enough public validation/test evidence for maintainer review.
  • Contributor workload = public contributor activity and cleanup pressure, not a repo-wide quality failure.
  • Contributor context = public GitHub/Gittensor identity context; non-Gittensor status is not a blocker.
[BETA] Chat with Gittensory

Ask Gittensory a question about this PR directly in a comment — grounded only in the same cached, public-safe facts shown above, never a new claim.

  • @gittensory ask &lt;question&gt; answers contribution-quality Q&A with source citations and freshness.
  • @gittensory chat &lt;question&gt; answers in natural prose from cached decision-pack facts via local inference (maintainer/collaborator; read-only).
  • A plain-language @gittensory mention with a real question is routed to the closest matching read-only command automatically -- no exact syntax required.

Full command reference: https://gittensory.aethereal.dev/docs/gittensory-commands

Visual preview
Route Viewport Before (production) After (this PR's preview) Diff
/ desktop before / after /
/ mobile before / (mobile) after / (mobile)

Click any thumbnail to open the full-size screenshot. Before = production · After = this PR's preview deploy.

🟩 Safe / merged · 🟦 Advisory · 🟨 Held for review · 🟥 Blocked / closed


💰 Earn for open-source contributions like this. Gittensor lets GitHub contributors earn for the work they already do — register to start earning →.

Checked by Gittensory, a quiet PR intelligence layer for OSS maintainers.

  • Re-run Gittensory review

@gittensory-orb gittensory-orb Bot added the manual-review Gittensor contributor context label Jul 12, 2026

@JSONbored JSONbored left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Screenshots are all blank, closing.

@JSONbored JSONbored closed this Jul 13, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

gittensor:bug Gittensor-scored bug fix or unsolicited PR — scores a 0.05x multiplier. manual-review Gittensor contributor context

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Activity heatmap day-cells use non-focusable divs, making the per-day tooltip unreachable by keyboard

2 participants