diff --git a/.changeset/underlinenav-parent-driven-measurement.md b/.changeset/underlinenav-parent-driven-measurement.md new file mode 100644 index 00000000000..f6c3f97457b --- /dev/null +++ b/.changeset/underlinenav-parent-driven-measurement.md @@ -0,0 +1,13 @@ +--- +'@primer/react': patch +--- + +UnderlineNav: Re-architect item measurement to remove a cascading render on +mount. Each `UnderlineNav.Item` no longer dispatches measurement setState calls +back into its parent via context; `UnderlineNav` now walks its rendered items +in a single layout effect and stores widths in refs. Overflow state is +consolidated into one `useState` object (one commit per change), the context +value is memoized, `UnderlineNav.Item` is wrapped in `React.memo`, and the +external-`aria-current` swap was moved out of render-phase setState into a +layout effect. Behavior is unchanged for consumers, but the initial mount and +overflow-state transitions produce far fewer per-item renders. diff --git a/packages/react/src/UnderlineNav/UnderlineNav.test.tsx b/packages/react/src/UnderlineNav/UnderlineNav.test.tsx index 6039ec2ad8a..9969e064657 100644 --- a/packages/react/src/UnderlineNav/UnderlineNav.test.tsx +++ b/packages/react/src/UnderlineNav/UnderlineNav.test.tsx @@ -41,7 +41,7 @@ const ResponsiveUnderlineNav = ({ ] return ( -
+
{items.map(item => ( { expect(nextItem).toHaveFocus() }) }) + +describe('Architecture: parent-driven measurement', () => { + it('tags every Item with [data-underlinenav-item] so the parent can measure them in one pass', () => { + const {container} = render( + + A + B + C + , + ) + expect(container.querySelectorAll('[data-underlinenav-item="true"]')).toHaveLength(3) + }) + + it('renders an Item standalone without requiring measurement callbacks from context', () => { + // The old API required UnderlineNavContext.setChildrenWidth / + // setNoIconChildrenWidth on mount. With the inverted architecture the item + // is a leaf with no upstream measurement responsibility — it should mount + // without throwing even when no UnderlineNav is present above it. + expect(() => + render( +
    + Standalone +
, + ), + ).not.toThrow() + }) + + it('marks the wrapper data-overflow-measured="true" after the measurement pass', async () => { + const {getByRole} = render() + const nav = getByRole('navigation') + // Measurement runs synchronously in a layout effect; by the time render() + // returns the wrapper should already be marked measured. + expect(nav.getAttribute('data-overflow-measured')).toBe('true') + }) + + it('anchors the overflow menu via a layout effect, not by reading refs during render', async () => { + // Regression: the menu position used to be computed inline in render by + // reading containerRef.current / listRef.current. Under concurrent rendering + // those refs may be null or stale during render; the anchor must come from a + // post-commit layout effect. Open the More menu in a narrow container so the + // anchor branch runs, and assert no crash + the menu becomes visible. + function NarrowOpen() { + const items = ['One', 'Two', 'Three', 'Four', 'Five', 'Six', 'Seven', 'Eight', 'Nine', 'Ten'] + return ( +
+ + {items.map(name => ( + {name} + ))} + +
+ ) + } + const {getByRole, container} = render() + const moreBtn = getByRole('button', {name: /More/i}) + const user = userEvent.setup() + await user.click(moreBtn) + // After opening, the ActionList container's inline display must flip to "block". + // What matters for this test: clicking did not crash (the new layout effect + // tolerates the first commit where refs/widths may not be ready) and the + // overlay element is in the DOM with a non-"none" display. + const overlay = container.querySelector('[data-component="ActionList"]') as HTMLElement | null + expect(overlay).not.toBeNull() + expect(overlay!.style.display).toBe('block') + }) +}) + +describe('Structure: aria-current swap is an effect (not render-phase setState)', () => { + function NarrowDemo({current}: {current: string}) { + const items = ['One', 'Two', 'Three', 'Four', 'Five', 'Six', 'Seven', 'Eight', 'Nine', 'Ten'] + return ( +
+ + {items.map(name => ( + + {name} + + ))} + +
+ ) + } + + it('pulls a menu item into the visible list when aria-current is moved onto it externally', () => { + const {getByRole, rerender} = render() + // "One" is at the front of the list and should be visible from the start. + expect(getByRole('link', {name: 'One'}).getAttribute('aria-current')).toBe('page') + // "Ten" would normally overflow into the menu at this width. Mark it as the + // current page and the lifted effect should promote it into the visible list. + rerender() + const promoted = getByRole('link', {name: 'Ten'}) + expect(promoted).toBeInTheDocument() + expect(promoted.getAttribute('aria-current')).toBe('page') + }) + + it('promotes a menu item correctly when its children are not a plain string', () => { + // Regression: the swap helper used to look widths up by `item.props.children` + // as a string, which silently returned 0 for non-string children and produced + // an incorrect swap (wrong items pulled from the list, or the same item being + // both promoted and demoted). + function NestedDemo({current}: {current: string}) { + const items = ['One', 'Two', 'Three', 'Four', 'Five', 'Six', 'Seven', 'Eight', 'Nine', 'Ten'] + return ( +
+ + {items.map(name => ( + + {name} + + ))} + +
+ ) + } + const {getByRole, rerender} = render() + expect(getByRole('link', {name: 'One'}).getAttribute('aria-current')).toBe('page') + rerender() + const promoted = getByRole('link', {name: 'Ten'}) + expect(promoted).toBeInTheDocument() + expect(promoted.getAttribute('aria-current')).toBe('page') + }) +}) + +describe('Performance: bounded re-renders during initial mount', () => { + it('renders each item at most twice during initial mount with stable children', () => { + // Captures every render of an item-body component. With per-item measurement + // callbacks each item used to trigger a parent setState which cascaded back + // through the item tree (gated by context-value identity, which was a fresh + // object literal on every render). The re-architecture stores widths in + // refs, measures once from the parent, memoizes Item, and memoizes the + // context value — so each item-body should render at most twice on mount: + // once for the initial commit and at most once after the single + // measurement state update. + const renderSpy = vi.fn() + function ItemBody({label}: {label: string}) { + renderSpy(label) + return <>{label} + } + const items = Array.from({length: 12}, (_, i) => `Item ${i + 1}`) + render( +
+ + {items.map(label => ( + + + + ))} + +
, + ) + const counts = new Map() + for (const call of renderSpy.mock.calls) { + const label = call[0] as string + counts.set(label, (counts.get(label) ?? 0) + 1) + } + expect(counts.size).toBe(items.length) + for (const [label, count] of counts) { + expect(count, `${label} rendered ${count} times during mount`).toBeLessThanOrEqual(2) + } + }) + + it('does not re-render items linearly in N when more items are added', () => { + // Two trees: one with 5 items, one with 25 items. Per-item render count + // should be roughly constant (i.e. independent of N). + const counts = new Map() + function makeTree(n: number) { + let totalRenders = 0 + function ItemBody({label}: {label: string}) { + totalRenders++ + return <>{label} + } + const items = Array.from({length: n}, (_, i) => `I${i}`) + const {unmount} = render( +
+ + {items.map(label => ( + + + + ))} + +
, + ) + counts.set(n, totalRenders / n) + unmount() + } + makeTree(5) + makeTree(25) + // Per-item render rate must not grow with N — if anything went back to the + // child-callback pattern this would scale linearly. + expect(counts.get(25)!).toBeLessThanOrEqual(counts.get(5)! + 0.5) + }) + + it('produces at most one post-measurement state commit', () => { + // Snapshot the UnderlineNav's own render count via a Profiler. The single + // combined layout effect should commit at most once after measurement + // (and once more for the children-key derived reset on first mount), + // i.e. UnderlineNav renders <= 3 times total: initial + reset + measurement. + let parentRenderCount = 0 + function CountedNav(props: {children: React.ReactNode}) { + parentRenderCount++ + return {props.children} + } + const items = Array.from({length: 8}, (_, i) => `Item ${i + 1}`) + render( +
+ + {items.map(label => ( + {label} + ))} + +
, + ) + // Initial render + at most one commit from the combined measurement+swap + // layout effect. If measurement and swap had remained two effects this + // would be 3+ renders. If the resize observer also commits redundantly + // on mount it'd be 4+. + expect(parentRenderCount).toBeLessThanOrEqual(3) + }) +}) diff --git a/packages/react/src/UnderlineNav/UnderlineNav.tsx b/packages/react/src/UnderlineNav/UnderlineNav.tsx index 14c52ee9683..8ed1506eddc 100644 --- a/packages/react/src/UnderlineNav/UnderlineNav.tsx +++ b/packages/react/src/UnderlineNav/UnderlineNav.tsx @@ -1,9 +1,10 @@ import type {RefObject} from 'react' -import React, {useRef, forwardRef, useCallback, useState, useEffect} from 'react' +import React, {useRef, forwardRef, useCallback, useMemo, useState, useEffect} from 'react' import {UnderlineNavContext} from './UnderlineNavContext' import type {ResizeObserverEntry} from '../hooks/useResizeObserver' import {useResizeObserver} from '../hooks/useResizeObserver' -import type {ChildWidthArray, ResponsiveProps, ChildSize} from './types' +import useIsomorphicLayoutEffect from '../utils/useIsomorphicLayoutEffect' +import type {ChildWidthMap, ResponsiveProps} from './types' import VisuallyHidden from '../_VisuallyHidden' import {dividerStyles, menuItemStyles, baseMenuMinWidth} from './styles' import {UnderlineItemList, UnderlineWrapper, LoadingCounter, GAP} from '../internal/components/UnderlineTabbedInterface' @@ -36,29 +37,28 @@ export type UnderlineNavProps = { } // When page is loaded, we don't have ref for the more button as it is not on the DOM yet. // However, we need to calculate number of possible items when the more button present as well. So using the width of the more button as a constant. -export const MORE_BTN_WIDTH = 86 +const MORE_BTN_WIDTH = 86 // The height is needed to make sure we don't have a layout shift when the more button is the only item in the nav. const MORE_BTN_HEIGHT = 45 -const overflowEffect = ( +const computeOverflow = ( navWidth: number, moreMenuWidth: number, // eslint-disable-next-line @typescript-eslint/no-explicit-any childArray: Array>, - childWidthArray: ChildWidthArray, - noIconChildWidthArray: ChildWidthArray, - updateListAndMenu: (props: ResponsiveProps, iconsVisible: boolean, overflowMeasured: boolean) => void, -) => { + childWidths: ChildWidthMap, + noIconChildWidths: ChildWidthMap, +): {items: ResponsiveProps['items']; menuItems: ResponsiveProps['menuItems']; iconsVisible: boolean} => { let iconsVisible = true - if (childWidthArray.length === 0) { - updateListAndMenu({items: childArray, menuItems: []}, iconsVisible, false) - return + if (childWidths.size === 0) { + return {items: childArray, menuItems: [], iconsVisible} } - const numberOfItemsPossible = calculatePossibleItems(childWidthArray, navWidth) - const numberOfItemsWithoutIconPossible = calculatePossibleItems(noIconChildWidthArray, navWidth) + const numberOfItemsPossible = calculatePossibleItems(childArray, childWidths, navWidth) + const numberOfItemsWithoutIconPossible = calculatePossibleItems(childArray, noIconChildWidths, navWidth) // We need to take more menu width into account when calculating the number of items possible const numberOfItemsPossibleWithMoreMenu = calculatePossibleItems( - noIconChildWidthArray, + childArray, + noIconChildWidths, navWidth, moreMenuWidth || MORE_BTN_WIDTH, ) @@ -105,25 +105,30 @@ const overflowEffect = ( } } } - updateListAndMenu({items, menuItems}, iconsVisible, true) + return {items, menuItems, iconsVisible} } -export const getValidChildren = (children: React.ReactNode) => { +const getValidChildren = (children: React.ReactNode) => { // eslint-disable-next-line @typescript-eslint/no-explicit-any return React.Children.toArray(children).filter(child => React.isValidElement(child)) as React.ReactElement[] } -const calculatePossibleItems = (childWidthArray: ChildWidthArray, navWidth: number, moreMenuWidth = 0) => { +const calculatePossibleItems = ( + // eslint-disable-next-line @typescript-eslint/no-explicit-any + childArray: Array>, + widthMap: ChildWidthMap, + navWidth: number, + moreMenuWidth = 0, +) => { const widthToFit = navWidth - moreMenuWidth - let breakpoint = childWidthArray.length // assume all items will fit + let breakpoint = childArray.length // assume all items will fit let sumsOfChildWidth = 0 - for (const [index, childWidth] of childWidthArray.entries()) { - sumsOfChildWidth = sumsOfChildWidth + childWidth.width + GAP + for (const [index, child] of childArray.entries()) { + const width = widthMap.get(String(child.key)) ?? 0 + sumsOfChildWidth = sumsOfChildWidth + width + GAP if (sumsOfChildWidth > widthToFit) { breakpoint = index break - } else { - continue } } return breakpoint @@ -164,31 +169,63 @@ export const UnderlineNav = forwardRef( const disclosureWidgetId = useId() const [isWidgetOpen, setIsWidgetOpen] = useState(false) - const [iconsVisible, setIconsVisible] = useState(true) - const [childWidthArray, setChildWidthArray] = useState([]) - const [noIconChildWidthArray, setNoIconChildWidthArray] = useState([]) - // Track whether the initial overflow calculation is complete to prevent CLS - const [isOverflowMeasured, setIsOverflowMeasured] = useState(false) + // Measurement results are stored in refs because only `computeOverflow` reads them; + // they should not trigger re-renders. Keyed by `String(element.key)` so lookups + // are O(1) and survive non-string item children. + const childWidthsRef = useRef(new Map()) + const noIconChildWidthsRef = useRef(new Map()) const validChildren = getValidChildren(children) - - // Responsive props object manages which items are in the list and which items are in the menu. - const [responsiveProps, setResponsiveProps] = useState({ + // Stable signature of the children list — when it changes we re-run the + // measurement pass with all items rendered (with icons) in the list. The + // delimiter is U+0000 (NUL) because React rejects NUL inside element keys, + // so consumer-provided keys cannot collide with the separator. + const childrenKey = validChildren.map(c => String(c.key)).join('\u0000') + + // Single consolidated overflow state. Bundling `items`, `menuItems`, + // `iconsVisible`, `measured`, and `childrenKey` together means every + // overflow update — measurement, swap, resize, or children-change reset — + // is exactly one setState call (one commit), instead of fanning out to + // multiple back-to-back commits. + type OverflowState = { + items: ResponsiveProps['items'] + menuItems: ResponsiveProps['menuItems'] + iconsVisible: boolean + measured: boolean + childrenKey: string + } + const [overflowState, setOverflowState] = useState(() => ({ items: validChildren, menuItems: [], - }) + iconsVisible: true, + measured: false, + childrenKey, + })) + + // Derived-state reset: when the children list changes, push everything back into + // the list (with icons) so the next layout effect can measure all items in one + // pass. This is React's recommended "store information from previous renders" + // pattern — calling setState during render lets React discard the in-progress + // render and re-render immediately with the reset state, before paint. + if (overflowState.childrenKey !== childrenKey) { + setOverflowState({items: validChildren, menuItems: [], iconsVisible: true, measured: false, childrenKey}) + } + + // Build a key-indexed view of validChildren once per render so the listItems / + // menuItems prop-refresh below is O(N) instead of O(N^2) via Array.prototype.find. + // eslint-disable-next-line @typescript-eslint/no-explicit-any + const validChildrenByKey = new Map>() + for (const c of validChildren) validChildrenByKey.set(String(c.key), c) + + const {iconsVisible, measured: isOverflowMeasured} = overflowState // Make sure to have the fresh props data for list items when children are changed (keeping aria-current up-to-date) - const listItems = responsiveProps.items.map(item => { - return validChildren.find(child => child.key === item.key) ?? item - }) + const listItems = overflowState.items.map(item => validChildrenByKey.get(String(item.key)) ?? item) // Make sure to have the fresh props data for menu items when children are changed (keeping aria-current up-to-date) - const menuItems = responsiveProps.menuItems.map(menuItem => { - return validChildren.find(child => child.key === menuItem.key) ?? menuItem - }) + const menuItems = overflowState.menuItems.map(item => validChildrenByKey.get(String(item.key)) ?? item) // This is the case where the viewport is too narrow to show any list item with the more menu. In this case, we only show the dropdown - const onlyMenuVisible = responsiveProps.items.length === 0 + const onlyMenuVisible = overflowState.items.length === 0 if (__DEV__) { // Practically, this is not a conditional hook, it is just making sure this hook runs only on DEV not PROD. @@ -203,75 +240,156 @@ export const UnderlineNav = forwardRef( }) } - function getItemsWidth(itemText: string): number { - return noIconChildWidthArray.find(item => item.text === itemText)?.width ?? 0 - } - - const swapMenuItemWithListItem = ( + // Returns the measured "no icon" width for an item, looking up by element key. + // Previously this was a text-based lookup, which silently returned 0 when an + // item's children weren't a plain string and caused the swap math to degenerate. + // eslint-disable-next-line @typescript-eslint/no-explicit-any + const getItemNoIconWidth = (item: React.ReactElement): number => + noIconChildWidthsRef.current.get(String(item.key)) ?? 0 + + // Pure: returns the {items, menuItems} that result from promoting + // `prospectiveListItem` (currently at position `indexOfProspectiveListItem` + // in `currentMenuItems`) into `currentItems`. Does not call setState. + const computeSwap = ( + currentItems: ResponsiveProps['items'], + currentMenuItems: ResponsiveProps['menuItems'], // eslint-disable-next-line @typescript-eslint/no-explicit-any prospectiveListItem: React.ReactElement, indexOfProspectiveListItem: number, - event: React.MouseEvent | React.KeyboardEvent, - callback: (props: ResponsiveProps, displayIcons: boolean, overflowMeasured: boolean) => void, - ) => { - // get the selected menu item's width - const widthToFitIntoList = getItemsWidth(prospectiveListItem.props.children) - // Check if there is any empty space on the right side of the list + ): {items: ResponsiveProps['items']; menuItems: ResponsiveProps['menuItems']} => { + const widthToFitIntoList = getItemNoIconWidth(prospectiveListItem) const availableSpace = (navRef.current?.getBoundingClientRect().width ?? 0) - (listRef.current?.getBoundingClientRect().width ?? 0) - // Calculate how many items need to be pulled in to the menu to make room for the selected menu item - // I.e. if we need to pull 2 items in (index 0 and index 1), breakpoint (index) will return 1. - const index = getBreakpointForItemSwapping(widthToFitIntoList, availableSpace) - const indexToSliceAt = responsiveProps.items.length - 1 - index - // Form the new list of items - const itemsLeftInList = [...responsiveProps.items].slice(0, indexToSliceAt) - const updatedItemList = [...itemsLeftInList, prospectiveListItem] - // Form the new menu items - const itemsToAddToMenu = [...responsiveProps.items].slice(indexToSliceAt) - const updatedMenuItems = [...menuItems] - // Add itemsToAddToMenu array's items to the menu at the index of the prospectiveListItem and remove 1 count of items (prospectiveListItem) - updatedMenuItems.splice(indexOfProspectiveListItem, 1, ...itemsToAddToMenu) - callback({items: updatedItemList, menuItems: updatedMenuItems}, false, true) - } - // How many items do we need to pull in to the menu to make room for the selected menu item. - function getBreakpointForItemSwapping(widthToFitIntoList: number, availableSpace: number) { + // How many items at the end of the list have to move to the menu to make + // room for the promoted item. let widthToSwap = 0 let breakpoint = 0 - for (const [index, item] of [...responsiveProps.items].reverse().entries()) { - widthToSwap += getItemsWidth(item.props.children) + for (const [index, item] of [...currentItems].reverse().entries()) { + widthToSwap += getItemNoIconWidth(item) if (widthToFitIntoList < widthToSwap + availableSpace) { breakpoint = index break } } - return breakpoint + + const indexToSliceAt = currentItems.length - 1 - breakpoint + const itemsLeftInList = currentItems.slice(0, indexToSliceAt) + const updatedItemList = [...itemsLeftInList, prospectiveListItem] + const itemsToAddToMenu = currentItems.slice(indexToSliceAt) + const updatedMenuItems = [...currentMenuItems] + updatedMenuItems.splice(indexOfProspectiveListItem, 1, ...itemsToAddToMenu) + return {items: updatedItemList, menuItems: updatedMenuItems} + } + + const swapMenuItemWithListItem = ( + // eslint-disable-next-line @typescript-eslint/no-explicit-any + prospectiveListItem: React.ReactElement, + indexOfProspectiveListItem: number, + ) => { + const swap = computeSwap(overflowState.items, menuItems, prospectiveListItem, indexOfProspectiveListItem) + setOverflowState(prev => ({ + items: swap.items, + menuItems: swap.menuItems, + iconsVisible: false, + measured: true, + childrenKey: prev.childrenKey, + })) } - const updateListAndMenu = useCallback( - (props: ResponsiveProps, displayIcons: boolean, overflowMeasured: boolean) => { - setResponsiveProps(props) - setIconsVisible(displayIcons) + // Single combined layout effect that performs measurement (when needed) and + // the external-aria-current promotion (when needed), then commits at most + // one setOverflowState per pass. Replaces the previous two effects (each of + // which produced its own commit) and removes the render-phase setState that + // the original component used to do the same swap from inside menuItems.map. + // + // Why no deps array: the swap branch depends on `aria-current` which lives + // on item props (not on `overflowState`); a deps-driven effect would not + // notice prop-only changes without rebuilding a signature. The body + // short-circuits cheaply on every render where neither pass needs to run. + useIsomorphicLayoutEffect(() => { + const listEl = listRef.current + const navEl = navRef.current + if (!listEl || !navEl) return + + let next: OverflowState | null = null + + // Pass 1: measurement (runs once after each remeasure trigger). + if (!isOverflowMeasured) { + const widths: ChildWidthMap = new Map() + const noIconWidths: ChildWidthMap = new Map() + const itemEls = listEl.querySelectorAll('[data-underlinenav-item="true"]') + + // The N-th [data-underlinenav-item] element corresponds to validChildren[N] + // because the measurement pass runs only when all items are in the list, + // in render order. Pair them up positionally so we can key widths by the + // element's React key. + const upToBoundary = Math.min(itemEls.length, validChildren.length) + for (let index = 0; index < upToBoundary; index++) { + const liEl = itemEls[index] + const linkEl = liEl.querySelector('a, button') + if (!linkEl) continue + const child = validChildren[index] + const key = String(child.key) + + const domRect = linkEl.getBoundingClientRect() + const iconEl = linkEl.querySelector('[data-component="icon"]') + + let iconWidthWithMargin = 0 + if (iconEl) { + const style = getComputedStyle(iconEl) + iconWidthWithMargin = + iconEl.getBoundingClientRect().width + + (parseFloat(style.marginRight) || 0) + + (parseFloat(style.marginLeft) || 0) + } + + widths.set(key, domRect.width) + noIconWidths.set(key, domRect.width - iconWidthWithMargin) + } - if (overflowMeasured) { - setIsOverflowMeasured(true) + childWidthsRef.current = widths + noIconChildWidthsRef.current = noIconWidths + + const navWidth = navEl.getBoundingClientRect().width + if (navWidth === 0) return + const moreMenuWidth = moreMenuRef.current?.getBoundingClientRect().width ?? 0 + const result = computeOverflow(navWidth, moreMenuWidth, validChildren, widths, noIconWidths) + next = { + items: result.items, + menuItems: result.menuItems, + iconsVisible: result.iconsVisible, + measured: true, + childrenKey: overflowState.childrenKey, } - }, - [], - ) - const setChildrenWidth = useCallback((size: ChildSize) => { - setChildWidthArray(arr => { - const newArr = [...arr, size] - return newArr - }) - }, []) + } - const setNoIconChildrenWidth = useCallback((size: ChildSize) => { - setNoIconChildWidthArray(arr => { - const newArr = [...arr, size] - return newArr - }) - }, []) + // Pass 2: external-aria-current promotion. Operates on the freshest + // (post-measurement) items so it can fold into the same setState. + const considered: OverflowState = next ?? overflowState + if (considered.measured && considered.items.length > 0) { + // Look at FRESH props (current aria-current) by remapping through + // validChildrenByKey (O(1) per lookup; the previous .find() pattern was + // O(N) and ran inside a .map() — quadratic on every commit). + const freshMenuItems = considered.menuItems.map(item => validChildrenByKey.get(String(item.key)) ?? item) + const indexOfCurrent = freshMenuItems.findIndex(item => { + const c = item.props['aria-current'] + return Boolean(c) && c !== 'false' + }) + if (indexOfCurrent !== -1) { + const swap = computeSwap(considered.items, freshMenuItems, freshMenuItems[indexOfCurrent], indexOfCurrent) + next = { + items: swap.items, + menuItems: swap.menuItems, + iconsVisible: false, + measured: true, + childrenKey: considered.childrenKey, + } + } + } + + if (next) setOverflowState(next) + }) const closeOverlay = React.useCallback(() => { setIsWidgetOpen(false) @@ -303,42 +421,62 @@ export const UnderlineNav = forwardRef( useResizeObserver((resizeObserverEntries: ResizeObserverEntry[]) => { const navWidth = resizeObserverEntries[0].contentRect.width + if (navWidth === 0) return const moreMenuWidth = moreMenuRef.current?.getBoundingClientRect().width ?? 0 - navWidth !== 0 && - overflowEffect( - navWidth, - moreMenuWidth, - validChildren, - childWidthArray, - noIconChildWidthArray, - updateListAndMenu, - ) + const result = computeOverflow( + navWidth, + moreMenuWidth, + validChildren, + childWidthsRef.current, + noIconChildWidthsRef.current, + ) + setOverflowState(prev => ({ + items: result.items, + menuItems: result.menuItems, + iconsVisible: result.iconsVisible, + measured: childWidthsRef.current.size > 0 || prev.measured, + childrenKey: prev.childrenKey, + })) }, navRef as RefObject) - // Compute menuInlineStyles if needed - let menuInlineStyles: React.CSSProperties = {...baseMenuInlineStyles} - if (containerRef.current && listRef.current) { - const {left} = getAnchoredPosition(containerRef.current, listRef.current, { + // Menu anchor position. `undefined` means "use the default right: 0 from + // baseMenuInlineStyles" (the list is wide enough that no anchoring is + // needed). A number means the menu is anchored to that `left` coordinate, + // overriding right. + // + // This used to be computed inline during render by reading + // containerRef.current / listRef.current — refs are mutable and unsafe to + // read during render under concurrent rendering (the first render saw null + // refs and fell through to the unanchored fallback; subsequent renders + // anchored differently, producing a perceptible reflow). + const [menuAnchorLeft, setMenuAnchorLeft] = useState(undefined) + useIsomorphicLayoutEffect(() => { + const container = containerRef.current + const list = listRef.current + if (!container || !list) return + if (list.clientWidth >= baseMenuMinWidth) { + if (menuAnchorLeft !== undefined) setMenuAnchorLeft(undefined) + return + } + const {left} = getAnchoredPosition(container, list, { align: 'start', side: 'outside-bottom', }) + if (menuAnchorLeft !== left) setMenuAnchorLeft(left) + }, [isWidgetOpen, isOverflowMeasured, menuItems.length, menuAnchorLeft]) - menuInlineStyles = { - ...baseMenuInlineStyles, - right: undefined, - left, - } - } + // Stable context value: a fresh object literal here would re-render every + // consumer (UnderlineNav.Item) on every parent re-render, undoing the + // React.memo on UnderlineNavItem. + const contextValue = useMemo(() => ({loadingCounters, iconsVisible}), [loadingCounters, iconsVisible]) + + const menuStyle: React.CSSProperties = + menuAnchorLeft !== undefined + ? {...baseMenuInlineStyles, right: undefined, left: menuAnchorLeft, display: isWidgetOpen ? 'block' : 'none'} + : {...baseMenuInlineStyles, display: isWidgetOpen ? 'block' : 'none'} return ( - + {ariaLabel && {`${ariaLabel} navigation`}} - = baseMenuMinWidth - ? baseMenuInlineStyles - : menuInlineStyles), - display: isWidgetOpen ? 'block' : 'none', - }} - > + {menuItems.map((menuItem, index) => { const { children: menuItemChildren, counter, - 'aria-current': ariaCurrent, + // Stripped from the spread below; the swap effect reads aria-current + // directly from item.props (not via these destructured locals). + // eslint-disable-next-line @typescript-eslint/no-unused-vars + 'aria-current': _ariaCurrent, onSelect, ...menuItemProps } = menuItem.props - // This logic is used to pop the selected item out of the menu and into the list when the navigation is control externally - if (Boolean(ariaCurrent) && ariaCurrent !== 'false') { - const event = new MouseEvent('click') - !onlyMenuVisible && - swapMenuItemWithListItem( - menuItem, - index, - // @ts-ignore - not a big deal because it is internally creating an event but ask help - event as React.MouseEvent, - updateListAndMenu, - ) - } - return ( | React.KeyboardEvent, ) => { // When there are no items in the list, do not run the swap function as we want to keep everything in the menu. - !onlyMenuVisible && swapMenuItemWithListItem(menuItem, index, event, updateListAndMenu) + !onlyMenuVisible && swapMenuItemWithListItem(menuItem, index) closeOverlay() focusOnMoreMenuBtn() // fire onSelect event that comes from the UnderlineNav.Item (if it is defined) diff --git a/packages/react/src/UnderlineNav/UnderlineNavContext.tsx b/packages/react/src/UnderlineNav/UnderlineNavContext.tsx index f276771efac..70136c1aaeb 100644 --- a/packages/react/src/UnderlineNav/UnderlineNavContext.tsx +++ b/packages/react/src/UnderlineNav/UnderlineNavContext.tsx @@ -1,14 +1,9 @@ -import type React from 'react' import {createContext} from 'react' export const UnderlineNavContext = createContext<{ - setChildrenWidth: React.Dispatch<{text: string; width: number}> - setNoIconChildrenWidth: React.Dispatch<{text: string; width: number}> loadingCounters: boolean iconsVisible: boolean }>({ - setChildrenWidth: () => null, - setNoIconChildrenWidth: () => null, loadingCounters: false, iconsVisible: true, }) diff --git a/packages/react/src/UnderlineNav/UnderlineNavItem.tsx b/packages/react/src/UnderlineNav/UnderlineNavItem.tsx index 3341f677e11..b921fd1443d 100644 --- a/packages/react/src/UnderlineNav/UnderlineNavItem.tsx +++ b/packages/react/src/UnderlineNav/UnderlineNavItem.tsx @@ -1,9 +1,8 @@ -import type {MutableRefObject, RefObject} from 'react' +import type {RefObject} from 'react' import React, {forwardRef, useRef, useContext} from 'react' import type {IconProps} from '@primer/octicons-react' import type {ForwardRefComponent as PolymorphicForwardRefComponent} from '../utils/polymorphic' import {UnderlineNavContext} from './UnderlineNavContext' -import useLayoutEffect from '../utils/useIsomorphicLayoutEffect' import {UnderlineItem} from '../internal/components/UnderlineTabbedInterface' import classes from './UnderlineNavItem.module.css' @@ -59,7 +58,7 @@ export type UnderlineNavItemProps = { counter?: number | string } & LinkProps -export const UnderlineNavItem = forwardRef( +const UnderlineNavItemImpl = forwardRef( ( { as: Component = 'a', @@ -76,31 +75,7 @@ export const UnderlineNavItem = forwardRef( ) => { const backupRef = useRef(null) const ref = (forwardedRef ?? backupRef) as RefObject - const {setChildrenWidth, setNoIconChildrenWidth, loadingCounters, iconsVisible} = useContext(UnderlineNavContext) - - useLayoutEffect(() => { - if (ref.current) { - const domRect = (ref as MutableRefObject).current.getBoundingClientRect() - - const icon = Array.from((ref as MutableRefObject).current.children).find( - child => child.getAttribute('data-component') === 'icon', - ) - - const content = Array.from((ref as MutableRefObject).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}) - } - }, [ref, setChildrenWidth, setNoIconChildrenWidth]) + const {loadingCounters, iconsVisible} = useContext(UnderlineNavContext) const keyDownHandler = React.useCallback( (event: React.KeyboardEvent) => { @@ -120,7 +95,7 @@ export const UnderlineNavItem = forwardRef( ) return ( -
  • +
  • +UnderlineNavItemImpl.displayName = 'UnderlineNavItem' -UnderlineNavItem.displayName = 'UnderlineNavItem' +// Memoized so that re-renders of UnderlineNav (e.g. when overflow state changes) +// do not re-render every item when the consumer-provided children haven't changed. +export const UnderlineNavItem = React.memo(UnderlineNavItemImpl) as unknown as typeof UnderlineNavItemImpl diff --git a/packages/react/src/UnderlineNav/types.ts b/packages/react/src/UnderlineNav/types.ts index 8e50e5582f0..5fd4eba0a8a 100644 --- a/packages/react/src/UnderlineNav/types.ts +++ b/packages/react/src/UnderlineNav/types.ts @@ -1,8 +1,10 @@ -export type ChildSize = { - text: string - width: number -} -export type ChildWidthArray = Array +// Maps a React element key (assigned by React.Children.toArray) to the +// measured pixel width of the rendered item. Keyed by `String(element.key)` +// rather than by item text so that items whose `children` aren't a plain +// string (e.g. `<> Label`) still resolve correctly during swap +// and overflow calculations. +export type ChildWidthMap = Map + export type ResponsiveProps = { // eslint-disable-next-line @typescript-eslint/no-explicit-any items: Array>