Repository navigation
Conversation
…lved 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 callstack#2714
|
Live evidence: Pixel_7_API_36, Android Settings. The list is inset at y=480 under the search bar. Steps: Comparison strip, left to right: main before, main after (no movement, 
main (0149438): base-scroll-up-0.8.mp4branch: head-scroll-up-0.8.mp4 |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
All reported issues were addressed across 8 files
You’re at about 90% of the monthly reviewed-line limit. You may want to disable incremental reviews to conserve quota. Reviews will continue until that limit is exceeded. If you need help avoiding interruptions, please contact contact@cubic.dev.
Reply with feedback, questions, or to request a fix.
View guided diff | Re-trigger cubic
|
At 95ba7e8 the lane clamp can start a Not blocking, and fine to take or leave: All 12 checks pass. I did not run the tests or a device, and the half-open edge reasoning above is not checked live. The existing live evidence predates the rebase and covered only The open inline threads still apply: the container-outside-padding return and the cross-axis centre padding. Three lower-priority threads also still apply. None of the open threads can be resolved yet. Once both endpoints stay inside the container's rect and the live |
The lane clamp treated the container's bottom edge as reachable, but Android bounds are half-open, so a shortened `scroll down` above a footer could start one pixel below the list. The usable band is now the container's half-open range inset by 8 px on both sides, on both axes. Along the scrolled axis it is intersected with the padded frame, which still wins when it leaves nothing of the container. Across it, the padded range is preferred, but a rail or chip row lying wholly in that padding is swiped along its own inset range rather than missed. A lane that only loses the rounding pixel of an odd span is no longer reported as shortened. Refs callstack#2714
airowe
changed the title
|
@thymikee Thanks. Fixed in 86214ad.
Live ADB run at 86214ad (Pixel_7_API_36): Clock's world-clock list, rect Strip, left to right: main before, main after (a Clock widget snackbar appeared; the list didn't move), branch before, branch after. 
main, base-scroll-down-0.8.mp4branch at 86214ad, head-scroll-down-0.8.mp4 |
There was a problem hiding this comment.
4 issues found across 4 files (changes from recent commits).
You’re at about 95% of the monthly reviewed-line limit. You may want to disable incremental reviews to conserve quota. Reviews will continue until that limit is exceeded. If you need help avoiding interruptions, please contact contact@cubic.dev.
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/platform-android/src/__tests__/input-actions.test.ts">
<violation number="1" location="packages/platform-android/src/__tests__/input-actions.test.ts:181">
P3: This weakened check no longer verifies the 8 px edge inset. Assert the exact inset start, `top === list.y + 8`, so a regression to 401 fails.</violation>
</file>
<file name="CHANGELOG.md">
<violation number="1" location="CHANGELOG.md:5">
P3: `scroll_no_progress` is documented as a consequence of endpoint placement alone. State that it requires an unchanged surface, an inside-container swipe, and hidden content remaining in that direction.</violation>
<violation number="2" location="CHANGELOG.md:5">
P3: This bullet overstates the guarantee. When the 10% padding leaves none of the container on the scrolled axis, the swipe keeps the padded-frame lane, so endpoints can fall outside the container. The cross-axis fallback can also leave the padding. Qualify the sentence so users don't rely on containment in those cases.</violation>
</file>
<file name="packages/contracts/src/scroll-gesture-container.test.ts">
<violation number="1" location="packages/contracts/src/scroll-gesture-container.test.ts:42">
P2: The shrink test no longer pins the 8 px inset. `strictlyInside` only excludes the edge row, so endpoints at y=179 or y=668 still pass. Restore an exact check on the band, for example `[186, 787]` for `INSET_CONTAINER` and `[186, 669]` for `FOOTERED_CONTAINER`.</violation>
</file>
Reply with feedback, questions, or to request a fix.
View guided diff | Re-trigger cubic
| const plan = buildScrollGesturePlan({ direction, amount: 0.75, ...FRAME, container }); | ||
|
|
||
| assert.ok(strictlyInside(plan, container), JSON.stringify(plan)); | ||
| assert.ok(Math.max(plan.y1, plan.y2) <= PADDED_BOTTOM, JSON.stringify(plan)); |
There was a problem hiding this comment.
P2: The shrink test no longer pins the 8 px inset. strictlyInside only excludes the edge row, so endpoints at y=179 or y=668 still pass. Restore an exact check on the band, for example [186, 787] for INSET_CONTAINER and [186, 669] for FOOTERED_CONTAINER.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. 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. At packages/contracts/src/scroll-gesture-container.test.ts, line 42:
<comment>The shrink test no longer pins the 8 px inset. `strictlyInside` only excludes the edge row, so endpoints at y=179 or y=668 still pass. Restore an exact check on the band, for example `[186, 787]` for `INSET_CONTAINER` and `[186, 669]` for `FOOTERED_CONTAINER`.</comment>
<file context>
@@ -4,44 +4,81 @@ import type { Rect } from '@agent-device/kernel/snapshot';
- );
- assert.equal(plan.pixels, PADDED_BOTTOM - INSET_CONTAINER.y);
+ assert.ok(strictlyInside(plan, container), JSON.stringify(plan));
+ assert.ok(Math.max(plan.y1, plan.y2) <= PADDED_BOTTOM, JSON.stringify(plan));
+ assert.equal(plan.pixels, Math.abs(plan.y2 - plan.y1));
+ assert.ok(plan.pixels < unclamped.pixels);
</file context>
| const [top, bottom] = swipeSpan(result); | ||
|
|
||
| assert.equal(result.keyboardAvoided, true); | ||
| assert.ok(top > list.y, `swipe start ${top} is on the list's top edge or above it`); |
There was a problem hiding this comment.
P3: This weakened check no longer verifies the 8 px edge inset. Assert the exact inset start, top === list.y + 8, so a regression to 401 fails.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. 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. At packages/platform-android/src/__tests__/input-actions.test.ts, line 181:
<comment>This weakened check no longer verifies the 8 px edge inset. Assert the exact inset start, `top === list.y + 8`, so a regression to 401 fails.</comment>
<file context>
@@ -167,7 +178,7 @@ test('scrollAndroid keeps the swipe inside a container that runs under the keybo
assert.equal(result.keyboardAvoided, true);
- assert.equal(top, list.y);
+ assert.ok(top > list.y, `swipe start ${top} is on the list's top edge or above it`);
assert.ok(bottom <= LOWER_HALF_KEYBOARD.y, `swipe endpoint ${bottom} landed on the keyboard`);
assert.equal(result.pixels, bottom - top);
</file context>
| assert.ok(top > list.y, `swipe start ${top} is on the list's top edge or above it`); | |
| assert.equal(top, list.y + 8, 'swipe start sits at the 8 px inset below the list top edge'); |
|
|
||
| ## Unreleased | ||
|
|
||
| - On adb-backed Android, a directional `scroll` keeps both swipe endpoints inside the scroll container the current snapshot resolves, inset from its edges and within the 10% edge padding, so a list under a header or above a footer no longer receives a drag that starts outside it and ends in `scroll_no_progress`. A swipe that already fits is unchanged; one longer than the container is shortened, and the response reports the dispatched `pixels` beside `requestedPixels`. Horizontal scrolls, scrolls after a mutation that has not been re-snapshotted, iOS, and WebDriver (Appium) sessions keep the unclamped swipe for now (#2714). |
There was a problem hiding this comment.
P3: scroll_no_progress is documented as a consequence of endpoint placement alone. State that it requires an unchanged surface, an inside-container swipe, and hidden content remaining in that direction.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. 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. At CHANGELOG.md, line 5:
<comment>`scroll_no_progress` is documented as a consequence of endpoint placement alone. State that it requires an unchanged surface, an inside-container swipe, and hidden content remaining in that direction.</comment>
<file context>
@@ -2,6 +2,7 @@
## Unreleased
+- On adb-backed Android, a directional `scroll` keeps both swipe endpoints inside the scroll container the current snapshot resolves, inset from its edges and within the 10% edge padding, so a list under a header or above a footer no longer receives a drag that starts outside it and ends in `scroll_no_progress`. A swipe that already fits is unchanged; one longer than the container is shortened, and the response reports the dispatched `pixels` beside `requestedPixels`. Horizontal scrolls, scrolls after a mutation that has not been re-snapshotted, iOS, and WebDriver (Appium) sessions keep the unclamped swipe for now (#2714).
- Add `uninstall <app>` and `client.apps.uninstall({ app })` to remove an installed app on adb-backed Android targets, iOS simulators, and CoreDevice iOS devices. Both report `wasInstalled`. A package name or bundle id that is not installed succeeds with `wasInstalled: false`. Other targets, Appium and Limrun sessions included, refuse it before anything runs.
- Add `settings mobile-data on|off` for adb-backed Android targets. It switches mobile data through `svc data` and reports the `mobileData` state stored in the global `mobile_data` row; multi-SIM read-back is not covered yet. Every other target refuses it before anything runs, and `--app` is refused because the switch is device-wide.
</file context>
| - On adb-backed Android, a directional `scroll` keeps both swipe endpoints inside the scroll container the current snapshot resolves, inset from its edges and within the 10% edge padding, so a list under a header or above a footer no longer receives a drag that starts outside it and ends in `scroll_no_progress`. A swipe that already fits is unchanged; one longer than the container is shortened, and the response reports the dispatched `pixels` beside `requestedPixels`. Horizontal scrolls, scrolls after a mutation that has not been re-snapshotted, iOS, and WebDriver (Appium) sessions keep the unclamped swipe for now (#2714). | |
| - On adb-backed Android, a directional `scroll` keeps both swipe endpoints inside the scroll container the current snapshot resolves, inset from its edges and within the 10% edge padding, so a list under a header or above a footer no longer receives a drag that starts outside it. `scroll_no_progress` remains limited to an unchanged surface after an inside-container swipe when the relevant container still has hidden content in that direction. A swipe that already fits is unchanged; one longer than the container is shortened, and the response reports the dispatched `pixels` beside `requestedPixels`. Horizontal scrolls, scrolls after a mutation that has not been re-snapshotted, iOS, and WebDriver (Appium) sessions keep the unclamped swipe for now (#2714). |
|
|
||
| ## Unreleased | ||
|
|
||
| - On adb-backed Android, a directional `scroll` keeps both swipe endpoints inside the scroll container the current snapshot resolves, inset from its edges and within the 10% edge padding, so a list under a header or above a footer no longer receives a drag that starts outside it and ends in `scroll_no_progress`. A swipe that already fits is unchanged; one longer than the container is shortened, and the response reports the dispatched `pixels` beside `requestedPixels`. Horizontal scrolls, scrolls after a mutation that has not been re-snapshotted, iOS, and WebDriver (Appium) sessions keep the unclamped swipe for now (#2714). |
There was a problem hiding this comment.
P3: This bullet overstates the guarantee. When the 10% padding leaves none of the container on the scrolled axis, the swipe keeps the padded-frame lane, so endpoints can fall outside the container. The cross-axis fallback can also leave the padding. Qualify the sentence so users don't rely on containment in those cases.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. 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. At CHANGELOG.md, line 5:
<comment>This bullet overstates the guarantee. When the 10% padding leaves none of the container on the scrolled axis, the swipe keeps the padded-frame lane, so endpoints can fall outside the container. The cross-axis fallback can also leave the padding. Qualify the sentence so users don't rely on containment in those cases.</comment>
<file context>
@@ -2,6 +2,7 @@
## Unreleased
+- On adb-backed Android, a directional `scroll` keeps both swipe endpoints inside the scroll container the current snapshot resolves, inset from its edges and within the 10% edge padding, so a list under a header or above a footer no longer receives a drag that starts outside it and ends in `scroll_no_progress`. A swipe that already fits is unchanged; one longer than the container is shortened, and the response reports the dispatched `pixels` beside `requestedPixels`. Horizontal scrolls, scrolls after a mutation that has not been re-snapshotted, iOS, and WebDriver (Appium) sessions keep the unclamped swipe for now (#2714).
- Add `uninstall <app>` and `client.apps.uninstall({ app })` to remove an installed app on adb-backed Android targets, iOS simulators, and CoreDevice iOS devices. Both report `wasInstalled`. A package name or bundle id that is not installed succeeds with `wasInstalled: false`. Other targets, Appium and Limrun sessions included, refuse it before anything runs.
- Add `settings mobile-data on|off` for adb-backed Android targets. It switches mobile data through `svc data` and reports the `mobileData` state stored in the global `mobile_data` row; multi-SIM read-back is not covered yet. Every other target refuses it before anything runs, and `--app` is refused because the switch is device-wide.
</file context>
| - On adb-backed Android, a directional `scroll` keeps both swipe endpoints inside the scroll container the current snapshot resolves, inset from its edges and within the 10% edge padding, so a list under a header or above a footer no longer receives a drag that starts outside it and ends in `scroll_no_progress`. A swipe that already fits is unchanged; one longer than the container is shortened, and the response reports the dispatched `pixels` beside `requestedPixels`. Horizontal scrolls, scrolls after a mutation that has not been re-snapshotted, iOS, and WebDriver (Appium) sessions keep the unclamped swipe for now (#2714). | |
| - On adb-backed Android, a directional `scroll` keeps both swipe endpoints inside the scroll container the current snapshot resolves, inset from its edges and, along the scrolled axis, within the 10% edge padding, so a list under a header or above a footer no longer receives a drag that starts outside it and ends in `scroll_no_progress`. When the padding leaves none of the container on the scrolled axis, the swipe stays on the padded frame instead. A swipe that already fits is unchanged; one longer than the container is shortened, and the response reports the dispatched `pixels` beside `requestedPixels`. Horizontal scrolls, scrolls after a mutation that has not been re-snapshotted, iOS, and WebDriver (Appium) sessions keep the unclamped swipe for now (#2714). |
|
The scroll fix at 86214ad now works as intended, and the findings from the earlier review (#3746 (comment)) are fixed. CI is green, with 12 checks and none failing, and I know of no conflicts. One open thread still needs a change before this is ready: the shrink test should pin the exact 8 px inset again (#3746 (comment)). Not blocking, and you can take or leave these: a shortened swipe that still makes no progress reports the dispatched span under On the open inline threads from the other reviewer, these still apply:
These threads do not apply, so you can resolve them:
I did not run the tests locally, so the claim that the new tests fail on 95ba7e8 comes from reading the old formula. I also did not see the live run output, only the PR body summary. The Settings |
This branch has not been deployed
2 participants
Summary
Clamps directional
scrollendpoints on adb-backed Android to the resolved scrollable (#2714), per the rule agreed with #1821. Endpoints stay strictly inside the container's half-open rect, inset 8 px, and within the 10% padding along the scrolled axis. A shortened swipe reportspixels(dispatched) besiderequestedPixels.buildScrollGesturePlantakes an optionalcontainer. A span that fits is unchanged.9 files, CHANGELOG included. The planner vectors are TS-only because the Swift twin doesn't clamp yet.
Not covered yet: iOS (including the Oct 7 keyboard-clip repro) and WebDriver/Appium. I'd like those as stacked follow-ups.
Validation
check:affected --run --head 86214adf3(exactCommit): format, lint, typecheck, layering, fallow and build pass. vitest-related: 856/857 files pass. The one failure is anew URL(import.meta.url).pathnametest that breaks on this host's space-containing path and fails identically on main. After that, run directly: unit has 14,575 passing and 6 failures that also fail on main; provider-integration andeager-closure-budgetspass.Live on Pixel_7_API_36:
scroll up 0.8(list top y=480): main starts on the search bar →scroll_no_progress; branch 480→2808,moved.scroll down 0.8(list 368..2756 above the nav bar) at 86214ad: y1 2747→376,pixels2371 <requestedPixels2496,moved. Main:scroll_no_progress.