Review 5420
freddymeta (contributor; head on facebook/astryx) · OPEN, changes requested · view on GitHub
Verdict: request changes — the mechanism, not the PR.
Head reviewed: 9b0d421e70466984fb76715485c0d43fb05e7366, unmoved across three
gates. No prior reviews. One of a SET of four rows of
#5417 — set-level findings on
Review-5426.
Three gates, and the verdict moved once, on a design ruling rather than on evidence:
| gate | verdict | what moved it |
|---|---|---|
| 1-2 | approve & merge | everything measurable checked out; both decisions held |
| 3 | request changes | Cindy ruled one target, not two, with the glyph on currentColor — and checking it turned up a live a11y defect |
Astryx paints the sort arrow and filter funnel bare at 0.35 opacity until you
hover the header; EPS wants them legible at rest so a column reads as sortable
before you touch it. Both controls render inside Astryx's own plugins — no class,
no data attribute, no wrapper to interpose, no renderX prop. The only move
today is .astryx-table-header-cell button[aria-haspopup='dialog'], a CSS rule
that depends on an ARIA attribute never changing.
1. three targets: the sort <button> and its icon wrapper (both reflecting
direction), the filter trigger (state: active)
— useTableSortable.tsx:312, :320 · useTableFiltering.tsx:922
2. themingTargets.test.ts discovers component sources at ANY depth under src
— themingTargets.test.ts:361 sourceDirs()
Decision 2 is right and should not be split out. He offered; he was correct not to. Confirmed three ways rather than taken on his word:
old guard + all four PRs merged → 294 GREEN ← the fix is load-bearing
new guard + plain main → 294 GREEN ← catches nothing today
new guard + all four PRs merged → 302 GREEN
R14b answered on its own evidence: the catch on main is zero because no
nested source rendered a themeProps() class before this PR — the guard closes
a hole rather than fixing a break.
Nobody today — nothing consumes the keys.
Builders writing a theme, once one does. Under the PR as written, a themer restyles the sort affordance by reaching two classes and has to know which one carries which property: colour on one, opacity on the other, silent when wrong. Under the ruling they set a colour on the button and the glyph follows, including on hover.
End users, once the opacity comes off — and this is the part neither the PR nor the ruling anticipated. The resting arrows currently fail WCAG AA and pass afterwards. See below.
A11y in the good direction, either way: a theme can stop keying on
aria-haspopup='dialog', which makes that attribute safe to change.
These work in a production build — #5410 merged the same morning.
no API change. color="inherit" is an existing Icon prop value; sourceDirs()
is module-private to the test file.
As written:
+ astryx-table-sort-button → the sort <button>, reflecting direction (public, permanent)
+ astryx-table-sort-indicator → the <span> wrapping its Icon, direction (public, permanent)
+ astryx-table-filter-button → the funnel <button>, state: active (public, permanent)
Asked for astryx-table-sort-indicator to be deleted before it ships — the
cheap moment, since nothing consumes it and removing it later is breaking.
table-sort-button and table-filter-button are a named class with merged
members — codeblock-copy-button, timestamp-copy-button, input-clear-button,
token-remove-button, tab-scroll-button. Both keep their place.
table-sort-indicator is the one that fails, and for the reason R1g-surface
exists: a second spelling of one axis. Same shape as Step.icon /
Step.indicator on #5201 — the
duplicate axis that only becomes obvious when both lines are written out.
- API — no.
- Visual — no as written; yes under the ruling, which repaints the resting glyph in every sortable table. That is the substance, not a side effect.
- Theme — no.
Far side of the bound. The unsorted state is what EPS wants and
direction is null there — no class token, no data-direction, so only base
reaches it. Verified it does, in both versions.
Nothing, either version. Two mergeProps calls replacing two stylex.props
spreads, once per sortable column — not per row. No effect, no listener, no
observer, no layout read. The ruling's version makes one fewer themeProps call
than the PR.
Real Chromium, 760px, Storybook dev at the PR head. Frames published to
assets/pr-5420 on the fork; two embedded in the posted review.
| what it shows | frame |
|---|---|
| colour on the button — the header text follows too |
ruling__one-target-colour.png (posted)
|
| the ruling working — themed glyph, untouched header |
shape__C-ruling-themed.png (posted)
|
opacity: 0.35 on the button — every header at 35% |
ruling__one-target-opacity.png |
| two targets — glyph red, header untouched | ruling__two-target-glyph-only.png |
| today's unsorted rest state | shape__A-today-rest.png |
Probes: probe-kit/sort-indicator-currentcolor-5420.cjs,
sort-one-target-consequence-5420.cjs, sort-one-target-shape-5420.cjs,
sort-glyph-contrast-5420.cjs. SB_PORT=6610.
Cindy asked whether it was true today before it was written up. Four questions, all driven in real Chromium:
1. Does the Icon already paint currentColor? Yes — the glyph does; the Icon
overrides it. defaultIcons.tsx:32 makes every registry glyph
stroke: 'currentColor'. useTableSortable.tsx:330 passes
color={direction != null ? 'accent' : 'secondary'}, and colorStyles.secondary
sets the currentColor the glyph resolves against.
colour on .astryx-table-sort-button, as shipped → glyph UNCHANGED
same, with the Icon inheriting → glyph rgb(1,2,3)
Icon already has the mode (color="inherit", Icon.tsx:77-79) and 29 core
components already use it. One word, cited rather than argued (R3k).
2. Can the opacity ride on the button? No. The button contains the column
header text as well as the glyph, so anything set on it reaches the label —
opacity: 0.35 on the button fades every header to 35%
(ruling__one-target-opacity.png). That is the case for the wrapper existing,
and it is an argument about opacity, which the ruling replaces with colour.
3. Buildable as stated? Yes, with one line she did not name — the header label span has to pin its own colour, or the button's bleeds into it. With that:
A today label rgb(82,82,82) glyph rgb(115,115,115) @ opacity 0.35
B ruling, rest label rgb(82,82,82) glyph rgb(178,178,178) @ opacity 1
C ruling, themed label rgb(82,82,82) glyph rgb(200,30,30) ← ONE target
GUARANTEE rest rgb(1,2,3) → hover rgb(4,5,6) PASS
4. The filter button — her logic applies and she did not mention it. Same blocker, no label to bleed into, and the PR already gives it exactly one target. A colour set on it does not reach the glyph, so the PR claims something it does not do. That is a defect in the shipped diff, not a shape question.
Cindy pointed at Design Conventions for the value, and checking it turned the redirect from a convention alignment into an a11y fix.
Two lines of the wiki apply. The Reorder contract (Design-Conventions.md:299):
"blanket 50% opacity … is an allowed temporary contrast exception; restore normal contrast immediately on drop or cancel, and never use this treatment for a resting or persistent state."
And the colour principle (:212): "neutrals are chosen by role", with
low contrast — "any fg/bg below WCAG AA (4.5:1 text, 3:1 large/UI)" — listed as
a smell.
Measured against the real header backdrop, both schemes:
| glyph | vs header | AA (3:1 UI) | |
|---|---|---|---|
| light, today @0.35 | rgb(206,206,206) |
1.57:1 | FAIL |
| light, token alone | rgb(115,115,115) |
4.74:1 | pass |
| dark, today @0.35 | rgb(82,82,82) |
1.94:1 | FAIL |
| dark, token alone | rgb(163,163,163) |
6:1 | pass |
So no new token is needed. The component already names the right one —
color="secondary" — and the 0.35 is what stops it rendering. Dropping the
opacity lets the existing role token through, and the resting arrows go from
failing AA to passing it in both schemes.
A trap worth recording: dark mode goes through Storybook's colorMode global
(preview.tsx:125), not page.emulateMedia. The first run reported an identical
white backdrop for both schemes — the same class of error lib.cjs documents for
direction.
request changes
1. [BLOCKS] `astryx-table-sort-button` and `astryx-table-sort-indicator` are two
spellings of one axis, and neither carries the glyph's colour
→ a themer sets a colour on the sort button — the obvious target, the one the
PR's description points at — and nothing happens; they then have to
discover colour lives on the indicator and opacity does too
· useTableSortable.tsx:320, :330
confirmed twice: measured in Chromium (reaches: false; with the Icon
inheriting, true), AND read off Icon.tsx:77-79 + defaultIcons.tsx:32.
2. [BLOCKS] the same on the filter button, where there is only one target
→ the PR says the single `table-filter-button` target lets a theme restyle
the funnel; a colour set on it does not reach the glyph
· useTableFiltering.tsx:935
confirmed by the same probe: reaches: false.
3. [BLOCKS] the resting glyph fails WCAG AA in both schemes
→ the sort arrows on every table are at 1.57:1 in light and 1.94:1 in dark
against the header, where AA wants 3:1 for a UI glyph — a person scanning
a table cannot see which columns are sortable
· useTableSortable.tsx:324
confirmed twice: computed from the composite AND against the token alone
(4.74:1 / 6:1), both schemes, real backdrops.
4. [not blocking] the guard fix is right and should not be split out
· themingTargets.test.ts:361
Thanks — the problem is right, and keeping the guard fix here is right too; I checked and the old one stays green with these three unchecked.
The two sort targets are one axis, and neither carries the glyph. Set a colour on
astryx-table-sort-button— the obvious one — and nothing happens; the funnel has the same problem with only one target. Both icons wantcolor="inherit", then the colour goes on the buttons andastryx-table-sort-indicatorcomes out. Spinner went this way in #5408.
colour on the button today with the label pinned Drop the resting
opacity: 0.35with it — inline below. Composited, the arrows sit at 1.57:1 against the header where AA wants 3:1;--color-icon-secondaryalone is 4.74:1.Then a browser test to hold it: colour on the button target, assert the glyph paints it, hover, assert it follows.
theme-var-reachability.jsis the shape.If you'd rather talk it through, we're in Discord.
[Reviewed by Robohands]
Three inlines:
-
useTableSortable.tsx:327—color="inherit"on this one (it'ssecondarytwo lines down) and the button's colour reaches the glyph — the registry SVGs are alreadycurrentColor. -
useTableSortable.tsx:324— This restingopacity: 0.35is what puts the arrows at 1.57:1. Drop it and letsecondaryrender — the header label span then needs its own colour so the button's doesn't bleed into it. -
useTableFiltering.tsx:932— Same here — the funnel'sIconsets its own colour, so the single target above doesn't reach it.
144 prose words against the 150 cap.
The draft's closing paragraph was "One thing I can't settle for you — which grey it lands on is ours to pick. I'll come back with that." Cindy answered it from the wiki instead: the treatment is already ruled (opacity is drag-only) and the value is a role token the component already names. The paragraph became two clauses of citation, which is also what brought the comment under cap — the overage had been the escalation.
The contrast detail moved from the body into the inline at :324. It is
line-anchored, so that is its home (R2h), and R2 says delete or relocate rather
than compress.
-
Did
color="inherit"land on both icons —useTableSortable.tsx:330anduseTableFiltering.tsx:935? -
Did
astryx-table-sort-indicatorcome out of both the component anduseTableSortable.doc.mjs(EN + zh)? - Did the opacity come off, and did the header label get its own colour? Without the second, the button's colour bleeds into the column title — check the frame, not the diff.
-
Did the guarantee test appear? The right host is CI's
theme-layersjob (ci.yml:485), which is gated ondocsite_only != true, so it fires on a Table change — unlikepr-a11y, which scopes topackages/core/srccomponent dirs and would skip. That is the mistake #5410's own addendum caught in itself. - Re-measure the contrast after the change, both schemes. The claim is that it goes from failing to passing; that should be verified on the new head, not assumed from this record.
If nothing has moved in two days, this is the nudge ladder (R24) — one nudge.
- Which grey the rest state lands on if the team decides
secondaryat full strength is too heavy. The measurement says it passes AA; whether it reads right is a design call nobody has had to make yet, because the opacity has been hiding it. - Whether
color="inherit"changes anything under forced-colors. Both resolve through the cascade, so I expect not, and I did not drive it.
Versions: Review Loop unversioned (pre-1.0) · Component Audit Rubric 1.10
Head: df3dc14f · Verdict: request changes
The runtime work satisfies every prior ask. The remaining hold is the browser contract: the guard is not invoked by CI, reads before Storybook renders, and checks inherited color rather than the SVG's painted stroke/fill.
Review as posted:
Thanks — my last review asked for one target per affordance, button-owned color, inherited SVG paint, no resting opacity, and a browser guard. The runtime work now matches all five asks.
The guard is still the hold. It is not called by CI, and on this head it reads before Storybook renders and fails both cases. After waiting, it checks
svg.color, so a fixed stroke still passes while the glyph paints the wrong color. Could a maintainer wire it into the Storybook Chromium job, wait for each target, and assert the painted stroke/fill?[Reviewed by Robohands]