perf(ActionList, TreeView): avoid whole-subtree style invalidation on hover, focus and attribute changes

.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

.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

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 `<li>`.
+   *
+   * 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) {

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<typeof ActionList.Item>) {
+      const {container} = HTMLRender(
+        <ActionList role="menu" aria-label="Menu">
+          <ActionList.Item {...props}>
+            Label
+            <ActionList.TrailingVisual>
+              <CounterLabel>3</CounterLabel>
+            </ActionList.TrailingVisual>
+          </ActionList.Item>
+          <ActionList.Item>
+            Enabled
+            <ActionList.TrailingVisual>
+              <CounterLabel>4</CounterLabel>
+            </ActionList.TrailingVisual>
+          </ActionList.Item>
+        </ActionList>,
+      )
+      const [item, enabled] = container.querySelectorAll<HTMLElement>('[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(
+        <ActionList>
+          <ActionList.Item inactiveText="Unavailable" aria-disabled>
+            <ActionList.LeadingVisual>
+              <BookIcon />
+            </ActionList.LeadingVisual>
+            Label
+            <ActionList.TrailingVisual>
+              <CounterLabel>3</CounterLabel>
+            </ActionList.TrailingVisual>
+          </ActionList.Item>
+          <ActionList.Item inactiveText="Unavailable">Inactive</ActionList.Item>
+        </ActionList>,
+      )
+      const [item, inactiveOnly] = container.querySelectorAll<HTMLElement>('[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)
+    })
+  })
 })

packages/react/src/ActionList/Item.tsx

@@ -318,6 +318,12 @@ const UnwrappedItem = <As extends React.ElementType = 'li'>(
   // 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 `<li>` 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 (
     <ItemContext.Provider value={itemContextValue}>
       <li
@@ -332,7 +338,12 @@ const UnwrappedItem = <As extends React.ElementType = 'li'>(
         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,
+        )}
       >
         <ConditionalTooltip ref={forwardedRef} text={_PrivateTooltipText ?? truncatedText} enabled={buttonSemantics}>
           <ItemWrapper

packages/react/src/TreeView/TreeView.module.css

@@ -214,24 +214,29 @@
     height: 100%;
     border-right: var(--borderWidth-thin) solid;
 
-    /*
-     * `--tree-line-color` is set on the root `<ul>` 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);
     }
   }
 

packages/react/src/TreeView/TreeView.test.tsx

@@ -581,6 +581,36 @@
   })
 })
 
+describe('Focus within', () => {
+  it('sets data-focus-within on the root only while focus is inside the tree', async () => {
+    render(
+      <>
+        <button type="button">Before</button>
+        <TreeView aria-label="Test tree">
+          <TreeView.Item id="item-1">Item 1</TreeView.Item>
+          <TreeView.Item id="item-2">Item 2</TreeView.Item>
+        </TreeView>
+        <button type="button">After</button>
+      </>,
+    )
+    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)
@@ -1921,7 +1951,7 @@
       await userEvent.keyboard('n')
 
       // Cantalope should be focused
       await expect.element(cantalope).toHaveFocus()
     })
 
     it('prioritizes items following the current aria-activedescendant', async () => {

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<TreeViewProps> = ({
   }, [])
 
   useTreeRovingTabIndex({containerRef, mouseDownRef})
+  useFocusWithinAttribute(containerRef)
   useTypeahead({
     containerRef,
     onFocusChange: element => {

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<HTMLElement | null>) {
+  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])
+}