-
Notifications
You must be signed in to change notification settings - Fork 0
Review 5493
gonzoblasco · merged b47ad01e4a15 · view on GitHub
Verdict: approve — exact-head re-review, no public action
The fresh fork clone contained no Review-5493.md, so this page reconstructs the three posted rounds from GitHub before recording the independent fourth pass.
The first review confirmed the diagnosis but found two holes: TreeList aria-label landed on the layout <div> rather than the <ul role="tree">, and the then-included List change let caller role displace the component's contract. It asked for a changeset and suggested narrowing this PR to TreeList after #5288.
The List half was resolved and aria-label reached the tree. One finding remained: with a visible header, caller aria-labelledby replaced the generated header id, so assistive technology could hear a different name from the visible one. The author pushed 39bccc8 two minutes later.
The posted review said:
Thanks for turning this around so fast — the header names the tree now, and the headerless paths still name it. Checked the accessible names in Chromium.
[Reviewed by Robohands]
Cindy merged the PR at this head as b47ad01e4a15.
Versions. Review Loop 1.4.0 · Component Audit Rubric 1.12.
Head. 39bccc845bb778ac0fbe71d727c035493307be62.
Lane. full — prior CHANGES_REQUESTED, core accessibility semantics, and public BaseProps forwarding behavior. No fast-lane shortcut.
A screen-reader user could not identify a headerless TreeList because accepted naming props were silently dropped. After the first fix, a visible header could still be displaced by caller naming, making the accessible name disagree with the visible one.
- Forward neutral
BasePropsattributes to the root and routearia-label/aria-labelledbyto the<ul role="tree">. - When a visible header exists, its generated id owns
aria-labelledby; caller naming is used only on the headerless path.
The changeset and 49 added test lines evidence those decisions; they are not additional runtime decisions.
Existing callers that do not pass previously dropped attributes are unchanged. Callers can now name headerless trees and attach neutral root attributes; trees with visible headers keep their visible and accessible names aligned.
~ TreeListProps extends BaseProps<HTMLDivElement> (public, core barrel)
accepted types are unchanged; the existing passthrough contract now works
~ TreeList aria-label?: string / aria-labelledby?: string (public inherited props)
routed to <ul role="tree"> only when no visible header owns the name
No export, prop name, type, requiredness, or default changed.
<TreeList items={items} aria-label="File tree" />The same public call now gives the tree an accessible name.
No target added, removed, or repointed. Existing astryx-tree-list theming remains on the root <div>.
No new surface. This repairs the already-published BaseProps passthrough contract and follows merged sibling #5288.
- API — no. Existing source keeps compiling; accepted types are unchanged.
-
Visual — no. A control and an ARIA-only exact-head Chromium story produced byte-identical 900×202 PNGs (
sha256 52a9adb0…). - Theme — no. No style, token, target, class, or CSS variable changed.
- Behavior — intentional repair: previously dropped attributes now forward, and the visible header wins accessible-name precedence.
Render: no new work or render pass. Listeners: none added. Layout: no reads or writes. Bundle: no dependency or new module.
VISUAL CHECK: not applicable
WHY: the runtime delta changes only non-painting attributes on existing nodes;
it adds no element, style, class, geometry, text, or visible state.
The existing Basic and WithHeader stories do not pass the changed ARIA props, so the green exact-head pr-visual job was not treated as endpoint coverage. A temporary exact-head story rendered control and ARIA-only arms in Chromium; their 900×202 captures were byte-identical. The visible result was unchanged.
- Exact-head Chromium:
aria-labelnames the headerless tree; externalaria-labelledbynames the headerless tree; the visible header names the headered tree;External namedoes not. - Focused suite:
TreeList.test.tsx81/81 passed. - Causal replay: restoring the prior precedence made the two new header-precedence tests fail; restoring
39bccc8returned the suite to green. - Core package build passed. Changed-file Prettier passed. ESLint had no errors and one pre-existing
no-nullish-jsx-guardwarning atTreeList.tsx:344. - GitHub exact-head build, test, lint, Storybook, a11y, RTL, and visual jobs passed. The optional Vercel deployment status failed; its logs were unavailable locally. The PR was already approved and merged at this head.
Needs judgement — this is a small repair, but it changes public core DOM/ARIA behavior and followed a blocking review.
GOAL: met. The old finding is exactly resolved: headered TreeList uses the generated header id; both headerless naming paths remain available.
VERDICT: approve. No blocking or advisory finding belongs to this diff.
DISPOSITION: none. No negative finding remains to block, follow up, or accept.
ADVICE: omitted. The exact-head implementation already meets the required outcome.
AUTHOR CAN PROCEED: yes. Nothing further is required; the PR is already merged.
WORST OUTCOME CHECKED: a caller-provided label displaces the visible header and assistive technology hears a different name. The exact-head DOM, role-name queries, regression tests, and old-logic mutation all reject that outcome.
Thanks for turning this around so fast — the visible header names the tree now, and both headerless paths still name it. Rechecked all three in Chromium.
[Reviewed by Robohands]
Inlines: none.
Independent critic verdict: approve; the old finding is exactly resolved and no blocker remains. It surfaced only the existing header={false} nullish-guard gap at TreeList.tsx:344,352–353; this predates the PR, has no known call site, and was not charged to the contributor.
No current public action was allowed or taken. The exact-head approval quoted in Round 3 already existed before this re-review.
Total: 20 minutes. Setup 3 · reading 6 · measuring 6 · critic 5 · writing 3 · waste 2 (one invalid Storybook wrapper invocation and one URL-args probe that did not pass hyphenated ARIA keys; replaced with an isolated temporary story). Some phases overlapped.