fix(android): include the from point in the swipe path - #42349
fix(android): include the from point in the swipe path#42349Lazizbek Ergashev (lazerg) wants to merge 5 commits into
from point in the swipe path#42349Conversation
Lazizbek Ergashev (lazerg)
changed the title
from point in the swipe path
Yury Semikhatsky (yury-s)
left a comment
Yury Semikhatsky (yury-s)
left a comment
There was a problem hiding this comment.
The change looks good. Any chance you can add a test for it?
|
Added a test in c658786 (tests/android/input.spec.ts). It scrolls a long page with input.swipe(from, [to], steps) and checks scrollY moved. The old code dropped It goes through the real androidDevice fixtures like the rest of tests/android, so it only runs under npm run atest against the ADB emulator, not locally here. CI's Android job triggers on push or the CQ1 label, not automatically on external PRs, so it hasn't run there yet. |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
The new test failed on CI: https://github.com/microsoft/playwright/actions/runs/32510988871/job/96862912472?pr=42349 |
CI showed the swipe assertion timing out with scrollY stuck at 0 on every retry, even though the swipe call itself completed without error. androidDevice.input.swipe() injects a raw touch event through the OS input pipeline, independent of the CDP channel used by setContent(), so the gesture can land before the new content is actually composited and presented on the device screen. Wait for two animation frames after setContent() so the swipe reliably hits the rendered page.
|
Checked the job log. The swipe call did not throw. But window.scrollY stayed at 0 through every retry, so the gesture never scrolled the page. This looks like a race in the test, not a problem with the fix itself. androidDevice.input.swipe() sends a raw touch event through ADB. That event does not go through the CDP channel that setContent() uses. On a real device the touch can land before the new page content is actually painted on screen, so the swipe likely hit the old blank frame. Added a short wait for two animation frames after setContent(), before the swipe runs, in commit 9b39365, pushed to fix-42348. Still only runs under npm run atest / CQ1, so it will need another labeled run to confirm. |
|
Confirmed the root cause independently: \AndroidDispatcher.inputSwipe\ forwards the params verbatim to the on-device agent (\packages/playwright-core/src/server/dispatchers/androidDispatcher.ts), so before this change \ rom\ never reached the driver and every gesture started at \segments[0]. The fix is correct and minimal — thanks. On the flaky test: the double- \\ s so a single dropped first gesture doesn't fail the run. |
| // pipeline, independent of the CDP connection used by setContent() above. Wait for | ||
| // the new content to actually be composited and presented on the device screen, | ||
| // otherwise the swipe can land on the previous (blank) frame and never scroll anything. | ||
| await page.evaluate(() => new Promise(resolve => requestAnimationFrame(() => requestAnimationFrame(resolve)))); |
There was a problem hiding this comment.
You can use rafraf from ../config/utils. I doubt this will help though, if the test is still flaky, let's revert it and keep just the fix.
There was a problem hiding this comment.
Switched to rafraf in 6de3e90.
Worth noting the wait has never actually run on the emulator. The only Android job on this branch is run 32510988871, which built c658786, before the wait existed. tests 2 starts only on the labeled event, so the pushes after the CQ1 label did not re-trigger it. So there is no evidence yet either way.
If you re-apply CQ1 and it still fails, I will drop the test and leave only the fix.
| @@ -0,0 +1,31 @@ | |||
| /** | |||
| * Copyright 2020 Microsoft Corporation. All rights reserved. | |||
There was a problem hiding this comment.
It is not 2020 anymore, look at other files.
There was a problem hiding this comment.
Right, copied it from the older files in this folder. Changed to the Copyright (c) Microsoft Corporation. header in 6de3e90.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Test results for "MCP"1 failed 8141 passed, 1361 skipped Merge workflow run. |
Test results for "tests 1"3 flaky51371 passed, 1238 skipped Merge workflow run. |
|
The new test still fails. I'd recommend to run it locally fist, before sending updates to the PR. |
Test results for "tests 2"1 failed 24 flaky110794 passed, 4724 skipped Merge workflow run. |
|
Ran it on a local emulator this time, an arm64 AVD with the same API 35 Nexus 5X profile The gesture never reaches the page. I attached So the render race I guessed at earlier was wrong, and no amount of waiting or retrying gets past it. Nothing else in the repo covers I removed the test and kept the one line fix, which is what Yury suggested if it stayed flaky. |
4 participants
Summary
AndroidInput.swipe()accepts afrompoint and then drops it: onlysegmentsreaches the protocol, so the gesture actually starts atsegments[0].fromis the first point of the path, so it is prepended tosegmentson the client. No protocol or driver change is needed, since the driver passes the array straight toUiDevice.swipe, which starts at its first point.swipe(from, [to], steps)used to send a one-element array, which the driver turns into a touch down and up on the same point, so it did not move at all. Anyone who worked around this by repeatingfrominsidesegmentsnow gets a zero-length leading segment: same path, one segment worth of extra time.tests/androidhas no coverage ofAndroidInputand no channel-level harness to assert the payload against, so this would need new test infrastructure.Fixes #42348