Skip to content
Open
5 changes: 5 additions & 0 deletions .changeset/navlist-parent-flicker-fix.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"@primer/react": patch
---

Fix NavList parent item flicker during static-to-interactive transitions when navigating between current sub-items in a SubNav.
18 changes: 18 additions & 0 deletions packages/react/src/NavList/NavList.test.tsx
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import {describe, it, expect, vi} from 'vitest'
import {render, fireEvent, act} from '@testing-library/react'
import React from 'react'
import {renderToStaticMarkup} from 'react-dom/server'
import {NavList} from './NavList'
import {ReactRouterLikeLink} from '../Pagination/mocks/ReactRouterLink'
import {implementsClassName} from '../utils/testing'
Expand Down Expand Up @@ -148,6 +149,23 @@ describe('NavList.Item with NavList.SubNav', () => {
expect(queryByRole('list', {name: 'Item 2'})).toBeNull()
})

it('renders parent item expanded on initial static render when SubNav contains the current item', () => {
// intentionally suppress the expected React SSR useLayoutEffect warning
// this test focuses specifically on the initial SSR render
const container = document.createElement('div')
const consoleSpy = vi.spyOn(console, 'error').mockImplementation(() => null)
try {
container.innerHTML = renderToStaticMarkup(<NavListWithCurrentSubNav />)
} finally {
consoleSpy.mockRestore()
}

const item2Button = container.querySelector('button[aria-expanded]')
expect(item2Button).not.toBeNull()
expect(item2Button).toHaveAttribute('aria-expanded', 'true')
expect(item2Button?.textContent).toBe('Item 2')
})

it('hides SubNav by default if SubNav does not contain the current item', () => {
const {queryByRole} = render(<NavListWithSubNav />)
const subNav = queryByRole('list', {name: 'Item 2'})
Expand Down
48 changes: 36 additions & 12 deletions packages/react/src/NavList/NavList.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -123,23 +123,47 @@ const ItemWithSubNavContext = React.createContext<{buttonId: string; subNavId: s
isOpen: false,
})

function hasCurrentNavItem(node: React.ReactNode): boolean {
if (
!isValidElement<{
children?: React.ReactNode
'aria-current'?: NavListItemProps['aria-current']
}>(node)
) {
return false
}

const ariaCurrent = node.props['aria-current']
if (Boolean(ariaCurrent) && ariaCurrent !== 'false') {
return true
}

if (!node.props.children) {
return false
}

return React.Children.toArray(node.props.children).some(hasCurrentNavItem)
}

function ItemWithSubNav({children, subNav, depth: _depth, defaultOpen, style}: ItemWithSubNavProps) {
const buttonId = useId()
const subNavId = useId()
const [isOpen, setIsOpen] = React.useState((defaultOpen || null) ?? false)
const subNavRef = React.useRef<HTMLDivElement>(null)
const [containsCurrentItem, setContainsCurrentItem] = React.useState(false)

// We have to use recursion to check if the current nav item is part of the subnav before initial render
// which is why we can't use the querySelector on the ref as it will cause the parent item to blink during first render.
const hasCurrentItem = React.useMemo(() => hasCurrentNavItem(subNav), [subNav])
const [isOpen, setIsOpen] = React.useState((defaultOpen || null) ?? hasCurrentItem)
const subNavRef = React.useRef<HTMLUListElement>(null)
const [containsCurrentItem, setContainsCurrentItem] = React.useState(hasCurrentItem)

useIsomorphicLayoutEffect(() => {
if (subNavRef.current) {
// Check if SubNav contains current item
// valid values: page, step, location, date, time, true and false
const currentItem = subNavRef.current.querySelector('[aria-current]:not([aria-current=false])')

if (currentItem) {
setContainsCurrentItem(true)
setIsOpen(true)
}
// Check if SubNav contains current item
// valid values: page, step, location, date, time, true and false
const currentItem = hasCurrentNavItem(subNav)
setContainsCurrentItem(Boolean(currentItem))

if (currentItem) {
setIsOpen(true)
}
}, [subNav, buttonId])

Expand Down