perf(UnderlineNav): replace JS DOM measurement with CSS overflow + IntersectionObserver#7541
Open
hectahertz wants to merge 23 commits intomainfrom
Open
perf(UnderlineNav): replace JS DOM measurement with CSS overflow + IntersectionObserver#7541hectahertz wants to merge 23 commits intomainfrom
hectahertz wants to merge 23 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: c02d93c 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 |
Contributor
|
👋 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
github-actions
bot
requested a deployment
to
storybook-preview-7541
This was referenced Feb 15, 2026
hectahertz
added
the
update snapshots
francinelucca
added
update snapshots
|
|
||
| return ( | ||
| <li className={classes.UnderlineNavItem}> | ||
| <li className={classes.UnderlineNavItem} aria-hidden={isHidden || undefined}> |
Contributor
Author
There was a problem hiding this comment.
The aria-hidden prop can be either a boolean (true) or a string ("true"), since React and HTML handle them differently. This normalizes both into a single boolean so we can use it to:
- Set
aria-hiddenon the<li>to hide overflow items from the accessibility tree - Set
tabIndex={-1}on the inner element so overflowed items can't be focused with keyboard
Previously overflow was handled by removing/adding DOM nodes. Now overflowed items stay in the DOM but are hidden from assistive tech and non-focusable.
Comment on lines
+82
to
+85
| // Destructure iconsVisible to prevent it from spreading onto the DOM via ...rest. | ||
| // Icon visibility is now controlled via CSS (data-icons-visible on wrapper). | ||
| // eslint-disable-next-line @typescript-eslint/no-unused-vars | ||
| iconsVisible: _iconsVisible, |
Member
There was a problem hiding this comment.
does that mean this prop should be deprecated?
Contributor
Author
There was a problem hiding this comment.
No, iconsVisible is still actively used by UnderlinePanels (which also consumes UnderlineItem), so it needs to stay in the shared type.
For UnderlineNav specifically, icon visibility is now controlled via CSS using a data-icons-visible attribute on the wrapper, so UnderlineNav no longer passes it. But it can't be removed from the shared component because UnderlinePanels still relies on it.
The destructure here just prevents it from spreading onto the DOM as an invalid HTML attribute via ...rest.
github-actions
bot
removed
the
update snapshots
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #
UnderlineNav uses synchronous DOM measurement APIs (
getBoundingClientRect,getComputedStyle) insideuseLayoutEffectandResizeObservercallbacks to determine which items overflow. This causes forced reflows, 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+ CSS Anchor Positioning, eliminating all synchronous layout-forcing calls.How it works
<ul>hasoverflow: hidden. Items that don't fit are clipped.IntersectionObserver(root =<ul>) detects which items are clipped.rootMargin: '0px -80px 0px 0px'reserves space for the "More" button.overflowStartIndexis set via React state. Items from that index onward getaria-hidden+tabIndex={-1}and are duplicated in the overflow dropdown.Icon toggle state machine
When overflow is first detected, icons are hidden to try to fit more items. If that's not enough, the overflow menu appears. When the viewport grows, icons are retried.
To prevent flicker during the icon retry, overflow is NOT cleared until IO confirms the final state after icons are toggled.
Performance measurements
All measurements on the overflow story (9 nav items), no CPU/network throttling, Chrome DevTools Protocol with
initScriptinstrumentation.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)Accessibility audit
aria-labelon<nav><h2>heading for screen readersaria-current="page"on selected linkaria-hidden="true"on<li>tabIndex={-1}on linksaria-expandedtoggles correctlyaria-controlspoints to overflow menuidaria-hidden="true"+role="presentation"Changelog
New
N/A
Changed
overflow: hidden+IntersectionObserver, eliminating all forced reflowsRemoved
N/A
Rollout strategy
Testing & Reviewing
Merge checklist