Review 5567
ernestt · open · view on GitHub
Verdict: request changes · re-review of 0720144f9b57eccb2ebeca10f6ebe950b7b715f9
#5567 feat(Collapsible): animate the content reveal, and rewrite the block examples by ernestt (bucket: internal — .github/DESIGNOWNERS, write permission)
0720144f9b57eccb2ebeca10f6ebe950b7b715f9
LOOP VERSION: 1.5.0 AUDIT RUBRIC: 1.12
LANE: full WHY: three runtime decisions, intended visual and default-behavior changes, two new Effects, imperative listeners/timing, and a released theme target changing element.
WHY 1: A Collapsible panel snaps open and closed, a closed FAQ answer is hard to discover with browser find, and an undensified trigger can be only 17px tall. WHY 2: The abrupt change gives weak spatial orientation, search can miss the answer a reader wants, and a small target is harder to operate. WHY 3: A disclosure exists to hide optional detail without making it hard to find or control. USER-FACING PROBLEM: Someone scanning or searching a disclosure-heavy page should be able to find, reveal, and operate the right section without a jarring jump or tiny target. PROBLEM SEVERITY: harmful friction — motion alone is polish; findability and target size affect discovery and operation.
VERDICT: clear
The panel stays mounted on a clipped height track and changes height and opacity when its open state changes. The component separately coordinates browser hiding, focus exclusion, animation frames, transition completion, and browser-find events so a closed panel can supposedly remain searchable. The trigger gains a minimum pointer size, and the examples teach row-versus-heading typography.
SOLUTION (3 runtime decisions · ~142 added non-comment lines of 824 additions)
- animate disclosure height and opacity around the existing open state — stated motion problem
- replace
display:nonewith aninert/hidden="until-found"lifecycle andbeforematchreopening — findability is described as a benefit, not established as a starting problem - raise every trigger to 24px on fine pointers and 44px on coarse pointers — not stated in the opening problem
The eight block rewrites are one holistic example-design pass by a design owner, not eight independent runtime decisions.
BURDEN: high — extra wrapper, one state value, two Effects, two animation frames, a per-instance listener, a retained transition listener, a 1.5s timer, shared reset change, and a moved released theme target. BURDEN MATCH: disproportionate — most machinery serves polish, and the findability half does not work while the same subtree is inert.
VERDICT: BLOCKS — the high-burden timing protocol misses its findability outcome and changes a released theming seam.
OWNER: Collapsible owns disclosure state, accessibility, and motion; CollapsibleGroup owns coordination and density.
TIER 1: native hidden="until-found" / beforematch, inert, CSS transitions, and the existing useCollapsible state owner.
TIER 2: target size via the shared spacing scale; no icon/status subsystem change.
SEAMS: standalone and grouped state, controlled and uncontrolled state, arbitrary focusable children, Card/Section composition, and the released collapsible-content theme target.
BEHAVIOR UNIT: inline — two Effects coordinate a ref, React state, rAF, a timer, transition events, and native attributes; the real browser path has no focused test.
| seam | driven result |
|---|---|
| browser find on a rested closed panel | fails by platform contract: inert subtrees are excluded from find-in-page, so beforematch cannot rescue this state |
released collapsible-content target with padding-block:16px
|
main: closed panels 0px; head: each closed panel leaves a 32px box |
| focusable last child in an open panel | main overflow visible; head overflow hidden and clips the 5px-outset focus ring |
| standalone/grouped rapid toggles | settles to matching aria-expanded, inert, hidden, and 0px height |
The timing protocol crosses the mechanical complex-behavior threshold and remains embedded in the component with only jsdom tests. The shipped CSS-grid disclosure precedent keeps motion declarative; this change has a second job—browser find—but that does not make the combined protocol testable inline.
VERDICT: BLOCKS — complex behavior has no named testable unit, and three documented seams fail.
Every app taking the next release gets animated disclosure and taller triggers without opting in. A reader using browser find still misses text inside every closed panel; a keyboard user can lose the bottom of a focused child’s outline; and a theme author’s existing content-padding override now leaves visible gaps where rows are closed.
VERDICT: BLOCKS — the change introduces reachable search, focus, and theme regressions.
No prop, export, accepted value, or default changes.
OSSIFICATION: no new public TypeScript surface. Four new --_collapsible-* variables are private and undocumented, although the description and changeset call them theme-tunable; supported theme compilation rejects direct private-var configuration.
VERDICT: note — no public API change, but the advertised tuning path is not part of the supported theme API.
The released astryx-collapsible-content target existed in v0.5.0 on the padded painting element. This head keeps the name but moves it to the zero-height overflow:hidden animation track, while the new padded inner element has no target. A normal unlayered override (the same cascade level consumers may use) and the sensored !important attribution arm both produce the same result: 0px closed panels on main become 32px on head.
The duration/easing defaults use tokens, but the four new private variables are absent from Collapsible.doc.mjs and cannot be configured directly through defineTheme.
VERDICT: BLOCKS — a released target now points at layout machinery and changes existing override behavior.
BEHAVIOR: yes — open/close becomes asynchronous, browser-find handling is added, and closed descendants become inert. API: no — signatures, values, and defaults are unchanged. VISUAL: yes, intentional — reveal motion and trigger height change; the 640px Single Mode frame measures triggers at 17px on main and 24px on head. THEME: yes, unintended — the released content target moved from the padded content box to the clipping track; padding overrides leave 32px closed gaps.
VERDICT: BLOCKS — existing theme overrides can change meaning after upgrade.
EFFECTS: two added.
| Effect + deps | external system | why render/handler cannot do it | measured render cost | lifetime + cleanup | focused test |
|---|---|---|---|---|---|
useLayoutEffect [canInterpolateSize,isOpen] Collapsible.tsx:452
|
DOM attributes, CSS transition completion | controlled changes and native reveal are not always initiated by this component’s click handler | N=3 group toggle: 1→2 React commits; Layout 1→30; UpdateLayoutTree 8→33; Paint 8→43 | rAF and 1.5s timer clean up; transitionend remains attached while the item stays closed |
none |
useEffect [isOpen,isDisabled,toggle] Collapsible.tsx:520
|
native beforematch event |
no React event prop/type exists for this browser event | no state update until event; listeners scale exactly 2/4/6 at N=2/4/6 | one listener per mounted Collapsible; cleanup present | jsdom dispatch only; no native-find test |
RENDER: delayed visual state adds one commit per N=3 group toggle in Chromium (1→2).
LISTENERS/OBSERVERS: one permanent beforematch listener per instance; one transitionend listener remains on each item after it closes until that Effect reruns or unmounts; no observers or global listeners.
LAYOUT: the intended disclosure animation adds 29 Layout, 25 UpdateLayoutTree, and 35 Paint events for the measured N=3 toggle versus main.
BUNDLE: no dependency; exact-head CI reports 1.2KB gzipped but no before delta.
The layout work is the feature rather than an independent regression. The avoidable issue is an extra React commit and permanent/per-state listeners inside a behavior unit with no focused browser test.
VERDICT: note — the measured cost is the intended animation; the focused-test failure is carried by ARCHITECTURE.
VISUAL CHECK: manual frames required WHY: pixels and default geometry intentionally change, and the two blocking visual claims are absent from the exact-head visual job’s stories/states.
| Sensor | Expected | Observed | Pass? |
|---|---|---|---|
| Build | main f4aab4c2532b3cbf2c80ca7f46eb3a36c3b82663; head 0720144f9b57eccb2ebeca10f6ebe950b7b715f9
|
exact SHAs from both worktrees at capture | yes |
| Story | core-collapsible--single-mode |
same in both | yes |
| Theme | default global; rendered neutral
|
{}; neutral
|
yes |
| Color mode | default; computed light
|
light |
yes |
| Direction | default; computed ltr
|
ltr |
yes |
| Viewport/media | 640×520 @1; forced colors/reduced motion/coarse false, hover true | same in both | yes |
| Rendered state | 3 triggers; [open, closed, closed]; padding-block:16px arm |
exact match | yes |
| Subject geometry | 3 targets; first visible/non-zero/in viewport | main 536×72; head 536×76 | yes |
| Settled render | fonts loaded; 0 animations; no page/Storybook errors | exact match | yes |
| Image | PNG, 640×520, non-blank hash | 18,316 bytes / 5c235df45da23ebfec82b458d66df5c063d1af71ff4038237d4a870326e7dd64; 18,335 bytes / ec2dce7007228345b29e4db8907a8643ab2175c4911955454c88364027938cca
|
yes |
ARM: content-target padding
BASE: each exact build above
DELTA: one injected declaration, .astryx-collapsible-content { padding-block:16px !important; }; a second run without !important measured the same 0→32px result
UNRELATED DELTA: none
| Before | After |
|---|---|
 |
 |
Difference: intentional target override; unintentional target behavior change. Main closed heights are 0,0; head is 32,32.
| Sensor | Expected | Observed | Pass? |
|---|---|---|---|
| Build | same main/head SHAs | exact SHAs from both worktrees at capture | yes |
| Story | core-collapsible--single-mode |
same in both | yes |
| Theme / mode / direction | neutral / light / ltr | same in both | yes |
| Viewport/media | 640×520 @1; default media | same in both | yes |
| Rendered state | first panel open; final child focused; 2px outline + 3px offset | exact match | yes |
| Subject geometry | 3 targets; first visible/non-zero/in viewport | main 536×44; head 536×64 | yes |
| Settled render | fonts loaded; 0 animations; no page/Storybook errors | exact match | yes |
| Image | PNG, 640×520, non-blank hash | 19,688 bytes / c3ea0d8e7fa5e681b3a7d8447797ce3c573da64c484a8adfc1fa5ec4403c0826; 20,221 bytes / 20bb775c48d4539fc60b6081822174f5058317ea72308626d84e21e687fd744c
|
yes |
ARM: focused final child BASE: each exact build above DELTA: append one final focusable anchor to the open content and apply the system’s 2px outline + 3px offset UNRELATED DELTA: none
| Before | After |
|---|---|
 |
 |
Difference: unintentional. The ring extends 5px below the content box in both arms; main paints it with overflow:visible, while head clips it with overflow:hidden.
Containment assumptions: the target is the full-width content box; the child ends at its block-end edge; the parent Card is unconstrained. Narrow/wide, RTL, zoom, forced-colors, and icon-only do not alter either causal boundary, so they were omitted after the decisive pairs. Exact-head pr-visual is green and reports 2/20 changed shots (Single Mode neutral light/dark), but it covers neither an existing target override nor a focused child.
VERDICT: BLOCKS — the target move creates collapsed gaps, and the permanent clip cuts descendant focus paint.
Auto-covered: exact-head pr-a11y and pr-rtl are green; .github/a11y-baseline.json is unchanged. Browser-checked: trigger height is 24px on fine pointers and 44px on coarse; rapid toggles settle; reduced motion resolves to 1ms; the open panel clips a focused child’s 5px-outset outline.
The remaining prior-review failure is structural: inert={!isOpen} at Collapsible.tsx:592 keeps every rested closed subtree out of browser find. MDN says inert content “are not searchable via browser find-in-page features”; the HTML standard says the user agent should ignore inert nodes for find-in-page. The unit test at Collapsible.test.tsx:183 dispatches beforematch directly, so it cannot catch that the browser will not produce the event in this state.
No new translatable runtime string or physical-direction property is introduced. Template copy is demo content and exact-head pr-rtl is green.
VERDICT: BLOCKS — closed content is still not findable, and open descendant focus paint is clipped.
| slot | verdict |
|---|---|
| PROBLEM | clear |
| SOLUTION | BLOCKS — burden does not deliver findability and changes a released seam |
| ARCHITECTURE | BLOCKS — complex inline timing state machine; failed seams |
| IMPACT | BLOCKS — search, focus, and theme regressions |
| API | note — advertised private tuning is unsupported |
| THEMING | BLOCKS — released target moved onto layout machinery |
| BREAKING | BLOCKS — existing theme overrides change meaning |
| PERFORMANCE | note — intended animation cost measured; no leak found |
| VISUAL | BLOCKS — collapsed gaps and clipped focus paint |
| A11Y & I18N | BLOCKS — inert defeats find and overflow clips focus |
PRIOR REVIEW: cixzhang asked for the hidden="until-found" reset conflict and an actual browser-path regression. Commit 0720144 fixes the reset selector; the test still dispatches beforematch directly, and inert keeps the native search path blocked. This review extends that unresolved ask and adds two shape-level regressions; it does not silently reverse the prior conclusion.
GOAL: partly met — the reset now leaves hidden="until-found" at display:block, motion runs, and targets grow; inert still excludes the closed subtree from find-in-page.
DISPOSITION: inert/find conflict → blocks now; released target relocation → blocks now; complex inline timing protocol without focused browser coverage → blocks now; focused-child clipping → blocks now; stale description/private-var claim → omit from public review until the shape is corrected.
ADVICE: bounded direction — preserve three outcomes: native find opens any closed panel, the released target still styles the padded content without affecting collapsed geometry, and a focused descendant’s full ring remains visible; put the timing protocol behind a named unit with a browser test covering open, close, interruption, reduced motion, and native find. Reproduction: pr-5567-r2-probe.cjs and pr-5567-r2-visual.cjs, with results and receipts beside them.
AUTHOR CAN PROCEED: yes — the required outcomes and reproductions are explicit; the implementation choice remains theirs.
WORST OUTCOME: “A reader using browser find still misses text inside every closed panel.” → request changes.
JUDGEMENT NEEDED: none — these are regressions against existing browser, accessibility, and theming contracts.
request changes
-
The collapsed content remains inert. → browser find ignores every closed answer, so
beforematchnever gets a chance to open it ·packages/core/src/Collapsible/Collapsible.tsx:592 -
The released content target moved onto the zero-height clip track. → a theme’s existing 16px content padding leaves each closed row 32px tall ·
packages/core/src/Collapsible/Collapsible.tsx:594 -
The animation track keeps clipping while open. → a keyboard user focusing the last child loses the bottom 5px of its outline ·
packages/core/src/Collapsible/Collapsible.tsx:164
Thanks — the reset exemption fixes the selector from my last review, but the browser-find path is still blocked. The closed panel remains inert, and inert subtrees are excluded from find-in-page, so the browser never reaches beforematch; the test still dispatches that event directly.
The released collapsible-content target also moved from the padded content to the zero-height clipping track. An existing 16px padding override now leaves every closed row 32px tall, and the permanent clip cuts the bottom 5px from a focused last child. This timing now spans two Effects, rAF, a timer, events, and native attributes without a focused browser test.
Could we preserve those content/focus contracts and move the timing protocol into a focused unit with real find-in-page coverage?
[Reviewed by Robohands]
-
packages/core/src/Collapsible/Collapsible.tsx:592—inertprevents browser find from reaching closed content, sobeforematchcannot run. -
packages/core/src/Collapsible/Collapsible.tsx:594— This moved the released target onto layout machinery; padding now keeps closed rows 32px tall. -
packages/core/src/Collapsible/Collapsible.tsx:164— This stays clipped while open and cuts a focused child’s bottom outline.
- The description still names
visibility:hiddenand a staggered fade, while this head usesinert/hidden="until-found"and a parallel ramp. - The four new private motion variables are absent from
Collapsible.doc.mjs, despite the changeset calling them theme-tunable. - The prior review accepted the target-size and example updates; the response commit changes only
reset.css.
TIME total 28m
setup 5m dedicated worktree + clone/reconcile install + one build + Storybook dev on port 6317 (warm main reused: yes, port 6100)
reading 9m full brief/critic/harness, prior review/gates, body, complete diff, source, tests, rubric, and conventions
measuring 8m 72 targeted tests + 13 browser arms/traces + 4 sensor-receipted frames; 2 re-measures to isolate theme/focus behavior
writing 6m three presentations + three critic gates
waste 3m missing $KIT lookup, one malformed CLI invocation, a mis-scoped docsite test, and one baseline recapture invalidated when the shared main checkout moved
- Browser chrome’s find UI cannot be driven reliably through Playwright on this Mac; the finding uses the HTML/MDN inert contract plus the exact-head DOM state. The requested native-path regression test is still absent.
- Safari and Firefox cannot launch under this Mac’s managed-device policy, so their fallback path was not driven.