Repository navigation
Conversation
… too architecture:interaction-modality INV3: one focus move has one owning indicator. Item and Token drew their ring with focusWithin (:has(:focus-visible)), which matches any focused descendant, so a control in an Item's start or end slot, or a Token's remove button, drew its own ring and the row or token drew a second around it; a row with no action of its own rang too. A new shared style, focusWithinOwner, rings a wrapper only for the direct child marked data-astryx-focus-owner (FOCUS_OWNER_ATTRIBUTE). Item marks its invisible button and link (delegation mode keeps focusWithin: the nested control is the row's sole tab stop by contract); Token marks its button and link. Other controls ring themselves. New stories (Item With End Actions, Token Clickable With Remove) and FocusRingOwner.a11y.chromium.spec.ts: one ring at every tab stop at 1280 and 390 px; it fails on main (two rings while Share or Remove is focused). focusRings.ts is a shared helper for such specs.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
meta-cla
Bot
added
the
CLA Signed
|
AI review status for this pull request.
|
![astracat-bot[bot]](https://avatars.githubusercontent.com/in/4796231?s=60&v=4){kind=small}
There was a problem hiding this comment.
The focus-ring fix works: each tab stop in the new Item and Token stories now paints one outline, as component:Token AR4 and architecture:interaction-modality INV3 require. Approval waits on one decision: focusWithinOwner and FOCUS_OWNER_ATTRIBUTE are reachable from the published @astryxdesign/core/utils and @astryxdesign/core/utils/focusOutline.stylex entry points, which makes them public API under architecture:public-component-api INV12 even though the description calls them internal, and no current spec admits them, so an engineering owner either admits them by commenting /approve-spec <full head SHA> on this pull request (the author may do so as an engineering owner) or asks for the owner condition to stay component-local in Item and Token so nothing new is exported. Independently, please fix two displaced doc comments:
apps/storybook/stories/Item.stories.tsx:/** Disabled items. */now opens the With End Actions description and leaves Disabled with none; move it back aboveexport const Disabled.packages/core/src/utils/focusOutline.stylex.ts:FOCUS_OWNER_ATTRIBUTEsits betweenFOCUS_OUTLINE_PARTSand its JSDoc, so that export loses its documentation; declare the constant above that block or afterFOCUS_OUTLINE_PARTS_NONE.
[Automated review]
| }; | ||
|
|
||
| /** Disabled items. */ | ||
| /** |
There was a problem hiding this comment.
This block landed under /** Disabled items. */ (line 254), so Storybook now gives With End Actions the description "Disabled items." followed by this text, and Disabled (line 292) has none. Move /** Disabled items. */ down to sit directly above export const Disabled.
| * ``` | ||
| */ | ||
| /** Marks the direct child a `focusWithinOwner` wrapper rings for. */ | ||
| export const FOCUS_OWNER_ATTRIBUTE = 'data-astryx-focus-owner'; |
There was a problem hiding this comment.
This sits between the JSDoc at lines 61-84 and FOCUS_OUTLINE_PARTS (line 88), so FOCUS_OUTLINE_PARTS loses its documentation, including the warning that it is NOT usable inside another file's stylex.create. Declare the constant above that block or after FOCUS_OUTLINE_PARTS_NONE.
| * so a nested component's own owner never rings this wrapper. Pair it | ||
| * with {@link focusOutlineStyles.suppressed} on the owner. | ||
| */ | ||
| focusWithinOwner: { |
There was a problem hiding this comment.
focusWithinOwner is reachable through focusOutlineStyles and focusOutlineProps from @astryxdesign/core/utils, and with FOCUS_OWNER_ATTRIBUTE from @astryxdesign/core/utils/focusOutline.stylex. Under architecture:public-component-api INV12 that makes them public API despite the internal label, so an engineering owner needs to admit them; otherwise the owner condition can stay component-local in Item and Token, composing the focusVars tokens, so nothing new is exported.
| '@astryxdesign/core': patch | ||
| --- | ||
|
|
||
| [fix] Focusing a control inside an `Item` (an action in `startContent` or `endContent`) or a `Token`'s remove button no longer also rings the whole row or token: each tab stop shows one focus ring. A row or token rings only for its own action, and a row without one draws no ring. |
There was a problem hiding this comment.
Suggestion: delegation rows (CheckboxListItem, RadioListItem, any Item with interactiveRef) keep the any-descendant ring, so an endContent control there still shows the row's ring too. Consider scoping this sentence to rows and tokens that have their own button or link.
github-actions
Bot
added
the
needs:design-review
…abled story's doc comment CodeBunny: focusWithinOwner and FOCUS_OWNER_ATTRIBUTE were reachable from the published @astryxdesign/core/utils and utils/focusOutline.stylex entry points, which would make them public API (public-component-api INV12). They move to src/internal/focusOwner.ts, which no entry point exports, so nothing new is published and focusOutline.stylex.ts is unchanged from main. Item.stories: '/** Disabled items. */' goes back above Disabled.
….stylex.ts The repo's focus-outline-shared lint rule allows rings only in a focusOutline.stylex file; the internal file keeps that rule and the shared tokens, and no package entry point exports it.
PR Analysis Report
📚 Storybook Preview🧪 Sandbox PreviewModified ComponentsItem (@astryxdesign/core) · View in Storybook
Token (@astryxdesign/core) · View in Storybook
Component Size
Sizes are gzip. Import counts the JavaScript the import loads eagerly; CSS counts the package stylesheet rules referenced by the modules it can load from its own package, lazily loaded ones included. Bundle Size SummaryGzip figures measure each package's entry file, not the code an import pulls in.
Published Package Size
10 other published packages are unchanged. Download is the gzip tarball npm fetches; unpacked is what lands in Accessibility AuditStatus: Accessibility did not run on this pull request; add Generated by PR Enrichment workflow | Storybook | Sandbox | View full report |
Follows #8869: focusWithinOwner and FOCUS_OWNER_ATTRIBUTE now come from the unexported internal/focusOutline.stylex.ts, so nothing new is published.
… ring Follows #8869: focusWithinOwner and FOCUS_OWNER_ATTRIBUTE come from the unexported internal/focusOutline.stylex.ts (focusOwnerStyles for the two selectors' stylex.props lists), so nothing new is published.
There was a problem hiding this comment.
Requesting changes: flat Item rows and both token structures now show one focus ring per tab stop, but two regressions need fixing first.
- In a numbered list (
<List listStyle="decimal">), aListItemwithonClickorhrefnow shows no focus indicator at all, because Item wraps the marked button in its marker-column span and:has(> [data-astryx-focus-owner]:focus-visible)only matches a direct child, so please let the condition reach it and add a numbered row toFocusRingOwner.a11y.chromium.spec.ts. astryx swizzle Itemandastryx swizzle Tokennow emit an import from@astryxdesign/core/internal/focusOutline.stylex, whichpackages/core/package.jsondoes not export, so please keep the owner condition in each component's own directory or move it to an exported utility that an engineering owner admits.
[Automated review]
| focusWithinOwner: { | ||
| outlineWidth: { | ||
| default: '0', | ||
| ':has(> [data-astryx-focus-owner]:focus-visible)': |
There was a problem hiding this comment.
In a numbered list (<List listStyle="decimal">), Item wraps its content in the markerContent span (packages/core/src/Item/Item.tsx:936-943), so the marked button or link is a grandchild of the <li> and this direct-child condition never matches. The button and link suppress their own outline, so a clickable or link numbered row shows no focus indicator at all (0 painted outlines in Chromium, 1 on the base). Let the condition reach the owner through that wrapper, for example :has(> [data-astryx-focus-owner]:focus-visible, > * > [data-astryx-focus-owner]:focus-visible), and add a numbered row to the Chromium spec.
| import { | ||
| FOCUS_OWNER_ATTRIBUTE, | ||
| focusWithinOwnerProps, | ||
| } from '../internal/focusOutline.stylex'; |
There was a problem hiding this comment.
astryx swizzle rewrites this import to @astryxdesign/core/internal/focusOutline.stylex (packages/cli/api/swizzle/copy/copy.mjs:71-73), which packages/core/package.json does not export, so a swizzled Item, and a swizzled Token (Token.tsx:43), fails with ERR_PACKAGE_PATH_NOT_EXPORTED. An ./internal/* entry would not help consumer copies, since those subpaths are reserved for Core and Lab, so keep the owner condition in the component's own directory or move it to an exported utility that an engineering owner admits.
…and Token resolve the owner module CodeBunny on #8869: - In a numbered List, Item wraps a row's button or link in the content cell beside the marker, so the owner condition's direct-child :has() missed it and the row showed no ring. The cell now carries data-astryx-focus-owner-group, and the condition also matches an owner one level down inside such a group. A numbered story and a Chromium case check one ring for a button row and a link row. - Swizzled Item and Token import internal/focusOutline.stylex, which Core did not export. It is now a ./internal/* entry like internal/touch: reachable for Core, Lab and swizzled copies, outside the compatibility promise (architecture:public-component-api INV12).
There was a problem hiding this comment.
Requesting changes: Item rows (flat and numbered) and both token structures now show one focus ring per tab stop, but two things need fixing first.
astryx swizzle Itemandastryx swizzle Tokennow emit an import of@astryxdesign/core/internal/focusOutline.stylex, and./internal/*subpaths are reserved for Astryx's own packages and sit outside the compatibility promise, so please define the owner condition in each component's own styles fromfocusVarsand drop the new export, or have an engineering owner admit it as a public utility.- The
lintcheck fails onpackages/core/src/internal/focusOutline.stylex.ts:59(@typescript-eslint/no-unnecessary-type-assertion); dropping theas neveronstyles.focusWithinOwnerfixes it.
The inline comments cover three smaller fixes: the module's stale header, the browser spec's @pr-a11y scope line, and the changeset's wording about delegation rows.
[Automated review]
| "types": "./dist/internal/tokenizer-presentation.d.ts", | ||
| "default": "./dist/internal/tokenizer-presentation.js" | ||
| }, | ||
| "./internal/focusOutline.stylex": { |
There was a problem hiding this comment.
This entry exists so a swizzled Item or Token resolves @astryxdesign/core/internal/focusOutline.stylex, but ./internal/* subpaths are reserved for Astryx's own packages, outside the compatibility promise and limited by lint to Core and Lab (AST-017 FR21). An ejected copy is frozen at one version, so a later patch that changes focusWithinOwnerProps would break its build without a release signal. Defining the owner condition in Item's and Token's own stylex.create from focusVars (already public through theme/tokens.stylex) would let swizzle copy it and make this entry, and the sync-exports.js line, unnecessary.
|
|
||
| /** The owner ring, with caller styles last (as focusOutlineProps does). */ | ||
| export function focusWithinOwnerProps(...extra: unknown[]) { | ||
| return stylex.props(styles.focusWithinOwner as never, ...(extra as never[])); |
There was a problem hiding this comment.
CI's lint job fails here: @typescript-eslint/no-unnecessary-type-assertion on styles.focusWithinOwner as never (an error in strict mode, which CI enables). Dropping that as never clears it.
| * @output FOCUS_OWNER_ATTRIBUTE and focusOwnerProps, the ring a wrapper paints | ||
| * only for its marked primary control | ||
| * @position The internal half of the focus outline utility: same tokens as | ||
| * utils/focusOutline.stylex, but no published entry point exports it, so the | ||
| * owner marker and its ring stay a detail of Item and Token | ||
| * (`architecture:public-component-api` INV12) |
There was a problem hiding this comment.
The header names focusOwnerProps (the export is focusWithinOwnerProps), leaves out FOCUS_OWNER_GROUP_ATTRIBUTE, and says no published entry point exports the module, which the new package.json entry makes untrue. utils/focusOutline.stylex.ts:14-16 also still says every ring except Switch's is drawn there.
| @@ -0,0 +1,90 @@ | |||
| // Copyright (c) Meta Platforms, Inc. and affiliates. | |||
| // @pr-a11y scope ^packages/core/src/Item/ ^packages/core/src/Token/ ^packages/core/src/utils/focusOutline ^apps/storybook/stories/(Item|Token) | |||
There was a problem hiding this comment.
This routing line still points at ^packages/core/src/utils/focusOutline; the condition this spec proves lives in packages/core/src/internal/focusOutline.stylex.ts, so a pull request that changes only that file will not run it. Adding ^packages/core/src/internal/focusOutline fixes that.
| '@astryxdesign/core': patch | ||
| --- | ||
|
|
||
| [fix] Focusing a control inside an `Item` (an action in `startContent` or `endContent`) or a `Token`'s remove button no longer also rings the whole row or token: each tab stop shows one focus ring. A row or token rings only for its own action, and a row without one draws no ring. |
There was a problem hiding this comment.
Delegation rows (CheckboxListItem, RadioListItem, any Item with interactiveRef) keep the any-descendant ring, so an end control there still shows the row's outline too (2 outlines in Chromium). Consider scoping this sentence to rows and tokens that have their own button or link.
…, privateAttributes/focusOwner.stylex.ts Per architecture:interaction-modality INV3 and public-component-api INV23: the private data-astryx-focus-owner attribute, its group attribute and their selector are written once, in privateAttributes/focusOwner.stylex.ts, which exports only the shared focusWithinOwner wrapper ring and the props that set the attributes. Item and Token spread those exports and never write the attribute; TokenLink takes them through its link props without typing the name. No package entry exports the module: the ./internal/focusOutline.stylex entry and its export-sync line are gone. focus-outline-shared allows the owner module to write its ring. A source-build test checks the compiled ring sits on the wrapper, keyed to the marked control's :focus-visible, never :has(...) * on every descendant; a Chromium case checks a Token inside an Item shows one ring at each stop.
…ner-item-token # Conflicts: # packages/core/src/Item/Item.tsx # packages/core/src/Token/Token.tsx # packages/core/src/Token/TokenLink.tsx
There was a problem hiding this comment.
The fix works where it applies: in the new stories, each tab stop in an Item row with its own action, a Token nested in a row, and a clickable or link Token with Remove now paints one focus ring instead of two. Approval waits on an engineering owner deciding whether packages/core/src/privateAttributes/focusOwner.stylex.ts and its private-attribute convention become the owner of the wrapper focus ring, which #9182, #9183 and #9190 propose but no current record establishes; an engineering owner records that decision by approving those records or by commenting /approve-spec <full head SHA> on this pull request, and the author may do so as an engineering owner. Two fixes are needed either way:
astryx swizzle Itemandastryx swizzle Tokennow emit an import of@astryxdesign/core/privateAttributes/focusOwner.stylex, which Core does not export, so the swizzle copy change has to land in or before this pull request.- The changeset says no control inside an
Itemrings the row any more, but delegation rows (CheckboxListItem,RadioListItem, anyItemwithinteractiveRef) still show two rings; scope it to rows with their ownonClickorhref.
[Automated review]
| '@astryxdesign/core': patch | ||
| --- | ||
|
|
||
| [fix] Focusing a control inside an `Item` (an action in `startContent` or `endContent`) or a `Token`'s remove button no longer also rings the whole row or token: each tab stop shows one focus ring. A row or token rings only for its own action, and a row without one draws no ring. |
There was a problem hiding this comment.
Delegation rows (CheckboxListItem, RadioListItem, any Item with interactiveRef) keep the any-descendant ring (packages/core/src/Item/Item.tsx:924), so a button in their endContent still shows the row's ring beside its own. Please scope this to rows with their own onClick or href, for example: "Focusing a control in the start or end slot of an Item with its own onClick or href, or a Token's remove button, no longer also rings the whole row or token."
| focusOwnerGroupProps, | ||
| focusOwnerProps, | ||
| focusWithinOwner, | ||
| } from '../privateAttributes/focusOwner.stylex'; |
There was a problem hiding this comment.
astryx swizzle rewrites this import to @astryxdesign/core/privateAttributes/focusOwner.stylex (packages/cli/api/swizzle/copy/copy.mjs:71-73), which packages/core/package.json does not export, so a swizzled Item, and a swizzled Token (Token.tsx:43), no longer resolves its imports until the swizzle copy change lands.
| @@ -0,0 +1,100 @@ | |||
| // Copyright (c) Meta Platforms, Inc. and affiliates. | |||
| // @pr-a11y scope ^packages/core/src/Item/ ^packages/core/src/Token/ ^packages/core/src/utils/focusOutline ^apps/storybook/stories/(Item|Token) | |||
There was a problem hiding this comment.
This routing line names ^packages/core/src/utils/focusOutline, which this pull request leaves unchanged, but not the module the spec proves; adding ^packages/core/src/privateAttributes/focusOwner makes a change to that module alone run this spec.
Part 1 of 3 for #8850: Item and Token. ClickableCard and SelectableCard follow in part 2, Thumbnail and Selector in part 3. ListItem gets its own follow-up after #8803.
User impact
A keyboard user who tabs to an action inside a row (a Share button in an
Item'sendContent) or to aToken's remove button sees two focus rings: the control's own, and a second around the whole row or token. That makes it unclear which one has focus. A row with no action of its own rang too. Now each tab stop shows one ring.Problem and solution fit
ItemandTokenpaint their ring withfocusOutlineStyles.focusWithin, which compiles to:has(:focus-visible). That matches any focused descendant, not only the control the wrapper paints for.privateAttributes/focusOwner.stylex.ts(architecture:public-component-apiINV23). It writes the attribute and its selector once and exports only the shared wrapper ringfocusWithinOwnerand the props that mark the control (focusOwnerProps,focusOwnerGroupProps). A wrapper rings only while its marked control, a direct child or one level down inside a marked group, has keyboard focus. No package entry exports the module.focusOwnerGroupProps, so a numbered row keeps its ring. Delegation mode (interactiveRef) keepsfocusWithin, because its nested control is the row's sole tab stop by contract.Current specification
architecture:interaction-modalityINV3 (current since spec(interaction-modality): wrapper focus rings come from one shared owner module #9183): a Core wrapper that rings for one primary control takes the shared focus-owner style and owner attribute fromprivateAttributes/focusOwner.stylex.ts; no component writes the attribute or its selector.architecture:public-component-apiINV23 (spec(public-component-api): each private data attribute has one owner module (INV23) #9182): each private data attribute has one owner module.architecture:interaction-modalityINV3 (current): "One focus move has one owning indicator … sibling controls paint their own focus". Its failure signal lists "two indicators".Evidence
FocusRingOwner.a11y.chromium.spec.ts(real Chromium, 1280 and 390 px): tabs through every stop and asserts exactly one painted outline at each. Onmainit fails with two rings while Share or Remove is focused. It uses a new shared helper,__tests__/focusRings.ts(ringedElements), which lane 4 can reuse for [Bug] Seven tab stops draw the browser's default focus ring instead of the theme's (Table, CodeBlock, Carousel, ChatMessageList, Tooltip, HoverCard) #8852.focusOwner.source-build.test.mjscompiles the owner module and checks every ring rule sits on the wrapper, keyed to the marked control's:focus-visible, never:has(...) *on every descendant.<List listStyle="decimal">) and asserts one ring at each.Scope
utils/focusOutline.stylex.tsis unchanged. Thefocus-outline-sharedlint allows the owner module to write its ring.Changeset:
[fix]patch for@astryxdesign/core.LOC
+421 / −13: about 70 lines in the components, the owner module and the lint rule, 80 in stories, about 190 in the Chromium spec, its ring helper and the compiled-CSS test, plus the changeset.
Merge gate
Merges after the swizzle copy change, which makes
astryx swizzle Itemandastryx swizzle Tokencopy the owner module into the ejected output.