Review 5563
Review #5563 — BaseProps pass-through forwarding at af004c6
cixzhang · the maintainer · OPEN · view on GitHub
Pinned historical review. The requested head is af004c602b65f7f85c3b5a17111492a50a7560a1, 22 files and +818/−53. The live PR later moved to 84f3a176516b0ee34daeb3acb969f997a0fe67ba; this record judges only the requested bytes.
#5563 fix(core): forward pass-through props on ten components that declare BaseProps and drop them by cixzhang (bucket: the maintainer)
af004c602b65f7f85c3b5a17111492a50a7560a1 — every claim below was verified at this commit.
Merge-base for the pinned diff: 9345b268ba64b799b1e32e6d8588e820bb87165d.
LOOP VERSION: 1.5.0 AUDIT RUBRIC: 1.12
The task named loop 1.4.1, but gate 1 read the source of truth after it had advanced to 1.5.0. R36 requires the version actually used, so this run stays on 1.5.0 through its critic pass.
LANE: full
WHY: cross-cutting public prop behavior across ten components, 11 runtime files, conditional DOM targets, six owned-handler compositions, intended visible caller-style behavior, and more than one decisive check. The exact-head visual job reported success at the check level but its payload was status: skipped because 344 shots exceeded the 240-shot budget, so manual frames were required.
Clear. The exact head belongs to facebook/astryx. The 22-file diff contains one changeset and component source/tests only: no dependency, lockfile, workflow, install hook, executable script, credential, environment read, or build-network path changed.
WHY 1: Ten components accept BaseProps at compile time but silently discard attributes and event handlers they do not explicitly destructure.
WHY 2: Builders can write standard accessible names, telemetry hooks, ids, and event handlers, see the code type-check, and ship a DOM tree where those props do nothing; a headerless List or TreeList cannot receive the accessible name the builder supplied.
WHY 3: A design-system prop contract must make ordinary builder code predict the DOM and accessibility tree; silent no-ops leave end users unable to identify a surface and leave builders without a failure signal.
USER-FACING PROBLEM: A screen-reader user reaches a headerless list/tree whose builder supplied an accepted accessible name, but the name never reaches the semantic element, so the user cannot identify what collection they entered. PROBLEM SEVERITY: broken task — the affected user cannot perceive the collection's identity.
VERDICT: clear
Each component separates caller styling, component-owned attributes, owned handlers, and neutral attributes. Styling combines with existing component styles; neutral props reach the rendered element; component-owned semantics win only when present; and an owned handler composes consumer-first so preventDefault() can opt out. Conditional and multi-root components choose a target instead of duplicating ids or handlers.
SOLUTION (6 decisions · 213 changed runtime lines of 871 total changed lines)
- Capture and forward neutral BaseProps on ten component render paths — serves the silent no-op problem.
- Combine
className/style/xstylethroughmergeProps, newly wiring that surface for TypeaheadItem and both top-nav menu triggers — serves the same dropped-prop problem. - Keep owned roles/ARIA/tab stops after the spread, and emit conditional ownership only when it has a value — serves the accessible-name and contract-precedence problem.
- Compose owned handlers consumer-first on NavHeadingMenu, TokenLink, TopNavMenu, and TopNavMegaMenu — serves the handler no-op/clobber problem.
- Split multi-root targets deliberately: MetadataListItem uses wrapper-or-
dt; TreeList puts naming onrole="tree"and other props on its layout root — serves valid DOM and naming semantics. - Leave caller-owned
TypeaheadItem.item.elementuntouched — serves the same anti-clobber contract for a value that may be a fragment, array, string, or caller element.
DECISION COUNT: accepted for this pinned review. All six decisions trace to the one declared pass-through/precedence defect; none introduces an unstated problem, and separating the categories would repeat the same contract review rather than isolate an independent choice.
BURDEN: low — no new public type, state, Effect, listener, observer, timer, dependency, or shared-system owner; the maintenance burden is one established merge/composition rule plus focused tests. BURDEN MATCH: proportionate — the defect is a broken declared contract with an accessibility consequence, and the implementation reuses existing utilities.
VERDICT: note — the shape is proportionate, but TreeList does not complete decision 4 for its own keyboard model.
OWNER: each component owns the boundary between its public props and its rendered root/semantic element.
TIER 1: BaseProps, mergeProps, composeEventHandlers, and the existing focus/menu/tree hooks — reused, not reimplemented.
TIER 2: none.
SEAMS: context providers and fragments; block/inline Markdown; stacked/inline MetadataListItem; relative/absolute Timestamp; Token's four root variants; desktop/drawer/mobile-bar top-nav modes; TreeList's layout root versus semantic tree; caller-owned item.element.
BEHAVIOR UNIT: existing pure composeEventHandlers utility — focused utility tests already prove ordered cancellation; component tests must prove each site actually routes through it.
| seam | driven result |
|---|---|
| List provider, with/without header | props reach the semantic ul/ol; owned role="list" and present header id win |
| Markdown block/inline | props reach div[role=document] or inline span; block role remains owned |
| MetadataListItem stacked/inline | props reach wrapper div or dt; ids/handlers are never duplicated onto dd
|
| NavHeadingMenu | consumer onKeyDown.preventDefault() runs before list navigation; Chromium control stays on “Menu one” |
| Timestamp | neutral props/ref reach <time> under Text/HoverCard; relative name and tooltip tab stop remain owned |
| Token variants | props reach the actual anchor/span/TokenLink container; hidden-label and disabled semantics win; TokenLink cancellation stops delegated middle-click |
| TopNavMenu/MegaMenu desktop/drawer | neutral props reach each trigger; caller styling combines; click/hover handlers compose; mobile-bar still intentionally renders nothing |
| TreeList root/tree split | names reach role="tree", but consumer keyboard cancellation on the outer root runs after the inner tree model |
| TypeaheadItem default/caller element | default div forwards and merges; caller-owned item.element remains untouched as declared |
Refs are unchanged by this PR: each existing ref target stays where it was. The pre-existing top-nav drawer/mobile-null and Typeahead caller-element ref limitations are outside the BaseProps pass-through change and are not charged here.
VERDICT: BLOCKS — TreeList gives one keyboard event two nested owners in the wrong order, so the public consumer handler cannot cancel the built-in model.
Nine component paths and TreeList's attributes now honor the props builders already wrote. Headerless List/TreeList instances become nameable; test/telemetry ids arrive; top-nav and token handlers no longer replace built-in behavior; requested caller styling becomes visible on TypeaheadItem and the two top-nav triggers.
One TreeList path remains a footgun. A builder passes onKeyDown={event => event.preventDefault()} to reserve ArrowDown, but the inner ul moves the roving tab stop before the event bubbles to the outer div. In exact-head Chromium the consumer observed “Tree two” and the second item held tabindex="0"; the NavHeadingMenu control observed “Menu one” and kept focus there.
VERDICT: BLOCKS — the callback fires, but too late to provide the consumer-first cancellation contract this PR applies everywhere else.
No signature, prop, optionality, default, export, or type changes. The semantic public change is that already-declared BaseProps begin affecting DOM output.
| component | public behavior change | exact target | precedence / seam verdict |
|---|---|---|---|
| List | accepted props stop being no-ops |
ul/ol
|
owned role and present header id win; clear |
| Markdown | accepted props stop being no-ops | block div, inline span
|
block role wins; clear |
| MetadataListItem | accepted props stop being no-ops | stacked wrapper div, inline dt
|
no duplicated ids/handlers; clear |
| NavHeadingMenu | props plus consumer key handler work | div[role=menu] |
consumer-first compose; clear |
| Timestamp | neutral props stop being no-ops | inner time
|
relative name/tab stop win; clear |
| Token | props stop being no-ops on every root variant | actual anchor/span/TokenLink container | conditional owned ARIA wins; clear |
| TopNavMegaMenu | neutral/styling/handlers reach desktop + drawer triggers | button |
popover ARIA and consumer-first handlers win; clear |
| TopNavMenu | neutral/styling/handlers reach desktop + drawer triggers | button |
popover ARIA and consumer-first handlers win; clear |
| TreeList | root props and tree naming stop being no-ops | layout div; names on inner ul[role=tree]
|
key handler is not consumer-first; finding |
| TypeaheadItem | default item forwards and merges | default item div
|
caller-owned item.element excluded; clear |
OSSIFICATION: no new concept or spelling. This implements the released BaseProps contract and the public API Conventions' existing combine/component-wins/compose/forward policies. The cost of being wrong is behavioral: a handler that appears forwarded but cannot cancel is a builder footgun.
VERDICT: BLOCKS — TreeList's ordinary onKeyDown spelling does not receive the same consumer-first semantics as the other owned-handler sites.
No theme target, token, variable, variant, or style contract is added or removed. Existing targets remain on the same painting elements. The only styling-path change is that caller className/style/xstyle now reaches TypeaheadItem, TopNavMenu, and TopNavMegaMenu through mergeProps, after the components' own styles as the public convention requires.
VERDICT: clear
BEHAVIOR: yes, intentionally — accepted props that were silent no-ops now affect DOM/handlers; one TreeList handler collision remains incomplete. API: no compile-time surface change; existing code keeps compiling. VISUAL: yes only when callers already pass styling to the three formerly-dropping components; the manual pair isolates exactly those requested outlines, with no other pixel delta. THEME: no target/token/variable/override changes.
The [fix] patch changeset describes a defect repair against the declared contract. Nothing removed or renamed needs a codemod.
VERDICT: note — intentional additive behavior/visual output; no API/theme break.
EFFECTS: zero added/changed/moved Effects. Existing Timestamp interval and TopNavMegaMenu anchor Effect are untouched. RENDER: no state or render-pass change; four components create consumer-first composed React handlers during render using the existing utility. LISTENERS/OBSERVERS: no native listener or observer added; React handler routing only. LAYOUT: no production layout read/write or measurement added. BUNDLE: no dependency or new utility; exact-head build/checks passed. The PR Analysis artifact reports the touched core bundle at 4.8 KB / 1.2 KB gzip but provides no before delta, so no delta is claimed.
VERDICT: clear — zero Effect/state/listener/layout additions and no new dependency.
VISUAL CHECK: manual frames required
WHY: the exact-head visual payload is explicitly skipped (344 shots exceeds its 240-shot budget), while the change intentionally makes three existing caller-style props visible and claims the remaining default pixels stay stable.
The same hand-authored matrix renders all ten components. The before arm restores only the 11 implementation files to merge-base 9345b268; the after arm is exact head af004c6. The harness and exact arm diff are banked; expectations were authored before capture. Both receipts match on story, empty globals, neutral/light/LTR, 1200×900@1, media, ten named cases, semantic list/tree state, geometry, loaded fonts, zero animations, and zero visible page errors.
| Sensor | Before | After |
|---|---|---|
| Build |
af004c6 checkout + exact implementation arm diff restoring 11 files to 9345b268
|
exact af004c6
|
| Receipt | JSON | JSON |
| Story | review-pr5563--matrix |
same |
| Theme / mode / direction | neutral / light / LTR | same |
| Viewport / media | 1200×900 @1; forced colors off; reduced motion off; fine pointer; hover | same |
| Semantic state | ten exact headings; semantic list + tree; Products/Solutions triggers; Typeahead label | same |
| Geometry | matrix 980×460 at (32,32) | same |
| Settled / errors | fonts loaded; 0 animations; no visible Storybook/page error | same |
| Merge-base implementation arm | Exact requested head |
|---|---|
 |
 |
PIXEL JUDGEMENT: 3,232 of 1,080,000 pixels changed (0.299%), in exactly three connected boxes. Their diff boxes match the 2px-expanded DOM bounds of TopNavMenu (30,334–140,370), TopNavMegaMenu (532,334–644,370), and TypeaheadItem (532,414–1014,438). All 3,232 changed pixels are inside those three boxes; the dominant after color is the story's requested magenta outline. No text, component geometry, or other case changed. This is the intended visible effect of making caller style work, not an accidental component visual change.
Evidence: pixel classification · DOM boxes · harness · probe.
VERDICT: clear — only the three explicitly requested caller-style outlines become visible; no unclaimed pixel difference remains.
Exact-head pr-a11y and pr-rtl both ran and passed; the baseline file is untouched. List and TreeList naming, component-owned roles/names/tab stops, and handler composition were checked in focused tests. The real-Chromium keyboard probe confirms NavHeadingMenu's consumer-first control and exposes TreeList's late consumer handler. No user-visible or AT-facing string, locale logic, physical CSS property, or directional glyph changes.
The default TreeList APG model still works. The defect is the declared extension path: a builder's accepted keyboard handler cannot cancel that model before focus moves.
VERDICT: BLOCKS — custom keyboard handling can silently lose to built-in TreeList navigation.
| slot | verdict |
|---|---|
| PROBLEM | clear |
| SOLUTION | note — one handler path incomplete |
| ARCHITECTURE | BLOCKS — TreeList event has nested owners in the wrong order |
| IMPACT | BLOCKS — consumer cancellation arrives after focus moves |
| API | BLOCKS — TreeList onKeyDown is not consumer-first |
| THEMING | clear |
| BREAKING | note — intended additive behavior/visual output |
| PERFORMANCE | clear |
| VISUAL | clear — sensored pair, three intended boxes only |
| A11Y & I18N | BLOCKS — custom keyboard path cannot cancel navigation |
GOAL: partly met — all neutral attributes and nine owned-handler/component paths work, but real Chromium proves TreeList still executes built-in ArrowDown before the consumer can cancel it.
DISPOSITION: TreeList keyboard ordering → blocks this pinned head now. This is one root defect cross-referenced by ARCHITECTURE, IMPACT, API, and A11Y, not four defects.
ADVICE: bounded direction — preserve TreeList's semantic naming/root targeting, but make the accepted consumer key handler run before focus/toggle behavior; prove it by adding a preventDefault() cancellation case beside the existing coexistence test. The exact mechanism may stay with the component owner.
AUTHOR CAN PROCEED: yes — acceptance criteria are complete: on the TreeList public handler path, consumer preventDefault() runs before built-in navigation/toggle, ArrowDown leaves focus/tab stop unchanged, and ordinary non-canceling handlers still observe the event while the APG model works.
WORST OUTCOME: “A builder reserves a tree key with the accepted consumer handler, but focus still moves before their cancellation runs, giving the end user conflicting keyboard behavior.” → request changes
JUDGEMENT NEEDED: none — this is a proven defect against the existing consumer-first handler contract, not a new API/design decision.
COMMENT — semantic request changes
- [BLOCKS] TreeList consumer cancellation runs after built-in navigation
→ a builder reserves ArrowDown with
preventDefault(), but the user's focus still moves to the next tree item ·packages/core/src/TreeList/TreeList.tsx:343-358
Independent confirmation: source/event phase shows inner ul handling before outer div; real Chromium records treeConsumerSaw: "Tree two", second item tabindex="0", while the NavHeadingMenu control records "Menu one" and keeps focus there.
Historical attribution: relative to the requested head's merge-base, af004c6 introduces this incomplete forwarding path. #5493 later landed the same TreeList split on current main and the live #5563 dropped TreeList; that changes current ownership, not the semantic verdict on the pinned bytes, and is why no public action follows this record.
A TreeList builder who calls
preventDefault()in the acceptedonKeyDownpath still moves the user's focus before their handler runs; this is the one blocker. Atpackages/core/src/TreeList/TreeList.tsx:343-358, the consumer handler sits on the outer<div>after the inner tree model. Could we make consumer cancellation run first and cover that case? Thanks—the other target and precedence work is thorough;NavHeadingMenuhas the consumer-first shape.[Reviewed by Robohands]
Public review length: 71 words before attribution (request-changes cap: 150). Required GitHub action would be COMMENT, carrying semantic request changes, because the author is cixzhang. No public action was taken.
None. The one finding is cross-cutting event ownership and belongs in the summary; the exact causal range is included there.
- Timestamp still splits caller styling onto Text and neutral props/ref onto
<time>; that target split predates this PR and is not charged. - Top-nav drawer/mobile modes still do not forward the explicit
ref;refis not part of BaseProps and this behavior predates the pass-through fix. -
TypeaheadItem.item.elementstill bypasses rest/style/ref by design; the body states the caller owns that ReactNode and this PR does not invent clone semantics.
TIME total 59m setup 7m exact-head worktree + cloned install + one Storybook-config build; warm main reused: no (same-worktree baseline arm) reading 8m kit, critic rules, fresh fork/public wikis, PR edit history, diff, tests, siblings measuring 11m 448 focused tests, core typecheck, 5 jsdom probes, 2 Chromium interaction arms, 2 sensored frames, pixel classification writing 10m full presentation + critic pass + rewrite + wiki record waste 23m Storybook browser auto-open timeout; capture readiness/selector corrections; PIL unavailable; wiki evidence whitespace retry and turn latency
- Safari/WebKit behavior; this Mac's Playwright harness supports Chromium only.
- The file viewer exposed image metadata rather than inline pixels, so visual judgement used fail-closed sensor receipts plus an exhaustive pixel diff mapped to exact DOM boxes.
- Focused Vitest: 10 files, 448/448 passed (log).
- Core typecheck: passed (log).
- Targeted jsdom DOM probe: 5/5 passed, including TreeList's late-cancellation observation and three consumer-first controls (log).
- Real Chromium: TreeList consumer observed/focused “Tree two”; NavHeadingMenu control observed/kept “Menu one” (result).
- Exact-head CI: build, test, lint, a11y, RTL, Storybook, theme layers, and smoke passed; visual payload skipped due the 344/240 budget (run · visual verdict).
- Manual visual pair: both sensor receipts passed; exhaustive pixel diff isolated the three intended caller-style outlines and zero pixels elsewhere.
- Diff hygiene:
git diff --checkclean; public-leak scan clean; no changed dependency/effect/listener/observer/state line.
DRAFT: COMMENT with semantic request changes for TreeList consumer cancellation running after built-in navigation.
CRITIQUE 1: failed R2c because a COMMENT action cannot rely on GitHub's review state and the public text said only that a collision “remains,” not that it blocks. Failed R29 because the six-decision solution did not close the >2 smell. Needed an explicit R3i attribution because current main later acquired the same split through #5493. Visual structure passed R38 only provisionally until the linked assets were pushed and resolved. The block itself passed R1i, R14, and R16g: ordinary accepted code reaches it; the exact causal range exists; source event phase and real Chromium independently agree.
REWRITE: added “This is the one blocker,” recorded that every decision traces to the one stated defect, and separated the pinned-head verdict from current-main ownership. The final standing impact gate then moved the affected builder and broken focus behavior to the first sentence. No finding or semantic verdict changed.
CRITIQUE 1 RESULT AFTER REWRITE: passes R1/R1i, R2 (71 words), R3/R4/R5, R12 attribution/voice, R13 inherited-debt exclusion, R14 exact anchor/head, R16/R36/R37 full presentation and proceed criteria, R16g independent confirmation, R39 manual obligation, and matching WORST OUTCOME. Final R38 gate is the durable wiki push verified below.
Nothing was posted to the PR. The proposed public COMMENT text is archived exactly as drafted; no GitHub review, comment, approval, request-changes action, merge, label change, or contributor-branch push was made.
The finding persists. #5563 merged at head 84f3a176516b0ee34daeb3acb969f997a0fe67ba as d1807b87, after narrowing to seven components and dropping TreeList. Current origin/main at cddf57e5 therefore retains #5493's split: ...restProps puts the consumer handler on the outer <div>, while the inner ul[role="tree"] runs handleKeyDown first (source).
A focused acceptance test was run against that exact current-main commit. It failed 1/1: after onKeyDown={event => event.preventDefault()} and ArrowDown, focus landed on Two with tabindex="0"; One became tabindex="-1" (log). A builder reserving ArrowDown therefore still cannot stop the user's focus from moving.
GitHub issue search found no issue matching TreeList consumer onKeyDown, preventDefault, or late focus movement. #5476 is closed and tracks the original dropped-forwarding defect; #3343 is the broader composite-navigation program and does not carry this handler-contract reproduction or acceptance test.
Filed #5584, “TreeList consumer onKeyDown cannot cancel built-in navigation,” with the current-main anchor, exact failing test, builder/user impact, and acceptance criteria. It now owns the live fix. The earlier statement that no public action followed this record is superseded by this reconciliation: no PR comment was posted because the PR had merged, but the persistent defect did not evaporate.
AUTHOR CAN PROCEED: yes — #5584 has the complete acceptance test.