Skip to content

fix: make Hook Logs table rows keyboard-accessible #533

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
Abhi-DevHub wants to merge 1 commit into
base: main
Choose a base branch
from
+5 −0
Open

fix: make Hook Logs table rows keyboard-accessible #533

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions app/components/session-hooks-panel.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -328,6 +328,10 @@ export default function SessionHooksPanel({ sessionId, initialData }: SessionHoo
<React.Fragment key={`${item.timestamp}-${i}`}>
<tr
onClick={() => toggleRow(i)}
onKeyDown={(e) => { if (e.key === "Enter" || e.key === " ") { e.preventDefault(); toggleRow(i); } }}

@hermes-exosphere hermes-exosphere Jul 16, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🔵 INFO: No tests exist for SessionHooksPanel component. The new keyboard accessibility behavior (tabIndex, onKeyDown, aria-expanded) should ideally have test coverage:

  • Test that Enter/Space toggles row expansion
  • Test that aria-expanded reflects correct state
  • Test that tabIndex makes rows focusable

Consider adding tests in tests/components/session-hooks-panel.test.tsx.

tabIndex={0}
role="button"

@hermes-exosphere hermes-exosphere Jul 16, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟡 ACCESSIBILITY CONCERN: role="button" on a tr element strips table semantics from the accessibility tree. Consider either converting the table to role="treegrid" or moving the button behavior to a native button inside a td cell.

aria-expanded={isExpanded}
Comment on lines +331 to +334

@coderabbitai coderabbitai Bot Jul 16, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

# Inspect the relevant component around the changed lines
file="app/components/session-hooks-panel.tsx"
wc -l "$file"
sed -n '250,390p' "$file"

Repository: FailproofAI/failproofai

Length of output: 7227

🌐 Web query:

WAI-ARIA role button on tr table row semantics aria-expanded treegrid table row accessibility

💡 Result:

Adding role="button" to a table row (tr element) is not recommended and negatively impacts accessibility [1][2][3]. Using this role on a tr element can remove the row from the accessibility tree in some screen readers, causing users to lose both the semantic information of the row and its functionality [2]. Regarding aria-expanded and treegrid semantics: 1. Treegrid Usage: The aria-expanded attribute is specifically valid for rows (tr) or their cells when they are part of a treegrid [4][5]. In a treegrid, this attribute indicates whether a parent row is expanded or collapsed to show or hide child rows [6][5]. 2. Standard Table/Grid Usage: Using aria-expanded on a tr element within a standard table or grid (role="table" or role="grid") is incorrect [4]. These attributes have no function in standard tables and can cause unpredictable behavior in assistive technologies [4]. 3. Accessibility Best Practices: - Avoid making entire table rows interactive by adding roles like "button" [7][2]. - Instead, place interactive controls (such as actual button or link elements) inside the cells of the row [7][2]. This approach ensures standard behavior, proper focus management, and better compatibility across different browsers and screen readers [2][8]. - If you need a tabular structure with expandable rows, implement the treegrid pattern correctly, ensuring the container has role="treegrid" and follow WAI-ARIA authoring practices for keyboard interaction and state management [4][6]. In summary, do not add role="button" to a tr, and reserve aria-expanded for genuine treegrid implementations where row expansion is natively supported by the design pattern [2][4].

Citations:

Move the disclosure control out of the <tr>
role="button" on the row can strip table semantics from the accessibility tree, and aria-expanded belongs on a real treegrid pattern. Put a <button> in a cell, or switch the whole table to treegrid.

🤖 Prompt for AI Agents 
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@app/components/session-hooks-panel.tsx` around lines 331 - 334, Update the
row disclosure control around toggleRow so the `<tr>` no longer carries
role="button", keyboard handling, tabIndex, or aria-expanded; place a native
`<button>` with the same Enter/Space toggle behavior inside a table cell,
preserving the existing isExpanded state and toggleRow(i) action.

className={`cursor-pointer transition-colors ${
isDeny
? "bg-red-500/[0.03] hover:bg-red-500/[0.07] border-l-2 border-l-red-500/40"
Expand All @@ -343,6 +347,7 @@ export default function SessionHooksPanel({ sessionId, initialData }: SessionHoo
className={`h-3.5 w-3.5 text-muted-foreground transition-transform duration-150 ${
isExpanded ? "rotate-0" : "-rotate-90"
}`}
aria-hidden="true"
/>
</td>
<td className="px-3 py-2">
Expand Down