UnderlineNav: Fix flickering by moving icon visibility control to pure CSS#8108
UnderlineNav: Fix flickering by moving icon visibility control to pure CSS#8108iansan5653 wants to merge 15 commits into
UnderlineNav: Fix flickering by moving icon visibility control to pure CSS#8108Conversation
🦋 Changeset detectedLatest commit: b3e85f5 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 |
|
There was a problem hiding this comment.
Pull request overview
This PR updates UnderlineNav to avoid SSR→hydration icon flicker by removing the JS “has ever overflowed” state and attempting to move icon-hiding logic entirely into CSS via scroll-driven animations.
Changes:
- Removed
hasEverOverflowedstate and thedata-hide-iconsattribute fromUnderlineNav.tsx. - Added a new
hide-iconsanimation inUnderlineNav.module.css, intended to run once and then keep icons hidden permanently by controlling animation play state via the existing scroll-driven overflow detection. - Minor string literal change for the overflow menu button
aria-label.
Show a summary per file
| File | Description |
|---|---|
| packages/react/src/UnderlineNav/UnderlineNav.tsx | Removes JS-based icon hiding (hasEverOverflowed / data-hide-icons) to rely on CSS-only behavior. |
| packages/react/src/UnderlineNav/UnderlineNav.module.css | Adds a one-shot animation approach intended to hide icons without JS and prevent overflow-driven flicker loops. |
Review details
- Files reviewed: 3/3 changed files
- Comments generated: 2
- Review effort level: Low
| @keyframes hide-icons { | ||
| 0% { | ||
| display: inline; | ||
| } | ||
|
|
||
| 100% { | ||
| display: none; | ||
| } | ||
| } |
There was a problem hiding this comment.
I'm like 99% sure this is incorrect. Browsers will animate the property but (obviously) not smoothly; they'll discretely step from one value to the next.
There was a problem hiding this comment.
Checking https://primer-770a2beee9-13348165.drafts.github.io/storybook/?path=/story/components-underlinenav-features--with-icons I don't think the icons are hiding
There was a problem hiding this comment.
This turned out to be due to not setting an initial value for the variable. Which feels like a browser bug, but it's easy to solve by initializing it.
|
366cedc to
b79ae2e
Compare
| overflow: hidden; | ||
| flex: 1; |
There was a problem hiding this comment.
overflow: hidden had no effect here because there's no explicit height set on this container. And it's unnecessary since the parent already has that applied.
flex: 1 fixes a separate bug where sometimes the button wouldn't float all the way to the right. It makes the tab containers take up more space to push the overflow button further out. This prevents it from looking like it's floating in space.
| animation: | ||
| detect-overflow linear, | ||
| delay-visibility forwards 0ms 5ms; |
There was a problem hiding this comment.
When testing on Chrome with 20x CPU slowdown, I could see an initial paint where the icons are visible, followed by a quick update to hide them. I tried several things to resolve this, including conditionally setting the animation duration to 0 seconds and also conditionally changing the start state of the animation, but they all caused unexpected side effects.
I actually think this initial flicker may be entirely unavoidable, because the browser has to paint the tabs to determine if overflow happens in order to determine whether to hide the icons or not. So I just accepted it and decided to hide the tabs with a very short animation, which delays their initial render by an almost imperceptible 5 ms. This is just long enough to get that initial paint out of the way.
| animation-fill-mode: forwards; | ||
| animation-timing-function: linear; | ||
| animation-play-state: var(--UnderlineNav_hide-icons-play-state); | ||
| animation-duration: 0.1ms; /* must be greater than 0 */ |
There was a problem hiding this comment.
Interestingly, this can't be arbitrarily small; 0.0001ms is treated as 0 in Chrome. 0.1ms seems to work very consistently though.
I did try making the duration dynamic, instead of the play state, and setting the duration to 0 to immediately resolve the animation when I want to toggle it. That didn't work though; the style didn't 'stick' if the duration was increased again.
| @keyframes hide-icons { | ||
| 0% { | ||
| display: inline; | ||
| } | ||
|
|
||
| 100% { | ||
| display: none; | ||
| } | ||
| } |
There was a problem hiding this comment.
This turned out to be due to not setting an initial value for the variable. Which feels like a browser bug, but it's easy to solve by initializing it.
|
The icons in
UnderlineNavcan cause flickering as they cause/un-cause the container to overflow when hidden/shown. We use JS to update state to prevent this flickering, but that doesn't run until after hydration. This means that, for certain screen sizes, the icons will flicker after SSR and before client-size hydration.To fix this, I'm moving the visibility control to pure CSS. However, to make this work we must use CSS as a state machine; otherwise we'll just get flickering for the entire page lifecycle. So I've introduced a second animation,
hide-icons, which only plays once and remains applied when paused. Then I updated the scroll-driven animation to control thehide-iconsplay state, rather than control the visibility directly. The result is that once the animation plays once, it will remain in that state forever - no need for any JS at all.Changelog
New
Changed
UnderlineNavRemoved
Rollout strategy
Testing & Reviewing
Merge checklist