-
Notifications
You must be signed in to change notification settings - Fork 0
Review 5513
jiunshinn · open · view on GitHub
Verdict: request changes (drafted — no public action taken)
HEAD REVIEWED: a30ff297a8e31123c1dfa65ea6a5cf3948686929. Source, tests, and behavior findings below refer to this exact commit. Merge-base 41494fcd41310c819f0501df2e798235c4b41135.
Versions. Review Loop 1.4.0 · Component Audit Rubric 1.12.
Bucket: collaborator. The repository permission endpoint currently reports read, but jiunshinn is a recurring contributor with more than twenty merged Astryx PRs and the request identifies them as a collaborator. Apply the collaborator/internal posture: they can follow up; no Discord clause and no full-review link in the public text.
LANE: full. Mandated for this run and independently required: this changes keyboard/focus behavior across BottomSheetPanel, the shared focusable-element model, and useFocusTrap; it adds state, a layout Effect, and a subtree MutationObserver. The fast lane is unavailable.
STEP 0: clean. The seven changed files are source, tests, one story, and one changeset. No package scripts, dependency/lockfile changes, workflows, shell/build scripts, environment reads, credential paths, or build-time network calls changed. The exact fork head was safe to check out and run.
Prior review: none. GitHub returned no review submissions and no inline review comments. Nothing is being extended or contradicted.
Main did not invalidate this. Current main advanced past the merge-base, but none of the six existing files this PR changes moved on main; the changeset is new. GitHub reported the head mergeable at gate 1. One adjacent system PR is open: #5553 defines conditional scroll-container tab stops and explicitly requires real overflow; it has not landed and does not make this PR's local correctness checks disappear.
A text-only BottomSheet can overflow without exposing any sequential focus target. A keyboard user cannot reach the scrolling body on engines that do not implicitly focus scroll containers, so Arrow/Page Down cannot reveal content below the fold. The reported issue is real, already classified serious by axe, and the new open-by-default story exercises the missing state.
VERDICT: clear
The sheet starts with a conservative server-rendered body tab stop, then checks its rendered descendants before paint. If it sees a focusable descendant it removes the body stop; if it does not, it keeps the body reachable. A subtree observer repeats that decision when descendants are added, removed, enabled, disabled, hidden, or revealed. The focus-trap scanner moves into a shared internal utility, with a boolean form that stops running style checks after the first accepted candidate.
SOLUTION (2 decisions · ~85 runtime lines of 273 added)
- Keep the BottomSheet body in the sequential focus order when its descendants provide no keyboard route. This serves the reported text-only scrolling failure.
- Share the existing focusability filter and add a boolean scan so lazy/nested descendant changes use the same model as the focus trap. This serves the PR body's stated dynamic-content requirement.
The story, changeset, and test additions are evidence/support rather than independent product decisions.
The implementation does solve the main long-text case, but its predicate omits two facts that decide whether the stop is useful: whether the body actually scrolls, and whether a descendant is truly visible and sequentially focusable.
VERDICT: BLOCKS — the predicate produces a dead stop for fitting content and removes the required stop for CSS-hidden or other non-sequential descendants
OWNER: BottomSheetPanel owns its scrolling body; the cross-component concept “a tab stop exists only while a container actually scrolls” is the shared hook/system being developed in #5553.
TIER 1: useFocusTrap and the shared focusable-element utility. The PR reuses them rather than duplicating the selector.
TIER 2: none.
SEAMS: standalone native-dialog host; BottomSheetSwitcher shared dialog/focus trap; nested focus trap; lazy subtree changes; hidden/disabled/removed controls; SSR/hydration; reduced motion; Escape dismissal and trigger focus restoration.
BEHAVIOR UNIT: currently split between an inline layout Effect/observer in BottomSheetPanel and a pure shared query utility. The behavior is separately nameable and overlaps the in-flight useScrollableTabStop work in #5553. That is non-blocking context, not an implementation ask: this PR can proceed against the outcome-based acceptance criteria below, and the shared owner can be reconciled separately if that hook lands.
| seam | driven result |
|---|---|
| standalone text-only, long | body tabindex="0"; Tab reaches it; Page Down moves 331px in the dedicated long fixture |
existing TallSheet, main → head |
body attribute absent → 0; Page Down remains 206px in Chromium on both, because Chromium implicitly focuses scroll containers |
BottomSheetSwitcher + nested useFocusTrap
|
Escape closes only the inner trap; outer dialog stays open; body becomes the sole tab stop and the outer trap keeps focus inside |
| reduced motion + dismissal | reduced motion matched; Page Down moves 206px; Escape closes the sheet and restores focus to “Open nearby places” |
| hidden descendant | fails: a button under a display:none ancestor removes the body tab stop despite zero visible sequential descendants |
VERDICT: note — local behavior overlaps in-flight shared work; no action requested in this PR
The intended improvement reaches every standalone and switcher-managed BottomSheet whose body scrolls and has no visible control. Long text becomes explicitly reachable and keyboard-scrollable.
Two new failure states also reach consumers:
- A short, fitting sheet becomes an extra generic focus stop that cannot scroll. In Chromium the measured body was
scrollHeight=56,clientHeight=56; Tab landed on it and Page Down moved0px. - A scrollable sheet containing only CSS-hidden controls loses its explicit body tab stop. The hidden button had no client rect and
checkVisibility=false, but its own computeddisplaystayedinline-block; the helper counted it, leaving zero visible sequential descendants and notabindexon the body. A visibletabindex="-2"target produces the same mismatch because the selector excludes only-1, while every negative value is outside sequential navigation.
The two source anchors confirm the browser results independently:
// BottomSheetPanel.tsx:521-522 — no overflow condition participates.
const needsTabIndex = !hasFocusableDescendant(body);// focusableSelector.ts:23-40 — only -1 is excluded, and display is read on
// the candidate rather than its CSS-hidden ancestor.
'[tabindex]:not([tabindex="-1"])'
const style = window.getComputedStyle(element);
if (style.visibility === 'hidden' || style.display === 'none') {VERDICT: BLOCKS — the change fixes the headline case but creates or preserves broken keyboard routes at both boundaries of its predicate
No public API change. No prop, exported component/hook/type, signature, default, accepted value, package barrel, or package export changed. getFocusableElements and hasFocusableDescendant are exported only from the internal source file; packages/core/src/hooks/index.ts is unchanged.
| change | public? | class | doc'd? | verdict | |
|---|---|---|---|---|---|
+ |
hasFocusableDescendant(container: HTMLElement): boolean |
internal file only | boolean companion to the existing internal list query | internal JSDoc | ok if its sequential-visibility contract is corrected |
~ |
getFocusableElements(container: HTMLElement): HTMLElement[] moved from useFocusTrap.ts
|
internal file only | existing behavior | internal JSDoc | no public change |
OSSIFICATION: none at the package boundary. The internal utility still matters architecturally because two focus owners now depend on its exact meaning; its name/JSDoc promise “currently perceivable focusable descendants,” which the CSS-hidden and negative-tabindex cases disprove.
VERDICT: clear — no public surface
No target, token, CSS variable, StyleX declaration, class contract, status, size, or theme behavior changed. The temporary browser fixture was not part of the reviewed diff and was removed after measurement.
VERDICT: clear
BEHAVIOR: yes, intentionally — text-only sheet bodies enter sequential focus order. Unintended at the boundaries: fitting bodies also enter it, while CSS-hidden and other non-sequential descendants can remove it.
API: no; public signatures and exports are unchanged.
VISUAL: yes, intentionally during interaction — keyboard focus ownership changes and Page Down moves the existing content. No static layout, paint, or style change was introduced.
THEME: no; the same elements, targets, tokens, and style contracts remain.
VERDICT: BLOCKS — the intended focus-order change is sound, but the two predicate boundaries alter behavior incorrectly
EFFECTS: one new layout Effect per mounted BottomSheetPanel, dependent on the stable body ref. It synchronizes React state with an external DOM subtree and disconnects cleanly.
RENDER: the layout Effect adds one render pass whenever its computed predicate differs from current state, including mount when a candidate exists; later observer callbacks add one pass when that predicate flips and equal values bail out. No user-facing degradation was observed in the stated count harness.
LISTENERS/OBSERVERS: one MutationObserver per mounted panel, watching childList plus twelve focusability-related attributes over the full subtree; cleanup disconnects it. The body’s own React-owned tabindex write is explicitly ignored.
LAYOUT: counted in Chromium at N=1/20/100 hidden candidates and N=100 visible candidates. Each batched insertion caused one body scan; the selector materialized 1/20/100/100 candidates. The helper performed 0/0/0/1 computed-style reads respectively, confirming the second commit short-circuits style resolution after the first accepted candidate. Selector collection remains O(N), but no repeated scan or user-facing degradation was observed. No performance finding is made.
BUNDLE: no dependency was added; exact bundle delta was not separately claimed. CI build and dependency checks are green.
VERDICT: clear — one bounded extra render pass on predicate changes was considered and is non-blocking; counted work is one scan per mutation batch with style checks short-circuited
VISUAL CHECK: visual gate sufficient
WHY: pixels are not the claim. No CSS, StyleX, static layout, paint, theme, or rendered-content change was introduced; interaction intentionally changes focus ownership and scroll position. Exact-head pr-visual ran green; the PR Analysis Report records no change across the two compared frames. KeyboardScrollableText renders the text-only endpoint and HugHeight renders the control-bearing endpoint.
The acceptance states are temporal: which node receives Tab, whether an explicit tabindex is present, whether Page Down changes scrollTop, which nested trap owns Escape, and where focus returns. A still frame cannot establish causation for any of those; the Chromium behavior trace does. Per the review request, that trace is decisive when pixels are not the claim. No manual frames were required, so screenshot sensor receipts were not created. The blocking fitting-body and CSS-hidden-control cases are likewise behavior failures, proved by geometry/focus/scroll and source anchors rather than appearance claims.
VERDICT: clear
Automated: exact-head pr-a11y and pr-rtl ran green; the changeset adds no baseline entry. No translated or directional string was added to runtime; story prose is English test content only.
Browser-checked in Chromium:
| path | result |
|---|---|
| long text, no controls | pass — explicit body tabindex="0"; Tab reaches it; Page Down moves 331px |
| control-bearing sheet | pass — body has no extra tabindex; existing controls remain the route |
| no-scroll body | fail — 56/56 geometry still gets tabindex="0"; Tab lands; Page Down moves 0px |
| CSS-hidden control | fail — zero visible sequential descendants, but the body loses tabindex
|
tabindex="-2" descendant |
fail — zero visible sequential descendants, but the body loses tabindex
|
| nested focus trap | pass — one Escape closes inner only; outer remains open; body becomes the trap’s sole stop |
| reduced motion + outer dismissal | pass — scrolling works, Escape closes, opener regains focus |
The two failures share one root: bodyNeedsTabIndex is computed from a selector/visibility approximation rather than the actual invariant “this body scrolls and no visible sequential descendant can receive Tab.”
VERDICT: BLOCKS — a real keyboard path still has no explicit focus target, and a fitting body gains a dead target
| slot | verdict |
|---|---|
| PROBLEM | clear |
| SOLUTION | BLOCKS — incomplete tab-stop predicate |
| ARCHITECTURE | note — local behavior overlaps in-flight shared work; no action requested here |
| IMPACT | BLOCKS — hidden/non-sequential descendants and fitting bodies fail |
| API | clear — no public API |
| THEMING | clear |
| BREAKING | BLOCKS — unintended focus-order behavior at both boundaries |
| PERFORMANCE | clear — one bounded render pass on predicate flips; one scan per batch at N=1/20/100 |
| VISUAL | clear — visual gate sufficient; behavior trace decisive |
| A11Y & I18N | BLOCKS — keyboard route remains wrong in reachable states |
GOAL: partly met — the exact long text-only case gains tabindex="0" and Page Down moves 331px, but the same body loses that explicit route in the CSS-hidden and negative-tabindex cases, while a 56/56 non-scroll body gains a dead stop.
DISPOSITION:
-
[BLOCKS] A fitting body becomes a dead Tab stop. → A keyboard user lands on a target that cannot reveal any content. ·
BottomSheetPanel.tsx:521-522CONFIRMATION: Chromium measuredscrollHeight=56,clientHeight=56, Tab landing on the body, and Page Down moving0px; independently, the source predicate reads only descendant focusability and never reads overflow geometry. -
[BLOCKS] CSS-hidden or negative-tabindex descendants suppress the body stop. → On engines that need an explicit scroll-container stop, a keyboard user still cannot reach overflowed text. ·
focusableSelector.ts:23-40CONFIRMATION: Chromium measured zero visible sequential descendants while the body lackedtabindex; independently, the source reads only the candidate’s own computeddisplayand excludes only the literaltabindex="-1".
ADVICE: bounded direction — the acceptance contract is “body can actually scroll AND no visible sequential descendant exists.” Preserve the already-passing nested-trap, reduced-motion, Escape, and focus-restoration results. No code shape is prescribed; #5553 is still in flight.
AUTHOR CAN PROCEED: yes — no human API/design choice is required. The two conditions and regression matrix above are complete outcome-based acceptance criteria.
WORST OUTCOME: “On engines that need an explicit scroll-container stop, a keyboard user still cannot reach overflowed text.” → request changes.
JUDGEMENT NEEDED: none.
Verdict: request changes. The main fix is real, but a keyboard-accessibility change cannot land while reachable members of the same class still suppress the only explicit route and non-scroll content gains a dead stop.
Thanks for covering the dynamic-content cases. Two keyboard paths still break:
- A short sheet becomes a dead Tab stop even though it cannot scroll (
BottomSheetPanel.tsx:510-557).- A scrollable sheet with only CSS-hidden or negative-tabindex controls loses its explicit body stop, so the fix misses engines that need it (
focusableSelector.ts:23-42).Could the condition require actual overflow and no visible sequential-focus descendant?
[Reviewed by Robohands]
Word count: 64 (request-changes cap: 150). No Discord clause: collaborator bucket. No full-review link: collaborator bucket, not ENGOWNERS/DESIGNOWNERS.
None. The two measured cases are one cross-cutting predicate defect; splitting them into inlines would duplicate the summary.
- All exact-head repository workflows completed successfully: CI, test, build, Storybook build,
pr-a11y,pr-visual,pr-rtl, lint, dependency check, and smoke test. - The focused unit suites passed locally: 3 files, 59 tests.
- The server-rendered body starts with
tabindex="0", avoiding an inaccessible initial SSR state.
Not posted. This review was drafted read-only; no public action was taken on the PR — no comment, approval, request-changes, label change, branch push, or upstream wiki edit.
Pass 1 — FAIL; verdict confirmed. The critic agreed with request-changes and the 150-word cap, then found missing source anchors/independent confirmations, an unsupported performance clear, ambiguous ownership advice, and public wording that led with mechanics instead of user impact. It also applied two stale pre-1.4 format rules (ARCHITECTURE omitted and THEME TARGETS naming); the current 1.4.0 presentation explicitly requires ARCHITECTURE after SOLUTION and THEMING, so those two were not adopted.
Rewrite: pasted the two exact source anchors; added source + Chromium confirmation for both blocks; counted observer scans at N=1/20/100; made the architecture overlap non-blocking; changed BREAKING/Visual to intentional interaction movement; added exact fixture/story/viewport details; and cut the public text from 91 measured words to 64 while leading with the user-visible failures. The critic also asked for screenshot receipts; the operator’s explicit rule for this review says a behavior trace is decisive when pixels are not the claim, and the current visual-obligation rule permits the exact-head visual gate for behavior-only work. The visual slot now states why temporal focus/scroll causation is not a frame claim.
Pass 2 — FAIL, three exact consistency edits. The visual decision passed under the operator clarification: pixels are not the claim, temporal behavior traces are decisive, and no receipts are required. Remaining edits: remove one stale architecture ask, count the Effect’s bounded extra render pass, and align VISUAL wording with BREAKING’s intentional focus/scroll movement. It also corrected the canonical public count to 64 words.
Pass 3 — PASS. Verdict, slot floors, source/browser confirmations, visual obligation, performance accounting, architecture routing, AUTHOR CAN PROCEED, and 64-word public text are internally consistent. No further rewrite.
- Focused unit tests:
vitest run BottomSheetPanel.test.tsx BottomSheetSwitcher.test.tsx useFocusTrap.test.tsx→ 3 files, 59/59 pass. - Chromium
151.0.7922.34, 640×600 viewport. Baselinecore-bottomsheet--tall-sheeton currentmain; the same story pluscore-bottomsheet--keyboard-scrollable-texton the exact head. - Isolated local-only fixture stories:
core-bottomsheet--review-no-scroll,--review-long-text-only,--review-css-hidden-control,--review-negative-tab-index, and--review-nested-focus-trap. Command:BEFORE_PORT=6100 AFTER_PORT=6254 node review-5513-behavior.cjs. - Count harness on
core-bottomsheet--keyboard-scrollable-text: N=1/20/100 hidden candidates and N=100 visible candidates; one body query per mutation batch, candidate lists 1/20/100/100, helper style reads 0/0/0/1. - The fixture diff was inspected, then reverted. The review worktree returned clean at the pinned head.
TIME total 46m
- setup 10m — fresh exact-head worktree, clone-install, warm main, build helper, Storybook dev
- reading 10m — kit/version/rulings/presentation/evidence/measurement, public API/design/rubric, PR/issue/history/full diff
- measuring 17m — 59 tests; Chromium behavior matrix; N=1/20/100 observer counts; reduced-motion opener rerun
- writing 9m — R16 draft, three critic passes, and wiki record
- waste 4m — Storybook does not serve a new untracked story module; moved the identical fixture into a tracked story and reverted it
- Safari/WebKit. This Mac permits Chromium only. The explicit
tabindexinvariant, rather than Chromium’s implicit scroll-container focus, is therefore the cross-engine evidence. - Vercel deployment is red. The repository’s Storybook and sandbox builds are green and their GitHub Pages previews were published; prior jiunshinn fork PRs show the same non-blocking Vercel artifact.