Review 5553
Review #5553 — useScrollableTabStop
cixzhang · maintainer · OPEN · view on GitHub
#5553 feat(hooks): useScrollableTabStop — a keyboard tab stop that appears only while a container actually scrolls by cixzhang (bucket: the maintainer; ENGOWNER)
8427b7e64f7a8a2fd368cf7f93988fcf8de38b03
LOOP VERSION: 1.4.1
AUDIT RUBRIC: 1.12
Gate 1 read both versions from the M5 source of truth. Loop 1.5.0 was published while this review was already running; it does not retroactively change this 1.4.1 record.
LANE: full WHY: new public hook, changed public observer semantics, ResizeObserver/MutationObserver lifecycle, keyboard/focus behavior, five existing component families, and an uncovered visible focus endpoint.
Clear. The remote head matched the requested SHA, all 16 changed files were reviewed, the working tree was clean, and no public GitHub or contributor-branch action was taken.
A Safari keyboard user cannot enter a scroll container with no sequentially focusable content, leaving everything below the fold pointer-only. The need is demonstrated by axe's serious scrollable-region-focusable rule, seven landed candidates named in the PR, issue #5207, and the open BottomSheet implementation in #5513.
VERDICT: clear
A scroll box watches its own size, its direct children's sizes, and direct child-list changes. While content exceeds an axis that the browser allows to scroll, the box gets a keyboard stop; when it stops scrolling, that stop is removed without dropping live focus. One shared size watcher fans notifications to independent features, while each caller removes only its own registration.
SOLUTION (2 decisions · ~341 runtime additions of 1,052 changed lines)
- Add a public callback-ref behavior and a pure DOM unit that conditionally owns
tabindex="0"for scroll containers. - Change the public shared-resize utility from one callback per element to fan-out, then migrate every core cleanup to callback identity.
Both decisions are stated and justified in the PR body.
VERDICT: BLOCKS — the tab-stop condition omits visible sequential-focus descendants, and the observer signals cannot maintain the promised “exactly while scrollable” state.
OWNER: the scroll container owns keyboard entry; the named hook owns the reusable behavior.
TIER 1: shared resize observer reused; the canonical focusable-descendant model exists in the overlapping #5513 branch but is not composed here.
TIER 2: none.
SEAMS: nested controls, class/style changes to overflow, consumer-authored tabIndex, callback-ref replacement, Strict Mode, SSR/hydration, child replacement, and direct-child fan-out.
BEHAVIOR UNIT: useNamedHook plus pure utility — lifecycle is isolated from React and directly testable.
| seam | driven result |
|---|---|
| static overflowing / fitting / clipping containers | works in Chromium at exact head |
| visible focusable child inside an overflowing container | fails: container keeps tabindex="0"; Tab stops on container, then child |
overflow: hidden → auto → hidden without geometry change |
fails both directions; state updates only after a later resize |
| focused container shrinks to fit | works: focus and tabindex="0" are held; stop clears after focus leaves |
enabled flips false while focused |
works: focus remains, managed attribute is removed, observers detach |
consumer writes tabindex="-1" after attachment |
note: a later measure removes the consumer value because managed remains true; docs prohibit dual ownership |
| Strict Mode callback-ref replay / unmount | works: two development attaches, matching cleanup, no observer survives |
| SSR → hydration | works: server emits no tab index, hydration logs no mismatch |
All existing in-repo observer consumers now unregister by callback identity. The exported one-argument unobserveResize(element) remains able to remove every independent callback; the PR body explicitly retains that compatibility path, and no repository caller uses it.
VERDICT: BLOCKS — the behavior is at the right hook tier, but its keyboard ownership and dynamic-scroll seams are incomplete.
No shipped component adopts the hook, so this release fixes no end-user path yet; builders gain a public primitive intended for seven landed components. #5513 is active work on the same user failure and already records the missing combined condition: actual overflow and no visible sequential-focus descendant. Existing users of the public resize utility get fan-out semantics; all repository consumers were migrated safely.
VERDICT: note — intentional groundwork only; the seven named component defects remain until adoption, with #5513 as the active first consumer.
const scrollRef = useScrollableTabStop({enabled: hasFixedHeight});
<div ref={scrollRef} aria-label="Activity">{children}</div>| change | public? | class | doc'd? | verdict | |
|---|---|---|---|---|---|
+ |
useScrollableTabStop(options?: UseScrollableTabStopOptions): React.RefCallback<HTMLElement> |
yes — root and ./hooks barrels |
seven landed scroll-container candidates; #5513 is a real eighth path | HookDoc + dense docs | finding: condition incomplete |
+ |
UseScrollableTabStopOptions.enabled?: boolean = true |
yes — root and ./hooks barrels |
optional-behavior hook precedent | JSDoc + HookDoc | ok |
~ |
observeResize(element, callback): void changes replacement semantics to callback fan-out |
yes — ./utils
|
shared observer system | JSDoc + changeset | note |
~ |
unobserveResize(element, callback?: ResizeCallback): void adds callback-scoped cleanup |
yes — ./utils
|
shared observer system | JSDoc + changeset | note: one-arg form remains unsafe for independent owners |
OSSIFICATION: the hook serves a demonstrated class and matches the established “behavior as hook” convention; useScrollOverflow returns React state and does not own a generic keyboard stop. The maintainer authored the new capability and its tier, so no separate human API hold remains. Removal later is breaking; shipping the incomplete descendant/transition contract would freeze the wrong promise.
VERDICT: BLOCKS — the public hook contract is broader than the behavior currently proven.
No production StyleX, theme target, token, variable, or style contract changes. Story-only styles use existing semantic tokens.
VERDICT: clear
BEHAVIOR: existing component behavior should remain unchanged except shared resize callbacks now coexist; all eight repository cleanup sites were migrated and focused tests pass. The new hook is opt-in, but its reachable dynamic states are wrong as described above.
API: additive hook/type and optional callback parameter; no removed or required surface. Same-element double registration changes observeResize behavior from replacement to fan-out.
VISUAL: no existing component pixels change; the new story gains a visible native focus treatment only on the overflowing region. Exact-head pr-visual reported status: skipped because 782 shots exceeded budget.
THEME: no change.
VERDICT: BLOCKS — the new opt-in behavior can leave an unreachable scroll region or a dead extra stop.
EFFECTS: zero new Effects. Existing Effects only changed cleanup to callback identity.
RENDER: the new hook writes the DOM from observer/ref callbacks and adds no React state or render pass. Strict Mode and hydration probes passed.
LISTENERS/OBSERVERS: one shared ResizeObserver for the page; one MutationObserver per enabled hook instance; root plus every direct child registered. Detach removes the listener, MutationObserver, queued frame, root registration, and child registrations. All existing consumers were checked.
LAYOUT: each logical measure reads four geometry fields and conditionally one computed style. In one browser delivery, resizing 20 / 50 / 200 direct children caused 20 / 50 / 200 computed-style reads. Every one of those calls also scans both the observed-child set and the live child collection, making the batch quadratic (at least ~800 / 5,000 / 80,000 child visits). Repro: behavior.cjs with the exact-head receipt, from the exact CI Storybook artifact.
BUNDLE: CI analysis reports +4.8 KB CJS / +1.2 KB gzip for core; no dependency change.
VERDICT: BLOCKS — a single resize batch remeasures once per direct child and rescans all direct children each time; the acknowledged N=200 case lacks a coalescing invariant/perf test.
VISUAL CHECK: manual frames required
WHY: exact-head pr-visual was green as a job but its artifact says status: skipped (782 shots exceeded the 240-shot budget), and the keyboard-focus endpoint is not rendered by the static story state.
| Sensor | Expected | Observed | Pass? |
|---|---|---|---|
| Build | exact head 8427b7e…; downloaded CI Storybook artifact |
same SHA recorded | yes |
| Story | hooks-usescrollabletabstop--overflowing-and-fitting |
exact id | yes |
| Theme / mode / direction | neutral / light / LTR | matched | yes |
| Viewport / media | 900×400@1; normal colors/motion; fine pointer; hover | matched | yes |
| Focused state | overflowing; tabindex=0; focused |
matched | yes |
| Control state | fits; no tabindex; unfocused |
matched | yes |
| Geometry | one visible 280×140 target in viewport | matched | yes |
| Settled/errors | fonts loaded; 0 animations; no Storybook/page errors | matched | yes |
| Overflowing keyboard stop receives focus treatment | Fitting region remains outside the tab order |
|---|---|
 |
 |
| sensor receipt | sensor receipt |
Both frames were opened during the review. Pixel comparison localizes all change to the 280×140 overflowing target: 1,385 pixels become accent blue (0,95,204) on focus; the fitting control remains visually unchanged. This is intentional evidence for the PR's stated keyboard-reachability goal. Capture receipt.
VERDICT: clear
Auto-covered: exact-head pr-a11y passed without a baseline diff, but its report scoped only AppShell/Chat/TabList/Table/Text and did not include the new hook stories. A separate exact-artifact axe run over all four hook stories produced 0 WCAG A/AA violations.
Checked in Chromium: Tab reaches the overflowing static region and skips fitting/clipping regions; RTL resolves on the target; focus survives shrink and enabled=false; the stop clears after focus leaves.
Blocking paths: an overflowing container with a visible button receives an extra generic stop before that button; a hidden→auto transition with unchanged geometry remains unreachable, and auto→hidden remains a dead stop, until another resize.
I18n: no production user/AT string added; story labels are test content. Directional CSS is untouched.
VERDICT: BLOCKS — two reachable keyboard paths violate the hook's stated “exactly while needed” contract.
| slot | verdict |
|---|---|
| PROBLEM | clear |
| SOLUTION | BLOCKS — incomplete ownership/signals |
| ARCHITECTURE | BLOCKS — descendant and dynamic-scroll seams fail |
| IMPACT | note — no production adopter yet |
| API | BLOCKS — public contract exceeds proven behavior |
| THEMING | clear |
| BREAKING | BLOCKS — opt-in keyboard regressions |
| PERFORMANCE | BLOCKS — quadratic resize batch |
| VISUAL | clear |
| A11Y & I18N | BLOCKS — extra/dead tab stops |
GOAL: not met — static overflow/fitting behavior works, but exact-head Chromium proves the stop is wrong for focusable descendants and for overflow-mode-only transitions. DISPOSITION: focusable-descendant condition → blocks now; overflow-mode transitions → blocks now; uncoalesced direct-child batch → blocks now; no production adopter → accepted as independently landable groundwork with #5513 active; one-argument public cleanup → accepted as an explicit compatibility tradeoff by the maintainer-author, with zero repository callers. ADVICE: bounded direction — preserve one invariant: the container owns a stop iff it is actually scrollable and lacks a visible sequential-focus descendant; react to every transition that can change that truth, and perform at most one logical measure per observer delivery. #5513 establishes the descendant half; the exact-head probes establish the other acceptance tests. AUTHOR CAN PROCEED: yes — add focused tests for visible descendants and overflow-mode-only transitions, coalesce a multi-child observer batch to one logical measure, and rerun the exact Chromium/scale probes plus visual receipt. WORST OUTCOME: “a hidden→auto transition with unchanged geometry remains unreachable” → request changes.
JUDGEMENT NEEDED: none — the maintainer authored the capability/tier; the remaining issues are reproducible correctness and performance defects.
Semantic verdict: request changes. GitHub action: COMMENT because the author cannot review her own PR.
- The condition ignores visible sequential-focus descendants.
→ a keyboard user stops on the generic container before the control that already provides access ·
packages/core/src/hooks/scrollableTabStop.ts:81 - Scrollability-mode changes are not observed.
→ a keyboard user cannot reach newly scrollable content, or hits a dead stop after scrolling is disabled ·
packages/core/src/hooks/scrollableTabStop.ts:159 - Every resized child triggers a full child rescan.
→ a 200-child batch performs 200 style reads and at least ~80,000 child visits in one update ·
packages/core/src/hooks/scrollableTabStop.ts:120
Thanks — the static overflow detection is careful, but this needs three fixes before it becomes public.
The hook adds a generic stop even when the scroller has a visible sequential-focus control, so Tab stops on the container before the control. #5513 already established the condition as actual overflow and no visible sequential-focus descendant.
Changing overflow alone leaves stale behavior: hidden → auto stays unreachable and auto → hidden stays tabbable until a later resize. Chromium reproduced both at this head.
One batched resize of 20/50/200 direct children also caused 20/50/200 style reads, while every pass rescanned all children. Could we cover the combined ownership condition, both overflow-mode transitions, and one coalesced measure per delivery before export?
[Reviewed by Robohands]
Public COMMENT length: 121 words (request-changes cap: 150).
-
packages/core/src/hooks/scrollableTabStop.ts:81— This still adds a generic stop when a visible sequential-focus descendant already provides access. -
packages/core/src/hooks/scrollableTabStop.ts:159— Class/style-only overflow changes never trigger this observer, so tab-stop state goes stale. -
packages/core/src/hooks/scrollableTabStop.ts:120— One batch remeasures per child; might need to coalesce before rescanning the set.
- 220 focused tests across the new units and every changed consumer passed; core typecheck and docs typecheck passed.
- SSR emitted no managed attribute, hydration logged zero errors, and Strict Mode's two attaches had matching cleanup.
- Current main has no drift in the 16 files; exact-head CI is green, while its visual artifact explicitly skipped comparison.
TIME total 40m setup 7m safety, fresh wikis, exact-head worktree, install; warm main reused: yes reading 13m kit, guidance, body, all 16 files, overlap and main drift measuring 14m focused tests, exact CI artifacts, 8 browser probes, 2 visual captures writing 6m draft + critic + rewrite waste 6m dev Storybook emitted an unexplained 404; replaced it with the exact CI artifact, and one headed static probe was cancelled
- Safari behavior; Playwright WebKit is blocked by managed-Mac policy.
- 220 focused tests across the new units and every changed consumer passed.
- Core typecheck and docs typecheck passed.
- Chromium exact-head probes reproduced both overflow-mode failures and the extra focus stop; the 20/50/200-child scale probe measured 20/50/200 style reads. Behavior receipt · repro.
- SSR emitted no managed attribute, hydration logged zero errors, and Strict Mode's attaches had matching cleanup. Receipt.
- Focus continuity passed for shrink-to-fit and
enabled=false. Receipt. - Exact-artifact axe over all four hook stories reported zero WCAG A/AA violations; RTL resolved on the target. Receipt.
- Exact-head CI was green, but the visual job's own artifact explicitly reported a skipped comparison.
Two critic passes were completed. Pass 1 kept the semantic request-changes verdict but rejected the draft because its closing ask left two keyboard acceptance criteria implicit and the completed frames had not reached the visual slot. The rewrite named all three outcomes, embedded exact-head visual evidence, and reduced the public comment from 138 to 121 words. Pass 2 found no R1–R15 violations and confirmed that every block had independent evidence.
Nothing was posted to the PR. The only public mutation from this run is this required fork-wiki review record and its evidence assets. The exact proposed GitHub action remains a COMMENT because the author cannot review her own PR.
- Remote head before and after cleanup:
8427b7e64f7a8a2fd368cf7f93988fcf8de38b03. - Current
origin/main:d4d4fc58b0bd2e21ad7ab23a492edcb4aafa4bc5; zero commits since the PR base touch any of the 16 reviewed files. - Dedicated worktree
review-5553-scrollable-tab-stop-r16: removed; registry entry removed. - Pre-removal status: clean;
git diff --checkpassed; changed-file count: 16. - Public GitHub actions: none.
#5553 fix(core): make text-only Bottom Sheets keyboard-scrollable by cixzhang (bucket: the maintainer)
e59a09909a031daf7c0a5c50024365a2a94472f8 <- every claim below was verified at this commit
LOOP VERSION: 1.6.0 AUDIT RUBRIC: 1.13
LANE: full WHY: promoted at STEP 1 — a public hook export, an unresolved review of ours from 40 minutes ago, a shared internal helper touched beside a released focus hook, and more than one decisive check (tests, ring geometry, forced colors). Any one forecloses fast.
This run opened on 677052136d52 and re-verified on e59a09909a03, which landed
while the draft was being written. At 677052 the sheet's new focus ring drew at
outline-offset: 3px and every side of it fell outside the sheet's
overflow: hidden box — a blocking finding, with the arm proving it banked at
assets/pr-5553/gate-10/armF-outset.diff. e59a0990 is exactly that two-line inset. The
block is deleted rather than carried, and the verdict flips to approve, so this
is a fresh draft and not an edit of the old one.
What I claimed. That this PR narrowed FOCUSABLE_SELECTOR from
[tabindex]:not([disabled]) to [tabindex]:not([tabindex="-1"]):not([disabled]),
and that useFocusTrap — whose own file is unchanged — silently inherited it.
What is true. The literal is byte-identical at base and at head.
git show 89abbb3edc1:packages/core/src/hooks/focusableSelector.ts | grep 'button:not'
git show e59a0990:packages/core/src/hooks/focusableSelector.ts | grep 'button:not'
diff <(…base…) <(…head…) → identical
Within the branch the constant moved twice and came back: 9e188f8 dropped
:not([tabindex="-1"]), 677052 restored it. Net against base: nothing.
useFocusTrap sees exactly what main ships.
How I got it wrong. Two reading errors, compounding:
- I ran the right command —
git diff <base> <head> -- focusableSelector.ts— and misread a context line as an addition. The selector line came back with a leading space, not a+. R3i's whole subject is "does base already do it?", and the answer was in the output I already had. - I then read commit
677052's patch in isolation, where the line genuinely is-/+, and treated its-side as base. A single commit's patch is a claim about its parent, never about the merge-base.
What it invalidates. ARM C is retracted as evidence: it reverted the selector to the branch's intermediate spelling, not to base, so its 489 green tests never tested the claim they were cited for. The two-sided Chromium drives stand — they found no delta, which is what an unchanged constant predicts, and they are the reason this was a nit under an approve rather than a block.
What it cost, and what caught it. The wrong nit was posted and withdrawn on
the PR within two minutes, in the summary and at its anchor. A colleague caught it
by running the base-to-head diff, which is the check R3i already names. Nothing in
this loop's own passes caught it: critic 1 and critic 2 grade the artifact and are
blind to the code by design, so a false premise stated consistently sails through
both. That is the gap worth writing down — R16g asks a [BLOCKS] finding for a
second, independent confirmation, and this was a nit, so it was never owed one.
The cheapest fix is to extend R16g's cheapest row to every finding about a shared
or pre-existing line: before claiming a line changed, diff it against the
merge-base and paste the result. One command, and it would have killed this at
the draft.
Five artifacts, all ours, all signed [Reviewed by Robohands]; no colleague to
contradict, and a standing duty to walk each point at this head.
-
Review COMMENTED 2026-08-26T23:01:25Z (
8427b7e) — three asks "before export": the combined ownership condition, both overflow-mode transitions, one coalesced measure per delivery. All three were satisfied at9e188f8and none of the three files has moved since (git diff 9e188f8..e59a0990 -- scrollableTabStop.ts useScrollableTabStop.ts sharedResizeObserver.ts→ empty). -
Comment 2026-08-26T13:00:19Z, two nits. Test-plan wording: satisfied — the body's Testing section names areas, not counts. TabList comment: satisfied, see 4.
-
Cindy, 2026-08-27T03:44:17Z: "do not land this as hook-only groundwork … wire the first real in-tree consumer." Satisfied at
9e188f8and still true here. -
Review COMMENTED 2026-08-27T17:59:02Z (
9e188f8) — our gate-9 review. Point by point at this head:-
finding 1, the focus-trap consolidation is unrecorded and turns
testred — satisfied.useFocusTrap.tsis byte-identical to the PR's base (git diff 89abbb3edc1 e59a0990 -- useFocusTrap.ts→ empty): its ownisVisiblyFocusableandgetFocusableElementsare back, andfocusableSelector.tsno longer exportsgetFocusableElements. All five previously red files pass. One line of that change survives — see the CORRECTION above — I thought a fragment of that change survived inFOCUSABLE_SELECTOR, and it does not; the constant matches base. -
finding 2, the BottomSheet observer is no longer identifiable —
satisfied.
BottomSheet.test.tsxnow narrows the lookup to the observer watching both the input and the sheet, and the behavioural assertion (scrollTop448) is untouched. Green. -
finding 3, the stop's ring is outside
focusOutline.stylex— satisfied, then over-corrected, then fixed. Taking the shared ring at its default+3pxoffset put it outside a clipping parent;e59a0990insets it. Measured below. -
inline,
TabList.tsxclaims the shared observer keeps one callback per element — satisfied. Rewritten to "the wrapper is observed because it owns the available width; the scroll strip can keep its own independent observer through useScrollOverflow", which is true once the observer is multi-callback.
Our own ask produced the over-correction, and the review says so. Gate 9 asked, correctly, that the ring be drawn from the shared file, and did not say it had to stay inside a parent that clips. The previous head's browser ring was at least painted; the first fix for our finding painted nothing. That is worth recording against the loop, not against the author.
-
finding 1, the focus-trap consolidation is unrecorded and turns
origin/main is e6507a95fb13cae83f94e2d4b58009eaf584f1c6; the PR's base is
89abbb3edc1. Two commits separate them — fix: stabilize default resize snaps (#5276) and Add trusted visual acceptance recovery dispatch (#5600).
git log 89abbb3edc1..origin/main -- <the PR's 22 files> is empty: no file
this PR changes has moved on main, so nothing here reviews dead code.
mergeable: MERGEABLE.
Head repository is facebook/astryx, not a fork — trusted. Checked anyway across
the 22 changed files: no package.json script change, no dependency or version
change, no off-registry lockfile resolution, nothing under .github/workflows/,
no new *.sh/*.mjs/*.cjs under scripts, tools or .husky, nothing
reading env, ~/.npmrc or ~/.ssh.
WHY 1: A keyboard user presses Tab inside a text-only Bottom Sheet and focus leaves the sheet's content entirely — no key moves the text. WHY 2: They can read the first screenful and cannot reach the rest, and there is no route to it that does not involve a pointer. WHY 3: A Bottom Sheet is the surface this system hands people for release notes, terms and detail views — content whose whole job is to be read. A reading surface you cannot scroll has not done the one thing it exists for.
USER-FACING PROBLEM: A keyboard user opens a text-only Bottom Sheet, reads to the bottom of the visible area and is stuck: Tab skips the scrolling region, so the rest of the sheet is unreachable. Issue #5207 is still open on current main. PROBLEM SEVERITY: broken task — the person cannot complete reading, cannot recover, and has no pointer-free path.
VERDICT: clear
A scroll container the keyboard cannot reach only matters while it is actually scrollable and holds nothing focus can land on. So the container becomes a focus stop under exactly those two conditions and stops being one the moment either goes false. It watches its own size, its children's sizes and its own subtree, so the answer stays current without anything re-rendering. A region that already has a button or a link keeps that control as its keyboard route instead of gaining a second, generic stop in front of it.
SOLUTION (4 decisions · 406 runtime lines of 1308 added, across 22 files)
Counted, not estimated: gh api repos/facebook/astryx/pulls/5553/files --paginate
summed over packages/*/src/** excluding *.test.*, *.doc.mjs and stories →
406 of +1308/−65. The rest is stories, tests, docs and a changeset.
- the ownership condition — a container owns a stop iff it is scrollable AND has no visible sequential-focus descendant [the fix]
- the shared resize observer becomes multi-callback;
unobserveResizetakes the callback to remove [enables 1 · recorded in the body] - BottomSheet adopts the hook on its scrolling body [Cindy's 03:44 direction]
- the adopted body draws the shared focus ring, inset so the sheet's own
overflow: hiddendoes not clip it [answers our gate-9 finding 3]
1 and 3 must ship together — 3 is what proves 1. 2 is recorded in the body's API section. 4 is recorded in the changeset and the body. Every decision traces to a stated problem; there is no piggyback. A fifth decision was drafted here and withdrawn — see CORRECTION.
BURDEN: low — zero React Effects, zero React state, one hook export, one
element-scoped focusout listener, one MutationObserver per enabled instance, and
registrations on the ResizeObserver the repo already shares. The behavior is a
plain DOM function with its own non-React test file.
BURDEN MATCH: proportionate — a broken task, answered with a ref callback that
costs no render.
VERDICT: clear — four decisions, each tracing to a stated problem
OWNER: the scrolling element, through a ref callback; BottomSheetPanel owns the
lifecycle for the sheet's body. The ring is owned by the styling layer that owns
the look (focusOutline.stylex composed at the body), not by the behavior hook —
which is where R7 says a visual variant belongs.
TIER 1: reuses sharedResizeObserver, the focusableSelector helpers and
focusOutlineProps; adds no parallel mechanism.
TIER 2: none.
SEAMS: useMergedRefs composition with useSheetGestures' body ref. The hook
returns a React 19 ref callback whose return value IS its cleanup, so flipping
enabled detaches through the same path unmount does. BottomSheet exposes no
render prop or replaceable surface for its body, so the behavior cannot be
composed out from under its owner.
BEHAVIOR UNIT: pure utility — attachScrollableTabStop in scrollableTabStop.ts,
tested in scrollableTabStop.test.ts without React. Zero Effects, so R35's
two-Effect state-machine threshold is not reached.
| seam | driven result |
|---|---|
built-in sheet body (core-bottomsheet--keyboard-scrollable-text) |
works — tabindex="0", one Tab lands on the body, ArrowDown ×3 moves scrollTop 0 → 113 |
body holding a consumer control (--capped-height-with-long-content) |
works — no container stop; Tab lands on "Done", which rings at the shared +3px offset |
merged ref beside the gesture ref (BottomSheetPanel.tsx:497) |
works — both attach; the gesture body ref still receives the element |
the trap that reads the changed constant (useFocusTrap via usePopover) |
no delta — driven two-sided, see BREAKING · Behavior |
The ownership question gate 9 raised is closed, and closed completely.
useFocusTrap.ts is byte-identical to the PR's base, and so is the one thing it
imports: FOCUSABLE_SELECTOR's literal is the same string at base and at head
(diff <(git show 89abbb3edc1:…) <(git show e59a0990:…) on that line → identical).
The branch removed :not([tabindex="-1"]) at 9e188f8 and 677052 put it back,
so the trap ends where it started. Nothing shared changed under it.
VERDICT: clear — the trap's focusable model is base's, in the file and in the constant it reads
End users. Nothing changes until someone opens a Bottom Sheet whose body
overflows and holds no button, link or input. Those people get what they did not
have: a Tab stop on the sheet body, arrow-key scrolling, and — since e59a0990 —
a focus ring they can actually see, in the same 2px the Button beside it draws.
Everyone else's sheet is unchanged: the capped story's tab order, first stop and
outer box are identical to main.
Builders. One new export. Nothing existing changes meaning at compile time:
unobserveResize grew an optional second argument and every in-repo caller was
updated in this diff.
What landing this newly exposes. 29 files in packages/core/src declare a
scrollable box; 1 adopts the hook. The person who can now scroll a Bottom Sheet
still cannot scroll a LayoutContent, a CommandPalette list or a DropdownMenu.
That gap is not new and it is not this PR's debt — Cindy asked for one real
adopter and this is it — but after the merge the inconsistency is visible in a way
it was not before: the same keyboard user gets a route in one scroll region of an
app and not the next.
VERDICT: note — 1 of 29 scroll containers, by design
// packages/core/src/BottomSheet/BottomSheetPanel.tsx:497 — the real call site
const scrollableTabStopRef = useScrollableTabStop();
const scrollBodyRef = useMergedRefs(bodyProps.ref, scrollableTabStopRef);| change | public? | class | doc'd? | verdict | |
|---|---|---|---|---|---|
+ |
useScrollableTabStop(options?: {enabled?: boolean}): RefCallback<HTMLElement> |
yes — core/hooks barrel |
29 core files declare a scrollable box; the docblock names LayoutContent with isScrollable as the next fit |
useScrollableTabStop.doc.mjs, docs + docsDense — the pair useScrollOverflow.doc.mjs carries |
ok |
+ |
UseScrollableTabStopOptions |
yes — exported beside the hook | — | documented inline on the interface | ok |
~ |
unobserveResize(element, callback?) — optional second argument |
internal (utils, not the package barrel) |
mirrors observeResize's shape |
file docblock and example updated | ok |
~ |
focusableSelector + hasFocusableDescendant and its private visibility filter; FOCUSABLE_SELECTOR's value is unchanged from base
|
internal — the file states "Not exported from the public barrel" | — | file docblock rewritten | ok |
− |
focusableSelector.getFocusableElements (added earlier in this branch, removed at 677052) |
internal, never released | — | — | ok |
OSSIFICATION, per line. The hook serves a class, not a case: 29 core files declare
a scrollable box, and it is not a new concept — it is the DOM's own
scrollable-region-focusable requirement wrapped in a ref. Being wrong costs one
hook export deprecated with no runtime contract to unwind, because it writes one
attribute and owns its own removal. patch matches precedent:
#5266 added useMergedRefs on a
patch changeset. The focusableSelector additions are internal, so they ossify
nothing. hasFocusableDescendant is new and has one consumer, the new hook;
FOCUSABLE_SELECTOR, which useFocusTrap also reads, ends the branch on base's
exact string.
VERDICT: clear
No theme target added, removed or renamed. No token substitution. No astryx-*
class added or dropped.
grep -nE "#[0-9a-fA-F]{3,8}|rgba?\(|hsla?\(|boxShadow|light-dark\(" over the
changed runtime files → 0.
The gate-9 theming note is closed, and measured closed. The sheet body now
composes focusOutlineProps.focusVisible, so --focus-outline-* reaches it:
light resolves 2px solid rgb(38,38,38), dark resolves 2px solid rgb(235,235,235)
— the two halves of --focus-outline-color: light-dark(#262626, #ebebeb), read off
the focused element in both modes. It is the 58th file in packages/core/src to
draw from focusOutline.stylex rather than the only one that did not.
The outlineOffset override is not a departure from that file; the file sanctions
it in its own docblock: "A component may still override the OFFSET (a ring that
must sit inset, or clear of a field border); width, style and color are not
restated anywhere." Width, style and colour are not restated here.
VERDICT: clear
BEHAVIOR: no, and this is the row I got wrong first — see CORRECTION.
useFocusTrap.ts is byte-identical to base and FOCUSABLE_SELECTOR's literal is
byte-identical to base, so the focus trap's model of "focusable" is exactly what
main ships. Within the branch the constant moved twice — 9e188f8 dropped
:not([tabindex="-1"]), 677052 restored it — and net against base that is zero.
The two-sided drive below was run while I still believed the constant had changed. It stands as evidence and it is worth keeping, because it is the reason the wrong claim never became a block: driven in real Chromium, main against head, on the two shapes that could have shown a delta, and nothing moved — which is exactly what an unchanged constant predicts.
| case | main | head |
|---|---|---|
core-dialog--with-subtitle — focus on open |
h2[tabindex="-1"] "Edit User Profile" |
same |
| — Tab from "Save Changes" | BODY | same |
| — Shift+Tab from "Close" | BODY | same |
core-dropdownmenu--default — old-selector matches inside the menu root |
4 | 4 |
| — new-selector matches | 0 | 0 |
| — focus on open | div[role=menuitem][tabindex="-1"] "Edit" |
same |
| — after Tab | menu open, focus BODY | same |
Reproduced against both the current baseline and exact-head local Storybook. Dialog uses the native top layer, not this trap; DropdownMenu handles Tab itself. No behavioural delta reproduced — as expected once the constant is known to be unchanged.
ARM C (assets/pr-5553/gate-10/armC-revert.diff) is retracted as evidence. It reverted
the selector to [tabindex]:not([disabled]), which is the branch's intermediate
spelling and not base, so its 7 files / 489 green tests say only that the
intermediate state also passes. It never tested the claim I made with it.
API: no. Every unobserveResize caller in the repo was updated in this diff and
the new argument is optional, so no external call site changes meaning.
VISUAL: yes, and it is now correct. A focus ring appears on the sheet body where
no focus state was reachable before. Nothing else moves and nothing grew: the
body's box is 1, 274.6, 418 × 445.4 and the sheet's 0, 273.6, 420 × 494.4 in
every arm — head, ARM D (previous head), ARM F (offset reverted), light, dark,
forced colors. An outline takes no layout space, and that is measured here rather
than asserted from the spec, so R15d's growth question is answered no.
THEME: no — every existing target, token and override still resolves, and the one ring that did not reach them now does.
VERDICT: clear — the only behaviour change is the intended one; nothing shared moved against base
EFFECTS: zero added, changed, kept or moved React Effects.
useScrollableTabStop is a useCallback ref whose return value is its cleanup;
BottomSheetPanel adds no effect and no state. Nothing sets state, so there is no
second render pass — growing the content adds the tab stop with no re-render at
all. The mandatory Effect-cost gate does not fire because the count is zero, and
the count is said out loud rather than left to silence.
RENDER: no reachable change. The attribute is written from a DOM callback.
LISTENERS/OBSERVERS, per enabled instance: one focusout on the element
(scrollableTabStop.ts:122, removed at :153), one MutationObserver (:143,
disconnected at :152), one shared-RO registration for the element (:119) and
one per direct child (:90), each unregistered by callback identity at
:158–:161. Nothing on document or window; the whole set is gated by
enabled, so a container that cannot scroll is never observed. R18f satisfied.
LAYOUT: one getComputedStyle per measurement, and only after the overflow check
has already gone positive (:59–:64) — a container that fits never reads style.
The coalescing number is carried, not re-measured: gate 9 drove one batched
resize of every direct child and read a flat 2 container style reads at N = 20 /
50 / 200 (story
hooks-usescrollabletabstop--only-what-can-actually-scroll), and
git diff 9e188f8..e59a0990 -- packages/core/src/hooks/scrollableTabStop.ts is
empty, so the measured bytes are the shipped bytes. Nothing in 677052 or
e59a0990 touches a measurement path.
BUNDLE: no new dependency. The new head adds one StyleX declaration.
VERDICT: clear
VISUAL CHECK: manual frames required
WHY: the PR body makes a positive appearance claim — "the same themeable focus
outline as every other core focusable surface" — and the state it claims is a
focused sheet body, which no story renders: :focus-visible needs a key
press, and the snapshot jobs render stories at rest. Stable visual regression is
green on 677052 and there is no Visual acceptance job on either head, so no
visual job covers the changed endpoint. That promotes the decision to manual
frames.
SENSOR RECEIPT — the frames below, identical on every sensor except Build and the intended state:
| Sensor | Expected | Observed | Pass? |
|---|---|---|---|
| Build |
e59a09909a031daf7c0a5c50024365a2a94472f8, isolated exact-head local Storybook |
same (git rev-parse) |
yes |
| Story |
core-bottomsheet--keyboard-scrollable-text (control: --capped-height-with-long-content) |
same | yes |
| Theme | global astryxTheme:neutral → rendered data-astryx-theme="neutral"
|
same | yes |
| Color mode | global colorMode:light → color-scheme: light (dark arm: dark) |
same; surface rgb(255,255,255) / rgb(38,38,38)
|
yes |
| Direction | global direction:ltr → computed ltr on the subject |
same | yes |
| Viewport/media | 420×720 @1 DPR, reduced-motion off, fine pointer, hover; forced-colors off except the A14 arm | same | yes |
| Rendered state |
tabindex="0" present and focus ON the body (control: absent, focus on "Done") |
same, every arm | yes |
| Subject geometry | exactly 1 scrolling body, visible, non-zero box, inside the viewport | 1 · x 1 y 274.6 · 418×445.4
|
yes |
| Settled render | fonts loaded, 0 running animations, no Storybook error screen, no page error | pass; the only 4xx is /favicon.ico, which storybook dev does not serve and the static baseline does — proven the sole 4xx by a targeted browser check; every other console error still fails the sensor |
yes |
One sensor expectation is authored from a platform fact, and it is worth naming
so it does not read as laundering. The forced-colors arm expects
color-scheme: light dark, not light. That is the UA's answer, not the page's:
about:blank, with no author styles at all, reports exactly the same string the
moment forced-colors is emulated in an unstyled browser control check. I checked that
before writing the expectation down, rather than copying a failed run's output
back into it.
ARM F — the inset reverted (assets/pr-5553/gate-10/armF-outset.diff, 1 file, −2, no unrelated delta) |
Head e59a0990
|
|---|---|
 |
 |
2,552 of 190,680 pixels change between them, 2,098 of them rgb(38,38,38) —
the light half of --focus-outline-color. Computed from the PNGs themselves.
Why: the sheet computes overflow: hidden at 0, 273.6, 420 × 494.4, and the
body sits 1px inside it. At +3px the ring's box is −4 … 424 across a 0 … 420
sheet with its top edge 2px above the sheet's — clipped on all four sides. At
−2px it is 1 … 419 by 274.6 … 720, inside both the sheet and the viewport.
Reproduced in the isolated exact-head local Storybook.
2px solid rgb(235,235,235) at −2px, against a rgb(38,38,38) surface: the
dark half of the same token. On the previous head this ring was Chrome's own blue
in both modes.
forced-colors: active, :focus-visible true, ring 2px solid rgba(5,0,73,0.8)
at −2px — the UA substitutes a system colour and the ring survives. Gate 9 left
A14 not looked at because the ring was about to change; it has changed, so this
is the check that deferral owed.
core-bottomsheet--capped-height-with-long-content: no container stop, Tab lands
on the "Done" Button, which rings 2px solid rgb(38,38,38) at the shared +3px.
One frame proving two things — the condition does not fire where a control exists,
and the inset is the sheet body's local override rather than a change to the
shared offset.
Every difference, labelled:
| difference | intentional? | source |
|---|---|---|
| a focus ring appears on the sheet body | intentional | the PR's own sentence: "A text-only sheet can be reached and scrolled with the keyboard" — a reachable element must show focus |
the ring is 2px solid var(--focus-outline-color) rather than the UA's 1px auto
|
intentional | the changeset: "with the standard themeable focus outline" |
the offset is −2px where the shared default is +3px
|
intentional |
focusOutline.stylex.ts: "A component may still override the OFFSET (a ring that must sit inset, or clear of a field border)", and the landed precedent at TransferList.tsx:228
|
| the PR body still says the ring has a "3px offset" | unintentional — finding 2 | no source; the head ships −2px
|
| nothing else in the frame moves | intentional | the PR's "no intended geometry, color, or layout change" — confirmed: identical body and sheet boxes in every arm |
On publication. These frames are archived with this wiki round. They were not embedded in the PR review, whose approve text makes no appearance claim. The author's own published pair matches the independent measurement here.
VERDICT: clear — the ring is drawn from the shared file, reaches both modes, and survives forced colors
REMEDY SEARCH: not triggered — no proven visual defect
Recorded, because one ran earlier in this session and its outcome is why the head
moved. At 677052 the ring was a proven visual defect:
TRIGGERED BY: the sheet body's focus ring drew outside the sheet's clipping box on all four sides
ELAPSED: 3m
OUTCOME: existing-pattern
EXISTING PATTERN: `TransferList.tsx:228` and `ChatEmojiPicker.tsx:156` —
`outlineOffset: {default: '0', ':focus-visible': -2}`, each with a comment
giving the same reason; sanctioned by `focusOutline.stylex.ts`'s own docblock
REQUESTED BY: none — no extension
EXTENSION ELAPSED: 0m
HUMAN HANDOFF: not applicable
e59a0990 ships that pattern, so at this head there is nothing left to search
for.
§1, the automated half. pr-a11y is green on 677052 and
.github/a11y-baseline.json is untouched by the diff — no silence bought. pr-rtl
green on 677052. Both are queued on e59a0990, whose only delta is one StyleX
outlineOffset declaration. No disabled={isLoading} in the diff. No input ARIA
wiring touched. Worth saying once: axe does not test whether a focus ring is
visible, so pr-a11y was green on the head where the ring painted nothing —
green there is not evidence about A15.
§1, the half that needs a browser — all driven in Chromium against the exact-head local Storybook at 420×720:
| check | result |
|---|---|
| A4 the keyboard reaches the region | checked by me — one Tab from the sheet's programmatic focus target lands on the body |
| A4 the arrow keys answer the stop | checked by me — ArrowDown ×3 moves scrollTop 0 → 113. This is what makes the stop honest rather than dead |
A5 focus is never dropped to <body>
|
checked by me — the attribute is removed only when the element is not activeElement (scrollableTabStop.ts:74), and a focusout re-measures afterwards |
| A5 a focusable element is not removed to express state | checked by me — the stop is added and removed, never while it holds focus |
| A15 the ring is visible, in every theme, and lives on the owner | checked by me — now passes. Light 2px solid rgb(38,38,38), dark 2px solid rgb(235,235,235), both at −2px and both inside the clipping sheet. It was failing one commit ago, and it is the reason this run re-verified rather than posting |
A14 painted state survives forced-colors: active
|
checked by me — passes, 2px solid rgba(5,0,73,0.8) at −2px, :focus-visible true |
| A1 the stop has an accessible name |
recorded, not raised. The focused body has no aria-label, aria-labelledby or role — read off the live DOM, all three null. The hook's docblock asks adopters for one; all four of its own stories pass aria-label and the in-tree adopter does not. The sheet is a role="dialog" named "Release notes", so the user is inside a named surface and the region's content is the reading. I could not write the → line, so it is not a finding (R16c) |
| A13 entry/exit animation | not applicable — the hook animates nothing |
§9 — strings. No user-visible or AT-facing string is added, so no catalog key
and no ICU is owed. grep -nE "useTranslator|t\('@astryx" over the changed runtime
files → 0. packages/core/locales/en.json untouched, correctly.
§9 — direction. The hook is direction-neutral by construction: it reads
overflowX/overflowY and scrollWidth/scrollHeight, never a physical side,
and writes one attribute. outlineOffset is direction-neutral too. No logical or
physical inset property is added anywhere in the diff, so I8–I12 cannot be reached.
VERDICT: clear — reachable, answers the arrow keys, and rings visibly in light, dark and forced colors
| slot | verdict |
|---|---|
| PROBLEM | clear |
| SOLUTION | clear |
| ARCHITECTURE | clear |
| IMPACT | note — 1 of 29 scroll containers, by design |
| API | clear |
| THEMING | clear |
| BREAKING | clear |
| PERFORMANCE | clear |
| VISUAL | clear |
| A11Y & I18N | clear |
Read against the bodies, not the labels. Three slots carried a note that was
one thing wearing three labels — the FOCUSABLE_SELECTOR edit — and reading
them together is what exposed it as one claim rather than three. It then turned
out to be one claim that was false. Seeing the compounding did not save it; a
second look at base did. That is the lesson this run pays for, recorded in
CORRECTION.
I looked for the promotion in the other direction too and did not find it. The A11Y and VISUAL bodies contain no sentence in which a person is stuck, cannot reach something, hears nothing or loses their place — one commit ago they both did, which is exactly why the check is worth running rather than assuming.
GOAL: met — the stated problem is fixed and driven. A text-only Bottom Sheet takes
tabindex="0", one Tab reaches it, three ArrowDowns move scrollTop 0 → 113 in
Chromium at this head, and the focus that lands there is now visible in light,
dark and forced colors. A sheet whose body holds a control is identical to main in
tab order and outer box. The five test files gate 9 blocked on pass, with 616
tests green across the nine files this diff can reach.
DISPOSITION — no negative finding survives, and both that were drafted are recorded rather than dropped:
- drafted finding 1, a
FOCUSABLE_SELECTORnarrowing → withdrawn, after posting. It was false: the literal is byte-identical at base and at head. It was posted, then withdrawn on the PR in the summary and at the anchor within two minutes. See CORRECTION. - drafted finding 2, the PR body's stale "3px offset" → withdrawn before posting. Real when the draft was written; the body was corrected while this ran and now reads "inset 2px so the sheet's clipping edge cannot hide it". R14d deletes a finding the head already fixed rather than thinning it.
Nothing is blocks now and nothing is accepted: the verdict is approve and
there is no surviving negative finding to dispose of.
Two prior-round items resolved to nothing and are recorded where they were noticed
rather than here, because DISPOSITION takes numbered findings and nothing else:
A1's missing accessible name (no writable → line, so not a finding), and
1-of-29 adoption (Cindy's own direction of 2026-08-27T03:44, quoted above — not an
acceptance I gave in secret).
BREAKING's non-"no" axes (R16d). BEHAVIOR is "no" against base. VISUAL is "yes" and is not worth raising: a focus ring now appears on the sheet body where no focus state was reachable before, which is the point of the change, sourced to the changeset's "with the standard themeable focus outline", and measured to move nothing else — identical body and sheet boxes in all six arms.
ADVICE: proven existing pattern — for the ring, TransferList.tsx:228 and
ChatEmojiPicker.tsx:156 both inset the shared offset for exactly this reason,
and focusOutline.stylex's own docblock sanctions it. The head already applies
it, so the advice is a citation for the record rather than an ask.
AUTHOR CAN PROCEED: yes — nothing is asked of her. Every gate-9 finding is satisfied at this head, both drafted nits are withdrawn, and no decision here is anyone's but hers, which she has already made.
WORST OUTCOME: none found. No slot body contains a sentence in which a person is stuck, cannot reach something, hears nothing, loses their place or gets something wrong. One commit ago the A11Y and VISUAL bodies each held one — a keyboard user with no visible focus on the region they had just reached — which is exactly why the gate is worth running rather than assumed. Compatible with approve.
JUDGEMENT NEEDED: none — the capability question was settled by Cindy's own direction on 2026-08-27, and everything else here is a reproducible fact against the existing contract.
Semantic verdict: approve. GitHub action: COMMENT, because GitHub refuses
APPROVE and REQUEST_CHANGES from a pull request's own author — so the comment
says which verdict it is.
No surviving findings. Both drafted ones are withdrawn and recorded above; the withdrawal of the posted one is on the PR, in the summary and at its anchor.
Thanks — approve from me. outlineOffset: -2 was the missing piece, and the five red files pass. Withdrawing my selector nit: that line matches base, I misread the diff.
[Reviewed by Robohands]
Posted text, then the anchored line as read at head (R14e — banked here so the next check is a string comparison, and not posted).
-
packages/core/src/hooks/focusableSelector.ts:17— "Withdrawn — this matches base; the negative-tabindex exclusion was already there. I misread the diff" · anchored line:'button:not([disabled]), a[href], area[href], input:not([disabled]), select:not([disabled]), textarea:not([disabled]), [tabindex]:not([tabindex="-1"]):not([disabled]), [contenteditable]:not([contenteditable="false"]), audio[controls], video[controls], iframe, details > summary:first-child';· inside the hunk@@ -2,24 +2,57 @@
- The
enabledgate, thefocusoutre-measure and the detach path were each driven again at this head and are each correct. - The changeset's
patchtier matches #5266, anduseScrollableTabStop.doc.mjscarriesdocs+docsDense, so the hook-doc build gate from #5109 is satisfied. -
getFocusableElementscame back intouseFocusTrap.tsbyte-identical to base, which is the cleanest possible answer to gate 9's ownership question and needed no sentence in the comment.
TIME total 34m setup 6m reading 5m measuring 14m writing 9m waste 4m — two probe-authoring mistakes were corrected and rerun before the final record
-
Safari. Playwright WebKit is blocked by managed-Mac policy. It still matters
here: Safari 17.0–17.3 is the only shipping browser without
Element.checkVisibility, whichhasFocusableDescendantfalls back from. -
Nothing on CI, in the end. All 22 checks completed green on
e59a0990before this closed —test,pr-a11y,pr-rtl,Stable visual regression,build,lint,smoke-testand the rest;initializecancelled and four gating jobs skipped, which is the normal shape here.mergeStateStatusisBLOCKEDon the review gate alone, which GitHub will not let the author clear herself. -
Nothing about
FOCUSABLE_SELECTOR, in the end. I spent two probes and an arm on a change that never happened; the honest entry is that the claim was mine, not the diff's, and the CORRECTION above carries it.
- Review: semantic approval at
e59a099 - Event:
COMMENTED, because GitHub does not allow a PR author to approve their own PR. - CI: all 22 checks completed green at the reviewed head.
- Public review body: 29 words, byte-for-byte from the
REVIEWblock above. - Final exit: semantic approval; author owns the merge decision.