Repository navigation
feat(extension): report editable element ids alongside the a11y snapshot - #2951
miguelg719 wants to merge 3 commits into
Conversation
|
There was a problem hiding this comment.
1 issue found across 5 files
Confidence score: 4/5
- The feature test doesn’t verify capture wiring in
packages/extension/understudy/a11y/snapshot/capture.ts, so regressions in frame aggregation or the scoped snapshot path could slip through. Add capture-level regression tests for both paths.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packages/extension/understudy/a11y/snapshot/capture.ts">
<violation number="1" location="packages/extension/understudy/a11y/snapshot/capture.ts:836">
P2: Add capture-level regression tests for aggregation across frames and for the scoped snapshot path. The current feature test leaves this wiring unverified, so a future propagation regression can ship while the editable-node test remains green.
(Based on your team's feedback about unit tests for new behavior.)</violation>
</file>
Architecture diagram
sequenceDiagram
participant Capture as Snapshot Capture
participant CDP as CDP Session
participant AX as Chrome AX Tree
participant A11y as a11yForFrame
participant Maps as Per-Frame Maps
participant Merge as Snapshot Merger
participant Snapshot as HybridSnapshot
Note over Capture,Snapshot: A11y snapshot carries editable element IDs as a side channel
Capture->>CDP: Accessibility.getFullAXTree(frameId)
CDP->>AX: Fetch full accessibility tree
AX-->>CDP: AX nodes with role, name, properties
CDP-->>Capture: AX nodes
Capture->>A11y: Build frame snapshot
A11y->>A11y: Filter ignored backend DOM nodes
A11y->>A11y: Decorate roles and build outline + urlMap
loop Each unignored node
A11y->>A11y: Inspect AX "editable" property
alt editable is plaintext or richtext
A11y->>A11y: Encode backendDOMNodeId and append to editableIds
end
end
A11y-->>Capture: outline, urlMap, editableIds
alt Full hybrid snapshot
Capture->>Maps: Store frame maps including editableIds
Capture->>Merge: Merge per-frame maps
Merge->>Snapshot: combinedEditableIds from flattened per-frame editableIds
else Scoped single-frame snapshot
Capture->>Snapshot: combinedEditableIds from this frame's editableIds
end
Note over Snapshot: Outline text, xpath map, and url map are unchanged<br/>LLM-visible content and cache keys are not affected
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| combinedTree, | ||
| combinedXpathMap, | ||
| combinedUrlMap, | ||
| combinedEditableIds: [...perFrameMaps.values()].flatMap((maps) => maps.editableIds ?? []), |
There was a problem hiding this comment.
P2: Add capture-level regression tests for aggregation across frames and for the scoped snapshot path. The current feature test leaves this wiring unverified, so a future propagation regression can ship while the editable-node test remains green.
(Based on your team's feedback about unit tests for new behavior.)
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/extension/understudy/a11y/snapshot/capture.ts, line 836:
<comment>Add capture-level regression tests for aggregation across frames and for the scoped snapshot path. The current feature test leaves this wiring unverified, so a future propagation regression can ship while the editable-node test remains green.
(Based on your team's feedback about unit tests for new behavior.) </comment>
<file context>
@@ -828,6 +833,7 @@ export function mergeFramesIntoSnapshot(
combinedTree,
combinedXpathMap,
combinedUrlMap,
+ combinedEditableIds: [...perFrameMaps.values()].flatMap((maps) => maps.editableIds ?? []),
perFrame: perFrameOutlines.map(({ frameId, outline }) => {
const maps = perFrameMaps.get(frameId);
</file context>
There was a problem hiding this comment.
Not changing this — the cross-frame aggregation is a one-line spread of per-frame results, and the a11yForFrame boundary (where the ids are computed) has its own test; capture-level tests need full CDP session mocks that this stack does not introduce. The editable ids are exercised end to end by the pipeline tests in #2953 (contenteditable targets).
df815d3 to
fb0c254
Compare
…le-ids # Conflicts: # packages/extension/understudy/a11y/snapshot/capture.ts # packages/sdk-go/internal/extensionassets/stagehand-extension.zip
Stack
Stack: #2951 snapshot editable ids → #2952 library → #2953 act pipeline → #2954 observe + cache check → #2955 extract. This is 1/5 (#2951). Each PR targets its predecessor.
Why
The a11y outline tells you an element's role, not whether you can type into it. A
contenteditablediv shows up asgeneric, and rich-text editors inside iframes look like plain containers. The decisions act path (next PRs) needs to know which nodes accept text to build its "input" candidate view; Chrome already reports this on each AX node as theeditableproperty.What
a11yForFramecollects the encoded ids of nodes whose AXeditableproperty isplaintextorrichtextand returns them aseditableIds.captureHybridSnapshot/tryScopedSnapshotmerge them intocombinedEditableIds(and per-frameeditableIds).Testing
a11y-editable-ids.test.ts: editable nodes are reported by encoded id, ignored nodes are skipped, the outline is untouched.oxfmt --check,extensionpack --checkpass locally.