diff --git a/.changeset/focus-ring-one-owner-item-token.md b/.changeset/focus-ring-one-owner-item-token.md new file mode 100644 index 0000000000000..fc72461f05355 --- /dev/null +++ b/.changeset/focus-ring-one-owner-item-token.md @@ -0,0 +1,7 @@ +--- +'@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. + +@cixzhang diff --git a/apps/storybook/stories/Item.stories.tsx b/apps/storybook/stories/Item.stories.tsx index a8c6e290e4566..30b5a8beae9d2 100644 --- a/apps/storybook/stories/Item.stories.tsx +++ b/apps/storybook/stories/Item.stories.tsx @@ -11,6 +11,7 @@ import {useState, type ReactNode} from 'react'; import {Button} from '@astryxdesign/core/Button'; import * as stylex from '@stylexjs/stylex'; import type {Meta, StoryObj} from '@storybook/react'; +import {IconButton} from '@astryxdesign/core/IconButton'; import {expect} from 'storybook/test'; import {Item} from '@astryxdesign/core/Item'; import {Avatar} from '@astryxdesign/core/Avatar'; @@ -21,6 +22,7 @@ import {Stack} from '@astryxdesign/core/Layout'; import {List, ListItem} from '@astryxdesign/core/List'; import {RadioList, RadioListItem} from '@astryxdesign/core/RadioList'; import {StatusDot} from '@astryxdesign/core/StatusDot'; +import {Token} from '@astryxdesign/core/Token'; const storyStyles = stylex.create({ markerAnatomy: { @@ -51,6 +53,8 @@ import { ArchiveBoxIcon, EnvelopeIcon, TrashIcon, + ShareIcon, + ArrowUturnLeftIcon, } from '@heroicons/react/24/outline'; const meta: Meta = { @@ -321,6 +325,69 @@ export const SearchResults: Story = { ), }; +/** + * Rows holding a control of their own in `endContent`: a row with an action + * rings for that action, a row without one draws no ring, and the end + * control rings itself, so each tab stop shows one focus ring. + */ +export const WithEndActions: Story = { + render: () => ( +
+ {}} + endContent={ + } + variant="ghost" + size="sm" + /> + } + /> + } + variant="ghost" + size="sm" + /> + } + /> +
+ ), +}; + +/** A row with its own action and a clickable Token in its end slot. */ +export const WithTokenAction: Story = { + render: () => ( +
+ {}} + endContent={ {}} />} + /> +
+ ), +}; + +/** Numbered rows with their own action: the marker sits beside the row. */ +export const NumberedWithActions: Story = { + render: () => ( +
+ + {}} /> + + +
+ ), +}; + /** Disabled items. */ export const Disabled: Story = { render: () => ( diff --git a/apps/storybook/stories/Token.stories.tsx b/apps/storybook/stories/Token.stories.tsx index c9e76d2796734..63e14350eaa3b 100644 --- a/apps/storybook/stories/Token.stories.tsx +++ b/apps/storybook/stories/Token.stories.tsx @@ -148,6 +148,20 @@ export const Clickable: Story = { ), }; +/** + * A token with its own action and a remove button: each of the two tab stops + * shows one focus ring, the token's around the token and the remove button's + * around the X. + */ +export const ClickableWithRemove: Story = { + render: () => ( +
+ {}} onRemove={() => {}} /> + {}} /> +
+ ), +}; + export const AsLink: Story = { render: () => (
diff --git a/internal/eslint-plugin-astryx/focus-outline-shared.js b/internal/eslint-plugin-astryx/focus-outline-shared.js index 847e8a24891f2..81520fd85bee4 100644 --- a/internal/eslint-plugin-astryx/focus-outline-shared.js +++ b/internal/eslint-plugin-astryx/focus-outline-shared.js @@ -37,8 +37,16 @@ const RING_APPEARANCE = new Set(['outline', 'outlineWidth', 'outlineStyle']); -/** The file that owns the ring is the one place allowed to write it. */ -const UTILITY_FILE = 'focusOutline.stylex'; +/** + * The files that own a ring are the only places allowed to write it: the + * shared utility, and the focus-owner attribute's owner module, which holds + * the wrapper ring keyed to that private attribute + * (`architecture:public-component-api` INV23). + */ +const RING_OWNER_FILES = [ + 'focusOutline.stylex', + 'privateAttributes/focusOwner.stylex', +]; function isFocusVisibleKey(key) { return typeof key === 'string' && key.includes(':focus-visible'); @@ -100,7 +108,8 @@ const rule = { schema: [], }, create(context) { - if (context.filename?.includes(UTILITY_FILE)) return {}; + const filename = context.filename?.replaceAll('\\', '/') ?? ''; + if (RING_OWNER_FILES.some(owner => filename.includes(owner))) return {}; return { Property(node) { diff --git a/internal/eslint-plugin-astryx/focus-outline-shared.test.mjs b/internal/eslint-plugin-astryx/focus-outline-shared.test.mjs index e033d4dcfc9b5..fd5e9d9de0765 100644 --- a/internal/eslint-plugin-astryx/focus-outline-shared.test.mjs +++ b/internal/eslint-plugin-astryx/focus-outline-shared.test.mjs @@ -70,6 +70,22 @@ ruleTester.run('focus-outline-shared', rule, { { code: `const doc = {outline: {':focus-visible': '2px solid blue'}};`, }, + // The focus-owner attribute's owner module writes the wrapper ring. + { + filename: + '/repo/packages/core/src/privateAttributes/focusOwner.stylex.ts', + code: ` + import * as stylex from '@stylexjs/stylex'; + const styles = stylex.create({ + focusWithinOwner: { + outlineWidth: { + default: '0', + ':has(> [data-astryx-focus-owner]:focus-visible)': WIDTH, + }, + }, + }); + `, + }, // The utility itself is the one place the ring is written. { filename: '/repo/packages/core/src/utils/focusOutline.stylex.ts', @@ -108,10 +124,7 @@ ruleTester.run('focus-outline-shared', rule, { }, }); `, - errors: [ - {messageId: 'handRolledRing'}, - {messageId: 'handRolledRing'}, - ], + errors: [{messageId: 'handRolledRing'}, {messageId: 'handRolledRing'}], }, // Reading the tokens directly is still a second definition of the ring. { diff --git a/packages/core/src/Item/Item.tsx b/packages/core/src/Item/Item.tsx index c1b3e5fa97d0b..63750f73b29b2 100644 --- a/packages/core/src/Item/Item.tsx +++ b/packages/core/src/Item/Item.tsx @@ -59,7 +59,12 @@ import {useLinkComponent} from '../Link/useLinkComponent'; import {useClickableContainer} from '../hooks/useClickableContainer'; import {useDevWarning} from '../hooks/useDevWarning'; import {themeProps} from '../utils/themeProps'; -import {focusOutlineProps} from '../utils/focusOutline.stylex'; +import {focusOutlineStyles} from '../utils/focusOutline.stylex'; +import { + focusOwnerGroupProps, + focusOwnerProps, + focusWithinOwner, +} from '../privateAttributes/focusOwner.stylex'; import {interactionOverlayStyles} from '../utils/interactionOverlay.stylex'; import {usePressFeedback} from '../hooks/usePressFeedback'; import type {ItemSwipeBridge} from './ItemSwipeLayer'; @@ -800,6 +805,7 @@ export function Item({ {...(isDisabled ? {role: 'link', 'aria-disabled': true, tabIndex: -1} : {href, target, rel})} + {...focusOwnerProps} {...stylex.props( styles.invisibleAnchor, isInline && styles.inlineContent, @@ -812,6 +818,7 @@ export function Item({ type="button" onClick={onClick} disabled={isDisabled} + {...focusOwnerProps} {...stylex.props( styles.invisibleButton, isInline && styles.inlineContent, @@ -909,7 +916,12 @@ export function Item({ {...(isInteractive ? pressable : undefined)} {...mergeProps( themeProps('item', {density, align}), - focusOutlineProps.focusWithin( + // The row rings for its own primary only: its invisible button or + // link, or in delegation mode the nested control that is its sole tab + // stop. Any other control in it (a start or end slot) rings itself + // (`architecture:interaction-modality` INV3). + stylex.props( + isDelegate ? focusOutlineStyles.focusWithin : focusWithinOwner, styles.root, densityStyles[density], align === 'start' && styles.alignStart, @@ -933,6 +945,9 @@ export function Item({ {isRenderable(marker) && marker} {isRenderable(marker) && columnMarker === marker ? ( { /** Ref forwarded to the container element. */ ref?: Ref; @@ -43,7 +44,9 @@ export interface TokenLinkProps extends HTMLAttributes { /** Resolved link component (native `` or a router link). */ LinkComponent: LinkComponentType; /** `stylex.props()` output applied to the invisible inner link. */ - linkStyleProps: {className?: string; style?: CSSProperties}; + linkStyleProps: {className?: string; style?: CSSProperties} & Partial< + typeof focusOwnerProps + >; /** Visible (or visually hidden) label node rendered inside the link. */ labelContent: ReactNode; /** Optional leading icon. */ diff --git a/packages/core/src/__tests__/FocusRingOwner.a11y.chromium.spec.ts b/packages/core/src/__tests__/FocusRingOwner.a11y.chromium.spec.ts new file mode 100644 index 0000000000000..38c55ac1b4280 --- /dev/null +++ b/packages/core/src/__tests__/FocusRingOwner.a11y.chromium.spec.ts @@ -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) + +/** + * @file FocusRingOwner.a11y.chromium.spec.ts + * @input The built Item "With End Actions" and Token "Clickable With Remove" + * stories in real Chromium, focus moved with Tab, at desktop and phone + * widths + * @output Browser evidence for `architecture:interaction-modality` INV3: each + * tab stop in a row or token shows exactly one focus ring. A row or token + * rings for its own action only; a control inside it (an end action, a + * remove button) rings itself, and the wrapper stays unringed + * @position Regression proof for focusWithinOwner in Item and Token + */ + +import {expect, test, type Page} from '@playwright/test'; +import { + DEFAULT_STORYBOOK_DIR, + serveStorybook, + type StaticServer, +} from '@astryxdesign/a11y-spec/storybook'; +import {ringedElements} from './focusRings'; + +let storybook: StaticServer; +test.beforeAll(async () => { + storybook = await serveStorybook( + process.env.ASTRYX_STORYBOOK_DIR ?? DEFAULT_STORYBOOK_DIR, + ); +}); +test.afterAll(async () => { + await storybook?.close(); +}); + +async function open(page: Page, story: string, width: number) { + await page.setViewportSize({width, height: 600}); + await page.goto(`${storybook.origin}/iframe.html?id=${story}&viewMode=story`); + await page + .locator('#storybook-root button, #storybook-root a') + .first() + .waitFor(); +} + +/** Tabs through every stop, returning the rings painted at each. */ +async function ringsAtEachStop(page: Page, stops: number) { + const out: {focused: string; rings: string[]}[] = []; + for (let i = 0; i < stops; i += 1) { + await page.keyboard.press('Tab'); + const focused = await page.evaluate( + () => + document.activeElement?.getAttribute('aria-label') ?? + (document.activeElement?.textContent ?? '').trim().slice(0, 30), + ); + const rings = (await ringedElements(page)).map(ring => ring.description); + out.push({focused, rings}); + } + return out; +} + +for (const width of [1280, 390]) { + test(`at ${width} px, each tab stop in an Item shows one ring`, async ({ + page, + }) => { + await open(page, 'core-item--with-end-actions', width); + // The first row's action, its Share button, then the second row's + // Restore button (that row has no action). + for (const {focused, rings} of await ringsAtEachStop(page, 3)) { + expect(rings, `rings while "${focused}" is focused`).toHaveLength(1); + } + }); + + test(`at ${width} px, a numbered row's own action shows its ring`, async ({ + page, + }) => { + await open(page, 'core-item--numbered-with-actions', width); + // The button row, then the link row: each rings once, beside its marker. + for (const {focused, rings} of await ringsAtEachStop(page, 2)) { + expect(rings, `rings while "${focused}" is focused`).toHaveLength(1); + } + }); + + test(`at ${width} px, a Token inside an Item rings itself, not the row`, async ({ + page, + }) => { + await open(page, 'core-item--with-token-action', width); + // The row's own action, then the Token's: one ring at each. + for (const {focused, rings} of await ringsAtEachStop(page, 2)) { + expect(rings, `rings while "${focused}" is focused`).toHaveLength(1); + } + }); + + test(`at ${width} px, each tab stop in a Token shows one ring`, async ({ + page, + }) => { + await open(page, 'core-token--clickable-with-remove', width); + // Design, Remove Design, Docs, Remove Docs. + for (const {focused, rings} of await ringsAtEachStop(page, 4)) { + expect(rings, `rings while "${focused}" is focused`).toHaveLength(1); + } + }); +} diff --git a/packages/core/src/__tests__/focusOwner.source-build.test.mjs b/packages/core/src/__tests__/focusOwner.source-build.test.mjs new file mode 100644 index 0000000000000..cde39ff941cb6 --- /dev/null +++ b/packages/core/src/__tests__/focusOwner.source-build.test.mjs @@ -0,0 +1,52 @@ +// Copyright (c) Meta Platforms, Inc. and affiliates. + +/** + * @file focusOwner.source-build.test.mjs + * @input Uses Babel, the core StyleX build config, the StyleX rule processor, + * and privateAttributes/focusOwner.stylex.ts + * @output Verifies the shared focus-owner ring compiles to a ring on the + * wrapper itself, keyed to its marked control's `:focus-visible`, and never + * to a rule that reaches every descendant (`:has(...) *`) + * @position Regression test for the focus-owner wrapper ring + * (`architecture:interaction-modality` INV3) + */ + +import fs from 'node:fs/promises'; +import path from 'node:path'; +import {fileURLToPath} from 'node:url'; +import {transformAsync} from '@babel/core'; +import stylexBabelPlugin from '@stylexjs/babel-plugin'; +import {describe, expect, it} from 'vitest'; + +const __dirname = path.dirname(fileURLToPath(import.meta.url)); +const SRC = path.resolve(__dirname, '..'); +const CORE_ROOT = path.resolve(SRC, '..'); + +async function compile(relative) { + const filename = path.join(SRC, relative); + const result = await transformAsync(await fs.readFile(filename, 'utf8'), { + babelrc: false, + configFile: path.join(CORE_ROOT, 'babel.config.json'), + filename, + }); + return stylexBabelPlugin.processStylexRules( + result?.metadata?.stylex ?? [], + false, + ); +} + +describe('focus-owner wrapper ring', () => { + it('rings the wrapper for its own marked control only', async () => { + const css = await compile('privateAttributes/focusOwner.stylex.ts'); + const ringRules = css + .split('}') + .filter(rule => rule.includes('var(--focus-outline-')); + expect(ringRules.length).toBeGreaterThan(0); + for (const rule of ringRules) { + // On the wrapper, while its marked control shows keyboard focus. + expect(rule).toMatch(/:has\(> \[data-astryx-focus-owner\]:focus-visible/); + // Never every descendant. + expect(rule).not.toMatch(/:has\([^{]*\)\)?\s+\*/); + } + }); +}); diff --git a/packages/core/src/__tests__/focusRings.ts b/packages/core/src/__tests__/focusRings.ts new file mode 100644 index 0000000000000..6dd0528e11715 --- /dev/null +++ b/packages/core/src/__tests__/focusRings.ts @@ -0,0 +1,51 @@ +// Copyright (c) Meta Platforms, Inc. and affiliates. + +/** + * @file focusRings.ts + * @input A Playwright page showing a built story + * @output `ringedElements`: every element under a root that paints an outline + * right now, for browser specs proving `architecture:interaction-modality` + * INV3 (one focus move has one owning indicator) + * @position Shared helper for focus-ring browser specs + */ + +import type {Page} from '@playwright/test'; + +export interface RingedElement { + /** The element's tag, its accessible label or text, and its role. */ + readonly description: string; + /** Whether it is the focused element itself. */ + readonly isFocused: boolean; +} + +/** + * Every element under `root` whose computed outline paints (a style other + * than `none` and a width above zero). Read after moving focus with the + * keyboard, so `:focus-visible` applies. + */ +export async function ringedElements( + page: Page, + root = '#storybook-root', +): Promise { + return page.evaluate(rootSelector => { + const scope = document.querySelector(rootSelector) ?? document.body; + const out: {description: string; isFocused: boolean}[] = []; + for (const element of [scope, ...scope.querySelectorAll('*')]) { + const style = getComputedStyle(element); + if ( + style.outlineStyle === 'none' || + Number.parseFloat(style.outlineWidth) <= 0 + ) { + continue; + } + const label = + element.getAttribute('aria-label') ?? + (element.textContent ?? '').trim().slice(0, 30); + out.push({ + description: `${element.tagName.toLowerCase()} "${label}"`, + isFocused: element === document.activeElement, + }); + } + return out; + }, root); +} diff --git a/packages/core/src/privateAttributes/focusOwner.stylex.ts b/packages/core/src/privateAttributes/focusOwner.stylex.ts new file mode 100644 index 0000000000000..033b5875bbd11 --- /dev/null +++ b/packages/core/src/privateAttributes/focusOwner.stylex.ts @@ -0,0 +1,63 @@ +// Copyright (c) Meta Platforms, Inc. and affiliates. + +/** + * @file privateAttributes/focusOwner.stylex.ts + * @input focus ring tokens + * @output focusWithinOwner, the ring a wrapper draws only while its primary + * control has keyboard focus, and focusOwnerProps / focusOwnerGroupProps, + * which mark that control (and, where layout puts it one level down, the + * cell holding it) + * @position Owner module of the private focus-owner attribute + * (`architecture:public-component-api` INV23): the only file that writes + * its name or its selector. Core wrappers spread these exports + * (`architecture:interaction-modality` INV3). No package entry exports it. + */ + +import * as stylex from '@stylexjs/stylex'; +import {focusVars} from '../theme/tokens.stylex'; + +/** Spread on the wrapper's primary control: the control the ring is for. */ +export const focusOwnerProps = {'data-astryx-focus-owner': ''} as const; + +/** + * Spread on a layout cell that holds the primary control one level down + * (Item's content cell beside a list marker), so the wrapper's ring still + * reaches it. + */ +export const focusOwnerGroupProps = { + 'data-astryx-focus-owner-group': '', +} as const; + +// Rings only for the wrapper's own marked control, a direct child or one level +// down inside a marked group, so every other control inside rings itself and a +// nested component's owner never rings this wrapper. Every condition of each +// property, rest state included, is here: a caller spreading this style does +// not set these properties again on the same element. Written as longhands, +// like focusOutlineStyles, so a base style cannot erase them through `outline`. +const styles = stylex.create({ + focusWithinOwner: { + outlineWidth: { + default: '0', + ':has(> [data-astryx-focus-owner]:focus-visible, > [data-astryx-focus-owner-group] > [data-astryx-focus-owner]:focus-visible)': + focusVars['--focus-outline-width'], + }, + outlineStyle: { + default: 'none', + ':has(> [data-astryx-focus-owner]:focus-visible, > [data-astryx-focus-owner-group] > [data-astryx-focus-owner]:focus-visible)': + focusVars['--focus-outline-style'], + }, + outlineColor: { + default: null, + ':has(> [data-astryx-focus-owner]:focus-visible, > [data-astryx-focus-owner-group] > [data-astryx-focus-owner]:focus-visible)': + focusVars['--focus-outline-color'], + }, + outlineOffset: { + default: '0', + ':has(> [data-astryx-focus-owner]:focus-visible, > [data-astryx-focus-owner-group] > [data-astryx-focus-owner]:focus-visible)': + focusVars['--focus-outline-offset'], + }, + }, +}); + +/** The wrapper's ring. Spread it first; caller styles that follow keep it. */ +export const focusWithinOwner = styles.focusWithinOwner;