diff --git a/.changeset/actionlist-inactive-disabled-invalidation.md b/.changeset/actionlist-inactive-disabled-invalidation.md new file mode 100644 index 00000000000..adc60584833 --- /dev/null +++ b/.changeset/actionlist-inactive-disabled-invalidation.md @@ -0,0 +1,5 @@ +--- +'@primer/react': patch +--- + +ActionList: Reduce style recalculation when unrelated elements on the page change hover state or attributes diff --git a/.changeset/treeview-level-line-invalidation.md b/.changeset/treeview-level-line-invalidation.md new file mode 100644 index 00000000000..9f0cfc7edef --- /dev/null +++ b/.changeset/treeview-level-line-invalidation.md @@ -0,0 +1,5 @@ +--- +'@primer/react': patch +--- + +TreeView: Reduce style recalculation when the pointer or focus enters or leaves the tree, and when focus moves between items in Safari diff --git a/packages/react/src/ActionList/ActionList.module.css b/packages/react/src/ActionList/ActionList.module.css index 7a4a9065c0a..35a3058a256 100644 --- a/packages/react/src/ActionList/ActionList.module.css +++ b/packages/react/src/ActionList/ActionList.module.css @@ -267,19 +267,24 @@ /* inactive */ &:where([data-inactive='true']) { - /* ignore tooltip */ - & * :not([popover], .InactiveWarning) { - color: var(--fgColor-muted); - } + /* text color is set by `.InactiveColors` below */ @media (hover: hover) { &:hover { cursor: not-allowed; background-color: transparent; - & * :not([popover], .InactiveWarning) { + /* outrank the danger variant's hover colors */ + & .LeadingAction, + & .LeadingVisual, + & .ItemLabel { color: var(--fgColor-muted); } + + /* an `aria-disabled` prop without `disabled`: the inactive color wins while hovered */ + &.DisabledColors { + --disabledItem-fgColor: var(--fgColor-muted); + } } } @@ -316,6 +321,31 @@ } } + /* + * Inactive and disabled items mute every descendant, including nested components that set + * their own color. `Item.tsx` sets `.InactiveColors` and `.DisabledColors` on the `
  • `. + * + * Key these universal descendant selectors on those classes, never on `:hover` or an + * attribute. Blink builds invalidation sets per feature for the whole page, so `:hover *` or + * `[aria-disabled] *` would restyle the entire subtree of any element, in any component, + * whose hover state or attribute changes. + */ + + /* ignore tooltip */ + &.InactiveColors * :not([popover], .InactiveWarning) { + color: var(--fgColor-muted); + } + + &.DisabledColors .ActionListContent * { + /* stylelint-disable-next-line primer/colors -- private custom property, defaults to a Primer token */ + color: var(--disabledItem-fgColor, var(--control-fgColor-disabled)); + } + + /* the tooltip and warning keep the disabled color, even while hovered */ + &.DisabledColors .ActionListContent :is([popover], .InactiveWarning) { + --disabledItem-fgColor: var(--control-fgColor-disabled); + } + /* if item has subitem, move hover styles to ActionListContent */ &[data-has-subitem='true'] { /* first child */ @@ -343,9 +373,7 @@ &[aria-disabled='true'], &[data-is-disabled] { - & .ActionListContent * { - color: var(--control-fgColor-disabled); - } + /* text color is set by `.DisabledColors` above */ & .ActionListContent { @media (hover: hover) { diff --git a/packages/react/src/ActionList/Item.test.tsx b/packages/react/src/ActionList/Item.test.tsx index b28b35d7fef..3dc0b6851bc 100644 --- a/packages/react/src/ActionList/Item.test.tsx +++ b/packages/react/src/ActionList/Item.test.tsx @@ -6,6 +6,7 @@ import {ActionList} from '.' import {AnchoredOverlay} from '../AnchoredOverlay' import {BookIcon} from '@primer/octicons-react' import {implementsClassName} from '../utils/testing' +import CounterLabel from '../CounterLabel' import classes from './ActionList.module.css' function SimpleActionList(): JSX.Element { @@ -610,4 +611,72 @@ describe('ActionList.Item', () => { expect(action).not.toHaveAttribute('data-has-label') }) }) + + describe('inactive and disabled text color', () => { + function renderItem(props: React.ComponentProps) { + const {container} = HTMLRender( + + + Label + + 3 + + + + Enabled + + 4 + + + , + ) + const [item, enabled] = container.querySelectorAll('[data-component="ActionList.Item"]') + const colorOf = (el: Element | null) => getComputedStyle(el!).color + return { + label: colorOf(item.querySelector(`.${classes.ItemLabel}`)), + nested: colorOf(item.querySelector('[data-component="CounterLabel"]')), + enabledNested: colorOf(enabled.querySelector('[data-component="CounterLabel"]')), + warning: item.querySelector(`.${classes.InactiveWarning}`), + } + } + + it('mutes nested components that set their own color in inactive items', () => { + const {label, nested, enabledNested, warning} = renderItem({inactiveText: 'Unavailable'}) + expect(nested).toBe(label) + expect(nested).not.toBe(enabledNested) + expect(getComputedStyle(warning!).color).not.toBe(label) + }) + + it('mutes nested components that set their own color in disabled items', () => { + const {label, nested, enabledNested} = renderItem({disabled: true}) + expect(nested).toBe(label) + expect(nested).not.toBe(enabledNested) + }) + + it('mutes every descendant while hovering an inactive item that also sets aria-disabled', async () => { + const {container} = HTMLRender( + + + + + + Label + + 3 + + + Inactive + , + ) + const [item, inactiveOnly] = container.querySelectorAll('[data-component="ActionList.Item"]') + const colorOf = (el: Element | null) => getComputedStyle(el!).color + const mutedColor = colorOf(inactiveOnly.querySelector(`.${classes.ItemLabel}`)) + + await userEvent.hover(item.querySelector(`.${classes.ItemLabel}`)!) + + await expect.poll(() => colorOf(item.querySelector(`.${classes.ItemLabel}`))).toBe(mutedColor) + expect(colorOf(item.querySelector('[data-component="CounterLabel"]'))).toBe(mutedColor) + expect(colorOf(item.querySelector(`.${classes.ActionListSubContent}`))).toBe(mutedColor) + }) + }) }) diff --git a/packages/react/src/ActionList/Item.tsx b/packages/react/src/ActionList/Item.tsx index f811925983f..ab72fa5f0a8 100644 --- a/packages/react/src/ActionList/Item.tsx +++ b/packages/react/src/ActionList/Item.tsx @@ -318,6 +318,12 @@ const UnwrappedItem = ( // attributes so the CSS only kicks in when the action is actually in the DOM. const trailingActionRendered = !inactive && !loading && !menuContext && Boolean(slots.trailingAction) + // Mirrors the `
  • ` attributes below so CSS can key descendant colors on classes instead of + // attributes. See `.InactiveColors` in ActionList.module.css. + const liAriaDisabled = 'aria-disabled' in containerProps ? containerProps['aria-disabled'] : undefined + const disabledColors = disabled || liAriaDisabled === true || liAriaDisabled === 'true' + const inactiveColors = Boolean(inactiveText) && !disabled && !slots.subItem + return (
  • ( data-has-description={slots.description ? true : false} data-has-trailing-action={trailingActionRendered ? true : undefined} data-trailing-action-loading={trailingActionRendered && slots.trailingAction?.props.loading ? true : undefined} - className={clsx(classes.ActionListItem, className)} + className={clsx( + classes.ActionListItem, + inactiveColors && classes.InactiveColors, + disabledColors && classes.DisabledColors, + className, + )} > ` and inherited down. On coarse pointers it - * stays unset and falls back to `muted` (lines always visible). On hover-capable devices it - * is initialized to `transparent` on the root and flipped to `muted` while the tree is - * hovered or focused, so the browser only has to propagate a single inherited custom - * property instead of re-matching `.TreeViewItemLevelLine` descendant selectors on every - * hover/focus change inside large trees. - */ - /* stylelint-disable-next-line primer/colors -- private custom property, defaults to a Primer token */ - border-color: var(--tree-line-color, var(--borderColor-muted)); + /* On devices without hover, the nesting indicator lines are always visible. */ + border-color: var(--borderColor-muted); } + /* + * On devices with hover, the lines fade in while the tree is hovered or has focus. + * + * Keep this a class-targeted descendant rule. Toggling an inherited custom property on the + * root instead forces every element in the tree to restyle on each pointer enter/leave and + * focus in/out; this rule restyles only the `.TreeViewItemLevelLine` elements. + * + * Focus uses `data-focus-within` (set by `useFocusWithinAttribute`), not `:focus-within`: + * WebKit re-toggles `:focus-within` on every focus move inside the tree, which restyled every + * level line on each arrow key press. + */ @media (hover: hover) { - --tree-line-color: transparent; + .TreeViewItemLevelLine { + border-color: transparent; + } - &:hover, - &:focus-within { - --tree-line-color: var(--borderColor-muted); + &:hover .TreeViewItemLevelLine, + &[data-focus-within] .TreeViewItemLevelLine { + border-color: var(--borderColor-muted); } } diff --git a/packages/react/src/TreeView/TreeView.test.tsx b/packages/react/src/TreeView/TreeView.test.tsx index 1b703083c71..a775c5fe97f 100644 --- a/packages/react/src/TreeView/TreeView.test.tsx +++ b/packages/react/src/TreeView/TreeView.test.tsx @@ -581,6 +581,36 @@ describe('SubTree accessible name', () => { }) }) +describe('Focus within', () => { + it('sets data-focus-within on the root only while focus is inside the tree', async () => { + render( + <> + + + Item 1 + Item 2 + + + , + ) + const root = page.getByRole('tree').element() + + expect(root).not.toHaveAttribute('data-focus-within') + + act(() => page.getByRole('button', {name: 'Before'}).element().focus()) + await userEvent.keyboard('{Tab}') + expect(root).toHaveAttribute('data-focus-within') + + await userEvent.keyboard('{ArrowDown}') + await expect.element(page.getByRole('treeitem', {name: 'Item 2'})).toHaveFocus() + expect(root).toHaveAttribute('data-focus-within') + + await userEvent.keyboard('{Tab}') + await expect.element(page.getByRole('button', {name: 'After'})).toHaveFocus() + expect(root).not.toHaveAttribute('data-focus-within') + }) +}) + describe('Tab order', () => { const getTabStops = (container: Element) => Array.from(container.querySelectorAll('[tabindex="0"]')).map(element => element.id || element.textContent) diff --git a/packages/react/src/TreeView/TreeView.tsx b/packages/react/src/TreeView/TreeView.tsx index e3e66b47511..441abd55a38 100644 --- a/packages/react/src/TreeView/TreeView.tsx +++ b/packages/react/src/TreeView/TreeView.tsx @@ -19,6 +19,7 @@ import {useSlots} from '../hooks/useSlots' import {getAccessibleName} from './shared' import {getFirstChildElement} from './useRovingTabIndex' import {useTreeRovingTabIndex} from './useTreeRovingTabIndex' +import {useFocusWithinAttribute} from './useFocusWithinAttribute' import {useTypeahead} from './useTypeahead' import {SkeletonAvatar} from '../SkeletonAvatar' import {SkeletonText} from '../SkeletonText' @@ -141,6 +142,7 @@ const Root: React.FC = ({ }, []) useTreeRovingTabIndex({containerRef, mouseDownRef}) + useFocusWithinAttribute(containerRef) useTypeahead({ containerRef, onFocusChange: element => { diff --git a/packages/react/src/TreeView/useFocusWithinAttribute.ts b/packages/react/src/TreeView/useFocusWithinAttribute.ts new file mode 100644 index 00000000000..8e54b8bcdac --- /dev/null +++ b/packages/react/src/TreeView/useFocusWithinAttribute.ts @@ -0,0 +1,33 @@ +import type React from 'react' +import {useEffect} from 'react' + +/** + * Mirrors `:focus-within` onto the tree root as `data-focus-within`. + * + * WebKit clears and re-sets `:focus-within` up the ancestor chain on every focus move, so a + * `:focus-within` rule on the root restyles every level line on each arrow key press. This + * attribute only changes when focus enters or leaves the tree. It is written to the DOM + * directly so the tree doesn't re-render. + */ +export function useFocusWithinAttribute(containerRef: React.RefObject) { + useEffect(() => { + const root = containerRef.current + if (!root) return + + const sync = (event?: FocusEvent) => { + const focused = event + ? event.type === 'focusin' || root.contains(event.relatedTarget as Node | null) + : root.matches(':focus-within') + root.toggleAttribute('data-focus-within', focused) + } + + sync() + root.addEventListener('focusin', sync) + root.addEventListener('focusout', sync) + return () => { + root.removeEventListener('focusin', sync) + root.removeEventListener('focusout', sync) + root.removeAttribute('data-focus-within') + } + }, [containerRef]) +}