Skip to content

Commit f88fd77

Browse files
Sg312Sg312
committed
Let hover actually see what it mounted
Four independent defects made browser_hover blind to the most common thing a hover produces — a row's action bar — so it reported no effect on a hover that worked, and the agent fell back to clicking pixels off screenshots. - The popup scan matched only role=tooltip/menu/listbox. Slack's message shortcuts bar is a labelled toolbar/group, so it registered as nothing at all. Added toolbar, menubar, labelled group, and [popover]. - The baseline was captured BEFORE prepareElementSurface scrolled the target into view, so scrollChanged was always set by the tool's own probe. That pinned every unproductive hover to 'background DOM churn' instead of the honest 'nothing happened', and hid scrolling the hover really caused. Re-baselined once the scroll settles and before the pointer moves. - The MutationObserver attached only on the first observation, while the roots list is rebuilt every call and grows as shadow roots mount. Components that appeared later were never observed, so their DOM changes raised no revision. Roots are now observed as they show up. - observationTruncated was computed and never read, so a scan capped at 12k nodes reported 'nothing appeared' with the same confidence as a complete one — and portalled overlays live at the end of <body>, exactly what the cap drops. Hover now says the page was too large to scan and to confirm visually.
1 parent b26072f commit f88fd77

3 files changed

Lines changed: 96 additions & 27 deletions

File tree

‎apps/desktop/src/main/browser-agent/driver.ts‎

Lines changed: 29 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -3002,16 +3002,29 @@ async function executeToolInner(
30023002
const elementId = requireNum(params, 'elementId')
30033003
const target = pageTargetForElement(contents, elementId)
30043004
const targetFrame = frameExecutionTarget(target, contents)
3005-
const beforePage = await pageActionState(target, true, elementId)
3006-
const beforeElement = await activeElementState(target)
3007-
const beforeTopPage = targetFrame ? await pageActionState(contents, true) : beforePage
3008-
const beforeTopElement = targetFrame ? await activeElementState(contents) : beforeElement
3005+
let beforePage = await pageActionState(target, true, elementId)
3006+
let beforeElement = await activeElementState(target)
3007+
let beforeTopPage = targetFrame ? await pageActionState(contents, true) : beforePage
3008+
let beforeTopElement = targetFrame ? await activeElementState(contents) : beforeElement
3009+
// Preparing the surface scrolls the element into view, so a baseline
3010+
// taken before it always reports scrollChanged — the tool's own probe,
3011+
// not the hover's effect. That pinned every unproductive hover to
3012+
// "background churn" instead of the honest "nothing happened", and hid
3013+
// real scrolling caused by the hover itself. Re-baseline once the scroll
3014+
// has settled and before the pointer moves.
3015+
const rebaseline = async (): Promise<void> => {
3016+
beforePage = await pageActionState(target, true, elementId)
3017+
beforeElement = await activeElementState(target)
3018+
beforeTopPage = targetFrame ? await pageActionState(contents, true) : beforePage
3019+
beforeTopElement = targetFrame ? await activeElementState(contents) : beforeElement
3020+
}
30093021
let trusted = false
30103022
let result: unknown
30113023
if (!targetFrame) {
30123024
assertCurrentExecution()
30133025
assertElementActionCurrent(contents, elementId, target)
30143026
let prepared = await prepareElementSurface(target, elementId, executionDeadline, true)
3027+
await rebaseline()
30153028
let stable = false
30163029
for (let attempt = 0; attempt < 2; attempt++) {
30173030
assertCurrentExecution()
@@ -3035,6 +3048,7 @@ async function executeToolInner(
30353048
assertCurrentExecution()
30363049
assertElementActionCurrent(contents, elementId, target)
30373050
let surface = await prepareElementSurface(target, elementId, executionDeadline, true)
3051+
await rebaseline()
30383052
assertCurrentExecution()
30393053
assertElementActionCurrent(contents, elementId, target)
30403054
let embedding = await assertFrameEmbeddingVisible(
@@ -3135,9 +3149,17 @@ async function executeToolInner(
31353149
effectObserved,
31363150
...(!effectObserved
31373151
? {
3138-
note: possibleEffectObserved
3139-
? 'Only background DOM/title churn followed the hover; a tooltip/menu was not confirmed.'
3140-
: 'No tooltip, menu, focus, or other strong hover effect was observed.',
3152+
// A capped scan cannot claim nothing appeared: overlays are
3153+
// commonly portalled to the END of <body>, which is exactly the
3154+
// part a truncated walk misses. Say so instead of reporting a
3155+
// partial look with full confidence.
3156+
note:
3157+
afterPage.observationTruncated === true ||
3158+
afterTopPage.observationTruncated === true
3159+
? 'This page is too large to scan completely, so a tooltip or menu that opened may not have been seen. Confirm with browser_snapshot or browser_screenshot before concluding the hover did nothing.'
3160+
: possibleEffectObserved
3161+
? 'Only background DOM/title churn followed the hover; a tooltip/menu was not confirmed.'
3162+
: 'No tooltip, menu, focus, or other strong hover effect was observed.',
31413163
}
31423164
: {}),
31433165
}

‎apps/desktop/src/main/browser-agent/page-functions.test.ts‎

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -860,6 +860,28 @@ describe('collectSnapshot', () => {
860860
expect(clickElement(secondRef)).toMatchObject({ dispatched: true })
861861
})
862862

863+
// The exact shape of the reported failure: hovering a Slack message mounts an
864+
// action bar, but it is a role="toolbar"/"group" — none of the three roles the
865+
// popup scan used to match. The hover therefore observed no popup change, no
866+
// target change, and so no effect at all, and the agent concluded hovering
867+
// did not work and fell back to clicking pixels off screenshots.
868+
it('sees a row action bar that mounts on hover', () => {
869+
document.body.innerHTML = '<div data-testid="message">Hello</div>'
870+
visible(document.querySelector('[data-testid="message"]') as HTMLElement)
871+
872+
const before = readPageActionState(true) as { popups: string[] }
873+
expect(before.popups).toEqual([])
874+
875+
const toolbar = visible(document.createElement('div'))
876+
toolbar.setAttribute('role', 'toolbar')
877+
toolbar.setAttribute('aria-label', 'Message shortcuts')
878+
document.body.append(toolbar)
879+
880+
const after = readPageActionState(false) as { popups: string[] }
881+
expect(after.popups).toEqual(['Message shortcuts'])
882+
expect(after.popups).not.toEqual(before.popups)
883+
})
884+
863885
it('reports a targeted control semantic disappearance after its panel closes', () => {
864886
document.body.innerHTML = `
865887
<aside aria-label="Thread panel"><button data-testid="close-thread">Close thread</button></aside>

‎apps/desktop/src/main/browser-agent/page-functions.ts‎

Lines changed: 45 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -34,6 +34,8 @@ declare global {
3434
root: Node
3535
observer: MutationObserver
3636
revision: number
37+
/** Roots already passed to observe(), so re-observing stays cheap. */
38+
observedRoots: WeakSet<ParentNode>
3739
}>
3840
__simAgentNextElementId?: number
3941
}
@@ -2008,6 +2010,23 @@ export function readPageActionState(resetMutationRevision = false, elementId?: n
20082010
if (allElements.length >= stateNodeCap) break
20092011
}
20102012

2013+
const mutationOptions: MutationObserverInit = {
2014+
subtree: true,
2015+
childList: true,
2016+
characterData: true,
2017+
attributes: true,
2018+
attributeFilter: [
2019+
'aria-activedescendant',
2020+
'aria-expanded',
2021+
'aria-hidden',
2022+
'aria-selected',
2023+
'checked',
2024+
'disabled',
2025+
'hidden',
2026+
'open',
2027+
'selected',
2028+
],
2029+
}
20112030
const mutationStates = (window.__simAgentMutationStates ??= [])
20122031
let mutationState = observationRoot
20132032
? mutationStates.find((state) => state.root === observationRoot)
@@ -2017,33 +2036,29 @@ export function readPageActionState(resetMutationRevision = false, elementId?: n
20172036
root: observationRoot,
20182037
observer: null as unknown as MutationObserver,
20192038
revision: 0,
2039+
observedRoots: new WeakSet<ParentNode>(),
20202040
}
20212041
const state = mutationState
20222042
state.observer = new MutationObserver((records) => {
20232043
state.revision += records.length
20242044
})
2025-
for (const root of roots) {
2026-
state.observer.observe(root, {
2027-
subtree: true,
2028-
childList: true,
2029-
characterData: true,
2030-
attributes: true,
2031-
attributeFilter: [
2032-
'aria-activedescendant',
2033-
'aria-expanded',
2034-
'aria-hidden',
2035-
'aria-selected',
2036-
'checked',
2037-
'disabled',
2038-
'hidden',
2039-
'open',
2040-
'selected',
2041-
],
2042-
})
2043-
}
20442045
mutationStates.push(state)
20452046
if (mutationStates.length > 10) mutationStates.shift()?.observer.disconnect()
20462047
}
2048+
// Observe on EVERY call, not only the first. The roots list is rebuilt each
2049+
// time and grows as shadow roots mount, so attaching once left every
2050+
// component that appeared later unobserved — its DOM changes raised no
2051+
// revision, and an action that mounted UI inside one reported no effect at
2052+
// all. `observe` on an already-observed root with the same options is a
2053+
// documented no-op, and observedRoots keeps the common case cheap.
2054+
if (mutationState) {
2055+
const state = mutationState
2056+
for (const root of roots) {
2057+
if (state.observedRoots.has(root)) continue
2058+
state.observer.observe(root as Node, mutationOptions)
2059+
state.observedRoots.add(root)
2060+
}
2061+
}
20472062
if (resetMutationRevision) {
20482063
mutationState?.observer.takeRecords()
20492064
if (mutationState) mutationState.revision = 0
@@ -2116,8 +2131,18 @@ export function readPageActionState(resetMutationRevision = false, elementId?: n
21162131
.replace(/[\uD800-\uDBFF]$/, '')
21172132
)
21182133

2134+
// Roles an app uses for something that APPEARS over the page. The first three
2135+
// were the whole list, which missed the most common hover affordance there
2136+
// is: a row's action bar (Slack's message shortcuts is role="toolbar"/"group"
2137+
// with an aria-label). A hover that mounted one produced no popup change, no
2138+
// target change, and so no observed effect at all — the agent concluded its
2139+
// hover had failed and escalated to clicking pixels.
21192140
const visiblePopupLabels = allElements
2120-
.filter((element) => element.matches('[role="tooltip"], [role="menu"], [role="listbox"]'))
2141+
.filter((element) =>
2142+
element.matches(
2143+
'[role="tooltip"], [role="menu"], [role="listbox"], [role="toolbar"], [role="menubar"], [role="group"][aria-label], [popover]'
2144+
)
2145+
)
21212146
.filter((element) => {
21222147
const rect = element.getBoundingClientRect()
21232148
const view = element.ownerDocument.defaultView

0 commit comments

Comments
 (0)