UnderlineNav v2 item can enter a selected non-current state

[pveierland]

Description

During use of the UnderlineNav v2 component, it appears that the UnderlineNav.Item can end up showing as being selected, while its prop aria-current is set to undefined or false.

This may be caused by a race condition, as it seems to be triggered when moving between pages quickly / when rendering has not been completed.

Example of "Meta" not being selected and "Inputs" being selected:

Props showing that "Meta" item has aria-current set to page and should be selected:

Props showing that "Inputs" item has aria-current set to false and should not be selected:

Testing performed using React Primer v35.19.0 and Next.js v13.1.6 with wrapper component as detailed in: https://primer.style/react/drafts/UnderlineNav2#with-nextjs

Steps to reproduce

1. Create page using https://primer.style/react/drafts/UnderlineNav2#with-nextjs template, with demanding components on each page being navigated between.
2. Navigate between pages using a separate navigation mechanism (not by clicking UnderlineNav.Item).
3. Sometimes the UnderlineNav.Item will end up in the state where it is shown visually as being selected, while having aria-current set to undefined or false.

Expected behavior would be for the aria-current and visual state of the UnderlineNav.Item to always be in sync.

Version

v35.19.0

Browser

Chrome, Firefox

[lesliecdubs]

@broccolinisoup do you want to take a look at this one?

[broccolinisoup]

I'm on it!

[pveierland]

Just noting a possibly related issue with the same component. Sometimes, not all items will be shown during a render (e.g. the last element might be missing), however when resizing the page the last element(s) will also be rendered. To be specific, it is not that the element is being shown in an extensible menu, it is that sometimes the last elements are not rendered at all.

[broccolinisoup]

@pveierland That is strange. "Sometimes" issues are tricky! - Thanks so much for letting us know. I plan to look into this issue next week and hopefully resolve 🤞🏻

[pveierland]

Just adding a screenshot of how it appeared now. In this case the final item "Settings" was not added at all, while "Resources" and "Versions" are shown in an extension menu, even though there is available layout space for all items without using the dropdown.

Resizing the page makes all items render:

[broccolinisoup]

@pveierland Thanks! I'll certainly look into why some items is missing in the first render but I should also add that in the first render, even though there is enough space on the viewport, the last two item being in the overflow menu is expected (in SSR). Because the placement of the elements is entirely dependent on the viewport width and when the HTML is rendered in the server, it doesn't know the viewport size so it renders as you just described. Let me know if I misunderstand anything about the behaviour you described! I feel like an item is being missing might be to do with the server render as well 🤔

[pveierland]

@broccolinisoup That does explain the behavior of the first render. The "Settings" entry also requires a flag which is not set during the server render, and will only render when this flag is set on the client-side. However, wouldn't the component be re-rendered automatically after hydration and when these props have changed?

[broccolinisoup]

Hey @pveierland 👋🏻 Sorry for the late response!

The "Settings" entry also requires a flag which is not set during the server render, and will only render when this flag is set on the client-side.

Thanks for letting me know about that!

However, wouldn't the component be re-rendered automatically after hydration and when these props have changed?

The component is re-rendered when the props are changed as well as when the viewport size is changed. However, regarding re-rendering automatically after hydration, I would say it is a option that needs to be chosen carefully as it will most likely cause a layout shift.

To my understanding and experience, the behaviour of a component both on the client and the server should be intentional. They can be different and that is completely fine as long as it is intentional by design. This is the case for UnderlineNav. The component relies on viewport size for accurate rendering on the client and for server, the last two item being hiding in the overflow menu in the first load is fine and we don't consider this as a bug.

Let us know if you have further questions 🙌🏻

[broccolinisoup]

@pveierland Regarding the original issue, thank you so much for reporting it again. I have found a few issues related to keeping the selected item visible in SSR and I believe the original issue you mentioned is something to do with it. I'll post my findings here in the next couple of days hopefully and come up with more SSR compatible solution.

[peterbe]

Hi, this is blocking us from moving to UnderlineNav2.

My use case is very simple. Basically

      <UnderlineNav>
       {options.map((option) => {
          return (
            <UnderlineNav.Item
              key={href}
              aria-current={option.value === currentValue ? 'page' : false}
             >{option.value}</UnderlineNav.Item>}
        )}
       </UnderlineNav>

Nothing fancy.
To be certain it had nothing to with onClick things my code was doing I added these debugging buttons:

      <hr />
      Change current value:
      <button onClick={() => setCurrentValue('linux')}>linux</button>
      <button onClick={() => setCurrentValue('mac')}>mac</button>
      <button onClick={() => setCurrentValue('windows')}>windows</button>

The confusing thing is that the DOM updates. But the styling, the little orange underlining, does not update.

Screen.Recording.2023-05-17.at.10.30.32.AM.mov

However, and sorry if it's hard to see in the video, but the bug is that the class name a.jyQmWO doesn't change. As the aria-current="page" is set, it needs to update the className too.

[peterbe]

Ah! I kinda get it. At least a small clue.
My mistake was that I had a...

    <UnderlineNav.Item
      key={href}
      aria-current={option.value === currentValue ? 'page' : undefined}
      href={href}
      onClick={(event) => {
        onClickChoice(option.value)
      }}

If I remove that onClick, now it kinda works! Perhaps I need to look into this onSelect or something.

However, I still can't control it from the outside.
See here what happens when I change the parent component's state so that the aria-current value does actually change:

Screen.Recording.2023-05-17.at.10.41.34.AM.mov

In my real final version I'm not going to have those buttons below that call setCurrent('linux') for example, but I might because of in the onload useEffect on the client side, I might do something like this:

useEffect(() => {
  // Focus the 'linux' button if the location hash says so
  if (window.location.hash.includes('linux') {
    setCurrentValue('linux')
  }
}, [])

Perhaps I can solve my use-case with

useEffect(() => {
  // Focus the 'linux' button if the location hash says so
  if (window.location.hash.includes('linux') {
    document.querySelector('[data-nav="linux"]').click()
  }
}, [])

but that feels hacky :)

[peterbe]

but that feels hacky :)

and obviously wouldn't work because a DOM click will trigger the underlying onSelect handler.

[broccolinisoup]

Hi @peterbe 👋🏻 Thanks for your comment, I can confirm now that indeed there is a bug here :)
The orange underline is managed internally by a state called selectedLink and what I realised is that in some cases (like described above), aria-current and the selectedLink gets out of sync. I half way solved the issue but wanted to look into a little deeper. I prioritised this issue to unblock you and the original reporter of the issue (apologise for @pveierland for a late response), hopefully I'll be pushing a fix for this in the next few days.

Thank you for taking the time to post reproducible info and screen recordings. I'll be coming here and update the issue once I resolve the problem. Thanks 🙏🏻

[broccolinisoup]

Hi 👋🏻 Thanks for your patience with me 🙏🏻 I think I got some good progress. I tested the scenarios on a sample SPA and seems like it is working. I still need to work through some styling and some bits and pieces - because I went ahead and did some refactoring - however I believe it is at a good stage to test out.

I pushed a PR with my changes and every PR publishes a canary build #3361 - would that be possible to test out the @primer/react@0.0.0-20230605081804 version and let me know if you have any issues/feedback/concern etc? I thought it would be good to check in before going too far. Let me know what you think! Looking forward to hearing from you 🙌🏻

cc @peterbe @pveierland

[peterbe]

Hi 👋🏻 Thanks for your patience with me 🙏🏻 I think I got some good progress. I tested the scenarios on a sample SPA and seems like it is working. I still need to work through some styling and some bits and pieces - because I went ahead and did some refactoring - however I believe it is at a good stage to test out.

I pushed a PR with my changes and every PR publishes a canary build #3361 - would that be possible to test out the @primer/react@0.0.0-20230605081804 version and let me know if you have any issues/feedback/concern etc? I thought it would be good to check in before going too far. Let me know what you think! Looking forward to hearing from you 🙌🏻

cc @peterbe @pveierland

I have not had a chance to fully try it yet.
I got half-way. Distracted when working to get back into it. Will try again next week.

[peterbe]

@broccolinisoup Good news! Your canary build solves the problem.
You can test on https://docs-internal-37792-34c9cc.preview.ghdocs.com/en/authentication/connecting-to-github-with-ssh/adding-a-new-ssh-key-to-your-github-account
Works with mouse and with keyboard.

[broccolinisoup]

@peterbe Great news! Thanks so much for testing and giving me feedback. I will work on the final details and getting the PR ready for review, hopefully we will release it soon!

[echarles]

Hi, I struggled with the exact same issue while trying to control the selected tab from a global state. I have tried @primer/react@0.0.0-20230605081804 which indeed fixed the selected tab, but the navbar renders in stretched mode, and I am not able to make it take the full horizontal space.

[github-actions]

Uh oh! @echarles, the image you shared is missing helpful alt text. Check #2865 (comment).

Alt text is an invisible description that helps screen readers describe images to blind or low-vision users. If you are using markdown to display images, add your alt text inside the brackets of the markdown image.

Learn more about alt text at Basic writing and formatting syntax: images on GitHub Docs.

[broccolinisoup]

hey @echarles I am sorry I missed this comment! I was aware of this overflowing styling issue. I finally managed to clean up my PR and open for review and it should fix that styling too.

@peterbe Sorry this took me so long! Hopefully I will manage to get it merged soon 🤞🏻