Fix and refactor UnderlineNav to resolve CLS issues and improve performance/code quality

.changeset/underline-nav-css-overflow.md

@@ -0,0 +1,5 @@
+---
+"@primer/react": minor
+---
+
+Refactors `UnderlineNav` overflow handling to use CSS-based overflow detection instead of JavaScript width measurements, eliminating layout shift (CLS) issues and improving performance. The overflow menu is now implemented with `ActionMenu`, and item registration uses a descendant registry instead of the `React.Children` API. Consumer-facing changes: items can now be wrapped in fragments or wrapper components; the current item may appear in the overflow menu when the viewport is narrow; and the overflow menu button is right-aligned.

e2e/components/UnderlineNav.test.ts

@@ -199,19 +199,19 @@ test.describe('UnderlineNav', () => {
           })
 
           // Default state
-          // expect(await page.screenshot()).toMatchSnapshot()
+          expect(await page.screenshot()).toMatchSnapshot()
 
           await page.setViewportSize({width: viewports['primer.breakpoint.sm'], height: 768})
-          await page.locator('button', {hasText: 'More Repository Items'}).waitFor()
+          await page.locator('button', {hasText: 'More items'}).waitFor()
 
           // Resize
-          // expect(await page.screenshot()).toMatchSnapshot()
+          expect(await page.screenshot()).toMatchSnapshot()
 
-          await page.getByRole('button', {name: 'More Repository Items'}).click()
-          // expect(await page.screenshot()).toMatchSnapshot()
+          await page.getByRole('button', {name: 'More items'}).click()
+          expect(await page.screenshot()).toMatchSnapshot()
 
-          await page.getByRole('link', {name: 'Settings (10)'}).click()
-          // expect(await page.screenshot()).toMatchSnapshot()
+          await page.getByRole('menuitem', {name: 'Settings (10)'}).click()
+          expect(await page.screenshot()).toMatchSnapshot()
         })
 
         test('Hide icons when there is not enough space to display all list items @vrt', async ({page}) => {
@@ -223,61 +223,13 @@ test.describe('UnderlineNav', () => {
           })
 
           // Default State
-          // expect(await page.screenshot()).toMatchSnapshot()
+          expect(await page.screenshot()).toMatchSnapshot()
 
           // Resize
           await page.setViewportSize({width: viewports['primer.breakpoint.md'], height: 768})
 
           // Icons should be hidden
-          // expect(await page.screenshot()).toMatchSnapshot()
-        })
-
-        test('Keep selected item visible @vrt', async ({page}) => {
-          await visit(page, {
-            id: 'components-underlinenav-features--overflow-template',
-            globals: {
-              colorScheme: theme,
-            },
-          })
-          await page.setViewportSize({width: viewports['primer.breakpoint.sm'], height: 768})
-
-          await page.locator('button', {hasText: 'More Repository Items'}).waitFor()
-          await page.getByRole('button', {name: 'More Repository Items'}).click()
-          await page.getByRole('link', {name: 'Settings (10)'}).click()
-
-          // State after selecting the second last item
-          // expect(await page.screenshot()).toMatchSnapshot()
-
-          // Resize
-          await page.setViewportSize({
-            width: 1100,
-            height: 480,
-          })
-          await page.locator('button', {hasText: 'More Repository Items'}).waitFor({
-            state: 'hidden',
-          })
-
-          // Current state
-          // expect(await page.screenshot()).toMatchSnapshot()
-
-          // Resize
-          await page.setViewportSize({
-            width: 800,
-            height: 480,
-          })
-          await page.locator('button', {hasText: 'More Repository Items'}).waitFor()
-
-          // Current state
-          // expect(await page.screenshot()).toMatchSnapshot()
-
-          // Resize
-          await page.setViewportSize({
-            width: 600,
-            height: 480,
-          })
-          await page.locator('button', {hasText: 'More Repository Items'}).waitFor()
-          // Current state
-          // expect(await page.screenshot()).toMatchSnapshot()
+          expect(await page.screenshot()).toMatchSnapshot()
         })
       })
     }

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

@@ -77,11 +77,7 @@ const items: {navigation: string; icon: React.ReactElement; counter?: number | s
 export const OverflowTemplate = ({initialSelectedIndex = 1}: {initialSelectedIndex?: number}) => {
   const [selectedIndex, setSelectedIndex] = React.useState<number | null>(initialSelectedIndex)
   return (
-    <UnderlineNav
-      aria-label="Repository"
-      // @ts-ignore UnderlineNav does not take selectionVariant prop, but we need to pass it to the underlying ActionList so it doesn't show Selections.
-      selectionVariant={undefined}
-    >
+    <UnderlineNav aria-label="Repository">
       {items.map((item, index) => (
         <UnderlineNav.Item
           key={item.navigation}

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

@@ -110,7 +110,7 @@ SelectAMenuItem.play = async ({canvasElement}: {canvasElement: HTMLElement}) =>
 
   await delay(1000)
 
-  const moreBtn = canvas.getByRole('button', {name: 'More Repository items'})
+  const moreBtn = canvas.getByRole('button', {name: 'More items'})
   userEvent.hover(moreBtn)
 
   await delay()
@@ -131,35 +131,4 @@ SelectAMenuItem.play = async ({canvasElement}: {canvasElement: HTMLElement}) =>
   expect(lastListItem).toEqual(menuListItem)
 }
 
-const KeepSelectedItemVisible = () => {
-  return <OverflowTemplate initialSelectedIndex={7} />
-}
-// eslint-disable-next-line @typescript-eslint/ban-ts-comment
-// @ts-ignore
-KeepSelectedItemVisible.play = async ({canvasElement}: {canvasElement: HTMLElement}) => {
-  const canvas = within(canvasElement)
-  // await delay(2000)
-  const selectedItem = canvas.getByRole('link', {name: 'Settings (10)'})
-  expect(selectedItem).toHaveAttribute('aria-current', 'page')
-  // change viewport
-  canvasElement.style.width = '900px'
-  await delay(1000)
-  expect(selectedItem).toHaveAttribute('aria-current', 'page')
-  canvasElement.style.width = '800px'
-  await delay(1000)
-  expect(selectedItem).toHaveAttribute('aria-current', 'page')
-  canvasElement.style.width = '700px'
-  await delay(1000)
-  expect(selectedItem).toHaveAttribute('aria-current', 'page')
-  canvasElement.style.width = '600px'
-  await delay(1000)
-  expect(selectedItem).toHaveAttribute('aria-current', 'page')
-  canvasElement.style.width = '500px'
-  await delay(1000)
-  const lastListItem = canvas.getByRole('list').children[2].children[0]
-  const menuListItem = canvas.getByRole('link', {name: 'Settings (10)'})
-  // expect Settings be the last element on the list.
-  expect(lastListItem).toEqual(menuListItem)
-}
-
-export {KeyboardNavigation, SelectAMenuItem, KeepSelectedItemVisible}
+export {KeyboardNavigation, SelectAMenuItem}

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

@@ -1,10 +1,62 @@
-.MenuItemContent {
+.UnderlineWrapper {
+  /* Progressive enhancement: Detect overflow using scroll-based animations.
+  The idiomatic way would be a scroll-state container query but browser support
+  is slightly better for animations. */
+  animation: detect-overflow linear;
+  animation-timeline: scroll(self block);
+
+  &[data-hide-icons='true'] {
+    --UnderlineNav_icons-display: none;
+  }
+
+  &[data-has-overflow='true'] {
+    --UnderlineNav_moreButton-visibility: visible;
+  }
+}
+
+@keyframes detect-overflow {
+  0%,
+  100% {
+    --UnderlineNav_moreButton-visibility: visible;
+    --UnderlineNav_icons-display: none;
+  }
+}
+
+.ItemsList [data-component='icon'] {
+  display: var(--UnderlineNav_icons-display, inline);
+}
+
+.MoreButtonContainer {
   display: flex;
+  visibility: var(--UnderlineNav_moreButton-visibility, hidden);
   align-items: center;
-  justify-content: space-between;
 }
 
-/* More button styles migrated from styles.ts (was moreBtnStyles) */
+.OverflowMenuItem [aria-current] {
+  position: relative;
+
+  .OverflowMenuItemLabel {
+    font-weight: var(--base-text-weight-semibold);
+  }
+
+  &::after {
+    content: '';
+    width: var(--base-size-2);
+    position: absolute;
+    inset: var(--base-size-2) auto var(--base-size-2) 0;
+    /* stylelint-disable-next-line primer/colors */
+    background: var(--underlineNav-borderColor-active);
+  }
+}
+
+.MoreButtonDivider {
+  display: inline-block;
+  border-left: var(--borderWidth-default) solid var(--borderColor-muted);
+  width: 0;
+  margin-inline: var(--base-size-4);
+  height: var(--base-size-24);
+}
+
 .MoreButton {
   margin: 0; /* reset Safari extra margin */
   border: 0;

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,
@@ -16,6 +16,7 @@ import {UnderlineNav} from '.'
 import {implementsClassName} from '../utils/testing'
 import classes from '../internal/components/UnderlineTabbedInterface.module.css'
 import {clsx} from 'clsx'
+import {page} from 'vitest/browser'
 
 const ResponsiveUnderlineNav = ({
   selectedItemText = 'Code',
@@ -78,7 +79,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 () => {
@@ -141,9 +143,10 @@ describe('UnderlineNav', () => {
     expect(counter.textContent).toBe('\u00A0(120)')
   })
 
-  it('respects loadingCounters prop', () => {
+  it('respects loadingCounters prop', async () => {
+    await page.viewport(1000, 500)
     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('')