-
Notifications
You must be signed in to change notification settings - Fork 1.3k
Add focusable map to preserve semantic elements #1458
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
base: main
Are you sure you want to change the base?
Conversation
🦋 Changeset detectedLatest commit: 353fcb5 The changes in this PR will be included in the next version bump. This PR includes changesets to release 3 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
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.
2 issues found across 7 files
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="packages/core/lib/v3/understudy/a11y/snapshot/domTree.ts">
<violation number="1" location="packages/core/lib/v3/understudy/a11y/snapshot/domTree.ts:24">
P2: Native form elements with `disabled` attribute are not focusable. The function should check for the `disabled` attribute before returning `true` for native focusable tags, otherwise `<button disabled>` and `<input disabled>` will incorrectly be marked as focusable.</violation>
<violation number="2" location="packages/core/lib/v3/understudy/a11y/snapshot/domTree.ts:24">
P2: Anchors are treated as focusable even when they lack an href, so `<a>` elements that are not actually focusable will be marked as focusable in focusableMap.</violation>
</file>
Reply to cubic to teach it or ask questions. Re-run a review with @cubic-dev-ai review this PR
Greptile SummaryAdds focusability tracking to preserve semantic interactive elements in accessibility tree snapshots. The implementation introduces a Key Changes:
Impact: Confidence Score: 4/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant Capture as capture.ts
participant DomTree as domTree.ts
participant A11yTree as a11yTree.ts
Note over Capture: tryScopedSnapshot() called
Capture->>DomTree: domMapsForSession()
DomTree->>DomTree: getDomTreeWithFallback()
DomTree->>DomTree: Traverse DOM nodes
loop For each DOM node
DomTree->>DomTree: isNodeFocusable(node)
Note over DomTree: Check tag in NATIVE_FOCUSABLE_TAGS<br/>or tabindex >= 0
DomTree->>DomTree: Store in focusableMap[encodedId]
end
DomTree-->>Capture: Return {tagNameMap, xpathMap, scrollableMap, focusableMap}
Capture->>A11yTree: a11yForFrame(session, frameId, opts)
Note over Capture: Pass focusableMap in opts
A11yTree->>A11yTree: Get accessibility tree
A11yTree->>A11yTree: buildHierarchicalTree(nodes, opts)
loop For each a11y node
A11yTree->>A11yTree: Check if isFocusable via opts.focusableMap
alt isFocusable == true
Note over A11yTree: Keep node in tree even if<br/>structural or has no children
A11yTree->>A11yTree: Preserve role or use tagName
else Not focusable
A11yTree->>A11yTree: Apply normal pruning rules
end
end
A11yTree-->>Capture: Return outline with preserved focusable elements
Capture-->>Capture: Merge frames into final snapshot
|
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.
7 files reviewed, 1 comment
why
tabindex=0was not present in a11y treewhat changed
focusableMapto track focusable/interactive nodes, preserving them in a11y treetest plan
Summary by cubic
Add a focusable map to the DOM/a11y snapshot pipeline to mark focusable nodes and keep them in the accessibility tree. This preserves semantic elements (like links, buttons, and tabindex>=0) that were previously pruned.
Written for commit 353fcb5. Summary will update automatically on new commits.