-
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 (r1) · 121967c3028 (r2) · f1a5b4cbf95 (r3) · a64ea2821f7 (r4) · 194a4d03aea (r5)
LOOP VERSION: 1.5.0 · AUDIT RUBRIC: v1.13
Lane: full, all three rounds
Outcome: request changes x4, then approve on round 5. Escalated to Cindy after the
second fix round; she ruled on the mechanism, the implementation was reviewed twice more.
Not merged by the loop — the visual baseline gate is hers, by her own instruction.
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 |
| 4 | watch the overflowStart/overflowEnd transition that disables the button |
the parent finding is closed. Round 4's block has a different parent: the Effect is right about the moment and wrong about the person |
| 5 | the same, plus a blur handler that forgets the person when they leave | approve. Both parents false; what remained was the record, not the mechanism |
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.
Cindy ruled the focus-moving Effect acceptable, narrowly, on six conditions. The author
implemented it: the Effect fires on the overflowStart/overflowEnd transition, moves
focus to the opposite arrow, and clears its tracker before focusing so it cannot fire
twice.
ARCHITECTURE went clear, and three prior findings died with it — the reduced-motion
regression, the snap case and the scrollend latency. Focus now lands within 2ms of the
button disabling, on every story that can reach an edge, and never passes through <body>
at all. Because the receiver is a Button, it also carries the shared 2px accent ring
rather than the scroll container's hairline, which quietly retired one of the accepted
findings on the ordinary path.
BLOCKS, and this one is worse than main rather than better: the focus tracker was
only ever cleared by a hand-off, so it kept naming a person after they left, and the
Effect's guard accepts <body>. Someone who touched an arrow, clicked back into the page,
and then reached an edge by any means — a swipe, a reflow, no press at all — had focus
pulled back onto the other arrow. On main they stay where they put themselves. It also
breaks the first of the ruling's own conditions: only the currently focused button.
Fixed by clearing the tracker on a blur the person caused and leaving the one the commit
caused alone, which is separable because React sets disabled before the browser blurs.
Driven both directions against a main control; all twelve hand-off arms unchanged.
Round 4's remedy was authored by the previous reviewing session, so nobody independent had
judged it. This round did, and the honest finding is that its stated justification was
wrong while its result was right. Round 4 argued the ordering assumption was proved
behaviourally — that if disabled read false at blur time no hand-off would happen at all.
Round 5 falsified that by construction: replace the guard with a constant true, the worst
the ordering could do, and every hand-off arm still hands off and all 37 unit cases stay
green. The reason is in the code round 4 was reading: the Effect clears the tracker itself
before it focuses the receiver, so the commit's own blur finds nothing left to clear. The
assumption is separately true — observed, not inferred, by reading disabled inside the
button's own focusout — but it is not load-bearing, which retires the Chromium-only
caveat on it.
The block itself is closed across seven arms and three routes: wheel, a real touch drag
driven through CDP, and a content reflow with no pointer at all, in both directions, LTR
and RTL, with main as the control each time. The hand-off is intact on all fourteen
stories, 0-2ms from the button disabling.
Two record-level notes were taken and fixed rather than argued: a test named for something
it could not test, and a PR body still telling the scrollend story. A fourteen-line
comment duplicated verbatim on both buttons was cut to one line each, with the reasoning
moved to the Effect's own comment.
One thing left unverified and named rather than dropped: React fires no blur when a
focused element is removed, so a consumer toggling hasButtons while someone stands on an
arrow would leave the tracker set through a door onBlur does not cover. Driving it needs a
scratch story; the reviewer judged the reachability too low to spend one.
- 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.
Two reasons, in order. The standing rule caps the loop at two fix rounds; both were spent
by round 3, and its block needed an Effect, which is a house block — a ruling rather than a
correction. Cindy ruled yes and asked for one clean verification loop. That loop was not
clean: it found the stale-tracker defect above, which is a regression against main and a
violation of one of her own conditions.
Her instruction for that case was to stop and report rather than invent another hand-off. The one-line guard was applied because it enforces her condition rather than changing the ruled mechanism, and because the read-only reviewer had already built and driven it across every arm — but the PR was left open for her, not merged.
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.