mirror of
https://github.com/openclaw/openclaw.git
synced 2026-08-27 21:07:01 -06:00
fix(browser): resolve unnamed role refs and initialize AX markers (#130881)
* fix(browser): resolve unnamed role refs and initialize AX markers Keep explicit raw ARIA empty names distinct from omitted serializer names and share one dependency-faithful role lookup. Bind the DOM document in the marker-owning CDP session without adding a round trip. Related: #130879. The separate snapshot membership and ordering failures remain open. * chore(browser): shrink ref assertion safety baseline Record the five type assertions removed by the ref-name repair. Tighten the canonical per-file allowance from seven to two; runtime and browser-test bytes are unchanged.
This commit is contained in:
Peter Steinberger
committed by
GitHub
GitHub
parent
879ab8254c
commit
f04a622c1f
@@ -340,6 +340,7 @@ Docs: https://docs.openclaw.ai
|
||||
- **Remote browser reliability:** bound persistent Playwright tab enumeration by the existing remote CDP timeout budget and retire timed-out connection attempts so late completions cannot restore a stuck connection. (#80147, #58968) Thanks @HemantSudarshan and @KeaneYan.
|
||||
- **Browser tab adoption:** preserve the prior implicit tab and stable aliases when new MCP, Playwright, or CDP targets fail final safety validation, abort after creation, or cannot be rediscovered; validate labels before creating tabs and limit managed cleanup to adopted targets. (#105301) Thanks @hugenshen.
|
||||
- **Browser snapshots:** preserve slash-only control names and keep ref-looking page text from retaining truncated refs or misplacing child frames. Fixes #130571.
|
||||
- **Browser refs:** keep unnamed and overlong-named controls distinct from ordinary named controls to prevent wrong-target actions, and initialize raw ARIA DOM markers in their owning CDP session.
|
||||
- **Browser attachment downloads:** return managed URL, filename, and path metadata when direct Playwright navigation starts an attachment download, while validating final URLs before saving bytes and preserving single-owner explicit downloads. (#48045, #89416) Thanks @zhangguiping-xydt.
|
||||
- **Browser action downloads:** return managed URL, filename, and path metadata when agent actions trigger downloads, while preserving explicit ownership, validating final URLs before saving bytes, and quarantining policy-denied tabs without closing them. (#93250, #93307) Thanks @sunlit-deng.
|
||||
- **Managed browser cookie persistence:** initialize new isolated macOS headless profiles with a non-interactive encryption key while preserving existing profile keys, and close Chromium through CDP before bounded signal fallback so persistent logins survive graceful browser and Gateway restarts. (#96704, #98284) Thanks @TurboTheTurtle.
|
||||
|
||||
@@ -85,7 +85,7 @@ extensions/browser/src/browser/paths.ts 1
|
||||
extensions/browser/src/browser/playwright-core.runtime.ts 2
|
||||
extensions/browser/src/browser/pw-download-capture.ts 1
|
||||
extensions/browser/src/browser/pw-role-snapshot.ts 1
|
||||
extensions/browser/src/browser/pw-session-actions.ts 7
|
||||
extensions/browser/src/browser/pw-session-actions.ts 2
|
||||
extensions/browser/src/browser/pw-session-cdp-transport.ts 1
|
||||
extensions/browser/src/browser/pw-session-navigation.ts 1
|
||||
extensions/browser/src/browser/pw-session.page-cdp.ts 4
|
||||
|
||||
@@ -310,6 +310,7 @@ OpenClaw supports three "snapshot" styles:
|
||||
- Actions: `openclaw browser click e12`, `openclaw browser highlight e12`.
|
||||
- Internally, the ref is resolved via `getByRole(...)` (plus `nth()` for duplicates).
|
||||
- Names containing quotes, backslashes, or YAML punctuation remain actionable; use the ref rather than reconstructing a locator from the displayed name.
|
||||
- A missing displayed name can mean an empty accessible name or one above Playwright's 900 UTF-16-unit limit; keep using the returned ref.
|
||||
- Add `--labels` to include a screenshot with overlayed `e12` labels. On
|
||||
Playwright-backed profiles this also returns per-ref bounding-box metadata
|
||||
(`annotations[]`).
|
||||
|
||||
@@ -2,9 +2,19 @@ import path from "node:path";
|
||||
import { afterEach, describe, expect, it } from "vitest";
|
||||
import { useAutoCleanupTempDirTracker } from "../../test-support.js";
|
||||
import { getPlaywrightCore } from "./playwright-core.runtime.js";
|
||||
import { closePlaywrightBrowserConnection } from "./pw-session.js";
|
||||
import {
|
||||
closePlaywrightBrowserConnection,
|
||||
refLocator,
|
||||
restoreRoleRefsForTarget,
|
||||
} from "./pw-session.js";
|
||||
import { BROWSER_REF_MARKER_ATTRIBUTE } from "./pw-session.page-cdp.js";
|
||||
import { clickViaPlaywright, typeViaPlaywright } from "./pw-tools-core.interactions.actions.js";
|
||||
import { snapshotAiViaPlaywright, snapshotRoleViaPlaywright } from "./pw-tools-core.snapshot.js";
|
||||
import {
|
||||
snapshotAiViaPlaywright,
|
||||
snapshotAriaViaPlaywright,
|
||||
snapshotRoleViaPlaywright,
|
||||
storeAriaSnapshotRefsViaPlaywright,
|
||||
} from "./pw-tools-core.snapshot.js";
|
||||
import { getFreePort } from "./test-port.js";
|
||||
|
||||
const tempDirs = useAutoCleanupTempDirTracker(afterEach);
|
||||
@@ -12,7 +22,7 @@ const tempDirs = useAutoCleanupTempDirTracker(afterEach);
|
||||
describe.runIf(process.env.OPENCLAW_BROWSER_SNAPSHOT_E2E === "1")(
|
||||
"Chromium snapshot-to-action name fidelity",
|
||||
() => {
|
||||
it("resolves encoded duplicate names, textboxes, and frame-qualified AI refs", async () => {
|
||||
it("resolves encoded and omitted names, raw AX names, and native frame refs", async () => {
|
||||
const rootDir = tempDirs.make("openclaw-snapshot-labels-");
|
||||
const port = await getFreePort();
|
||||
const cdpUrl = `http://127.0.0.1:${port}`;
|
||||
@@ -26,11 +36,25 @@ describe.runIf(process.env.OPENCLAW_BROWSER_SNAPSHOT_E2E === "1")(
|
||||
);
|
||||
try {
|
||||
const page = context.pages()[0] ?? (await context.newPage());
|
||||
await page.setContent('<main></main><output></output><iframe title="Nested"></iframe>');
|
||||
await page.setContent(
|
||||
'<style>button{min-width:30px;min-height:25px}</style><main></main><output></output><iframe title="Nested"></iframe>',
|
||||
);
|
||||
const buttonName = 'Save: "owner\'s" C:\\draft 🦞';
|
||||
const inputName = 'Project "path"';
|
||||
const nameControls = [
|
||||
{ id: "short", name: "Named control" },
|
||||
{ id: "empty", name: "" },
|
||||
{ id: "empty-again", name: "" },
|
||||
{ id: "name-900", name: "x".repeat(900) },
|
||||
{ id: "name-901", name: "x".repeat(901) },
|
||||
{ id: "surrogates-900", name: "😀".repeat(450) },
|
||||
{ id: "surrogates-901", name: "😀".repeat(450) + "x" },
|
||||
{ id: "normalized-empty", name: "\u200b\u00ad" },
|
||||
{ id: "normalized-899", name: " x \n".repeat(450) },
|
||||
{ id: "normalized-901", name: " x \n".repeat(451) },
|
||||
];
|
||||
await page.evaluate(
|
||||
({ buttonName: label, inputName: inputLabel }) => {
|
||||
({ buttonName: label, inputName: inputLabel, nameControls: controls }) => {
|
||||
for (const id of ["first", "second"]) {
|
||||
const button = document.createElement("button");
|
||||
button.textContent = label;
|
||||
@@ -48,10 +72,19 @@ describe.runIf(process.env.OPENCLAW_BROWSER_SNAPSHOT_E2E === "1")(
|
||||
document.querySelector("output")!.textContent = "/";
|
||||
});
|
||||
document.querySelector("main")!.append(slash);
|
||||
for (const { id, name } of controls) {
|
||||
const button = document.createElement("button");
|
||||
button.setAttribute("aria-label", name);
|
||||
button.textContent = name ? "control" : "";
|
||||
button.addEventListener("click", () => {
|
||||
document.querySelector("output")!.textContent = id;
|
||||
});
|
||||
document.querySelector("main")!.append(button);
|
||||
}
|
||||
document.querySelector("iframe")!.srcdoc =
|
||||
"<button onclick=\"this.textContent='Frame clicked'\">Frame: action</button>";
|
||||
},
|
||||
{ buttonName, inputName },
|
||||
{ buttonName, inputName, nameControls },
|
||||
);
|
||||
await page.frameLocator("iframe").getByRole("button").waitFor();
|
||||
const session = await context.newCDPSession(page);
|
||||
@@ -68,14 +101,13 @@ describe.runIf(process.env.OPENCLAW_BROWSER_SNAPSHOT_E2E === "1")(
|
||||
options: { interactive: mode !== "role" },
|
||||
});
|
||||
const buttons = Object.entries(snapshot.refs).filter(
|
||||
([, value]) => value.role === "button" && value.name === buttonName,
|
||||
([, value]) => value.role === "button" && value.name !== "Frame: action",
|
||||
);
|
||||
expect(buttons, mode).toHaveLength(2);
|
||||
const expectedButtons = ["first", "second", "/", ...nameControls.map(({ id }) => id)];
|
||||
expect(buttons, mode).toHaveLength(expectedButtons.length);
|
||||
for (const [index, [ref]] of buttons.entries()) {
|
||||
await clickViaPlaywright({ ...target, ref, timeoutMs: 1_000 });
|
||||
expect(await page.locator("output").textContent(), mode).toBe(
|
||||
index ? "second" : "first",
|
||||
);
|
||||
expect(await page.locator("output").textContent(), mode).toBe(expectedButtons[index]);
|
||||
}
|
||||
const input = Object.entries(snapshot.refs).find(
|
||||
([, value]) => value.role === "textbox" && value.name === inputName,
|
||||
@@ -83,10 +115,6 @@ describe.runIf(process.env.OPENCLAW_BROWSER_SNAPSHOT_E2E === "1")(
|
||||
expect(input, mode).toBeDefined();
|
||||
await typeViaPlaywright({ ...target, ref: input![0], text: mode, timeoutMs: 1_000 });
|
||||
expect(await page.getByRole("textbox").inputValue()).toBe(mode);
|
||||
const slash = Object.entries(snapshot.refs).find(([, value]) => value.name === "/");
|
||||
expect(slash, mode).toBeDefined();
|
||||
await clickViaPlaywright({ ...target, ref: slash![0], timeoutMs: 1_000 });
|
||||
expect(await page.locator("output").textContent(), mode).toBe("/");
|
||||
}
|
||||
const snapshot = await snapshotAiViaPlaywright(target);
|
||||
const nested = Object.entries(snapshot.refs).find(
|
||||
@@ -97,6 +125,48 @@ describe.runIf(process.env.OPENCLAW_BROWSER_SNAPSHOT_E2E === "1")(
|
||||
expect(await page.frameLocator("iframe").getByRole("button").textContent()).toBe(
|
||||
"Frame clicked",
|
||||
);
|
||||
|
||||
await page.setContent(
|
||||
"<style>button{min-width:30px;min-height:25px}</style><main></main><output></output>",
|
||||
);
|
||||
const rawControls = [
|
||||
{ id: "raw-short", name: "Raw named control" },
|
||||
{ id: "raw-empty", name: "" },
|
||||
{ id: "raw-long", name: "x".repeat(901) },
|
||||
];
|
||||
await page.evaluate((controls) => {
|
||||
for (const { id, name } of controls) {
|
||||
const button = document.createElement("button");
|
||||
button.setAttribute("aria-label", name);
|
||||
button.textContent = name ? "control" : "";
|
||||
button.addEventListener("click", () => {
|
||||
document.querySelector("output")!.textContent = id;
|
||||
});
|
||||
document.querySelector("main")!.append(button);
|
||||
}
|
||||
}, rawControls);
|
||||
const aria = await snapshotAriaViaPlaywright(target);
|
||||
const rawButtons = aria.nodes.filter((node) => node.role.toLowerCase() === "button");
|
||||
expect(rawButtons).toHaveLength(rawControls.length);
|
||||
expect(
|
||||
await page
|
||||
.getByRole("button", { name: "", exact: true })
|
||||
.getAttribute(BROWSER_REF_MARKER_ATTRIBUTE),
|
||||
).toBe(rawButtons[1]!.ref);
|
||||
await clickViaPlaywright({ ...target, ref: rawButtons[1]!.ref, timeoutMs: 1_000 });
|
||||
expect(await page.locator("output").textContent()).toBe("raw-empty");
|
||||
// Real AX names with unavailable DOM ids exercise the existing role fallback.
|
||||
// Restore onto the launcher's distinct Page wrapper to cross the target cache.
|
||||
await storeAriaSnapshotRefsViaPlaywright({
|
||||
...target,
|
||||
nodes: aria.nodes.map(({ backendDOMNodeId: _backendId, ...node }) => node),
|
||||
});
|
||||
expect(await page.locator(`[${BROWSER_REF_MARKER_ATTRIBUTE}]`).count()).toBe(0);
|
||||
restoreRoleRefsForTarget({ ...target, page });
|
||||
for (const [index, node] of rawButtons.entries()) {
|
||||
await refLocator(page, node.ref).click({ timeout: 1_000 });
|
||||
expect(await page.locator("output").textContent()).toBe(rawControls[index]!.id);
|
||||
}
|
||||
} finally {
|
||||
await closePlaywrightBrowserConnection({ cdpUrl });
|
||||
await context.close();
|
||||
|
||||
@@ -82,9 +82,10 @@ export function refLocator(page: Page, ref: string) {
|
||||
? ref.slice(4)
|
||||
: ref;
|
||||
|
||||
if (/^e\d+$/.test(normalized)) {
|
||||
const isRoleRef = /^e\d+$/.test(normalized);
|
||||
if (isRoleRef || AX_REF_PATTERN.test(normalized)) {
|
||||
const state = pageStates.get(page);
|
||||
if (state?.roleRefsMode === "aria") {
|
||||
if (isRoleRef && state?.roleRefsMode === "aria") {
|
||||
const scope = state.roleRefsFrame ?? page;
|
||||
return scope.locator(`aria-ref=${normalized}`);
|
||||
}
|
||||
@@ -95,39 +96,15 @@ export function refLocator(page: Page, ref: string) {
|
||||
);
|
||||
}
|
||||
const scope = state?.roleRefsFrame ?? page;
|
||||
const locAny = scope as unknown as {
|
||||
getByRole: (
|
||||
role: never,
|
||||
opts?: { name?: string; exact?: boolean },
|
||||
) => ReturnType<Page["getByRole"]>;
|
||||
};
|
||||
const locator = info.name
|
||||
? locAny.getByRole(info.role as never, { name: info.name, exact: true })
|
||||
: locAny.getByRole(info.role as never);
|
||||
return info.nth !== undefined ? locator.nth(info.nth) : locator;
|
||||
}
|
||||
|
||||
if (AX_REF_PATTERN.test(normalized)) {
|
||||
const state = pageStates.get(page);
|
||||
const info = state?.roleRefs?.[normalized];
|
||||
if (!info) {
|
||||
throw new Error(
|
||||
`Unknown ref "${normalized}". Run a new snapshot and use a ref from that snapshot.`,
|
||||
);
|
||||
}
|
||||
const scope = state.roleRefsFrame ?? page;
|
||||
if (info.domMarker) {
|
||||
if (!isRoleRef && info.domMarker) {
|
||||
return scope.locator(`[${BROWSER_REF_MARKER_ATTRIBUTE}="${normalized}"]`);
|
||||
}
|
||||
const locAny = scope as unknown as {
|
||||
getByRole: (
|
||||
role: never,
|
||||
opts?: { name?: string; exact?: boolean },
|
||||
) => ReturnType<Page["getByRole"]>;
|
||||
};
|
||||
const locator = info.name
|
||||
? locAny.getByRole(info.role as never, { name: info.name, exact: true })
|
||||
: locAny.getByRole(info.role as never);
|
||||
// Playwright omits empty names and names over 900 UTF-16 units from ARIA text.
|
||||
// Match that exact bucket before nth; raw AX names (including "") stay explicit.
|
||||
const locator = scope.getByRole(info.role as never, {
|
||||
name: info.name ?? /^$|^.{901,}$/s,
|
||||
exact: true,
|
||||
});
|
||||
return info.nth !== undefined ? locator.nth(info.nth) : locator;
|
||||
}
|
||||
|
||||
|
||||
@@ -58,9 +58,16 @@ describe("pw-session page-scoped CDP client", () => {
|
||||
expect(sessionDetach).toHaveBeenCalledTimes(1);
|
||||
});
|
||||
|
||||
it("marks backend DOM refs on the page", async () => {
|
||||
it("requests the document before marking backend DOM refs on the page", async () => {
|
||||
let documentRequested = false;
|
||||
const sessionSend = vi.fn(async (method: string, params?: Record<string, unknown>) => {
|
||||
if (method === "DOM.getDocument") {
|
||||
documentRequested = true;
|
||||
}
|
||||
if (method === "DOM.pushNodesByBackendIdsToFrontend") {
|
||||
if (!documentRequested) {
|
||||
throw new Error("Document needs to be requested first");
|
||||
}
|
||||
expect(params).toEqual({ backendNodeIds: [42, 84] });
|
||||
return { nodeIds: [101, 202] };
|
||||
}
|
||||
@@ -89,7 +96,8 @@ describe("pw-session page-scoped CDP client", () => {
|
||||
|
||||
expect(page.locator).toHaveBeenCalledWith(`[${BROWSER_REF_MARKER_ATTRIBUTE}]`);
|
||||
expect(evaluateAll).toHaveBeenCalledTimes(1);
|
||||
expect(sessionSend).toHaveBeenNthCalledWith(1, "DOM.enable", undefined);
|
||||
expect(marked).toEqual(new Set(["ax1", "ax2"]));
|
||||
expect(sessionSend).toHaveBeenNthCalledWith(1, "DOM.getDocument", { depth: 0 });
|
||||
expect(sessionSend).toHaveBeenNthCalledWith(2, "DOM.pushNodesByBackendIdsToFrontend", {
|
||||
backendNodeIds: [42, 84],
|
||||
});
|
||||
@@ -103,7 +111,6 @@ describe("pw-session page-scoped CDP client", () => {
|
||||
name: BROWSER_REF_MARKER_ATTRIBUTE,
|
||||
value: "ax2",
|
||||
});
|
||||
expect(marked).toEqual(new Set(["ax1", "ax2"]));
|
||||
expect(sessionDetach).toHaveBeenCalledTimes(1);
|
||||
});
|
||||
|
||||
|
||||
@@ -99,7 +99,9 @@ export async function markBackendDomRefsOnPage(opts: {
|
||||
) => Promise<unknown>
|
||||
)(method, params);
|
||||
|
||||
await send("DOM.enable").catch(() => {});
|
||||
// Backend-id pushes require a bound document in this fresh session.
|
||||
// getDocument also enables DOM; depth zero avoids fetching the subtree.
|
||||
await send("DOM.getDocument", { depth: 0 }).catch(() => {});
|
||||
|
||||
const backendNodeIds = uniqueValues(refs.map((entry) => Math.floor(entry.backendDOMNodeId)));
|
||||
const pushed = (await send("DOM.pushNodesByBackendIdsToFrontend", {
|
||||
|
||||
@@ -123,14 +123,19 @@ describe("pw-session refLocator", () => {
|
||||
expect(mocks.frameLocator).not.toHaveBeenCalled();
|
||||
});
|
||||
|
||||
it("uses page getByRole for role refs by default", () => {
|
||||
it.each([
|
||||
{ ref: "e1", name: "OK" },
|
||||
{ ref: "e1", name: "" },
|
||||
{ ref: "ax12", name: "OK" },
|
||||
{ ref: "ax12", name: "" },
|
||||
])("matches the exact name for unmarked $ref with name '$name'", ({ ref, name }) => {
|
||||
const { page, mocks } = fakePage();
|
||||
const state = ensurePageState(page);
|
||||
state.roleRefs = { e1: { role: "button", name: "OK" } };
|
||||
state.roleRefs = { [ref]: { role: "button", name } };
|
||||
|
||||
refLocator(page, "e1");
|
||||
refLocator(page, ref);
|
||||
|
||||
expect(mocks.getByRole).toHaveBeenCalled();
|
||||
expect(mocks.getByRole).toHaveBeenCalledWith("button", { name, exact: true });
|
||||
});
|
||||
|
||||
it("uses aria-ref locators when refs mode is aria", () => {
|
||||
@@ -153,16 +158,6 @@ describe("pw-session refLocator", () => {
|
||||
expect(mocks.locator).toHaveBeenCalledWith(`[${BROWSER_REF_MARKER_ATTRIBUTE}="ax12"]`);
|
||||
});
|
||||
|
||||
it("falls back to role heuristics for ax refs without backend markers", () => {
|
||||
const { page, mocks } = fakePage();
|
||||
const state = ensurePageState(page);
|
||||
state.roleRefs = { ax12: { role: "button", name: "OK" } };
|
||||
|
||||
refLocator(page, "ax12");
|
||||
|
||||
expect(mocks.getByRole).toHaveBeenCalledWith("button", { name: "OK", exact: true });
|
||||
});
|
||||
|
||||
it("rejects unknown ax refs instead of timing out on aria-ref locators", () => {
|
||||
const { page, mocks } = fakePage();
|
||||
|
||||
|
||||
@@ -424,6 +424,7 @@ describe("pw-tools-core aria snapshot storage", () => {
|
||||
nodes: [
|
||||
{ ref: "ax1", role: "Button", name: "OK", backendDOMNodeId: 42, depth: 0 },
|
||||
{ ref: "ax2", role: "Button", name: "OK", backendDOMNodeId: 84, depth: 0 },
|
||||
{ ref: "ax3", role: "Button", name: "", backendDOMNodeId: 126, depth: 0 },
|
||||
],
|
||||
});
|
||||
|
||||
@@ -434,6 +435,7 @@ describe("pw-tools-core aria snapshot storage", () => {
|
||||
refs: {
|
||||
ax1: { role: "button", name: "OK" },
|
||||
ax2: { role: "button", name: "OK", nth: 1 },
|
||||
ax3: { role: "button", name: "" },
|
||||
},
|
||||
mode: "role",
|
||||
});
|
||||
|
||||
@@ -109,8 +109,8 @@ function buildStoredAriaRefs(
|
||||
|
||||
for (const node of nodes) {
|
||||
const role = normalizeLowercaseStringOrEmpty(node.role) || "unknown";
|
||||
const name = node.name.trim() || undefined;
|
||||
const key = `${role}:${name ?? ""}`;
|
||||
const name = node.name.trim();
|
||||
const key = `${role}:${name}`;
|
||||
const nth = counts.get(key) ?? 0;
|
||||
counts.set(key, nth + 1);
|
||||
const refsForKey = refsByKey.get(key);
|
||||
@@ -121,7 +121,7 @@ function buildStoredAriaRefs(
|
||||
}
|
||||
refs[node.ref] = {
|
||||
role,
|
||||
...(name ? { name } : {}),
|
||||
name,
|
||||
...(nth > 0 ? { nth } : {}),
|
||||
...(markedRefs.has(node.ref) ? { domMarker: true } : {}),
|
||||
};
|
||||
|
||||
Reference in New Issue
Block a user