-
Notifications
You must be signed in to change notification settings - Fork 0
Review 5601
PR: #5601 · author cixzhang
(written by the nightly Component Auditor, held to a harder bar)
Heads reviewed: 38db8bed309 (round 1) · 121967c3028 (round 2) · f1a5b4cbf95 (round 3)
LOOP VERSION: 1.5.0 · AUDIT RUBRIC: v1.13
Lane: full, all three rounds
Outcome: request changes, three times. Not merged. Escalated to Cindy after the
second fix round, which is the cap.
All three rounds are children of one sentence: the hand-off is driven by something other than the thing that disables the button.
| round | how the press decided it had reached the edge | where that was wrong |
|---|---|---|
| 1 | read scrollLeft after scrollBy
|
reduced motion lands the scroll instantly, so the step counted twice and focus left the button one press early |
| 2 | read scrollLeft before scrollBy and add the step |
mandatory scroll-snap lands the container past what the press asked for, so core-carousel--cards still dropped focus to <body>
|
| 3 | wait for scrollend, then read where it rests |
correct everywhere the event fires; 1.03s late where it does not, which the repo's own useScrollSettle.ts names as a browser we target |
Round 1's gate raised this hypothesis and refuted it, calling prediction the right call and the defect one term of it. Round 2 revived it and blocked on it. Round 3 found it half-adopted, with the un-adopted half as the finding. Recording that arc is the point of this page: the loop's own early hypothesis was right and its first gate talked itself out of it.
BLOCKS: the position was read after the scroll. Under Reduce Motion scrollBy lands
instantly, so Carousel.tsx:460 counted the step twice and the hand-off fired a press
early, moving focus off a button that was still enabled and still the control in use.
Confirmed three ways: read from source, driven against a matched control on main that
does not show it, and reproduced inside the repo's own harness by tightening the PR's own
fixture from 2000 to 600.
Two nits: two comments still named the opposite button as the receiver, and the a11y
baseline rewrite had re-escaped ten unrelated Layout and Stepper keys as \u2014.
Gate 1 failed on R2 (length), R2h, R2j, R2k, R12d, R2f, and one dishonest line claiming a version read that had not happened. Gate 2 clean.
BLOCKS: Card Content sets hasSnap. Mandatory snapping lands the container where the
press did not ask: the prediction expected 420px of a 500px run, Chromium went to 500, the
trailing button disabled unpredicted, and focus fell to <body>. Thirteen of fourteen
shipped stories handed off correctly; --with-snap and --with-padding passed only
because one step already exceeded their whole run, which is geometry rather than
mechanism.
Recorded against the loop rather than the author: round 1's remedy was driven on one
non-snapping story, and it traded an accidental rescue on --cards for a correct one
elsewhere. Never shipped, so nobody was affected by the transition.
The author replaced prediction with a settle listener: arm scrollend before scrolling
(so an instant scroll cannot land before anyone is listening), then read where the element
actually came to rest. Every shipped story now hands off correctly in Chromium, both
directions, both motion modes, LTR and RTL.
BLOCKS: the hand-off's latency is set by an event rather than by the state that
disables the button. Where scrollend never arrives, the 1200ms fallback is how long
focus stays on <body> — measured 1.03s with no focus indicator anywhere, while the
button's own disable fired at 194ms off overflowEnd in the same run.
Confirmed by a matched control with the event suppressed, and out of the pixels: 491 ring pixels against 0, the whole 552-pixel difference confined to the scroll container's box.
- The focus ring on the scroll container is a hairline: the root is
overflow: clipwith a 1px bleed, so the browser's default ring is shaved. It strictly improves on no indicator at all, the PR discloses it, and the themeable-ring question is open in the ledger as an A15. An earlier commit tried the shared accent ring and backed it out, because inset it paints under the slides and reads weaker than what it replaced. - The settle protocol is inline while four named core hooks own the shape. Churn against a mechanism that may not survive the open finding.
-
TabListwas cited as precedent for the reflected props when it carries none. The precedent exists (25 sub-element targets already carry their style-driving props), so the conclusion stands and only the citation was loose.
The standing rule caps the loop at two fix rounds. Both were spent, the third gate still blocks, and the remedy the loop points at wants a state-transition watcher — which means an Effect, and moving focus from an Effect is a house block. That makes the remaining step a ruling rather than a correction, so it went to Cindy with the question stated once.
The class question the first two rounds carried is now #5602, filed by the author: the same disable-under-focus shape is live in Calendar, Lightbox, Pagination and TouchDateField, all driven and confirmed.
Posted as drafted, all three rounds. Frames were published to
assets/pr-5601/review-r1|r2|r3 on the fork and embedded; the attribution line was
appended at post time, as the drafts said it would be.