Skip to content

WIP: Fix UnderlineNav CLS and improve performance/code quality by rebuilding the whole overflow logic to use mostly CSS#7506

Draft
iansan5653 wants to merge 11 commits intotylerjdev/overflow-underlinenavfrom
underline-nav-full-css-spike
Draft

WIP: Fix UnderlineNav CLS and improve performance/code quality by rebuilding the whole overflow logic to use mostly CSS#7506
iansan5653 wants to merge 11 commits intotylerjdev/overflow-underlinenavfrom
underline-nav-full-css-spike

Conversation

@iansan5653
Copy link
Contributor

@iansan5653 iansan5653 commented Feb 6, 2026

This proof of concept goes a step beyond my previous spike (#7505) by ripping out all of the complex layout calculation logic and replacing it with CSS. See https://github.com/github/primer/issues/6291#issuecomment-3861950186 for context.

It works like this:

  1. As with the other PR, menu items wrap onto subsequent lines, which are clipped by their container using overflow: hidden, creating the illusion that they disappeared
  2. Also as with the other PR, the initial visibility of the overflow menu button and tab icons is controlled by scroll-state container queries to detect overflow using pure CSS
  3. Pure CSS cannot, however, tell us what to render in the overflow menu. For this, each list item registers itself using context:
    1. The parent component provides the current containerWidth (tracked via ResizeObserver) and register/unregister functions through context
    2. Each item component runs an effect when containerWidth or props change. In this effect, they determine whether they are overflowed by checking ref.current.offsetHeight (which will be greater than zero for an item that has wrapped). If they are overflowed, they set aria-hidden/tabIndex and notify the parent via the registry.
    3. The parent renders all currently-overflowing items into the overflow menu
    4. If any item has ever overflowed, all tab icons are suppressed via React state for the rest of the lifecycle of the component (this prevents flickering caused by the tab icons creating/removing space)

The benefits are pretty significant:

  1. The browser will calculate overflow for us before we ever even paint on the page - no waiting on a JS bundle or React hydration (this benefit is shared with the lighter-weight approach)
  2. Calculations remain pure CSS, so performance is buttery smooth for the whole component lifecycle
  3. We get to clean up tons of complex code, remove several effects, and remove hacky Children APIs entirely
  4. Bonus: Component consumption is much less fragile. Without Children APIs, we no longer need direct children, so consumers can wrap their items in fragments and even subcomponents

However, there are caveats:

  1. The container must keep overflow: hidden for the component lifecycle, so tabs cannot display popovers and tooltips without using portals. In my other PR, we are able to remove overflow: hidden after the initial calcs. However, this is relatively easy to work around: use the native CSS popover for tooltips (as we already are in Tooltip) and use Portal/Overlay for popovers rather than rendering DOM into the menu (which is an accessibility violation anyway)
  2. The menu positioning is not working right. Maybe because of the overflow: hidden? I'm not sure but it seems fixable. Maybe we can move to AnchoredOverlay here
  3. Wrapped items still take up horizontal space, so if you have a really long wrapped item it can prevent other items from wrapping. I think maybe we can solve this with some flex-shrink on the items? Found a CSS-based solution for this
  4. You can never get down to just the overflow menu, because the last tab can never wrap. We probably need a special case here to hide and overflow the first item if the containerWidth is smaller than that item. Although this seems like a rare edge case to me and I wonder if only showing "More" ever actually makes sense or would it just be confusing? Maybe the last tab should just get an ellipsis truncation instead of moving to the menu?
  5. CI is failing because the NextJS build for React 18 doesn't support the CSS scroll-state function. The React 19 build does. Do we still need this build, given that dotcom is fully migrated to React 19 now? Not sure how else we could work around this 😕

Closes #

Changelog

New

Changed

Removed

Rollout strategy

  • Patch release
  • Minor release
  • Major release; if selected, include a written rollout or migration plan
  • None; if selected, include a brief description as to why

Testing & Reviewing

Merge checklist

@changeset-bot
Copy link

changeset-bot bot commented Feb 6, 2026

⚠️ No Changeset found

Latest commit: b65d397

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@github-actions github-actions bot added the integration-tests: recommended This change needs to be tested for breaking changes. See https://arc.net/l/quote/tdmpakpm label Feb 6, 2026
@github-actions
Copy link
Contributor

github-actions bot commented Feb 6, 2026

👋 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 integration-tests: skipped manually label to skip these checks.

@iansan5653 iansan5653 changed the base branch from main to tylerjdev/overflow-underlinenav February 6, 2026 20:28
@github-actions github-actions bot requested a deployment to storybook-preview-7506 February 6, 2026 20:31 Abandoned
@github-actions github-actions bot temporarily deployed to storybook-preview-7506 February 6, 2026 20:40 Inactive
@iansan5653
Copy link
Contributor Author

BTW, I think the flex inspector in Chrome's devtools does a really nice job of showing what's going on here / where the tabs are going:

Screen.Recording.2026-02-06.at.5.00.33.PM.mov

@iansan5653
Copy link
Contributor Author

iansan5653 commented Feb 9, 2026

For the last item in the menu, I could come up with a way to move it to the overflow menu via JS. But I'm wondering if we really should? That's the current behavior, but does it ever make sense to just have a "more" menu standing alone? Perhaps we could consider truncating the item instead? I know that has accessibility concerns but it seems like the lesser of the two evils.

screenshot showing truncated first tab item

I also don't want to over-engineer this edge case as it seems practically impossible to actually reach in the real world - you'd need an extra long first item title and a very narrow container size at the same time.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

integration-tests: recommended This change needs to be tested for breaking changes. See https://arc.net/l/quote/tdmpakpm

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant