perf(UnderlineNav): replace JS DOM measurement with CSS overflow + IntersectionObserver#7541
perf(UnderlineNav): replace JS DOM measurement with CSS overflow + IntersectionObserver#7541hectahertz wants to merge 8 commits intomainfrom
Conversation
…tersectionObserver Replace synchronous getBoundingClientRect/getComputedStyle layout-forcing calls with CSS overflow: hidden + IntersectionObserver for overflow detection. - Eliminates all forced reflows from UnderlineNav (72 → 0 layout-forcing calls) - Removes useLayoutEffect (paint-blocking) in favor of async useEffect - Removes ResizeObserver + manual width calculation loop - Removes getAnchoredPosition dependency - Simplifies overflow menu item management with useMemo - Moves More button outside the <ul> to avoid overflow: hidden clipping - Adds visibility: hidden for aria-hidden overflow items to prevent partial clipping - Includes icon toggle loop prevention via width-based retry threshold
🦋 Changeset detectedLatest commit: 9a1a2b5 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
👋 Hi, this pull request contains changes to the source code that github/github-ui depends on. If you are GitHub staff, test these changes with github/github-ui using the integration workflow. Or, apply the |
github-actions
bot
added
the
integration-tests: recommended
…om context - Always render icons in DOM, hide via CSS [data-icons-visible='false'] on the wrapper instead of conditionally rendering with React state. This eliminates React re-render cycles when toggling icon visibility. - Use direct DOM attribute (setAttribute) for icon toggle in IO callback, so the browser relayouts and IO re-fires without a React reconciliation. - Remove iconsVisible from UnderlineNavContext (no longer needed). - Add flex-shrink: 0 on icon span to prevent icon compression. - Maintain backward compatibility with UnderlinePanels (still accepts iconsVisible prop, uses same CSS data attribute pattern).
hectahertz
force-pushed
the
hectahertz/perf/underlinenav-intersection-observer-overflow
branch
from
b88214f to
36fdb29
Compare
github-actions
bot
requested a deployment
to
storybook-preview-7541
- Add fixed height (48px) on the nav wrapper to lock vertical dimensions - Always render the More button container before IO has fired, hidden via visibility: hidden, so it reserves horizontal space from the first paint - Once IO determines overflow state, the More button becomes visible (overflow) or is removed entirely (no overflow) with no layout shift - Fix test to scope SVG count to the list element (More button's SVG is now always in the DOM during overflow)
- Remove dead export MORE_BTN_WIDTH (unused outside UnderlineNav) - Remove dead export getValidChildren (only used internally) - Migrate all remaining inline style objects to CSS Modules classes: overflowListStyles → .OverflowList dividerStyles → .Divider baseMenuInlineStyles → .OverflowMenu menuItemStyles → .MenuItem MORE_BTN_HEIGHT → removed (container auto-sizes) isWidgetOpen display toggle → .OverflowMenuOpen - Delete styles.ts (all styles now in CSS Modules) - Use Primer design token --shadow-resting-medium for overlay shadow - Remove duplicate icon-hiding CSS from UnderlinePanels.module.css (already handled by shared UnderlineTabbedInterface.module.css) - Fix UnderlineItemList to merge className props via clsx instead of overwriting (was causing flex layout to break when consumer passed className)
hectahertz
force-pushed
the
hectahertz/perf/underlinenav-intersection-observer-overflow
branch
from
b5d0c87 to
33731b8
Compare
github-actions
bot
requested a deployment
to
storybook-preview-7541
IntersectionObserver won't re-fire when all items are already fully visible and the root container just gets wider. This meant icons hidden during overflow would never reappear when resizing the viewport wider. Add a ResizeObserver on the list to detect when it grows past the width at which icons were disabled, triggering the 'trying-with-icons' phase to re-evaluate whether icons fit.
- Remove unused moreMenuRef (assigned but never read) - Normalize React.useCallback/React.useRef to destructured imports - Remove unnecessary setIsWidgetOpen dep from closeOverlay - Rename containerRef → menuListRef for clarity
github-actions
bot
requested a deployment
to
storybook-preview-7541
- Use Primer token var(--base-size-24) instead of raw 24px for divider height - Use padding shorthand instead of four individual padding properties - Simplify flex: 1 1 0% → flex: 1
github-actions
bot
requested a deployment
to
storybook-preview-7541
|
@iansan5653 I know you've been working on a couple of different approaches to improve this, could you please have a look at this proposal? :) |
There was a problem hiding this comment.
Pull request overview
This PR refactors UnderlineNav overflow handling to avoid forced synchronous layout reads by relying on CSS clipping (overflow: hidden) and IntersectionObserver to detect which items are clipped, while also migrating prior inline styles to CSS Modules and consolidating icon-hiding behavior with UnderlinePanels.
Changes:
- Replaced JS width-measurement overflow logic with CSS overflow +
IntersectionObserver(and aResizeObserverassist for icon re-enable). - Migrated
UnderlineNavstyling from inline style objects toUnderlineNav.module.cssusing Primer tokens. - Simplified related types/context and consolidated shared icon-visibility styling in
UnderlineTabbedInterface.module.css.
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/react/src/internal/components/UnderlineTabbedInterface.tsx | Fixes className merging for the list and moves icon rendering to CSS-controlled visibility. |
| packages/react/src/internal/components/UnderlineTabbedInterface.module.css | Adds shared rule to hide icons when wrappers set data-icons-visible='false'; prevents icon shrink. |
| packages/react/src/experimental/UnderlinePanels/UnderlinePanels.module.css | Removes duplicated icon-hiding CSS (now handled by shared module). |
| packages/react/src/UnderlineNav/types.ts | Removes dead types used by the old width-measurement strategy. |
| packages/react/src/UnderlineNav/styles.ts | Deletes inline style constants now replaced by CSS Modules. |
| packages/react/src/UnderlineNav/UnderlineNavItem.tsx | Removes per-item layout measurement; adds overflow aria-hidden + focus management. |
| packages/react/src/UnderlineNav/UnderlineNavItem.module.css | Hides overflowed list items visually while keeping them in the DOM for IO tracking. |
| packages/react/src/UnderlineNav/UnderlineNavContext.tsx | Simplifies context to only loadingCounters. |
| packages/react/src/UnderlineNav/UnderlineNav.tsx | Core rewrite: IO/RO overflow detection, overflow menu duplication, CSS-based icon toggling, CSS Modules usage. |
| packages/react/src/UnderlineNav/UnderlineNav.test.tsx | Updates icon counting assertion to target the list rather than the whole nav. |
| packages/react/src/UnderlineNav/UnderlineNav.module.css | Adds new layout + overflow/menu styling using design tokens; replaces prior inline styles. |
| .changeset/underlinenav-intersection-observer-overflow.md | Patch changeset describing the performance-oriented UnderlineNav update. |
| {menuItems.map(menuItem => { | ||
| const {children: menuItemChildren, counter, onSelect, ...menuItemProps} = menuItem.props | ||
|
|
||
| return ( | ||
| <ActionList.LinkItem | ||
| key={menuItemChildren} |
There was a problem hiding this comment.
key={menuItemChildren} is not guaranteed to be stable/unique (children can be non-string ReactNodes or repeated labels), which can cause React key warnings and incorrect reconciliation in the overflow menu. Prefer using menuItem.key (from the original child) or a stable identifier from props (e.g. id/href) and only fall back to an index if needed.
| {menuItems.map(menuItem => { | |
| const {children: menuItemChildren, counter, onSelect, ...menuItemProps} = menuItem.props | |
| return ( | |
| <ActionList.LinkItem | |
| key={menuItemChildren} | |
| {menuItems.map((menuItem, index) => { | |
| const {children: menuItemChildren, counter, onSelect, ...menuItemProps} = menuItem.props | |
| return ( | |
| <ActionList.LinkItem | |
| key={ | |
| menuItem.key ?? | |
| // eslint-disable-next-line @typescript-eslint/no-unsafe-member-access | |
| (menuItemProps as {id?: string}).id ?? | |
| // eslint-disable-next-line @typescript-eslint/no-unsafe-member-access | |
| (menuItemProps as {href?: string}).href ?? | |
| index | |
| } |
| useEffect(() => { | ||
| const list = listRef.current | ||
| const wrapper = navRef.current | ||
| if (!list || typeof IntersectionObserver === 'undefined') return |
There was a problem hiding this comment.
The new overflow behavior is driven by IntersectionObserver (+ ResizeObserver), but the current UnderlineNav unit tests don't mock either observer, so this code path typically won't run in JSDOM. Consider adding tests that mock IntersectionObserver/ResizeObserver to validate overflow detection, aria-hidden/tabIndex handling, and the aria-current swap behavior.
| if (!list || typeof IntersectionObserver === 'undefined') return | |
| if ( | |
| !list || | |
| typeof IntersectionObserver === 'undefined' || | |
| typeof ResizeObserver === 'undefined' | |
| ) { | |
| return | |
| } |
| counter, | ||
| onSelect, | ||
| 'aria-current': ariaCurrent, | ||
| 'aria-hidden': ariaHidden, | ||
| icon: Icon, |
There was a problem hiding this comment.
UnderlineNavItem now reads an aria-hidden prop, but UnderlineNavItemProps doesn’t declare it (and the type is exported). For better type safety (and to avoid needing any/casts in callers like React.cloneElement), consider extending UnderlineNavItemProps with an aria-hidden?: boolean | 'true' | 'false' (or moving this to an internal-only prop type).
| icon={leadingVisual ?? Icon} | ||
| loadingCounters={loadingCounters} | ||
| iconsVisible={iconsVisible} | ||
| tabIndex={isHidden ? -1 : undefined} | ||
| {...props} | ||
| > |
There was a problem hiding this comment.
tabIndex intended to make overflowed (aria-hidden) items non-focusable is set before {...props}. If a consumer passes tabIndex on UnderlineNav.Item, it will override the -1 value and allow focus into a visually/ARIA-hidden item. Consider spreading props first and then applying tabIndex, or explicitly omitting tabIndex from props when isHidden is true.
| // ResizeObserver to detect when the list grows while icons are hidden. | ||
| // IO won't re-fire when all items are already fully visible and the root | ||
| // just gets wider, so we need RO to trigger icon retry in that scenario. | ||
| const resizeObserver = new ResizeObserver(() => { | ||
| if (!iconsVisibleRef.current && iconPhaseRef.current === 'normal') { |
There was a problem hiding this comment.
ResizeObserver is instantiated unconditionally. In environments where IntersectionObserver exists but ResizeObserver does not, this will throw at runtime and break UnderlineNav. Previously this component used useResizeObserver, which guards typeof ResizeObserver === 'function' and falls back to window.resize. Consider adding the same guard here or reusing useResizeObserver for the width-growth retry logic.
1 participant
Summary
UnderlineNav currently uses synchronous DOM measurement APIs (
getBoundingClientRect,getComputedStyle) insideuseLayoutEffectandResizeObservercallbacks to determine which items overflow. This causes forced reflows — the browser must synchronously compute layout before JavaScript can continue, blocking paint and creating jank during resize. In dotcom, this manifests as a 298ms forced reflow on pages using UnderlineNav.This PR replaces that entire measurement strategy with CSS
overflow: hidden+IntersectionObserver:<ul>getsoverflow: hiddenso the browser natively clips items that don't fitIntersectionObserver(with the list asroot) asynchronously reports which items are clippedaria-hidden+visibility: hiddenand are duplicated in the overflow menuWhat's removed
useLayoutEffectinUnderlineNavItem(per-item DOM measurement)ResizeObserver+overflowEffectwidth calculation loopgetAnchoredPositionimport from@primer/behaviorschildWidthArray/noIconChildWidthArraystate + contextcalculatePossibleItems/swapMenuItemWithListItemhelpersuseMemowith index-based logicstyles.tsfile (all inline style objects)MORE_BTN_WIDTH/getValidChildrenexportsUnderlinePanels.module.cssUnderlineTabbedInterface.module.cssWhat's added
IntersectionObserverin a singleuseEffecton the parentResizeObserveralongside IO to detect when the list grows wider (re-enables icons after viewport returns to wide)overflow: hiddenon the item list with padding to preserve underline indicatorswidthWhenIconsDisabledRef) to avoid infinite show/hide cyclesvisibility: hiddenon[aria-hidden='true']items to prevent partially-clipped textWhat's changed (non-functional)
UnderlineNav.module.css)--base-size-*,--borderRadius-medium,--shadow-resting-medium, etc.)stylelint-disablecomments neededclassNamemerging fixed inUnderlineItemList— consumer-providedclassNameis now merged viaclsxinstead of being overwritten by{...rest}spreadReact.useCallback→ destructureduseCallback)Performance measurements
All measurements taken on the overflow story (9 nav items), no CPU/network throttling, using Chrome DevTools Protocol with
initScriptinstrumentation (counters injected before any page scripts execute and stack-trace filtered to UnderlineNav).Forced reflow API calls — initial render
getBoundingClientRectgetComputedStyleForced reflow API calls — resize cycle (1200px → 600px → 1200px)
getBoundingClientRectgetComputedStyleCore Web Vitals (Performance trace)
Architecture comparison
useLayoutEffect(synchronous)useEffect(async)overflow: hidden+ IntersectionObserverstyles.ts)Changed files
UnderlineNav.tsxUnderlineNav.module.cssUnderlineNavItem.tsxaria-hiddenhandling for overflow itemsUnderlineNavItem.module.cssvisibility: hiddenforaria-hiddenitemsUnderlineNavContext.tsxloadingCountersstyles.tstypes.tsChildSize/ChildWidthArraytypesUnderlineTabbedInterface.tsxclassNamemerging fix, icon-hiding CSS ruleUnderlineTabbedInterface.module.cssUnderlinePanels.module.cssUnderlineNav.test.tsxMORE_BTN_WIDTHHow did you test this change?
npm test— 19 UnderlineNav + 11 UnderlinePanels)aria-currentitem swaps to stay visible when it would overflowinitScriptinjection + stack-trace filtering, Performance traces for CWV metrics, viewport resize tests)