Review 5549
#5549 fix(Drawer): stop the panel repainting into the page after close() by freddymeta (bucket: collaborator; treat as internal)
da75e7b9d2c6d564268ccd20dc120666ad2c4436
LOOP VERSION: 1.5.0 AUDIT RUBRIC: 1.12
PRIOR REVIEW: none — GitHub reviews and inline comments checked at the exact head.
LANE: full WHY: the diff changes visible close behavior, adds render state, changes an Effect, adds a native transition listener and timer protocol, and is the base of the active Drawer set with #5550.
WHY 1: after dialog.close() releases a modal Drawer from the top layer, a transformed/container ancestor can become the fixed panel's containing block while the panel still paints.
WHY 2: someone who just dismissed the panel sees it jump back over the page they returned to; a start-side Drawer can also jump to the opposite edge when the caller clears the state that supplied side.
WHY 3: that breaks spatial continuity and makes the person reacquire the page after every affected close, even though focus and pointer hit testing already reach the page.
USER-FACING PROBLEM: a person dismissing a Drawer under a containing-block ancestor sees the dismissed panel reappear over unrelated content; a person closing a state-derived start Drawer sees it leave through the wrong edge.
PROBLEM SEVERITY: harmful friction — the task completes and the underlay remains hit-testable, but the visible return to page context is wrong for the tail of the transition.
VERDICT: clear
The panel waits for its own slide to finish rather than assuming a fixed duration. At completion, the native dialog closes and React hides the panel in the same task; the edge used at open is retained until the exit finishes. This removes the post-top-layer paint and keeps the departure on the edge the person opened.
SOLUTION (2 decisions · ~198 runtime changed lines of 245 total)
- Replace the fixed close timer/discrete-display hold with render-owned visibility, transition completion, a computed-duration backstop, and synchronous hide.
- Retain the opening
sidethrough exit so caller state reset cannot flip the departing panel. [NO STATED PROBLEM IN PR BODY]
BURDEN: high — one new state owner, one changed Effect, native listener, timer backstop, computed-style read, flushSync, a retained ref, and an added commit per closing Drawer.
BURDEN MATCH: disproportionate as currently packaged — the behavior is an independently nameable close protocol but remains inline and browser-untested; a focused unit and browser regression are the bounded direction.
VERDICT: BLOCKS — complex lifecycle behavior has no focused behavior unit or checked-in browser test
OWNER: Drawer owns its open/close lifecycle; the native <dialog> owns top-layer state.
TIER 1: the layer protocol suite, native dialog lifecycle, useScrollLock, and the existing transition-wait pattern in BottomSheetPanel.waitForTransition.
TIER 2: none.
SEAMS: modal and non-modal; transformed containing-block ancestors; start/end and RTL; close during entry; rapid reopen; repeated cycles; reduced motion; nested/stacked Drawers; Activity/unmount cleanup; #5550's bounded portal path.
BEHAVIOR UNIT: inline — render-owned visibility, a ref latch, the open/close Effect, the unmount Effect, a transition event, timer, computed-style read, and synchronous commit form one timing state machine.
| seam | driven result |
|---|---|
| transformed ancestor | head closes at display:none, 0×0; base still paints a 400×500 panel |
| close during entry | head ends closed, hidden, and non-modal |
| rapid reopen | head remains open, visible, and modal after the stale close is cancelled |
| 3 repeated cycles | all end closed/hidden |
| reduced motion | computed transition is 0.01s; final state is closed/hidden |
| nested/stacked | inner closes while outer stays open; both then close with zero visible dialogs |
| start-side prop reset | head retains data-side=start; base flips to end
|
The implementation passes every driven seam, but the PR's tests mock showModal()/close() in jsdom and cannot observe top-layer release, containing-block repaint, real transitions, or paint order. A future edit can restore the reported frame while all 37 tests remain green. Independently confirmed: the controlled Chromium arm exposes a visible 400×500 post-close panel on the base, while the four selected head tests run against that base fail only their timing/side assertions and never inspect the painted frame.
VERDICT: BLOCKS — the multi-part close protocol is inline and its defining browser failure is not regression-tested in a browser
Every Drawer caller gets theme-duration-aware exit completion. Apps rendering a modal Drawer under transform, filter, contain, container-type, backdrop-filter, or will-change stop seeing the dismissed panel repaint; builders deriving side from open-state data now keep the opening edge through exit. Pointer activation and trigger focus restoration are unchanged.
VERDICT: clear
No API signature, prop, export, or default changes.
| change | public? | class | doc'd? | verdict | |
|---|---|---|---|---|---|
| — | none | lab package is private | — | no API update needed | ok |
OSSIFICATION: no new surface. The existing side theme state changes meaning during exit, but the target is in the private lab package and no stable consumer surface is added.
VERDICT: clear
No target, token, CSS variable, or style contract is added or removed. Exit timing now reads the resolved transition duration, so a theme's --duration-medium value governs completion; the existing drawer[data-side] state stays on the opening side until hidden.
VERDICT: note — the existing side theme state changes during exit and the PR body does not record that second decision
BEHAVIOR: yes, intentionally — close timing follows the rendered transition and the opening side is retained through exit.
API: no signature/default/export change; @astryxdesign/lab is private.
VISUAL: yes, intentionally — post-close repaint disappears and start-side exit stays on start; outer geometry while open is unchanged.
THEME: no target/token removal; an existing data-side value is retained longer during exit.
VERDICT: clear — intentional lab-only behavior correction, with the unstated side decision carried as a note
EFFECTS: one existing Effect at Drawer.tsx:494 is changed; the existing unmount-only dialog cleanup Effect at :585 remains part of the same native-dialog lifecycle.
| Effect + deps | external system | why render/handler cannot do it | measured render cost | lifetime + cleanup | focused test |
|---|---|---|---|---|---|
useEffect [isOpen, hasScrim] Drawer.tsx:494
|
native dialog top-layer state + CSS transition | controlled isOpen can change from a caller render; transition completion is a browser event |
+1 commit per close at N=1 and N=2 (realistic repository maximum) | listener/timer exist only during close; both removed on finish or cleanup | no checked-in browser test |
RENDER: one additional commit per closing Drawer; base→head is 2→3 commits at N=1 and 1→2 for each closing Drawer at N=2.
LISTENERS/OBSERVERS: one transitionend listener only during close; no observer; cleanup passed rapid reopen and repeated cycles.
LAYOUT: one getComputedStyle() transition-timing read per close; no geometry read/write loop; transform remains compositor-only.
BUNDLE: no dependency change; CI bundle report stayed 3.5 KB CJS / 1.5 KB gzip for lab.
The extra commit is the render-owned hide that removes the invalid painted frame, and it remains O(1) per active closing Drawer; no fan-out or listener leak was measured.
VERDICT: note — +1 commit and one style read per close, bounded to the closing Drawer
VISUAL CHECK: manual frames required
WHY: visible close behavior is the acceptance criterion, and exact-head pr-visual compared only 2 shots while 12 Drawer shots had no baseline; no existing story contains the transformed-ancestor failure.
Before 58f9542
|
Head da75e7b
|
|---|---|
 |
 |
SENSOR RECEIPT: both review-drawer5549--lifecycle, neutral/light/LTR, 900×500@1, fine pointer/hover, identical transformed 620×360 subject, fonts loaded, zero page/Storybook errors, semantic state “drawer closed and focus restored.” Before: open=false, display:flex, visible dialog 400×500 at x≈730 with 2 transitions intentionally still running. Head: open=false, display:none, 0×0 with 0 running animations. Receipts: before · head.
Before 58f9542
|
Head da75e7b
|
|---|---|
 |
 |
SENSOR RECEIPT: same story/theme/mode/direction/viewport/media/geometry and semantic “closing after opening from start.” Before: data-side=end, transform +273px; head: data-side=start, transform −271px. Receipts: before · head.
ARM: review-only Storybook story
BASE: each checked-out commit above
DELTA: one untracked story adding a transformed ancestor, state-derived side, profiler, nested Drawers, and underlay target; production files unchanged
UNRELATED DELTA: none
Exact arm: visual · performance · pointer
Every observed difference is intentional: the repaint removal is the PR's stated bug; the side retention is implemented by the second commit but absent from the PR body.
VERDICT: clear
Exact-head pr-a11y is green and adds no baseline entry. Chromium verified trigger focus restoration after close, pointer hit testing under the residual rectangle (the underlying button remained activatable before and after), close during entry, rapid reopen, three repeated cycles, reduced motion, and nested/stacked close order; zero visible dialogs remained. No role, ARIA, keyboard mapping, string, locale, or direction API changes occur; start/end behavior was driven in LTR and the source continues to use logical anchoring. Only Chromium was available in this run; source inspection found no new engine dependency because the change uses long-supported transitionend/getComputedStyle browser APIs and ReactDOM flushSync, while removing reliance on discrete display transitions.
VERDICT: clear
| slot | verdict |
|---|---|
| PROBLEM | clear |
| SOLUTION | BLOCKS — no focused behavior unit/browser test |
| ARCHITECTURE | BLOCKS — inline close state machine |
| IMPACT | clear |
| API | clear |
| THEMING | note — unrecorded side-state change |
| BREAKING | clear |
| PERFORMANCE | note — +1 commit/style read per close |
| VISUAL | clear |
| A11Y & I18N | clear |
GOAL: met — at head the first frame after native close is display:none/0×0 under the transformed ancestor, and start-side exit remains start.
DISPOSITION: inline close protocol without focused browser regression → blocks now; unstated start-side behavior → blocks now as a PR-body/evidence update; +1 close commit/style read → bounded feature cost, no separate block.
ADVICE: bounded direction — move the close timing/lifecycle into a named hook or utility and check in the transformed-ancestor Chromium regression; BottomSheetPanel.waitForTransition is the landed transition-wait precedent, not a drop-in API.
AUTHOR CAN PROCEED: yes — acceptance is a named/testable close protocol plus a checked-in real-browser red→green repro, and recording the start-side behavior in the PR body.
WORST OUTCOME: “A future edit can restore the reported post-close painted frame while all 37 tests remain green.” → request changes.
JUDGEMENT NEEDED: none — correctness/testability against the existing Drawer contract.
request changes
- The close protocol is an inline native/React timing state machine with jsdom-only coverage.
→ someone closing a transformed-ancestor Drawer can get the same repainted panel again while every checked-in test stays green ·
Drawer.tsx:479-574 - The second commit retains
sidethrough exit but the PR body only describes repaint timing. → builders using the documented state-derivedsideshape get a different exit and theme-state value without that behavior being reviewable from the PR description ·Drawer.tsx:654-684
Thanks for chasing this down. The matched close frames show the fix:
| Before | Head |
|---|---|
 |
 |
Two things before merge. The close now coordinates render state, a transition listener, timer, flushSync, and unmount cleanup, but the tests mock the dialog in jsdom. A future change can restore the same post-close() frame for anyone using a transformed ancestor while all 37 tests stay green. Could you move the close timing into a named hook or utility and add that Chromium regression? BottomSheetPanel.waitForTransition is the closest existing pattern.
Please also add the start-side latch to the PR body: builders using the documented state-derived side shape now get a different exit and theme-state value, but that second change is not described.
[Reviewed by Robohands]
-
packages/lab/src/Drawer/Drawer.tsx:494— Could this close timing move to a named hook or utility with a Chromium test? -
packages/lab/src/Drawer/Drawer.tsx:654— Please add this second visible behavior change to the PR body.
- Exact-head CI is green, including
pr-a11y,pr-visual,pr-rtl, build, test, lint, sandbox, and Storybook. - The overlapping underlay target remained pointer-hit-testable even while the base panel still painted; this is a visual/context defect, not a pointer trap.
- The four selected head tests are red against the base runtime (3 failures; the unrelated-property control stays green).
#5550 stacks this exact head as its first two commits and says its bounded mode depends on this exit-state refactor. This candidate is request-changes on its own merits; under the set rule, post nothing and hold the set disposition until #5550's independent review is complete. Any close-protocol correction belongs at the bottom of the stack so both candidates inherit it.
TIME total 34m setup 6m — exact worktree + fast install + build config; warm main reused: no (controlled base/head arm used) reading 8m — kit, rubric 1.12, rulings, conventions, body, diff, siblings measuring 15m — 2 screenshot pairs, browser matrices, N=1/N=2 render counts, focused tests, negative control writing 5m — two draft/critic passes and final presentation waste 4m — first Storybook navigation timeout, one failed unbuilt-core typecheck, and a pointer target repositioned after its first placement missed the residual rectangle
- Safari and Firefox runtime behavior; only Chromium was available. Source inspection found no new engine-dependent CSS/API.
Not posted: critic pass 1 added the decisive embedded frame pair, made the independent confirmation explicit, replaced loop jargon with a named hook/utility direction, and fixed the two-ask wording. Critic pass 2 passed. The Drawer set remains held until #5550 is reviewed.