Review 5550
Review #5550 — bounded Drawer at 0019e5c
freddymeta · contributor · OPEN · view on GitHub
Pinned review. This record judges exactly 0019e5c9b3a20dcec8c5e4ec891026bf7e154cb7, four files and +641/−50 relative to its GitHub merge-base. No PR review or comment was posted.
CURRENT DISPOSITION — Round 2: request changes; the API direction is ratified and AUTHOR CAN PROCEED: yes. Drawer owns pane scope, while modality becomes an explicit independent axis. The exact current public draft is in Round 2. No PR action was taken.
#5550 feat(Drawer): containerRef — bind the drawer to an element, not the viewport by freddymeta (bucket: contributor)
0019e5c9b3a20dcec8c5e4ec891026bf7e154cb7 — every claim below was verified at this commit.
Feature parent: da75e7b9d2c6d564268ccd20dc120666ad2c4436, the exact head of #5549. Feature-only delta: four files, +426/−22.
Current main checked at cddf57e5d474281150bef92f1167228ab0a6e4a5; no Drawer or shared-layer change landed after this branch point, so main did not invalidate the review.
LOOP VERSION: 1.5.0 AUDIT RUBRIC: 1.12
LANE: full WHY: new exported prop and capability; portal/layer/focus/layout ownership; one added and one changed Effect; native dialog state; intended visual change; container lifetime, scroll, sizing, stacking, SSR, dismissal, and RTL all require browser evidence.
Clear. The exact four-file diff changes React/StyleX source, one test, one story, and typed docs. It changes no dependency, lockfile, workflow, lifecycle script, executable, credential/environment read, or network/shell path. Source inspection found no dynamic execution or request path. It was safe to install, compile, test, and render locally.
WHY 1: Drawer can only cover the viewport; it cannot express an inspector that belongs to one split-view pane, dashboard card, canvas, or chart panel. WHY 2: Builders either let a local inspector take over the whole page or hand-roll a second panel/layer implementation, losing Drawer’s shared dismissal, focus, and motion behavior. WHY 3: A design system should let the same inspector concept preserve scope and behavior across page and pane layouts rather than forcing product code to fork the interaction.
USER-FACING PROBLEM: A person inspecting one item in a multi-pane workspace loses the rest of the workspace to a viewport drawer, or gets a product-specific substitute with inconsistent layer behavior. PROBLEM SEVERITY: missing capability — the PR names four use-case classes and an existing product-system clone; the public Overlay specification also establishes container-confined overlays as a real class, while no landed Drawer API supplies it.
VERDICT: clear
The Drawer reads an external element ref after commit, portals a dialog and custom scrim into that element, and switches the panel from fixed viewport geometry to absolute container geometry. Bounded mode also changes a scrimmed Drawer from native modal to non-modal, clips the moving panel with an absolute wrapper, and warns when the target is not positioned. Existing viewport mode remains the fallback when the prop is absent.
SOLUTION (4 decisions · ~116 added non-comment runtime lines of 426 added lines)
- Add a public ref-selected portal host and container-relative panel sizing — serves pane scope.
- Redefine
hasScrimunder that mode as a pointer-blocking custom scrim without modality, focus trap, or body lock — serves local rather than page-wide blocking. - Add an absolute
overflow: clipwrapper with pointer routing — serves entry animation without making the wrapper a scroll container. - Mirror the target ref into state and validate its computed positioning — serves delayed ref availability and builder diagnostics.
BURDEN: high — a public cross-prop mode; one new state/Effect and one changed native-dialog Effect; portal host lifetime; custom scrim; wrapper and z-index participation; render-time style read; duplicated modal/non-modal behavior; 111 test and 128 story/doc additions.
BURDEN MATCH: [Needs human judgement] — pane-level inspectors are a real missing capability, but the proposed public mode permanently couples container ownership, modality, and hasScrim semantics. No landed public sibling exposes a portal host this way; anchorRef names an anchor, while the container Overlay owner returns container props and colocated content.
VERDICT: BLOCKS — the implementation does not preserve the promised pane scope through scrolling or target replacement.
OWNER: Drawer owns the panel; the layer protocol owns dismissal/depth, and the consumer’s pane owns its scrollport and node lifetime.
TIER 1: native <dialog>, useScrollLock, useDevWarning, overlayPaddingReset; shared layer dismissal is not reused (pre-existing Drawer debt, not charged here).
TIER 2: none.
SEAMS: external portal target; scrollable/replaceable pane; scrim/no-scrim; viewport/container sizing; sibling stacks; RTL; SSR hydration; xstyle/className/style on the panel.
BEHAVIOR UNIT: inline — the target state Effect and dialog Effect coordinate a mutable ref, portal remounts, native show/close, focus, a listener, and a timer. The current kit requires a named unit plus focused browser tests for that multi-Effect state machine; the PR has jsdom tests with mocked dialog methods and no checked-in browser test.
| seam | driven result |
|---|---|
| positioned 500×300 scroll pane | opens contained at 400×292; intended control passes |
| pane scroll 0→140 | fails: panel y 44→−96; visible height 292→156 |
| real wheel over the revealed pane | fails: scrollTop 0→180; panel y 44→−136 |
| same RefObject, target element replaced |
fails: document dialog count 1→0 while isOpen remains true |
| default bounded scrim, reverse Tab twice | fails: focus reaches obscured opener; Enter activates it while pointer path is covered |
| two sibling bounded Drawers | passes: z-index 1000/1001; Escape closes only inner |
| nested Drawer | intentionally unsupported by current docs; not charged |
RTL side="end"
|
passes: pane left 360, panel left 364 (inline-end is left in RTL) |
| initial-open SSR/hydration | server emits no dialog; hydration is mismatch-free and opens inside the pane after the Effect |
| 300px pane, 900px viewport | panel fills all 292px inner width; the 56px reveal appears only after viewport shrinks below 640px |
The scroll and replacement failures are independently confirmed by source ownership: the absolute wrapper is a child of the scroll node (Drawer.tsx:290-301,862-885), and the target Effect depends on ref identity plus isRendered, not containerRef.current or node attachment (Drawer.tsx:597-600).
VERDICT: BLOCKS — pane scroll and node lifetime are owned by an unobservable mutable ref, and the important portal/dialog state machine has no focused browser-test boundary.
The intended path works initially: a builder can render the Drawer elsewhere in React, bind it to a positioned pane, keep the rest of the page live, and retain LTR/RTL placement plus sibling LIFO dismissal.
Three reachable paths break. A user wheeling the revealed part of a scrollable inspector pane sees the 292px panel and scrim move upward until only 156px remains after 140px of scroll. A dashboard that replaces the pane element under the same stable ref loses the open Drawer entirely. With the default bounded scrim, a keyboard user can focus and activate the dimmed opener behind that scrim even though a pointer user is blocked.
VERDICT: BLOCKS — scroll, replacement, and keyboard paths each contradict the visible/controlled bounded-pane state.
Real call site from the story:
const paneRef = useRef<HTMLDivElement>(null);
<div ref={paneRef} style={{position: 'relative'}}>…</div>
<Drawer
isOpen={selected != null}
onOpenChange={isOpen => !isOpen && setSelected(null)}
label="Host details"
containerRef={paneRef}>
…
</Drawer>| change | public? | class | doc’d? | verdict | |
|---|---|---|---|---|---|
+ |
`containerRef?: React.RefObject<HTMLElement | null>` | yes, exported canary DrawerProps
|
no landed portal-host prop; anchorRef is trigger/position precedent only |
en + zh + dense + story |
~ |
hasScrim?: boolean becomes non-modal when containerRef exists |
existing public canary prop, changed combined semantics | no landed sibling where a scope prop removes scrim modality | en + zh + dense | P8 finding |
~ |
isFullWidthOnMobile resolves 100% against the pane, but still switches on viewport width |
existing public canary prop | no contained sibling | prose mentions the bounded size, not narrow-desktop behavior | note |
OSSIFICATION: the need is a class (split view, dashboard card, canvas, chart panel), but the spelling is a new concept. Tooltip/Popover/Typeahead use anchorRef for an external anchor, not a portal host. The existing container Overlay owner uses a hook that returns containerRef, container props, and colocated overlay content; its public design explicitly considered and rejected a ref-mutating container option. The cost of being wrong is permanent ref-lifetime behavior plus a sibling prop whose modality changes conditionally. API Conventions says “Prop independence. One prop never suppresses another prop’s output” and each prop is one orthogonal axis; this mode makes containerRef remove hasScrim’s native modality while leaving the dimming surface.
VERDICT: BLOCKS — the combined props produce modality-dependent behavior that differs by input modality. The separate question of whether Drawer should own this new portal-host/scope concept remains [Needs human judgement] in JUDGEMENT.
No target, component variable, or variant is added or removed. The panel keeps themeProps('drawer', {side}) on its painting <dialog>; all new paint uses existing surface, border, shadow, duration, easing, and overlay tokens. The custom bounded scrim is a sibling outside the astryx-drawer target and cannot receive the panel’s xstyle/class styling of ::backdrop; that reach question is part of the unresolved public API/design shape, not an independently actionable theme defect on this head.
VERDICT: clear — token integrity and existing targets are unchanged; new scrim reach is routed to the API decision.
BEHAVIOR: existing callers without containerRef remain on the parent path in focused tests and the control arm. The new bounded path has proven scroll, target-lifetime, and keyboard behavior failures.
API: additive in canary-only @astryxdesign/lab; no stable consumer migration or codemod is owed, but it is new exported surface.
VISUAL: yes, intentionally — panel/scrim move from viewport scope to pane scope. The control pair shows that intended change; reverse-Tab frames expose one unintentional focus state behind the scrim.
THEME: no existing target/token removed; bounded scrim has less component-specific reach than ::backdrop.
@astryxdesign/lab remains private: true/canaryOnly; exact-head check:changesets passed without a changeset, consistent with the package lifecycle.
VERDICT: note — existing no-prop behavior is stable and the canary API is additive; the broken opt-in behavior is owned by SOLUTION/IMPACT rather than counted again as an existing-consumer break.
EFFECTS: one added target-mirroring Effect [containerRef, isRendered]; one changed dialog Effect [isOpen, isModal, portalTarget]; existing callback-ref mirror, unmount close, and registry Effects kept. The first copies a DOM element into state; the second synchronizes native dialog state, focus, transition listener, and timer.
| Effect + deps | external system | why render/handler cannot do it | measured render cost | lifetime + cleanup | focused test |
|---|---|---|---|---|---|
target mirror [containerRef,isRendered] Drawer.tsx:598-600
|
consumer DOM ref / portal host | ref is null during initial render | parent→head Profiler callbacks: 1→2 at N=1, 3→6 at N=3, 10→20 at N=10 while every Drawer is closed; target getComputedStyle calls add the same N delta |
state retains stale detached target; no subscription to replacement | none in browser; jsdom only |
dialog sync [isOpen,isModal,portalTarget] Drawer.tsx:620-700
|
native dialog + transition event/timer | controlled state must reach imperative dialog | no further commit beyond the target update measured; one native open path per instance | listener/timer cleanup present; portal replacement remount loses the live node | jsdom mocks only |
RENDER: one extra commit per bounded Drawer on mount, measured at N=1/3/10 even while closed. The repo has no product callsite; the documented sibling stack makes N=3 the realistic upper check, while N=10 is stress only.
LISTENERS/OBSERVERS: transition listener and backstop timer are scoped to exit and cleaned; no observer/dependency added.
LAYOUT: getComputedStyle(portalTarget).position runs during bounded render; matched arms added N calls at N=1/3/10. Scroll geometry itself is CSS, but the absolute host follows scroll content rather than the scrollport.
BUNDLE: no dependency; feature-only runtime source adds 187/−16 lines gross, ~116 non-comment additions.
VERDICT: BLOCKS — the new ref-to-state Effect adds a measured render/style-read pass and still fails node replacement; the multi-Effect native/portal protocol lacks focused browser coverage.
VISUAL CHECK: manual frames required
WHY: the feature intentionally changes panel/scrim geometry and introduces a custom rendered scrim and clipping wrapper. Exact-head pr-visual is green, but source-gated snapshots cannot establish the new scrollport and focus behavior.
The same local story ran at the exact #5549 parent and exact #5550 head. The only probe-story arm differences are five containerRef={paneRef} lines across the scoped, performance, replacement, and stacking stories; the inspected exact arm diff and reviewed feature diff are banked. Expectations were authored before observation.
| Sensor | Parent / before | Exact head / after |
|---|---|---|
| Build | da75e7b9d2c6d564268ccd20dc120666ad2c4436 |
0019e5c9b3a20dcec8c5e4ec891026bf7e154cb7 |
| Receipt | control JSON · focus JSON | control JSON · focus JSON |
| Story | review-5550--scrollable-pane |
same |
| Theme / mode / direction | neutral / light / LTR | same |
| Viewport / media | 900×520 @1; forced colors off; reduced motion off; fine pointer; hover | same |
| Semantic state | named “Pane details” dialog open; pane text present; opener at activation 1 | same |
| Geometry | one visible 400×520 dialog | one visible 400×292 dialog |
| Settled / errors | fonts loaded; 0 animations; no Storybook/page error | same |
| #5549 parent | #5550 exact head |
|---|---|
 |
 |
PIXEL JUDGEMENT: intentional. This is the PR’s stated result: “the panel is inside the pane, the scrim dims only the pane.” The panel changes from x=500/y=0/400×520 to x=136/y=44/400×292; the pane remains 500×300.
| #5549 parent | #5550 exact head |
|---|---|
 |
 |
PIXEL JUDGEMENT: unintentional. The after control→reverse-Tab delta is 2,060/468,000 pixels (0.4402%) in one 78×52 box at (58,90), exactly the focus-ring expansion around the revealed part of the active 444×32 opener at (68,100). The parent control/focus images are byte-identical. DOM evidence confirms the after focus is inside the pane, outside the dialog, beneath a 492×292 scrim; Enter activates it. This contradicts the PR/docs statement that the scrim “dims and blocks the container.” Pixel classification · DOM boxes · probe results.
The frames were opened through the image reader; this harness exposed metadata rather than inline pixels, so the visual call is fail-closed on matching sensor receipts, exact hashes, exhaustive pixel classification, and DOM-box attribution rather than an unauditable prose impression.
VERDICT: BLOCKS — the intended containment renders, but the default scrim paints a blocked state while keyboard focus and activation reach the obscured control.
Exact-head pr-a11y and pr-rtl ran and passed; the baseline file is unchanged. The dialog is named, built-in close is named, reduced-motion is guarded, and no new runtime visible/AT string, locale formatter, physical layout property, or directional glyph was added. Real Chromium confirms close button, Escape, and scrim click each close and restore focus to “Open pane drawer (1)”; the outside-page button remains operable. RTL side="end" pins to the pane’s left edge correctly. Sibling bounded Drawers preserve inner-first Escape.
The default bounded scrim fails input-modality parity. Two reverse Tabs from the built-in close focus the obscured opener behind the scrim, and Enter runs it; pointer hit testing is covered by the scrim/panel. A keyboard user can therefore operate pane controls that the visual and pointer state present as unavailable.
SSR hydration emitted no mismatch and opened inside the pane, but an initially-open bounded Drawer is absent from server markup until the target Effect runs; first-paint timing was not visually captured and is not raised as a separate finding.
VERDICT: BLOCKS — keyboard users can activate controls behind the default container-blocking scrim.
| slot | verdict |
|---|---|
| PROBLEM | clear |
| SOLUTION | BLOCKS — scroll and replacement fail |
| ARCHITECTURE | BLOCKS — unstable scroll/node ownership and inline multi-Effect protocol |
| IMPACT | BLOCKS — panel can clip/disappear; keyboard reaches blocked content |
| API | BLOCKS — P8 cross-prop semantics; separate scope concept needs human judgement |
| THEMING | clear — token integrity/targets unchanged; scrim reach routed to API |
| BREAKING | note — additive canary path; existing callers stable |
| PERFORMANCE | BLOCKS — measured extra commit/style read plus stale target |
| VISUAL | BLOCKS — focus painted under scrim |
| A11Y & I18N | BLOCKS — keyboard activates obscured pane control |
GOAL: partly met — initial LTR/RTL containment, dismissal, focus restoration, and bounded sibling stacking work, but the canonical scrollable pane loses 136/292px after 140px of scroll, replacement removes the dialog, and the default scrim blocks pointer but not keyboard interaction.
DISPOSITION: scrollport ownership → blocks now; target replacement + extra mount pass/style read → one ref-lifetime root defect, blocks now; scrim keyboard/pointer mismatch → blocks now; new Drawer scope/modality concept → held for API judgement; target-specific theming reach and narrow-desktop reveal → included in that API/design decision, not separate asks.
ADVICE: bounded direction — the pane-level host must stay pinned to the pane’s visible scrollport, follow target replacement while open/closed, expose one modality contract across pointer/keyboard, and avoid mirroring a DOM node through an extra render. Preserve the verified exact-head outcomes: external page remains live, close/Escape/scrim restore focus, RTL logical edge works, and sibling Escape stays inner-first. The owner/API mechanism is not prescribed pending the API decision.
AUTHOR CAN PROCEED: no — the three defects have complete acceptance criteria, but a contributor should not ossify this new Drawer scope/modality concept until the API owner decides whether Drawer owns it and how hasScrim is represented.
WORST OUTCOME: “A keyboard user can activate pane controls beneath a scrim that blocks pointer users, while scrolling or replacing that same pane can clip or remove the open inspector.” → request changes
JUDGEMENT NEEDED: API — should Drawer own a public ref-selected bounded mode whose scrim is non-modal, or should pane-scoped panels compose through a different owner/API? Recommendation: do not accept containerRef plus conditional hasScrim semantics as-is; keep the need, settle an orthogonal scope/modality contract first.
request changes
-
[BLOCKS] The absolute portal host scrolls with the target’s content → a person wheeling the revealed part of a scrollable split pane moves the panel from y=44 to −136 and leaves only 116/292px visible ·
packages/lab/src/Drawer/Drawer.tsx:290-301,862-885 -
[BLOCKS] The ref-to-state target does not follow node replacement and adds a render pass → a responsive dashboard that swaps the pane under a stable ref removes its open inspector while
isOpenstays true; every mounted bounded Drawer also adds one commit and one render-time style read ·packages/lab/src/Drawer/Drawer.tsx:597-612 -
[BLOCKS] The default bounded scrim blocks pointer but not keyboard interaction → a keyboard user reverse-tabs to and activates the dimmed opener under the scrim, while a pointer user cannot reach it ·
packages/lab/src/Drawer/Drawer.tsx:864-880
Independent confirmations: each runtime defect is reproduced in real Chromium and follows directly from the causal source path. The performance delta is a matched Profiler/getComputedStyle count at N=1/3/10. Visual receipts match on every sensor except Build.
Thanks—this makes the pane-scoped case concrete, but four bounded-mode paths still break.
In a scrollable pane, wheel input over the reveal scrolls the absolute portal host:
scrollTop 0→180moved the 292px drawer from y=44 to −136 and left 116px visible (Drawer.tsx:290-301). The user can lose most of the panel and scrim.With the default scrim, two reverse Tabs focused the obscured opener and Enter activated it while pointer input stayed blocked (
Drawer.tsx:864-880). Keyboard users can operate controls the scrim says are unavailable.Replacing the target element behind the same ref left
isOpentrue but removed the dialog (Drawer.tsx:598-600). A responsive pane swap loses its inspector and dismissal path.Even closed, target resolution doubled Profiler commits at N=1/3/10 and added one render-time style read per Drawer (
Drawer.tsx:598-612). Please preserve the host across scroll/replacement, make scrim blocking consistent across modalities, and avoid the extra mount pass/style read.[Reviewed by Robohands]
Public review length: 145 words before attribution (request-changes cap: 150). Held draft only; no GitHub action was taken.
None. The three findings are cross-cutting ownership/behavior contracts; the causal ranges are in the summary.
- At a 900px viewport, a 300px pane gets a 292px panel and no reveal; at a 600px viewport the same pane gets 236px and the documented 56px reveal. The bounded mode adapts to viewport width rather than container width.
- The bounded custom scrim uses tokens but is outside the existing
astryx-drawertarget and the panel’sxstyle/::backdropstyling path. - The exact-head story still gives its pane
overflow: hiddeneven though the new docs say no overflow rule is needed; the dedicated scroll probe removed that masking assumption.
TIME total 34m setup 7m exact-head + #5549-parent worktrees, cloned installs, core/config builds; warm main reused: no (stack parent was the required before arm) reading 9m kit/critic, two fresh wikis, PR bodies/history, diff, siblings, shared layer/overlay owners measuring 12m 44 focused tests, 1 SSR test, typechecks, 2 matched Chromium arms, 4 sensored frames, 5 interaction/layout probes, N=1/3/10 counts writing 6m R16 draft, critic pass, rewrite, wiki record waste 3m first typecheck before core build; first parent probe used a pointer click blocked by its modal control; unavailable image compositor/PIL
- Safari/WebKit behavior; this Mac’s supported Playwright lane is Chromium only.
- First-paint timing for an initially-open SSR Drawer; hydration correctness passed, but the server markup intentionally contains no dialog.
- Focused Vitest: 44/44 passed at exact head (log).
- Lab typecheck and typed-doc typecheck: passed after building core.
- One-off SSR/hydration test: 1/1 passed; no hydration mismatch, dialog portals and opens after hydration (log).
- Exact-head CI: test, build, lint, Storybook, a11y, RTL, visual, theme layers, smoke, and Vercel passed;
review-requiredremains pending because no entitled review was posted. - Real Chromium: close, Escape, and scrim click close + restore focus; outside-page click remains live; sibling bounded z-index is 1000/1001 and Escape closes inner only; RTL end edge passes (behavior · full matched results / head).
- Scroll: programmatic 0→140 moves panel y 44→−96 and visible height 292→156; real wheel over the revealed pane moves scrollTop 0→180 and panel y 44→−136 (wheel result).
- Target lifetime: replacing the element behind the same ref changes document dialog count 1→0 while controlled
isOpenremains true. - Performance: matched Profiler callbacks 1→2, 3→6, 10→20; target style-read counts add 1, 3, 10.
- Visuals: four frames, four passing sensor receipts, matching non-build sensors, image hashes/dimensions recorded, and focus pixels mapped to the active DOM box.
- Diff hygiene: feature-only and probe-arm diffs banked; no dependencies/scripts/workflows; current main has no intervening Drawer change.
DRAFT: request changes on scrollport ownership, target lifetime/performance, and scrim modality; hold public action for API judgement and the #5549 set.
CRITIQUE 1: failed R16 because API combined BLOCKS and [Needs human judgement] on one verdict line instead of choosing one slot floor. Failed R14 because the public draft called the 140px programmatic scroll a wheel result; the real wheel run was 180px. Failed the standing impact rule because THEMING’s note named only a future reach risk, not a current affected theme author. PERFORMANCE needed to identify N=3 as the repo-realistic documented stack and N=10 as stress. BREAKING double-counted defects in an additive canary path rather than answering whether existing consumers break. The three runtime blocks themselves passed R14/R16g: exact-head source and real Chromium independently agree; their causal ranges open at the cited lines.
REWRITE: API now has one BLOCKS line and routes the separate concept decision through JUDGEMENT. The public finding uses the actual wheel 0→180/y 44→−136/116px-visible result. The unsupported theme note was cut, existing-caller BREAKING became a note, and the performance denominator now names N=3 as realistic. Public wording remains 145 words, has one impact per finding, names no private process, and retains the exact signature.
CRITIQUE 1 RESULT AFTER REWRITE: passes R1/R1g/R28 need and API framing; R2 at 145/150 words; R3/R4/R5 problem-first impact; R6d set hold; R12 voice and gratitude; R13 inherited layer debt exclusion; R14 exact head/anchors/current-main check; R15/R38/R39 sensored manual frames; R16 complete slot/verdict shape; R16g two-method confirmation; R18 measured counts at multiple N; R31b/R31c stack and main reconciliation; R32 class matrix; R35 behavior-unit gate; R36 exact versions; R37 AUTHOR CAN PROCEED; R41 three whys; R42 burden/severity match. Final R17 gate is the durable wiki push below.
Relationship to #5549
This is a strict stack. Commits 6ab800c and da75e7b are exactly #5549; #5550 adds dda9323 and 0019e5c. The bounded path depends on #5549’s isRendered exit lifetime, transition-end close, focus restoration, and latched side. This review isolated #5550’s +426/−22 feature delta and used #5549’s exact head as the before arm. All three blocking runtime findings are introduced only by #5550. The set verdict remains held until the independent #5549 review is reconciled; #5550 cannot land first or be judged as if its base mechanics were already accepted.
Nothing was posted to the PR. The proposed public request-changes text is archived exactly as drafted. No GitHub review, comment, approval, request-changes action, merge, label change, or contributor-branch push was made.
The decision window elapsed without an owner answer, so the loop applied its recorded recommendation under the Rulings policy. The reviewed head remains exactly 0019e5c9b3a20dcec8c5e4ec891026bf7e154cb7; no code, evidence, or blocking runtime finding changed.
Drawer owns pane-scoped side panels. Keep containerRef as the scope/host axis rather than creating another inspector component or routing this behavior through the media/card Overlay primitive.
Modality is a separate explicit axis. Add modality?: 'modal' | 'nonModal' with default 'modal'. containerRef chooses only viewport versus element scope; it never changes focus, inertness, pointer blocking, or aria-modal. hasScrim controls whether scrim paint is present; dismissal and underlying operability follow the chosen modality, never the scrim’s presence.
| explicit mode | required observable contract |
|---|---|
modal |
content behind the Drawer in the chosen scope is unavailable to both pointer and sequential keyboard interaction; focus restores on close; aria-modal is emitted only when its global semantic promise is true |
nonModal |
content behind the Drawer remains operable by both pointer and keyboard; a scrim cannot present or enforce a contradictory blocked state |
Concrete impact. A builder can move the same inspector between viewport and pane without silently changing what hasScrim means. A keyboard user never activates controls a pointer user is blocked from, and a non-modal caller never acquires an implicit focus policy merely by choosing a host.
This is canary-only lab surface, so the PR may migrate existing Drawer stories/docs/call sites from hasScrim-implied modality to the explicit axis without a stable-consumer codemod.
| slot | verdict |
|---|---|
| PROBLEM | clear |
| SOLUTION | BLOCKS — scroll and replacement fail |
| ARCHITECTURE | BLOCKS — unstable scroll/node ownership and inline multi-Effect protocol |
| IMPACT | BLOCKS — panel can clip/disappear; keyboard reaches blocked content |
| API | BLOCKS — current head conditionally changes modality; ruling supplies the accepted shape |
| THEMING | clear |
| BREAKING | note — additive canary path; existing no-prop behavior stable |
| PERFORMANCE | BLOCKS — measured extra commit/style read plus stale target |
| VISUAL | BLOCKS — focus painted under scrim |
| A11Y & I18N | BLOCKS — keyboard activates obscured pane control |
GOAL: partly met — the initial bounded render, dismissal, restoration, RTL, and sibling stack work; scroll, node replacement, input-modality parity, and mount cost remain broken.
DISPOSITION: scrollport ownership → blocks now; target replacement + extra mount pass/style read → blocks now; pointer/keyboard mismatch → blocks now; public owner/API choice → resolved by Ruling 15.
ADVICE: preserve one host against pane scroll and target replacement; remove the ref→state extra mount pass and render-time style read; implement the explicit modality axis so the chosen scope has one pointer/keyboard contract. Preserve the verified close/Escape/scrim restoration, RTL edge, outside-scope reachability, and sibling LIFO behavior.
AUTHOR CAN PROCEED: yes — acceptance is complete: the Drawer remains attached and fully visible through scroll/replacement; the selected modality behaves consistently for pointer and keyboard; containerRef never alters hasScrim or focus semantics; and N=1/3/10 returns to one mount commit with no target style read in render.
WORST OUTCOME: “A keyboard user can activate pane controls beneath a scrim that blocks pointer users, while scrolling or replacing that same pane can clip or remove the open inspector.” → request changes
JUDGEMENT NEEDED: none — Ruling 15 assigns pane scope to Drawer and modality to an explicit independent axis.
request changes
-
[BLOCKS] The absolute portal host scrolls with the target’s content → a person wheeling the revealed part of a scrollable split pane moves the panel from y=44 to −136 and leaves only 116/292px visible ·
packages/lab/src/Drawer/Drawer.tsx:290-301,862-885 -
[BLOCKS] The ref-to-state target does not follow node replacement and adds a render pass → a responsive dashboard that swaps the pane under a stable ref removes its open inspector while
isOpenstays true; every mounted bounded Drawer also adds one commit and one render-time style read ·packages/lab/src/Drawer/Drawer.tsx:597-612 -
[BLOCKS] Scope conditionally changes modality and the default scrim blocks pointer but not keyboard interaction → a keyboard user reverse-tabs to and activates the dimmed opener under the scrim, while a pointer user cannot reach it ·
packages/lab/src/Drawer/Drawer.tsx:596-603,864-880
Thanks—pane-scoped Drawer is the right owner. Please keep scope and modality orthogonal:
containerRefchooses the host only; expose explicitmodality('modal' | 'nonModal') rather than silently changinghasScrimand focus behavior.Three runtime paths still block:
- In a scrollable pane, wheel input over the reveal moved the absolute portal host:
scrollTop 0→180moved the 292px drawer from y=44 to −136, leaving 116px visible (Drawer.tsx:290-301).- Two reverse Tabs focused the dimmed opener, and Enter activated it while pointer input stayed blocked (
Drawer.tsx:864-880).- Replacing the target behind the same ref removed the dialog while
isOpenstayed true; even closed, target resolution doubled Profiler commits at N=1/3/10 and added one style read per Drawer (Drawer.tsx:598-612).Please preserve the host through scroll/replacement, make blocking consistent across input modes, and avoid the extra mount pass/read.
[Reviewed by Robohands]
Public review length: 133 words before attribution (request-changes cap: 150).
The ruling resolves the only withheld owner/API choice and leaves every measured defect intact. The revision passes the standing impact rule: each finding names the affected user/builder, state, and failure; the API sentence gives the accepted owner and orthogonal axis rather than handing a decision back to the contributor. It passes R2 at 133/150 words, R3/R4 problem-first wording, R12 gratitude/voice, R14 exact anchors/head, R16g independent evidence already banked in Round 1, R31 public/private separation, and R37 with AUTHOR CAN PROCEED: yes plus measurable acceptance criteria.
TIME total 7m setup 1m fresh fork-wiki clone and exact-head/no-review verification reading 2m current Review 5550, Reviews index, Rulings policy measuring 0m no code/head change; Round 1 evidence remains exact writing 4m ruling, judgement rewrite, critic pass, pull/rebase/push waste 0m
The API hold was resolved to Drawer + explicit modality and AUTHOR CAN PROCEED changed from no to yes. The public request-changes draft was rewritten to include that direction. Nothing was posted to the PR; no review, comment, approval, request-changes action, merge, label change, or contributor-branch push was made.