refactor(effect-schema-form-aria): move the focus ring from boxShadow to outline - #1185
Closed
schickling-assistant wants to merge 1 commit into
Conversation
… to outline Reserves 'outline' for the focus ring at all four ring sites, so a ring and a decorative shadow can coexist: StyleX writes one value per property, and a ring plus a shadow are two writers of boxShadow with no compile error when one silently wins. LiteralField already has a real elevation shadow on boxShadow for its popover -- a different element today, but a trigger is exactly the control that later grows one. VISIBLE DELTA, MEASURED -- this conversion is NOT pixel-equivalent, and the FocusRing stories added in #1183 are what caught it on their first use. Captured before and after on one host, with a self-comparison control at zero and identical canvas dimensions in every case, so this is paint and not layout: text / number / literal-trigger 3856/73920 px (5.216%) maxChannelDelta 83 optional-number (compact input) 351/61440 px (0.571%) maxChannelDelta 91 The bounding box covers the whole ring perimeter rather than the corners, so the band moves -- this is not anti-aliasing noise. 'box-shadow: 0 0 0 1px' and 'outline: 1px solid' are therefore not interchangeable, which contradicts the comment I first wrote here and which is now corrected in place at the site. THIS NEEDS A HUMAN DECISION and the PR says so: accepting the convention means accepting a small visible change to the focus ring on every field in the package. I have not tried to tune it back to byte-identical, because matching a box-shadow's exact band with outline-offset would trade a named visible delta for an unexplained coincidence. One site is called out in place: NumberField's compact input is the only ring keyed on a pseudo-class rather than an attribute, which makes it the site exposed to the upstream @stylexjs/shared priority-table defect where ':focus-visible' falls through to 40 against ':hover' at 130. Nothing writes ':hover' on outline there, so the two cannot meet -- and keeping the ring on its own property is what guarantees that rather than luck. VERIFIED: cold composite build ('tsc --build' in a fresh worktree with no dist) exits 0; zero boxShadow focus sites remain; both remaining boxShadow uses are real elevation shadows.
schickling-assistant
changed the title
Storybook Previews
Report historyPR 1185 · 2026-09-02 09:40 UTC
|
schickling-assistant
added a commit
that referenced
this pull request
Sep 2, 2026
Caught by accident and worth recording as a trap: PR #1185's branch was forked from #1183's branch rather than from #1184's, so it never contained #1184. The stack still LOOKED right, because 'gh stack link' set #1185's base to #1184 and GitHub computes a PR diff from the merge-base -- which was #1183's tip, so #1185's diff was exactly its own commit and read as correctly stacked. Setting a base does not make a branch contain that base. A clean-looking PR diff is equally consistent with 'correctly stacked' and 'forked from a sibling', and only ancestry distinguishes them: 'git merge-base --is-ancestor' over all eight heads found seven IN and one MISSING. It surfaced because a liveness probe tried to back up gate/project.unit.test.ts, the backup failed silently, and the file turned out to be untracked -- so a broken 'cp' in a throwaway check is what revealed a missing PR. The conflict was cleanly disjoint and both sides were needed: this branch carries the React alias/pin work (reactAliasRules, pinReactToConsumer) that makes cross-checkout consumers render at all, and #1184 carries the plugins option and the storybookPluginFor seam. Resolved by taking this side and re-applying #1184's threading, with the React pin kept FIRST so its alias applies to whatever a caller's transform emits. VERIFIED: both feature sets present, zero markers, and the plugin-placement unit test passes 4/4 in the merged tree -- the same test that fails 3 of 4 when the placement is reverted.
Collaborator
Author
|
Superseded by #1191, which collapses this stack onto Closing rather than merging: propagating This PR's content is in #1191, verified with This body stays as the record of the per-change evidence, which #1191 summarises but does not reproduce in full. Posted on behalf of @schickling
|
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.
Top of stack #1177, on #1184. Moves the focus ring from
boxShadowtooutlineat all four ring sites in the package.⚠ This needs a human decision
The conversion is not pixel-equivalent. Accepting the convention means accepting a small visible change to the focus ring on every field in the package. That is a design call, not mine, so this PR stays draft until someone makes it.
The measurement
The
FocusRingstories added in #1183 caught this on their first use — which is the entire argument for having landed them first.Captured before and after on one host. Self-comparison control at 0 differing pixels. Canvas dimensions identical in every case, so this is paint, not layout:
The bounding box covers the whole ring perimeter, not the corners — so the band moves. This is not anti-aliasing noise.
box-shadow: 0 0 0 1px Candoutline: 1px solid Care therefore not interchangeable, which contradicts what I first wrote in these files. That comment is corrected in place, at the site, with the numbers.I have not tuned it back to byte-identical. Matching a box-shadow's exact band with
outline-offsetwould trade a named visible delta for an unexplained coincidence, and the second is worse.Why do it at all
StyleX writes one value per property. A ring and a decorative shadow are two writers of
boxShadow, and when they meet one silently wins — no compile error, nothing for a type checker to see. That composed-shorthand collision is a confirmed defect class in this migration: the design system had 8 genuine same-element collisions across 9 files where the two coexisted.LiteralFieldalready carries a real elevation shadow onboxShadowfor its popover. Different element today — but a trigger is exactly the control that later grows one, and that is the pair that would have collided silently.One site called out in place
NumberField's compact input is the only ring keyed on a pseudo-class rather than an attribute, which makes it the site exposed to the upstream@stylexjs/shared@0.19.0priority-table defect: pseudo-classes are spelled camelCase in that table, so:focus-visiblefalls through to the unknown-pseudo default of 40 while:hoversits at 130.Nothing writes
:hoveronoutlinethere, so the two cannot meet — and keeping the ring on its own property is what guarantees that, rather than luck.Verified
tsc --buildin a fresh worktree with nodistexits 0. That is the configuration CI has, and the one a warm tree cannot see past.boxShadowfocus sites remain; both survivingboxShadowuses are real elevation shadows.What is not claimed
That the new ring looks better, or that 5.2% is acceptable. Only that it is 5.2%, that it is paint rather than layout, and that nothing else in these files changed.
Posted on behalf of @schickling
agent_identitysessionagent_personaagent_supervisoragent_toolagent_tool_versionagent_runtimetooling_profile