Repository navigation
fix(Thumbnail, Selector, MultiSelector): a nested button rings itself, not the thumbnail or trigger too - #8874
Conversation
…, not the thumbnail or trigger too architecture:interaction-modality INV3: one focus move has one owning indicator. Thumbnail's image container and the ghost and secondary Selector and MultiSelector trigger containers rang with focusWithin, so a Thumbnail's remove button, and a trigger's clear or status button, drew their own ring and the container a second around it. Each container now uses focusWithinOwner, and the Thumbnail's Open button and each trigger button carry FOCUS_OWNER_ATTRIBUTE. FocusRingOwner's browser spec gains a Thumbnail With Remove case and ghost and secondary Clearable cases for both selectors, at 1280 and 390 px; they fail on the previous head (two rings while Remove or Clear is focused). Stacked on #8872.
meta-cla
Bot
added
the
CLA Signed
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
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.
This change looks ready to merge once #8869 and #8872 land: focusing a Thumbnail's Remove, or the Clear or status button of a ghost or secondary Selector or MultiSelector, now shows a single ring, and the new browser cases fail before the change and pass after it. One optional follow-up on the same lines: the Thumbnail's Open button still keeps the browser's default focus ring inside the container's ring, so a Thumbnail with onClick shows two rings on Open; applying focusOutlineStyles.suppressed to that button, as focusWithinOwner expects, plus an Open-and-Remove browser case would close the rest of that gap.
[Automated review]
| aria-label={t('@astryx.thumbnail.open', {accessibleName})} | ||
| aria-disabled={isInteractive ? undefined : 'true'} | ||
| {...{[FOCUS_OWNER_ATTRIBUTE]: ''}} | ||
| {...stylex.props(styles.interactiveButton)}> |
There was a problem hiding this comment.
Optional, the same gap component:Thumbnail FR4 records: this Open button keeps the browser's default focus ring (outline: auto 1px in Chromium), so focusing Open still paints two rings, the container's focusWithinOwner ring and this one. focusWithinOwner asks for the owner to be paired with focusOutlineStyles.suppressed, which the already-imported helper applies:
{...focusOutlineProps.suppressed(styles.interactiveButton)}>A Thumbnail story with both onClick and onRemove would let the browser spec cover Open too.
…-owner-thumbnail-selector
… 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.
Changes requested for the Thumbnail part; the Selector and MultiSelector changes look ready once #8869 and #8872 land. With the image container no longer ringing for Remove, Remove's own focus ring is its only indicator, and over a dark image it is nearly invisible (about 1.08:1 against the image in the default light theme), so keyboard users lose track of focus on thumbnails with showRemoveOn="always" or with both onClick and onRemove. Please keep the container ring for Remove (leave Thumbnail.tsx out of this pull request) until Remove has a focus ring that reads on any image, or add such a ring with design review. Optionally, add ^packages/core/src/internal/focusOutline to the spec's @pr-a11y scope line so changes to the owner ring rerun these browser cases.
[Automated review]
| focusOutlineProps.focusWithin( | ||
| // Rings for the Open button only; the remove button rings itself | ||
| // (`architecture:interaction-modality` INV3). | ||
| focusWithinOwnerProps( |
There was a problem hiding this comment.
Over a dark image this leaves Remove without a visible focus indicator: the container's ring, drawn on the page background, no longer shows for Remove, and Remove's own accent ring paints over the image inside the container's overflow: hidden. With a #222222 image in the default light theme the ring is about 1.08:1 against the image, so focused and resting Remove look the same, and tabbing from Open to Remove makes the only visible ring disappear. component:Thumbnail FR4 asks for one indicator per focus move. Keeping the container ring for Remove until Remove has a ring that reads on any image (or adding one with design review) would avoid the regression.
| @@ -1,19 +1,20 @@ | |||
| // Copyright (c) Meta Platforms, Inc. and affiliates. | |||
| // @pr-a11y scope ^packages/core/src/(Item|Token|ClickableCard|SelectableCard)/ ^packages/core/src/utils/focusOutline ^apps/storybook/stories/(Item|Token|ClickableCard|SelectableCard) | |||
| // @pr-a11y scope ^packages/core/src/(Item|Token|ClickableCard|SelectableCard|Thumbnail|Selector|MultiSelector)/ ^packages/core/src/utils/focusOutline ^apps/storybook/stories/(Item|Token|ClickableCard|SelectableCard|Thumbnail|Selector|MultiSelector) | |||
There was a problem hiding this comment.
Optional: the owner ring now lives in internal/focusOutline.stylex.ts, which ^packages/core/src/utils/focusOutline does not match, so a later change to that file alone will not run this spec. Adding ^packages/core/src/internal/focusOutline here would cover it.
github-actions
Bot
added
the
needs:design-review
…-owner-thumbnail-selector; Thumbnail, Selector and MultiSelector spread the focus-owner owner module's ring and props
…-owner-thumbnail-selector
There was a problem hiding this comment.
Changes requested for the Thumbnail part; the Selector and MultiSelector changes look ready once #8869 and #8872 land. Over a dark image, Remove's own focus ring (about 1.08:1 against the image in the default theme) is now its only indicator, so keyboard users lose track of focus on thumbnails with showRemoveOn="always" or with both onClick and onRemove; please keep the container ring for Remove until Remove has a ring that reads on any image, or leave Thumbnail.tsx out of this pull request. Please also narrow the changeset's "each tab stop shows one focus ring": a Thumbnail's Open button still paints two rings (the container's and the browser default), unless you suppress Open's default ring and add a browser case for it.
[Automated review]
| // Rings for the Open button only; the remove button rings itself | ||
| // (`architecture:interaction-modality` INV3). | ||
| stylex.props( | ||
| focusWithinOwner, |
There was a problem hiding this comment.
With the container no longer ringing for Remove, Remove's own ring is its only indicator, and it paints over the image inside the container's overflow: hidden. Over a dark image (#222222) in the default theme that ring measures about 1.08:1, so a focused Remove looks the same as a resting one, and tabbing from Open to Remove makes the only visible ring disappear. Keeping the container ring for Remove until Remove has a ring that reads on any image would avoid this.
| '@astryxdesign/core': patch | ||
| --- | ||
|
|
||
| [fix] Focusing a `Thumbnail`'s remove button, or the clear or status button of a ghost or secondary `Selector` or `MultiSelector`, no longer also rings the thumbnail or the whole trigger: each tab stop shows one focus ring. |
There was a problem hiding this comment.
"each tab stop shows one focus ring" is broader than the fix: a Thumbnail with onClick still paints two rings on Open (the container's and the browser's default auto ring on the button, which has no outline reset). Either reword to say the focused remove, clear or status button shows the only ring, or suppress Open's default ring and add an Open-and-Remove browser case.
Part 3 of 3 for #8850: Thumbnail, Selector and MultiSelector. Stacked on #8872 (part 2), which is stacked on #8869 (part 1,
focusWithinOwner); retarget tomainonce they land.User impact
A keyboard user who tabs to a
Thumbnail's remove button, or to the clear or status button of a ghost or secondarySelectororMultiSelector, sees two rings: the button's, and a second around the whole thumbnail or trigger. Now each tab stop shows one ring.Problem and solution fit
focusWithin, which matches any focused descendant.focusWithinOwnerfromprivateAttributes/focusOwner.stylex.ts. The Thumbnail's Open button and each trigger button (both direct children) spreadfocusOwnerProps. Theinputvariant, which draws its focus as a border, is unchanged.Current specification
architecture:interaction-modalityINV3 (current): one focus move has one owning indicator; sibling controls paint their own focus.Evidence
FocusRingOwner.a11y.chromium.spec.ts: new cases for Thumbnail › With Remove and for ghost and secondary Clearable Selector and MultiSelector (story argsvariant:ghostandvariant:secondary), at 1280 and 390 px. On the previous head each shows two rings while Remove or Clear is focused. All 18 cases pass.maintoo.Scope
Changeset:
[fix]patch for@astryxdesign/core.