-
Notifications
You must be signed in to change notification settings - Fork 31
fix(dashboard): make Hook Logs table rows keyboard accessible (#525) #619
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.
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
AVPthegreat
wants to merge
2
commits into
FailproofAI:main
Choose a base branch
from
AVPthegreat:fix/dashboard-hook-logs-keyboard-accessibility
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
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
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
Repository: FailproofAI/failproofai
Length of output: 7279
🌐 Web query:
WAI-ARIA grid row role hidden semantics table row role aria-expanded button row💡 Result:
Under WAI-ARIA standards, a row (role=row) should contain only specific child roles—typically gridcell, columnheader, or rowheader [1][2][3][4]. Placing a button directly as a child of a row is considered invalid according to strict semantic structure, as accessibility APIs expect grid cells to be the immediate children of rows [5]. Regarding your specific architectural questions: 1. Hidden Semantics: If rows or cells are hidden (e.g., via display: none or not present in the DOM), they are excluded from the accessibility tree [6]. To maintain correct row counting and indexing for assistive technologies when rows or columns are missing from the DOM, you must use aria-rowcount, aria-colcount, aria-rowindex, and aria-colindex on the appropriate elements [7][1][6][3]. 2. Button within a Row: Placing a button as a direct child of a row violates the requirement that rows contain only cells [5]. The correct, accessible pattern for an interactive element (like an expand/collapse toggle) inside a row is to place the button inside a gridcell [6][8][5]. 3. aria-expanded: The aria-expanded attribute should be placed on the interactive control (the button) that performs the toggle action, not on the row element itself [9][8]. When a grid requires hierarchical "expand/collapse" behavior (where a row controls the visibility of child rows), the pattern is a treegrid, not a standard grid [10][8]. In a treegrid, the aria-expanded state is managed on the parent row or a cell within that row, indicating whether the child rows are currently visible [9][10]. For most web applications, if you need expandable rows, using the treegrid role with the proper hierarchical ARIA attributes (aria-level, aria-expanded) is the recommended approach [10][8]. Always prefer native HTML table elements (table, tr, td) if the data is tabular, as they provide these semantics automatically [1][3][4]. If you must use ARIA roles for custom layouts, ensure your DOM structure strictly adheres to the required parent-child relationships (grid > row > gridcell) [1][2][3]. Top results: [1], [3], [9], [8], [5]
Citations:
Keep expandable table rows as rows and use an inner disclosure button.
Both interactive rows assign
role="button"to native<tr>elements, which replaces the table row semantics from the descendant cells. Keep the<tr>native and move the focus/action semantics to a proper control inside a cell, such as an inner<button>witharia-expanded, keyboard handling, and focus styling.app/components/session-hooks-panel.tsx#L329-L331app/policies/hooks-client.tsx#L677-L679📍 Affects 2 files
app/components/session-hooks-panel.tsx#L330-L340(this comment)app/policies/hooks-client.tsx#L678-L688🤖 Prompt for AI Agents
Source: MCP tools