fix(humanize): re-scroll after the settle wait so clicks land (#329) - #468
fix(humanize): re-scroll after the settle wait so clicks land (#329)#468Cloak-HQ wants to merge 1 commit into
Conversation
An element scrolled into view can be pushed off screen again while we wait for a reflowing page to settle. Nothing scrolled a second time, so the click was dispatched outside the viewport and hit nothing, with no error raised. Also stop the pointer-events check from turning a confirmed miss back into 'undetermined' when a late probe times out, which let that click through. Measured on a page reflowing 10-25s: silent miss at ~32s before, clean click now. Static pages and pages reflowing past the timeout are unchanged.
|
Thank you! I will definitely test this on the existing code. However, I don't have any scripts that interact with pages where this could potentially be an issue. I’ll need approximately 1–3 days to verify this code. I hope you understand! ETA: 1–3 days |
|
Thanks, and no rush at all. Since you mentioned you don't have a page that hits this, here is the repro so you can see the before and after directly rather than only checking for regressions. Two files, saved next to each other.
<!doctype html>
<html><head><meta charset="utf-8"><title>settle-sweep</title>
<style>body{font-family:sans-serif;margin:0}.spacer{height:1800px;background:linear-gradient(#eee,#ccc)}
.row{height:3px}button{padding:14px 22px;font-size:18px}</style></head>
<body>
<div class="spacer">scroll</div>
<div id="grow"></div>
<button id="moving" onclick="window.__clicked=true">TARGET</button>
<div class="spacer"></div>
<script>
window.__clicked=false;
// settle_ms = how long the page keeps reflowing before it goes quiet.
const SETTLE = parseInt(new URLSearchParams(location.search).get('settle_ms') || '0', 10);
const grow=document.getElementById('grow');
const t0=performance.now();
const t=setInterval(()=>{
if (performance.now()-t0 >= SETTLE) { clearInterval(t); return; }
const d=document.createElement('div'); d.className='row'; grow.appendChild(d);
}, 50);
</script>
</body></html>
import threading, http.server, socketserver, functools, time, os
PORT = 8801
ROOT = os.path.dirname(os.path.abspath(__file__))
def serve():
h = functools.partial(http.server.SimpleHTTPRequestHandler, directory=ROOT)
socketserver.TCPServer.allow_reuse_address = True
with socketserver.TCPServer(("127.0.0.1", PORT), h) as s:
s.serve_forever()
threading.Thread(target=serve, daemon=True).start()
time.sleep(1)
BASE = f"http://127.0.0.1:{PORT}/page.html"
from cloakbrowser import launch
print(f"{'settle':>8} | {'result':>6} | {'elapsed':>8} | note")
print("-" * 62)
for settle_ms in [0, 5000, 10000, 15000, 20000, 25000, 30000]:
b = launch(headless=True, humanize=True)
try:
p = b.new_page()
p.goto(f"{BASE}?settle_ms={settle_ms}", wait_until="load")
err = None
t0 = time.monotonic()
try:
p.click("#moving") # default 30s timeout, as a user would
except Exception as e:
err = e
dt = time.monotonic() - t0
ok = p.evaluate("window.__clicked")
res = "PASS" if (ok and err is None) else "FAIL"
note = f"{type(err).__name__}: {str(err)[:50]}" if err else ("" if ok else "no exception, click MISSED")
print(f"{settle_ms:>8} | {res:>6} | {dt:>7.1f}s | {note}")
finally:
b.close()Run
The 5000 row sits near the boundary and is the least stable of the set, so treat a single run of it as indicative rather than definitive. The 10000 to 25000 rows are the reliable signal. One note if you go digging: when swapping between The reason a normal page rarely shows this is that the reflow has to outlast the initial scroll but finish inside the click timeout. Lazy-loaded images or comments resolving over ten to twenty seconds above the target is the usual shape. Regression coverage matters just as much here, so if your existing flows still behave, that is a meaningful result on its own. |
|
After tests and checks, as well as experiments with other approaches, I haven't yet managed to find a better solution to this problem. I will keep searching, but for now this approach is the best one. Thank you so much for submitting this PR and for keeping such a close eye on the issues of the humanize layer. I've had very little free time lately. |
|
Thanks @evelaa123 for testing this and confirming it's the best approach for the case. Merged to |
2 participants
Fixes the real cause behind #329.
Symptom
humanize=Truesilently misses clicks on pages that are still loading. No exception, the call returns as if it worked, and the click simply never happened.Root cause
The click pipeline scrolls the element into view, then waits for its position to settle if the page is reflowing. It never scrolls again. On a page that keeps reflowing, the element is pushed back off screen during that wait, so the coordinates we compute are outside the viewport and the click lands on nothing.
Instrumented run,
settle_ms=15000, viewport 1920x959, on currentmain:The element was at y=607 when we scrolled. Eleven seconds later, when it finally stopped moving, it was at y=1219 — below the fold. We clicked at 1238.
A second defect made it silent rather than loud.
check_pointer_eventscorrectly detected there was no element at those coordinates and retried for 14 seconds. But itsbounding_boxtimeout is computed asmax(1, min(deadline - now, 1000)), so as the deadline approaches it clamps to ~1ms and always throws. Theexcepttreats a throw as indeterminate and fails open. A miss the check had already proven became "unknown", and the click was allowed through.What we tried first, and why it did not work
The obvious readings of #329 are all wrong, and each was tested before being discarded.
Is the stability check too strict?
_boxes_differhas a 1px tolerance and retries with backoff, so it only fires on real movement. But it samples 100ms apart, where Playwright compares two consecutive animation frames (rafCountForStablePosition()returns 1 on Chromium). Rewrote it to match Playwright's definition using an in-page rAF comparison. Result: the check passed, and the click then silently missed. Loosening the check does not produce a landed click, it produces a click on whatever moved into that spot.Is the element moving during the mouse travel? Added a re-aim step after
human_move, re-reading the box and correcting. Missed by 5px. Changed it to re-aim at the element centre with an edge inset. Missed by 12px, landed on<HTML>.At that point the numbers stopped making sense — the corrections were not converging on the element at all — which is what prompted instrumenting the pipeline stage by stage. That trace is above, and it showed the element was never on screen to begin with.
Is our own scroll animation the cause? Ruled out by measurement: a fully static page passes 20/20.
Does #355's approach work? Tested it directly. Swallowing
ElementNotStableErrorletsensure_stableconsume the whole budget, so the next line'sbounding_boxgetsmax(1, ~0)and the user receivesTimeout 1ms exceededinstead of a message describing the problem. Still fails, with a worse error. Declined in #355 with these results.The fix
1. Re-scroll after the settle wait. The existing
bounding_box()re-read refreshed the coordinates but never checked they were still on screen. Replaced with the samescroll_to_elementcall already used before the wait. It returnsdid_scroll=Falsewhen the element is already visible, so it is a no-op on pages that never moved — which is why the static-page result is unchanged.2. Do not launder a confirmed miss.
check_pointer_eventsnow remembers a determined miss. A later attempt that merely errored can no longer turn it into a pass at the deadline. Genuinely indeterminate results still fail open, exactly as before, since failing closed would block legitimate clicks.Results
Test page: button below the fold, content growing above it every 50ms, stopping after
settle_ms. Real Chromium in Docker, default 30s click timeout.ElementNotStableErrorElementNotStableErrorElementNotStableErrorPages that reflow longer than the call's own timeout still raise, which is correct — the page never held still long enough to click.
Scope
Python (sync + async), JavaScript, .NET. Click, dblclick, hover.
Verification
chrome-headless-shellis not installed on the dev machine.checkPointerEventsHandle still throws when genuinely coveredtest, so the fail-open change did not open the closed path.dotnet testcould not run locally. CI should exercise it.Asking for a review and a test run
@evelaa123 you wrote most of this layer — could you review this and run it against your own setup?
click,dblclickandhoverunderhumanize=True, so it touches everything. We verified on a synthetic reflowing page and a static one, not against real sites. If you have flows that were intermittently missing clicks, this is the change most likely to move them, in either direction.exceptsays failing closed would block legitimate clicks, and I did not want to undo that reasoning — only to stop a proven miss being reclassified.The repro is one HTML file plus a script. Say the word and I will attach both so you can reproduce before and after directly.
@eofreternal this touches the pointer-events check from #303 — the iframe offset logic is untouched, only the fail-open branch. A look at that part would be appreciated.
Not included
No regression test in the suites. The mocked unit tests cannot catch this — they do not observe whether the click landed, which is why this survived. It needs a real-browser test asserting a click handler fired. Happy to add one here rather than as a follow-up.