🐛 [BUG] - SubNav focus trap issue

[stamat]

Describe the bug

On desktop size, when clicking on an aria-current="page" navigation item all the nav items shift (see video) and the focus trap is engaged for the nav items. This happens even if the current item doesn't have a submenu.

The issue can be replicated in Docs and Storybook as well. https://primer.style/brand/components/SubNav Clicking on an active nav item will shift items to the right, clicking again will shift them back to the starting position. Try using Tab after you first click on an active item, the focus trap is engaged and the focus cannot leave the nav items.

The reason this happens

When an active item is clicked two empty focusable spans with a class .sentinel are added which are not absolutely positioned, and they interfere with the margins of the items (see screenshot).

This behavior is due @primer/behaviors/src/focus-trap.ts https://github.com/primer/behaviors/blob/19997006736fd6ca0aee6f6478ea27d7dd2a3e64/src/focus-trap.ts#L82-L95

It is implemented in useFocusTrap that is used by SubNav here

brand/packages/react/src/hooks/useFocusTrap.ts

Line 63 in 1916153

abortController.current = focusTrap(containerRef.current, initialFocusRef?.current ?? undefined)

Focusable span.sentinel elements are added to the ul element as first and the last child. (This is not semantically correct btw, since the only children ul should accept are li) On focus of these elements focus is immediately shifted to the first or last nav item. This is the most performant focus trap tactic.

I wonder why these span elements are not absolutely positioned with an inline style when created in @primer/behaviors - this would solve the shifting and won't interfere with tab order.

But then again the focus trap shouldn't be activated if there is no submenu.

Solution

Since we are using React I think we have flexibility here to easily implement our own custom focus trap system for this case. Or continue to use the existing system but restructure the component in a way that the mobile menu has a unique trigger button visible only on mobile and within the container of the nav. This will increase the DOM depth by one level but it's still not a major issue since this component is not too deep. This way we can solve some of the A11y issues outlined here:

• [a11y] SubNav accessibility issues #762

Screenshots

368259118-bda2dc47-8b66-4a74-923c-c334544531c9.mp4