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

.changeset/announce-skip-empty-reflow.md

@@ -0,0 +1,5 @@
+---
+'@primer/react': patch
+---
+
+perf(Announce): skip getComputedStyle when there is no text content to announce

.changeset/button-optimize-render.md

@@ -0,0 +1,5 @@
+---
+'@primer/react': patch
+---
+
+perf(Button): fix CounterLabel remount and remove conditional DEV hook

.changeset/content-visibility-perf.md

@@ -0,0 +1,5 @@
+---
+'@primer/react': patch
+---
+
+Add `content-visibility: auto` to ActionList items to improve rendering performance for large lists by allowing the browser to skip layout and paint for off-screen items.

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/package.json

@@ -134,7 +134,7 @@
     "@types/react-is": "18.3.1",
     "@vitejs/plugin-react": "^4.3.3",
     "afterframe": "^1.0.2",
-    "ajv": "8.16.0",
+    "ajv": "8.18.0",
     "axe-core": "4.9.1",
     "babel-core": "7.0.0-bridge.0",
     "babel-plugin-add-react-displayname": "0.0.5",

packages/react/script/react-compiler.mjs

@@ -15,7 +15,6 @@ const unsupported = new Set(
   [
     'src/ActionMenu/**/*.tsx',
     'src/AvatarStack/**/*.tsx',
-    'src/Button/**/*.tsx',
     'src/ConfirmationDialog/**/*.tsx',
     'src/Pagehead/**/*.tsx',
     'src/Pagination/**/*.tsx',

packages/react/src/Button/ButtonBase.tsx

@@ -60,24 +60,19 @@ const ButtonBase = forwardRef(({children, as: Component = 'button', ...props}, f
   const ariaDescribedByIds = loading ? [loadingAnnouncementID, ariaDescribedBy] : [ariaDescribedBy]
 
   if (__DEV__) {
-    /**
-     * The Linter yells because it thinks this conditionally calls an effect,
-     * but since this is a compile-time flag and not a runtime conditional
-     * this is safe, and ensures the entire effect is kept out of prod builds
-     * shaving precious bytes from the output, and avoiding mounting a noop effect
-     */
-    // eslint-disable-next-line react-hooks/rules-of-hooks
-    React.useEffect(() => {
-      if (
-        innerRef.current &&
-        !(innerRef.current instanceof HTMLButtonElement) &&
-        !((innerRef.current as unknown) instanceof HTMLAnchorElement) &&
-        !((innerRef.current as HTMLElement).tagName === 'SUMMARY')
-      ) {
-        // eslint-disable-next-line no-console
-        console.warn('This component should be an instanceof a semantic button or anchor')
-      }
-    }, [innerRef])
+    // Validate that the element is a semantic button/anchor.
+    // This runs during render (not in an effect) to avoid a conditional hook call
+    // that prevents React Compiler from optimizing this component.
+    const el = innerRef.current
+    if (
+      el &&
+      !(el instanceof HTMLButtonElement) &&
+      !((el as unknown) instanceof HTMLAnchorElement) &&
+      !((el as HTMLElement).tagName === 'SUMMARY')
+    ) {
+      // eslint-disable-next-line no-console
+      console.warn('This component should be an instanceof a semantic button or anchor')
+    }
   }
   return (
     <ConditionalWrapper
@@ -154,11 +149,9 @@ const ButtonBase = forwardRef(({children, as: Component = 'button', ...props}, f
                   */
                 count !== undefined && !TrailingVisual
                   ? renderModuleVisual(
-                      () => (
-                        <CounterLabel className={classes.CounterLabel} data-component="ButtonCounter">
-                          {count}
-                        </CounterLabel>
-                      ),
+                      <CounterLabel className={classes.CounterLabel} data-component="ButtonCounter">
+                        {count}
+                      </CounterLabel>,
                       Boolean(loading) && !LeadingVisual,
                       'trailingVisual',
                       true,

packages/react/src/FilteredActionList/FilteredActionList.module.css

@@ -15,6 +15,14 @@
   height: 100%;
   overflow: auto;
   flex-grow: 1;
+
+  /* Allow the browser to skip rendering for off-screen items, reducing style recalc and layout costs in long lists.
+     Exclude items that show the active indicator line, as content-visibility: auto applies paint containment
+     which clips the absolutely-positioned ::after pseudo-element that renders outside the item bounds. */
+  & .ActionListItem:not(:focus, [data-is-active-descendant], [data-active], [data-input-focused]) {
+    content-visibility: auto;
+    contain-intrinsic-size: auto 32px;
+  }
 }
 
 .ActionList {

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

@@ -1440,16 +1440,15 @@ describe('Asynchronous loading', () => {
     const {getByRole} = renderWithTheme(<TestTree />)
 
     const doneButton = getByRole('button', {name: 'Load'})
-    const liveRegion = getLiveRegion()
-
-    // Live region should be empty
-    expect(liveRegion.getMessage('polite')).toBe('')
 
     // Click load button to mimic async loading
     await act(async () => {
       await user.click(doneButton)
     })
 
+    // Get live region after the first announcement creates it
+    const liveRegion = getLiveRegion()
+
     expect(liveRegion.getMessage('polite')).toBe('Parent content loading')
 
     // Click done button to mimic the completion of async loading

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}