From 95ba7e8239fbe6ac98cb6c697264279d2bd7caba Mon Sep 17 00:00:00 2001 From: Adam Rowe Date: Fri, 16 Oct 2026 14:56:31 -0400 Subject: [PATCH] fix(scroll): keep Android directional swipe endpoints inside the resolved scrollable A directional scroll centred its swipe on the window, so on an inset scrollable the drag could start on a header above the list and the command failed with scroll_no_progress. The daemon now resolves the scroll container from the current stored tree before the gesture and hands its rect to the owner. The shared planner keeps both endpoints inside the container's intersection with the padded frame: a span that fits is unchanged, one that leaves the band moves into it, and one longer than the band shrinks to it and reports `requestedPixels` beside the dispatched `pixels`. Android applies it after the keyboard clip, so the two compose. iOS plans in the runner and follows separately. Refs #2714 --- packages/contracts/src/scroll-command.ts | 22 ++++- .../src/scroll-gesture-container.test.ts | 94 +++++++++++++++++++ packages/contracts/src/scroll-gesture.ts | 78 +++++++++++++-- .../__tests__/scroll-runtime.test.ts | 67 +++++++++++++ .../daemon/src/execution/scroll-movement.ts | 39 ++++++-- .../daemon/src/execution/scroll-runtime.ts | 16 +++- .../src/__tests__/input-actions.test.ts | 50 ++++++++++ .../platform-android/src/input-actions.ts | 12 ++- 8 files changed, 353 insertions(+), 25 deletions(-) create mode 100644 packages/contracts/src/scroll-gesture-container.test.ts diff --git a/packages/contracts/src/scroll-command.ts b/packages/contracts/src/scroll-command.ts index 39de1e80cf..52e6f89fcb 100644 --- a/packages/contracts/src/scroll-command.ts +++ b/packages/contracts/src/scroll-command.ts @@ -1,6 +1,7 @@ import { AppError } from '@agent-device/kernel/errors'; import type { SettleObservation } from './interaction.ts'; import type { ScrollDirection } from './scroll-gesture.ts'; +import type { Rect } from '@agent-device/kernel/snapshot'; export const SCROLL_DURATION_MAX_MS = 10_000; export const DEFAULT_MOBILE_SCROLL_DURATION_MS = 300; @@ -38,13 +39,24 @@ export type ScrollTimingOptions = { export type ScrollCommandOptions = ScrollDistanceOptions & ScrollTimingOptions; -export type ScrollExecutionOptions = ScrollCommandOptions & { - releaseBehavior?: ScrollReleaseBehavior; +/** + * The scroll container the caller resolved from the current tree, in the snapshot's absolute space. + * An owner that plans its own swipe keeps both endpoints inside it (#2714); an owner without that + * planner ignores it. + */ +export type ScrollContainerTarget = { + containerRect?: Rect; }; -export type ResolvedScrollExecutionOptions = ScrollCommandOptions & { - releaseBehavior: ScrollReleaseBehavior; -}; +export type ScrollExecutionOptions = ScrollCommandOptions & + ScrollContainerTarget & { + releaseBehavior?: ScrollReleaseBehavior; + }; + +export type ResolvedScrollExecutionOptions = ScrollCommandOptions & + ScrollContainerTarget & { + releaseBehavior: ScrollReleaseBehavior; + }; export function resolveScrollExecutionOptions( options: ScrollCommandOptions, diff --git a/packages/contracts/src/scroll-gesture-container.test.ts b/packages/contracts/src/scroll-gesture-container.test.ts new file mode 100644 index 0000000000..e969e22b8c --- /dev/null +++ b/packages/contracts/src/scroll-gesture-container.test.ts @@ -0,0 +1,94 @@ +import { test } from 'vitest'; +import assert from 'node:assert/strict'; +import type { Rect } from '@agent-device/kernel/snapshot'; +import { buildScrollGesturePlan, type ScrollDirection } from './scroll-gesture.ts'; + +// The iPhone geometry #2714 was reported on: a 402x874 frame, the default 10% edge padding leaving +// y 87..787 for a swipe. +const FRAME = { referenceWidth: 402, referenceHeight: 874 }; +const PADDED_TOP = 87; +const PADDED_BOTTOM = 787; +const INSET_CONTAINER: Rect = { x: 18, y: 178, width: 366, height: 662 }; +const FITTING_CONTAINER: Rect = { x: 18, y: 62, width: 366, height: 729 }; + +function endpointsInside( + plan: { x1: number; y1: number; x2: number; y2: number }, + rect: Rect, +): boolean { + const inside = (x: number, y: number) => + x >= rect.x && x <= rect.x + rect.width && y >= rect.y && y <= rect.y + rect.height; + return inside(plan.x1, plan.y1) && inside(plan.x2, plan.y2); +} + +test.each(['up', 'down'])( + 'a %s swipe longer than an inset container shrinks to the container and reports both spans', + (direction) => { + const unclamped = buildScrollGesturePlan({ direction, amount: 0.75, ...FRAME }); + const plan = buildScrollGesturePlan({ + direction, + amount: 0.75, + ...FRAME, + container: INSET_CONTAINER, + }); + + assert.ok(endpointsInside(plan, INSET_CONTAINER), JSON.stringify(plan)); + assert.deepEqual( + [Math.min(plan.y1, plan.y2), Math.max(plan.y1, plan.y2)], + [INSET_CONTAINER.y, PADDED_BOTTOM], + ); + assert.equal(plan.pixels, PADDED_BOTTOM - INSET_CONTAINER.y); + assert.equal(plan.requestedPixels, unclamped.pixels); + assert.equal(Math.sign(plan.y2 - plan.y1), Math.sign(unclamped.y2 - unclamped.y1)); + }, +); + +test.each(['up', 'down', 'left', 'right'])( + 'a %s swipe already inside its container keeps today’s endpoints and distance', + (direction) => { + const unclamped = buildScrollGesturePlan({ direction, amount: 0.75, ...FRAME }); + const plan = buildScrollGesturePlan({ + direction, + amount: 0.75, + ...FRAME, + container: FITTING_CONTAINER, + }); + + assert.deepEqual(plan, unclamped); + assert.equal('requestedPixels' in plan, false); + }, +); + +test('a swipe that fits an offset container moves into it without shortening', () => { + const lowerContainer: Rect = { x: 0, y: 300, width: 402, height: 574 }; + const unclamped = buildScrollGesturePlan({ direction: 'down', amount: 0.5, ...FRAME }); + const plan = buildScrollGesturePlan({ + direction: 'down', + amount: 0.5, + ...FRAME, + container: lowerContainer, + }); + + assert.ok(endpointsInside(plan, lowerContainer), JSON.stringify(plan)); + assert.equal(Math.min(plan.y1, plan.y2), lowerContainer.y); + assert.equal(Math.abs(plan.y1 - plan.y2), Math.abs(unclamped.y1 - unclamped.y2)); + assert.equal(plan.pixels, unclamped.pixels); + assert.equal('requestedPixels' in plan, false); +}); + +test('a vertical swipe beside a narrow container runs down its centre line', () => { + const rightColumn: Rect = { x: 250, y: 0, width: 152, height: 874 }; + const unclamped = buildScrollGesturePlan({ direction: 'up', ...FRAME }); + const plan = buildScrollGesturePlan({ direction: 'up', ...FRAME, container: rightColumn }); + + assert.equal(plan.x1, rightColumn.x + rightColumn.width / 2); + assert.equal(plan.x2, plan.x1); + assert.deepEqual([plan.y1, plan.y2, plan.pixels], [unclamped.y1, unclamped.y2, unclamped.pixels]); +}); + +test('a container lying wholly in the edge padding leaves the padded swipe as it was', () => { + const statusBand: Rect = { x: 0, y: 0, width: 402, height: PADDED_TOP - 1 }; + const unclamped = buildScrollGesturePlan({ direction: 'up', ...FRAME }); + const plan = buildScrollGesturePlan({ direction: 'up', ...FRAME, container: statusBand }); + + assert.deepEqual(plan, unclamped); +}); diff --git a/packages/contracts/src/scroll-gesture.ts b/packages/contracts/src/scroll-gesture.ts index 5b8be2fe8f..bf450867d8 100644 --- a/packages/contracts/src/scroll-gesture.ts +++ b/packages/contracts/src/scroll-gesture.ts @@ -49,6 +49,12 @@ export type ScrollGestureOptions = { pixels?: number; referenceWidth: number; referenceHeight: number; + /** + * The scroll container the swipe is meant for, in the reference frame's own zero-origin space. + * Both endpoints stay inside its intersection with the padded frame (#2714): a span that already + * fits is unchanged, one that leaves it is moved in, and one longer than it is shortened. + */ + container?: Rect; }; export type ScrollGesturePlan = { @@ -60,7 +66,10 @@ export type ScrollGesturePlan = { referenceWidth: number; referenceHeight: number; amount?: number; + /** The span dispatched, which is less than `requestedPixels` when the container shortened it. */ pixels: number; + /** Present only when the container shortened the swipe: the span the padded frame would carry. */ + requestedPixels?: number; }; export type SwipePresetGesturePlan = { @@ -125,16 +134,65 @@ export function buildScrollGesturePlan(options: ScrollGestureOptions): ScrollGes pixels: travelPixels, }); - switch (direction) { - case 'up': - return buildPlan(centerX, centerY - halfTravel, centerX, centerY + halfTravel); - case 'down': - return buildPlan(centerX, centerY + halfTravel, centerX, centerY - halfTravel); - case 'left': - return buildPlan(centerX - halfTravel, centerY, centerX + halfTravel, centerY); - case 'right': - return buildPlan(centerX + halfTravel, centerY, centerX - halfTravel, centerY); - } + const vertical = direction === 'up' || direction === 'down'; + const unclamped = { + axisStart: (vertical ? centerY : centerX) - halfTravel, + axisEnd: (vertical ? centerY : centerX) + halfTravel, + cross: vertical ? centerX : centerY, + }; + const lane = options.container + ? keepSwipeInContainer(unclamped, options.container, vertical, { + min: edgePadding, + max: axisLength - edgePadding, + }) + : unclamped; + const shortened = lane.axisEnd - lane.axisStart < unclamped.axisEnd - unclamped.axisStart; + const plan = buildPlan( + vertical ? lane.cross : lane.axisStart, + vertical ? lane.axisStart : lane.cross, + vertical ? lane.cross : lane.axisEnd, + vertical ? lane.axisEnd : lane.cross, + ); + const placed = shortened + ? { ...plan, pixels: lane.axisEnd - lane.axisStart, requestedPixels: travelPixels } + : plan; + // `up`/`left` drag from the low end toward the high end; `down`/`right` drag the other way. + return direction === 'up' || direction === 'left' + ? placed + : { ...placed, x1: placed.x2, y1: placed.y2, x2: placed.x1, y2: placed.y1 }; +} + +type SwipeLane = { axisStart: number; axisEnd: number; cross: number }; + +/** + * Moves a centred swipe lane inside the container's intersection with the padded axis, shortening + * it only when the intersection is shorter than the lane. A container wholly inside the padding + * leaves nothing to swipe in, so the lane stays as the padded frame planned it. + */ +function keepSwipeInContainer( + lane: SwipeLane, + container: Rect, + vertical: boolean, + padded: { min: number; max: number }, +): SwipeLane { + const containerMin = vertical ? container.y : container.x; + const containerSize = vertical ? container.height : container.width; + const bandMin = Math.max(padded.min, Math.round(containerMin)); + const bandMax = Math.min(padded.max, Math.round(containerMin + containerSize)); + if (bandMax <= bandMin) return lane; + + const crossMin = vertical ? container.x : container.y; + const crossSize = vertical ? container.width : container.height; + const cross = + lane.cross >= crossMin && lane.cross <= crossMin + crossSize + ? lane.cross + : Math.round(crossMin + crossSize / 2); + + const length = lane.axisEnd - lane.axisStart; + if (length >= bandMax - bandMin) return { axisStart: bandMin, axisEnd: bandMax, cross }; + const shift = + lane.axisStart < bandMin ? bandMin - lane.axisStart : Math.min(0, bandMax - lane.axisEnd); + return { axisStart: lane.axisStart + shift, axisEnd: lane.axisEnd + shift, cross }; } /** diff --git a/packages/daemon/src/execution/__tests__/scroll-runtime.test.ts b/packages/daemon/src/execution/__tests__/scroll-runtime.test.ts index 0dd7118984..4219f10ba7 100644 --- a/packages/daemon/src/execution/__tests__/scroll-runtime.test.ts +++ b/packages/daemon/src/execution/__tests__/scroll-runtime.test.ts @@ -786,3 +786,70 @@ test('bound scroll will not bill an unread earlier gesture to the scroll it is d assert.equal(result.movement, 'unobserved'); assert.equal(captures.calls(), 0); }); + +/** + * The container a directional swipe is planned inside (#2714): read off the same current tree the + * movement claim compares against, before the gesture, and never off a tree that may no longer be + * the screen. + */ +async function containerHandedToOwner( + positionals: string[], + session: SessionState, + flags?: CommandFlags, +): Promise { + const calls: ScrollCall[] = []; + await runScroll( + positionals, + {}, + { + scroll: async (direction, options) => { + calls.push({ direction, options }); + return { ...OBSERVED_SWIPE }; + }, + }, + { session, ...(flags === undefined ? {} : { flags }) }, + ); + const [call] = calls; + assert.ok(call && calls.length === 1, `expected one scroll, got ${calls.length}`); + return (call.options as { containerRect?: unknown }).containerRect; +} + +test.each<[string, CommandFlags | undefined]>([ + ['an observed scroll', undefined], + ['a --settle scroll', { settle: true }], +])('bound scroll hands %s the container resolved from the current tree', async (_, flags) => { + const containerRect = await containerHandedToOwner( + ['down', '0.75'], + sessionWithStoredScreen(automationScreen(0)), + flags, + ); + + assert.deepEqual(containerRect, SCREEN_CONTAINER); +}); + +test('bound scroll hands no container from a tree that predates the last mutation', async () => { + const session = sessionWithStoredScreen(automationScreen(0)); + expireRefFrame(session); + + assert.equal(await containerHandedToOwner(['down', '0.75'], session), undefined); +}); + +test('bound scroll hands no container for a horizontal swipe the edge analyzer cannot place', async () => { + const containerRect = await containerHandedToOwner( + ['left'], + sessionWithStoredScreen(automationScreen(0)), + ); + + assert.equal(containerRect, undefined); +}); + +test('bound scroll hands no container to a desktop owner that never synthesizes a swipe', async () => { + const session = makeMacOsSession('scroll-desktop'); + session.snapshot = buildSnapshotState( + { nodes: automationScreen(0), backend: 'xctest', producer: 'apple-runner' }, + undefined, + ); + activateCompleteRefFrame(session); + + assert.equal(await containerHandedToOwner(['down', '0.75'], session), undefined); +}); diff --git a/packages/daemon/src/execution/scroll-movement.ts b/packages/daemon/src/execution/scroll-movement.ts index 250c25fb07..0fc1f0454e 100644 --- a/packages/daemon/src/execution/scroll-movement.ts +++ b/packages/daemon/src/execution/scroll-movement.ts @@ -164,17 +164,40 @@ export function readScrollSurfaceBaseline( function freezeScrollSurfaceBaseline(session: SessionState): ScrollMovementPlan { const baseline = readScrollSurfaceBaseline(session.snapshot); if (!baseline) return { kind: 'unobservable', reason: 'stored-surface-unusable' }; - if (refFrameState(session) !== 'active') { - return { kind: 'unobservable', reason: 'stored-surface-not-current' }; - } - if (isPostGestureStabilizationPending(session)) { - // A gesture nobody has read yet may still be moving this tree, which would bill its motion to - // the scroll being dispatched now. - return { kind: 'unobservable', reason: 'prior-gesture-unsettled' }; - } + const staleness = storedSurfaceStaleness(session); + if (staleness) return { kind: 'unobservable', reason: staleness }; return { kind: 'observe', baseline }; } +function storedSurfaceStaleness( + session: SessionState, +): Exclude | undefined { + if (refFrameState(session) !== 'active') return 'stored-surface-not-current'; + // A gesture nobody has read yet may still be moving this tree, which would bill its motion to + // the scroll being dispatched now. + if (isPostGestureStabilizationPending(session)) return 'prior-gesture-unsettled'; + return undefined; +} + +/** + * The scroll container a directional swipe should stay inside (#2714), read before the gesture off + * the same current tree a movement claim would compare against, through the edge verdict's own + * selection. A tree that may no longer be the screen names no container, and neither does a + * horizontal scroll, which the vertical-only edge analyzer cannot place, nor a platform that moves + * content without a synthesized swipe. + */ +export async function readScrollTargetContainer( + device: DeviceInfo, + session: SessionState, + direction: ScrollDirection, +): Promise { + if (!isMobilePlatform(device)) return undefined; + const edge = verticalEdgeFor(direction); + const nodes = session.snapshot?.nodes; + if (!edge || !nodes || nodes.length === 0 || storedSurfaceStaleness(session)) return undefined; + return (await readScrollEdgeState(nodes, edge)).containerRect; +} + /** * Observes one directional scroll and returns the movement its response may claim. * diff --git a/packages/daemon/src/execution/scroll-runtime.ts b/packages/daemon/src/execution/scroll-runtime.ts index 0716eadc0f..e58671749b 100644 --- a/packages/daemon/src/execution/scroll-runtime.ts +++ b/packages/daemon/src/execution/scroll-runtime.ts @@ -47,6 +47,7 @@ import { import { observeScrollMovement, planScrollMovement, + readScrollTargetContainer, reportScrollMovementNotApplicable, reportScrollMovementUnobserved, type ScrollMovementPlan, @@ -145,10 +146,23 @@ export async function resolveBoundScrollRuntime( flags: params.flags, session: params.session, }); + const containerRect = await readScrollTargetContainer( + params.device, + params.session, + target.direction, + ); + const directionOptions = + containerRect === undefined ? options : { ...options, containerRect }; return await resolveBoundGenericRuntime( { ...admission, use: plan.use }, async (runtime, dispatchContext) => - await executeDirectionScroll(runtime, target, options, dispatchContext, movementPlan), + await executeDirectionScroll( + runtime, + target, + directionOptions, + dispatchContext, + movementPlan, + ), ); } case 'edge': { diff --git a/packages/platform-android/src/__tests__/input-actions.test.ts b/packages/platform-android/src/__tests__/input-actions.test.ts index 35261477c9..29906d8337 100644 --- a/packages/platform-android/src/__tests__/input-actions.test.ts +++ b/packages/platform-android/src/__tests__/input-actions.test.ts @@ -134,6 +134,56 @@ test('scrollAndroid swipes the whole window when no IME window is on screen', as }); }); +function swipeSpan(result: Record): [number, number] { + const ys = [Number(result.y1), Number(result.y2)]; + return [Math.min(...ys), Math.max(...ys)]; +} + +test('scrollAndroid keeps both endpoints inside an inset container and reports the shortened span', async () => { + // A window below the status bar, holding a list that starts under a tall header: the unclamped + // 0.8 swipe would start in the header, so the gesture is shortened to the list (#2714). + const offsetWindow = { x: 0, y: 136, width: 1080, height: 2264 }; + const list = { x: 0, y: 436, width: 1080, height: 1700 }; + await withFakeAdb(helperRouteAdb(offsetWindow), async ({ device }) => { + const unclamped = await scrollAndroid(device, 'down', { amount: 0.8 }); + const result = await scrollAndroid(device, 'down', { amount: 0.8, containerRect: list }); + + assert.ok(swipeSpan(unclamped)[0] < list.y, 'the fixture must start outside the list'); + assert.deepEqual(swipeSpan(result), [list.y, list.y + list.height]); + assert.ok(Number(result.y1) > Number(result.y2), 'a down scroll still drags upward'); + assert.equal(result.pixels, list.height); + assert.equal(result.requestedPixels, unclamped.pixels); + }); +}); + +test('scrollAndroid keeps the swipe inside a container that runs under the keyboard', async () => { + // The keyboard clip alone keeps the frame's top, so a long swipe would start on the header above + // the list; the container and the clip together leave only the list above the keys. + const list = { x: 0, y: 400, width: 1080, height: 1880 }; + await withFakeAdb( + helperRouteAdb({ ...PORTRAIT_WINDOW, keyboard: LOWER_HALF_KEYBOARD }), + async ({ device }) => { + const result = await scrollAndroid(device, 'up', { amount: 0.8, containerRect: list }); + const [top, bottom] = swipeSpan(result); + + assert.equal(result.keyboardAvoided, true); + assert.equal(top, list.y); + assert.ok(bottom <= LOWER_HALF_KEYBOARD.y, `swipe endpoint ${bottom} landed on the keyboard`); + assert.equal(result.pixels, bottom - top); + }, + ); +}); + +test('scrollAndroid leaves a swipe that already fits its container unchanged', async () => { + const list = { x: 0, y: 100, width: 1080, height: 2100 }; + await withFakeAdb(helperRouteAdb({ ...PORTRAIT_WINDOW }), async ({ device }) => { + const unclamped = await scrollAndroid(device, 'down', { pixels: 600 }); + const result = await scrollAndroid(device, 'down', { pixels: 600, containerRect: list }); + + assert.deepEqual(result, unclamped); + }); +}); + test('scrollAndroid refuses rather than flinging into a keyboard that owns the window', async () => { // A landscape IME leaves 40px of a 900px window: a swipe there reads as a stuck surface, so the // command refuses with its own typed reason instead of the generic no-progress stop. diff --git a/packages/platform-android/src/input-actions.ts b/packages/platform-android/src/input-actions.ts index 49fbc0fbbc..342294b8f5 100644 --- a/packages/platform-android/src/input-actions.ts +++ b/packages/platform-android/src/input-actions.ts @@ -10,6 +10,7 @@ import { import { buildGesturePlan } from '@agent-device/contracts/gesture-plan'; import { GESTURE_DURATION_MIN_MS } from '@agent-device/contracts/gesture-plan-types'; import { + type ScrollContainerTarget, type ScrollReleaseBehavior, DEFAULT_MOBILE_SCROLL_DURATION_MS, } from '@agent-device/contracts/scroll-command'; @@ -220,7 +221,8 @@ export async function scrollAndroid( pixels?: number; durationMs?: number; releaseBehavior?: ScrollReleaseBehavior; - } & AndroidHelperSessionOptions, + } & ScrollContainerTarget & + AndroidHelperSessionOptions, ): Promise> { // The viewport read and the gesture are two helper calls one command apart: giving the read the // command's session scope keeps both on the same instrumentation. @@ -234,6 +236,9 @@ export async function scrollAndroid( pixels: options?.pixels, referenceWidth: swipeSurface.viewport.width, referenceHeight: swipeSurface.viewport.height, + ...(options?.containerRect === undefined + ? {} + : { container: relativeToViewport(options.containerRect, swipeSurface.viewport) }), }); const scrollPlan = { ...relativePlan, @@ -275,6 +280,11 @@ export async function scrollAndroid( }; } +/** The planner works in the swipe surface's zero-origin frame; snapshot rects are absolute. */ +function relativeToViewport(rect: Rect, viewport: Rect): Rect { + return { ...rect, x: rect.x - viewport.x, y: rect.y - viewport.y }; +} + /** * The band one Android scroll may swipe, refusing when the keyboard owns the window (#2500). *