WIP: Fix UnderlineNav CLS and improve performance/code quality by rebuilding the whole overflow logic to use mostly CSS

.changeset/clever-geese-cover.md

@@ -0,0 +1,5 @@
+---
+'@primer/react': patch
+---
+
+UnderlineNav: Adds `overflow: hidden` when calculating items to prevent CLS

packages/react/src/UnderlineNav/UnderlineNav.examples.stories.tsx

@@ -65,7 +65,7 @@ export const PullRequestPage = () => {
 }
 
 const items: {navigation: string; icon: React.ReactElement; counter?: number | string; href?: string}[] = [
-  {navigation: 'Code', icon: <CodeIcon />, href: '#code'},
+  {navigation: 'Code (really really long first item title)', icon: <CodeIcon />, href: '#code'},
   {navigation: 'Issues', icon: <IssueOpenedIcon />, counter: '12K', href: '#issues'},
   {navigation: 'Pull Requests', icon: <GitPullRequestIcon />, counter: 13, href: '#pull-requests'},
   {navigation: 'Discussions', icon: <CommentDiscussionIcon />, counter: 5, href: '#discussions'},

packages/react/src/UnderlineNav/UnderlineNav.module.css

@@ -4,6 +4,17 @@
   justify-content: space-between;
 }
 
+.MoreButtonContainer {
+  display: flex;
+  visibility: hidden;
+  align-items: center;
+
+  /* Visibility is controlled by scroll-state query instead of React state so that it's known during SSR */
+  @container underline-wrapper scroll-state(scrollable: block) {
+    visibility: visible;
+  }
+}
+
 /* More button styles migrated from styles.ts (was moreBtnStyles) */
 .MoreButton {
   margin: 0; /* reset Safari extra margin */

packages/react/src/UnderlineNav/UnderlineNav.test.tsx

@@ -1,6 +1,6 @@
 import {describe, expect, it, vi} from 'vitest'
 import type React from 'react'
-import {render, screen} from '@testing-library/react'
+import {render, screen, within} from '@testing-library/react'
 import userEvent from '@testing-library/user-event'
 import {
   CodeIcon,
@@ -78,7 +78,8 @@ describe('UnderlineNav', () => {
   it('renders icons correctly', () => {
     const {getByRole} = render(<ResponsiveUnderlineNav />)
     const nav = getByRole('navigation')
-    expect(nav.getElementsByTagName('svg').length).toEqual(7)
+    const list = within(nav).getByRole('list')
+    expect(list.getElementsByTagName('svg').length).toEqual(7)
   })
 
   it('fires onSelect on click', async () => {
@@ -143,7 +144,7 @@ describe('UnderlineNav', () => {
 
   it('respects loadingCounters prop', () => {
     const {getByRole} = render(<ResponsiveUnderlineNav loadingCounters={true} />)
-    const item = getByRole('link', {name: 'Actions'})
+    const item = getByRole('link', {name: 'Actions', hidden: true})
     const loadingCounter = item.getElementsByTagName('span')[2]
     expect(loadingCounter.className).toContain('LoadingCounter')
     expect(loadingCounter.textContent).toBe('')

packages/react/src/UnderlineNav/UnderlineNavContext.tsx

@@ -1,14 +1,14 @@
-import type React from 'react'
 import {createContext} from 'react'
+import type {UnderlineNavItemProps} from './UnderlineNavItem'
 
 export const UnderlineNavContext = createContext<{
-  setChildrenWidth: React.Dispatch<{text: string; width: number}>
-  setNoIconChildrenWidth: React.Dispatch<{text: string; width: number}>
   loadingCounters: boolean
-  iconsVisible: boolean
+  containerWidth: number
+  registerItem: (id: string, props: UnderlineNavItemProps | null) => void
+  unregisterItem: (id: string) => void
 }>({
-  setChildrenWidth: () => null,
-  setNoIconChildrenWidth: () => null,
   loadingCounters: false,
-  iconsVisible: true,
+  containerWidth: -1,
+  registerItem: () => {},
+  unregisterItem: () => {},
 })

packages/react/src/UnderlineNav/UnderlineNavItem.module.css

@@ -2,4 +2,5 @@
   display: flex;
   flex-direction: column;
   align-items: center;
+  overflow: hidden;
 }

packages/react/src/UnderlineNav/UnderlineNavItem.tsx

@@ -1,5 +1,5 @@
-import type {MutableRefObject, RefObject} from 'react'
-import React, {forwardRef, useRef, useContext} from 'react'
+import type {RefObject} from 'react'
+import React, {forwardRef, useRef, useContext, useEffect, useId, useState} from 'react'
 import type {IconProps} from '@primer/octicons-react'
 import type {ForwardRefComponent as PolymorphicForwardRefComponent} from '../utils/polymorphic'
 import {UnderlineNavContext} from './UnderlineNavContext'
@@ -59,86 +59,84 @@ export type UnderlineNavItemProps = {
   counter?: number | string
 } & LinkProps
 
-export const UnderlineNavItem = forwardRef(
-  (
-    {
-      as: Component = 'a',
-      href = '#',
-      children,
-      counter,
-      onSelect,
-      'aria-current': ariaCurrent,
-      icon: Icon,
-      leadingVisual,
-      ...props
+export const UnderlineNavItem = forwardRef((allProps, forwardedRef) => {
+  const {
+    as: Component = 'a',
+    href = '#',
+    children,
+    counter,
+    onSelect,
+    'aria-current': ariaCurrent,
+    icon: Icon,
+    leadingVisual,
+    ...props
+  } = allProps
+
+  const backupRef = useRef<HTMLElement>(null)
+  const ref = (forwardedRef ?? backupRef) as RefObject<HTMLAnchorElement>
+  const {loadingCounters, containerWidth, registerItem, unregisterItem} = useContext(UnderlineNavContext)
+
+  const id = useId()
+  const [isOverflowing, setIsOverflowing] = useState(false)
+
+  useLayoutEffect(() => {
+    if (ref.current) {
+      // Overflowing items wrap onto subsequent lines, so their `offsetTop` increases
+      const isOverflowing = ref.current.offsetTop > 0
+      setIsOverflowing(isOverflowing)
+
+      // Even if an item is not overflowing, it still needs to register itself to claim it's place in the registry.
+      // This preserves order - otherwise, items that overflow first would appear first in the menu.
+      registerItem(id, isOverflowing ? allProps : null)
+    }
+
+    // To preserve the item's spot in the registry, we don't unregister until we actually dismount the component.
+
+    // TODO: We should try to trim the registry down to the bare minimum needed to render a menu item so that we don't
+    // have to re-register every time `allProps` changes.
+    // See /workspaces/react/packages/react/src/ActionBar/ActionBar.tsx#531 for example.
+  }, [ref, containerWidth, registerItem, id, allProps])
+
+  // Unregister only on dismount:
+  useEffect(() => () => unregisterItem(id), [id, unregisterItem])
+
+  const keyDownHandler = React.useCallback(
+    (event: React.KeyboardEvent<HTMLAnchorElement>) => {
+      if ((event.key === ' ' || event.key === 'Enter') && !event.defaultPrevented && typeof onSelect === 'function') {
+        onSelect(event)
+      }
     },
-    forwardedRef,
-  ) => {
-    const backupRef = useRef<HTMLElement>(null)
-    const ref = (forwardedRef ?? backupRef) as RefObject<HTMLAnchorElement>
-    const {setChildrenWidth, setNoIconChildrenWidth, loadingCounters, iconsVisible} = useContext(UnderlineNavContext)
-
-    useLayoutEffect(() => {
-      if (ref.current) {
-        const domRect = (ref as MutableRefObject<HTMLElement>).current.getBoundingClientRect()
-
-        const icon = Array.from((ref as MutableRefObject<HTMLElement>).current.children).find(
-          child => child.getAttribute('data-component') === 'icon',
-        )
-
-        const content = Array.from((ref as MutableRefObject<HTMLElement>).current.children).find(
-          child => child.getAttribute('data-component') === 'text',
-        ) as HTMLElement
-        const text = content.textContent as string
-
-        const iconWidthWithMargin = icon
-          ? icon.getBoundingClientRect().width +
-            Number(getComputedStyle(icon).marginRight.slice(0, -2)) +
-            Number(getComputedStyle(icon).marginLeft.slice(0, -2))
-          : 0
-
-        setChildrenWidth({text, width: domRect.width})
-        setNoIconChildrenWidth({text, width: domRect.width - iconWidthWithMargin})
+    [onSelect],
+  )
+  const clickHandler = React.useCallback(
+    (event: React.MouseEvent<HTMLAnchorElement>) => {
+      if (!event.defaultPrevented && typeof onSelect === 'function') {
+        onSelect(event)
       }
-    }, [ref, setChildrenWidth, setNoIconChildrenWidth])
-
-    const keyDownHandler = React.useCallback(
-      (event: React.KeyboardEvent<HTMLAnchorElement>) => {
-        if ((event.key === ' ' || event.key === 'Enter') && !event.defaultPrevented && typeof onSelect === 'function') {
-          onSelect(event)
-        }
-      },
-      [onSelect],
-    )
-    const clickHandler = React.useCallback(
-      (event: React.MouseEvent<HTMLAnchorElement>) => {
-        if (!event.defaultPrevented && typeof onSelect === 'function') {
-          onSelect(event)
-        }
-      },
-      [onSelect],
-    )
-
-    return (
-      <li className={classes.UnderlineNavItem}>
-        <UnderlineItem
-          ref={ref}
-          as={Component}
-          href={href}
-          aria-current={ariaCurrent}
-          onKeyDown={keyDownHandler}
-          onClick={clickHandler}
-          counter={counter}
-          icon={leadingVisual ?? Icon}
-          loadingCounters={loadingCounters}
-          iconsVisible={iconsVisible}
-          {...props}
-        >
-          {children}
-        </UnderlineItem>
-      </li>
-    )
-  },
-) as PolymorphicForwardRefComponent<'a', UnderlineNavItemProps>
+    },
+    [onSelect],
+  )
+
+  return (
+    <li className={classes.UnderlineNavItem}>
+      <UnderlineItem
+        ref={ref}
+        as={Component}
+        href={href}
+        aria-current={ariaCurrent}
+        onKeyDown={keyDownHandler}
+        onClick={clickHandler}
+        counter={counter}
+        icon={leadingVisual ?? Icon}
+        loadingCounters={loadingCounters}
+        {...props}
+        aria-hidden={isOverflowing ? true : allProps['aria-hidden']}
+        tabIndex={isOverflowing ? -1 : allProps.tabIndex}
+      >
+        {children}
+      </UnderlineItem>
+    </li>
+  )
+}) as PolymorphicForwardRefComponent<'a', UnderlineNavItemProps>
 
 UnderlineNavItem.displayName = 'UnderlineNavItem'

packages/react/src/internal/components/UnderlineTabbedInterface.module.css

@@ -2,11 +2,15 @@
   display: flex;
   /* stylelint-disable-next-line primer/spacing */
   padding-inline: var(--stack-padding-normal);
-  justify-content: flex-start;
-  align-items: center;
+  padding-top: var(--base-size-8);
+  justify-content: space-between;
+  align-items: flex-start;
+  overflow: hidden;
+  /* stylelint-disable-next-line declaration-property-value-no-unknown */
+  container: underline-wrapper / scroll-state;
 
   /* make space for the underline */
-  min-height: var(--control-xlarge-size, 48px);
+  height: var(--control-xlarge-size, 48px);
 
   /* using a box-shadow instead of a border to accommodate 'overflow-y: hidden' on UnderlinePanels */
   /* stylelint-disable-next-line primer/box-shadow */
@@ -16,6 +20,10 @@
     /* stylelint-disable-next-line primer/spacing */
     padding-inline: unset;
   }
+
+  &[data-hide-icons='true'] [data-component='icon'] {
+    display: none;
+  }
 }
 
 .UnderlineItemList {
@@ -27,6 +35,9 @@
   list-style: none;
   align-items: center;
   gap: 8px;
+  flex-wrap: wrap;
+  /* Allows the menu to clip wrapped items to keep on shrinking even if a wrapped item was very wide */
+  overflow: hidden;
 }
 
 .UnderlineItem {
@@ -43,6 +54,7 @@
   background-color: transparent;
   border: 0;
   border-radius: var(--borderRadius-medium, var(--borderRadius-small));
+  max-width: 100%;
 
   /* button resets */
   appearance: none;
@@ -91,6 +103,16 @@
   color: var(--fgColor-muted);
   align-items: center;
   margin-inline-end: var(--base-size-8);
+
+  @container underline-wrapper scroll-state(scrollable: block) {
+    display: none;
+  }
+}
+
+.UnderlineItem [data-component='text'] {
+  overflow: hidden;
+  text-overflow: ellipsis;
+  white-space: nowrap;
 }
 
 .UnderlineItem [data-component='counter'] {

packages/react/src/internal/components/UnderlineTabbedInterface.tsx

@@ -36,6 +36,7 @@ type UnderlineWrapperProps<As extends React.ElementType> = {
 
 export const UnderlineWrapper = forwardRef((props, ref) => {
   const {children, className, as: Component = 'div', ...rest} = props
+
   return (
     <Component
       className={clsx(classes.UnderlineWrapper, className)}
@@ -62,7 +63,6 @@ export const LoadingCounter = () => {
 export type UnderlineItemProps<As extends React.ElementType> = {
   as?: As | 'a' | 'button'
   className?: string
-  iconsVisible?: boolean
   loadingCounters?: boolean
   counter?: number | string
   // eslint-disable-next-line @typescript-eslint/no-explicit-any
@@ -72,11 +72,11 @@ export type UnderlineItemProps<As extends React.ElementType> = {
 } & React.ComponentPropsWithoutRef<As extends 'a' ? 'a' : As extends 'button' ? 'button' : As>
 
 export const UnderlineItem = React.forwardRef((props, ref) => {
-  const {as: Component = 'a', children, counter, icon: Icon, iconsVisible, loadingCounters, className, ...rest} = props
+  const {as: Component = 'a', children, counter, icon: Icon, loadingCounters, className, ...rest} = props
   const textContent = getTextContent(children)
   return (
     <Component {...rest} ref={ref} className={clsx(classes.UnderlineItem, className)}>
-      {iconsVisible && Icon && <span data-component="icon">{isElement(Icon) ? Icon : <Icon />}</span>}
+      {Icon && <span data-component="icon">{isElement(Icon) ? Icon : <Icon />}</span>}
       {children && (
         <span data-component="text" data-content={textContent || undefined}>
           {children}