fix(indicator): omit tabIndex from IndicatorProps - #4947
Merged
Conversation
…idden subtree Indicators are unconditionally aria-hidden, so a focusable one is a focusable node inside a hidden subtree -- axe aria-hidden-focus, measured in real Chromium going from 0 violations to 1 the moment tabIndex is forwarded. #4921 created that trap: before it, a consumer could pair tabIndex with aria-hidden="false" and be valid. `tabIndex` joins the props IndicatorProps omits, and is dropped at runtime by a small internal helper for the case the type cannot see: TypeScript exempts hyphenated JSX attributes from excess-property checking, and a spread -- `<Mark {...props} />`, the ordinary host idiom -- bypasses the check for every member. Only tabIndex is removed; aria-hidden is settled by attribute order, and role/aria-label are inert inside a hidden subtree, so they are left alone rather than silently deleted. This is the salvage from #4938, which was closed: that PR escalated the same change to a [breaking] changeset plus an `astryx upgrade` codemod on the theory that IndicatorProps is public API. It is not, yet -- core@0.3.0 published 2026-08-05 with no ./Indicator subpath, Indicator landed 2026-08-11, and its changeset is still pending, so the layer ships for the first time next release. Nothing to break, nobody to migrate. Plain [fix] patch is the honest record. Closing that PR also surfaced a bug in it: the type half of this change was lost in a revert/restore during its development and never made it into the commit, while its changeset advertised it. Only the runtime drop shipped, and the tests passed because they exercise runtime. Both halves are here, and I checked the type change survived this time. Also corrects two doc claims, both from the same audit: - Indicator.doc.mjs:55 still said a replacement must render children "when present" -- verbatim the wording #4921 replaced in bestPractices BECAUSE "present" is the trap. - "passing role is a compile error" holds only for a literal attribute; a spread bypasses the check. A test pins that, deliberately without @ts-expect-error, so it fails the day TypeScript changes its mind. Verified: 4 new tests red against origin/main; 391 pass across the subject and every consumer; core tsc, typecheck:docs and strict lint clean.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
meta-cla
Bot
added
the
CLA Signed
Contributor
PR Analysis Report📚 Storybook PreviewView Storybook for this PR 🧪 Sandbox PreviewView Sandbox for this PR Modified ComponentsIndicator (@astryxdesign/core) · View in Storybook
Bundle Size Summary
Accessibility AuditStatus: No accessibility violations detected. Generated by PR Enrichment workflow | Storybook | Sandbox | View full report |
Cindy's review, and she is right on both counts. Nobody sets tabIndex on an indicator -- grepped every call site in core, zero. And `tabIndex` is a plain JS identifier, so excess-property checking applies and the Omit rejects the literal on its own; I verified the @ts-expect-error is consumed. The type was already doing the work. So stripSmuggledProps.ts is deleted and all three components go back to a plain {...rest}. What is left is one word in an existing Omit. The module was also inconsistent, which I should have noticed while writing it: we deliberately do NOT strip `role` or `aria-label` -- they are inert inside an aria-hidden subtree -- so singling out tabIndex for a runtime guard defended one spread-only case while leaving its siblings, at the cost of a module and an indirection. The runtime tests go with it; the spread case is now documented in the test that shows it compiling, with the reason it is left alone. Diff is now: one word in types.ts, one doc sentence, tests, changeset. No .tsx source change at all.
cixzhang
changed the title
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Slimmed after review — this is now one word in an existing
Omit, plus two doc corrections. No.tsxsource change at all.What's left
IndicatorPropsalready omitsaria-hidden,role,aria-labelandaria-labelledby. This addstabIndexto that list.An indicator is unconditionally
aria-hidden, so a tab stop on one is a focusable node inside a hidden subtree — an axearia-hidden-focusviolation. AndtabIndexis a plain JS identifier, so unlike the hyphenatedaria-*props, excess-property checking does apply: the omission makes a literal a compile error on its own, exactly asrolealready is. Verified — the@ts-expect-errorin the test is consumed.What I removed, and why
The first version added an internal
stripSmuggledPropsmodule and routed every{...rest}through it, to catch the spread case the type can't see. Cindy pushed back and was right on both counts:core: zero indicators receivetabIndex.It was also inconsistent, which I should have caught while writing it: we deliberately don't strip
roleoraria-label— they're inert inside anaria-hiddensubtree — so singling outtabIndexfor a runtime guard defended one spread-only case while leaving its siblings, at the cost of a module and an indirection on every render path.The spread case is now documented in the test that shows it compiling, with the reason it's left alone, rather than defended by a mechanism.
Also
Indicator.doc.mjs:55still told a replacement to render children "when present" — verbatim the wording fix(indicator): the decorative contract is not a caller's to switch off (#4918) #4921 replaced inbestPracticesbecause "present" is the trap.roleis a compile error" holds for a literal attribute; a spread bypasses the check.Test plan
242 pass across Indicator + CheckboxInput + RadioList + Selector; core
tscclean;typecheck:docsclean;check:changesets63 valid.The type assertion covers both props and is self-policing — if either stops being rejected, the unused
@ts-expect-errorfails the build.Rubric
This finding shouldn't have been a FIX, and the rubric now says so. v1.4 (wiki
0666f61) adds "a finding needs a reachable victim": a defect that needs code the type rejects, that no call site contains and no doc suggests, is a NIT at most — with a corollary that a finding closable by a type change should say so, instead of inviting a runtime mechanism. This PR is cited there as the worked example.Close it if you'd still rather not carry the one-word change; the doc fixes could ride along with anything else.