Preact menu tab focus fix

src/App/components/MapButton/MapButton.jsx

@@ -1,5 +1,5 @@
 // components/MapButton.jsx
-import React, { useState, useRef } from 'react'
+import React, { useState, useRef, useCallback } from 'react'
 import { stringToKebab } from '../../../utils/stringToKebab'
 import { Tooltip } from '../Tooltip/Tooltip'
 import { Icon } from '../Icon/Icon'
@@ -116,7 +116,7 @@ const getControlledElement = ({ idPrefix, panelId, buttonId, hasMenu }) => {
  * @param {string} options.className - CSS classes for the button
  * @param {Function} options.onClick - Click event handler
  * @param {Function} options.onKeyUp - Key up event handler
- * @param {Object} options.buttonRefs - React ref object for storing button references
+ * @param {Function} options.setButtonRef - Stable ref callback storing the button DOM node in buttonRefs
  * @param {boolean} options.isDisabled - Whether the button is disabled
  * @param {boolean} options.isPressed - Whether the button is in pressed state
  * @param {boolean} options.isExpanded - Whether content controlled by button is expanded
@@ -133,7 +133,7 @@ const buildButtonProps = ({
   onClick,
   onKeyUp,
   onKeyDown,
-  buttonRefs,
+  setButtonRef,
   isDisabled,
   isPressed,
   isExpanded,
@@ -159,16 +159,12 @@ const buildButtonProps = ({
     onClick,
     onKeyUp,
     onKeyDown,
-    ref: (el) => {
-      if (buttonRefs.current && buttonId) {
-        buttonRefs.current[buttonId] = el
-      }
-    },
+    ref: setButtonRef,
     'aria-disabled': isDisabled || undefined,
     'aria-expanded': ariaExpanded,
     'aria-pressed': typeof isPressed === 'boolean' ? isPressed : undefined,
     'aria-controls': controlledElement?.id,
-    'aria-haspopup': controlledElement?.type === 'popup' || undefined,
+    'aria-haspopup': controlledElement?.type === 'popup' ? 'menu' : undefined,
     ...(href
       ? { href, target: '_blank', onKeyUp: handleKeyUp, role: 'button' }
       : { type: 'button' })
@@ -224,6 +220,14 @@ export const MapButton = ({
   const [menuRect, setMenuRect] = useState(null)
   const menuRef = useRef(null)
 
+  // Stable across renders so Preact doesn't detach/reattach it (and transiently
+  // null out buttonRefs.current[buttonId]) on every unrelated re-render of this button.
+  const setButtonRef = useCallback((el) => {
+    if (buttonRefs.current && buttonId) {
+      buttonRefs.current[buttonId] = el
+    }
+  }, [buttonRefs, buttonId])
+
   const Element = href ? 'a' : 'button'
   const hasMenu = menuItems?.length >= 1
   const showIcon = iconId || iconSvgContent || hasMenu
@@ -264,7 +268,7 @@ export const MapButton = ({
     onClick: handleButtonClick,
     onKeyUp: handleButtonKeyUp,
     onKeyDown: handleButtonKeyDown,
-    buttonRefs,
+    setButtonRef,
     isDisabled,
     isPressed,
     isExpanded,

src/App/components/MapButton/MapButton.module.scss

@@ -172,7 +172,7 @@
 }
 
 // 4. State styles
-.im-c-map-button[aria-haspopup="true"][aria-expanded="true"] svg {
+.im-c-map-button[aria-haspopup="menu"][aria-expanded="true"] svg {
     transform: rotate(180deg);
 }
 

src/App/components/MapButton/MapButton.test.jsx

@@ -125,7 +125,7 @@ describe('MapButton', () => {
     renderButton({ idPrefix: 'prefix', menuItems: [{ label: 'Item' }] })
     const button = getButton()
     expect(button).toHaveAttribute('aria-controls', 'prefix-popup-test')
-    expect(button).toHaveAttribute('aria-haspopup', 'true')
+    expect(button).toHaveAttribute('aria-haspopup', 'menu')
   })
 
   it('toggles popup aria-expanded and startPos', () => {

src/App/components/PopupMenu/PopupMenu.test.jsx

@@ -145,6 +145,75 @@ describe('PopupMenu', () => {
     expect(mockSetIsOpen).toHaveBeenCalled()
   })
 
+  describe('Tab key focus handoff', () => {
+    let before, instigatorEl, after1, after2
+
+    beforeEach(() => {
+      before = document.createElement('button')
+      instigatorEl = document.createElement('button')
+      after1 = document.createElement('button')
+      after2 = document.createElement('button')
+      document.body.append(before, instigatorEl, after1, after2)
+      // jsdom never computes layout, so offsetParent is always null — findTabStop
+      // treats that as "hidden" and skips it, so it must be stubbed for these to look focusable.
+      ;[before, instigatorEl, after1, after2].forEach(el => Object.defineProperty(el, 'offsetParent', { value: true, configurable: true }))
+      mockUseApp.buttonRefs.current.instigator = instigatorEl
+    })
+
+    afterEach(() => {
+      [before, instigatorEl, after1, after2].forEach(el => el.remove())
+    })
+
+    it('moves focus to the tab stop after the instigator, not the first tab stop on the page', () => {
+      renderMenu()
+      fireEvent.keyDown(screen.getByRole('menu'), { key: 'Tab' })
+      expect(document.activeElement).toBe(after1)
+    })
+
+    it('Shift+Tab moves focus to the tab stop before the instigator', () => {
+      renderMenu()
+      fireEvent.keyDown(screen.getByRole('menu'), { key: 'Tab', shiftKey: true })
+      expect(document.activeElement).toBe(before)
+    })
+
+    it('falls back to computing the tab stop from the menu itself when the instigator is no longer available', () => {
+      mockUseApp.buttonRefs.current.instigator = null
+      renderMenu()
+      expect(() => fireEvent.keyDown(screen.getByRole('menu'), { key: 'Tab' })).not.toThrow()
+      expect(document.activeElement).toBe(before)
+      expect(mockUseApp.layoutRefs.viewportRef.current.focus).not.toHaveBeenCalled()
+    })
+  })
+
+  describe('Tab key inside a modal dialog', () => {
+    let dialogEl, insideBefore, instigatorEl, outsideAfter
+
+    beforeEach(() => {
+      dialogEl = document.createElement('div')
+      dialogEl.setAttribute('role', 'dialog')
+      dialogEl.setAttribute('aria-modal', 'true')
+      insideBefore = document.createElement('button')
+      instigatorEl = document.createElement('button')
+      dialogEl.append(insideBefore, instigatorEl)
+      outsideAfter = document.createElement('button')
+      document.body.append(dialogEl, outsideAfter)
+      ;[insideBefore, instigatorEl, outsideAfter].forEach(el => Object.defineProperty(el, 'offsetParent', { value: true, configurable: true }))
+      mockUseApp.buttonRefs.current.instigator = instigatorEl
+    })
+
+    afterEach(() => {
+      dialogEl.remove()
+      outsideAfter.remove()
+    })
+
+    it('wraps within the dialog instead of escaping to a tab stop outside it', () => {
+      renderMenu()
+      fireEvent.keyDown(screen.getByRole('menu'), { key: 'Tab' })
+      expect(document.activeElement).toBe(insideBefore)
+      expect(document.activeElement).not.toBe(outsideAfter)
+    })
+  })
+
   it('calls item onClick directly if no buttonConfig', () => {
     renderMenu()
     fireEvent.click(screen.getByText('Item 1'))
@@ -362,6 +431,16 @@ describe('PopupMenu', () => {
     expect(focusSpy).toHaveBeenCalled()
   })
 
+  it('Escape falls back to focusing viewport when instigator is no longer available', () => {
+    mockUseApp.buttonRefs.current.instigator = null
+    const focusSpy = jest.spyOn(mockUseApp.layoutRefs.viewportRef.current, 'focus')
+    renderMenu()
+    const ul = screen.getByRole('menu')
+    expect(() => fireEvent.keyDown(ul, { key: 'Escape' })).not.toThrow()
+    expect(focusSpy).toHaveBeenCalled()
+    focusSpy.mockRestore()
+  })
+
   it('direct render without startIndex respects startPos="first" (covers initializer and effect)', () => {
     renderDirect({ startPos: 'first' })
     expectSelected('Item 1')

src/App/components/PopupMenu/usePopupMenu.js

@@ -1,5 +1,6 @@
 import { useState, useMemo, useEffect } from 'react'
 import { stringToKebab } from '../../../utils/stringToKebab.js'
+import { findTabStop } from '../../../utils/findNextTabStop.js'
 import { useApp } from '../../store/appContext.js'
 
 /**
@@ -86,6 +87,54 @@ const resolveItemFocus = (item, buttonConfig) => ({
   keepFocus: buttonConfig[item.id]?.keepFocus ?? item.keepFocus
 })
 
+/**
+ * Focuses the instigator button if it's still in the DOM, otherwise falls back
+ * to the map viewport. The instigator can disappear while the menu is open
+ * (e.g. app state changes elsewhere remove/disable the triggering button),
+ * so callers must not assume it's still focusable.
+ *
+ * @param {HTMLElement|null} instigator - DOM node of the trigger button, may no longer exist.
+ * @param {object}           viewportRef - Ref to the map viewport element, used as fallback.
+ */
+const focusInstigatorOrViewport = (instigator, viewportRef) => {
+  if (instigator) {
+    instigator.focus()
+  } else {
+    viewportRef.current?.focus()
+  }
+}
+
+// A Panel rendered as a modal dialog (see Panel.jsx's computePanelState/getPanelRole)
+// carries both of these attributes together and traps focus within itself via
+// useModalPanelBehaviour's focusin redirect — tab-stop search must stay inside it.
+const MODAL_DIALOG_SELECTOR = '[role="dialog"][aria-modal="true"]'
+
+/**
+ * Moves focus to the tab stop immediately after (or before, for Shift+Tab)
+ * the instigator button, continuing the page's natural tab sequence rather
+ * than returning focus into the popup itself. The popup is rendered via a
+ * portal elsewhere in the DOM, so the browser's own Tab default action can't
+ * be relied on to continue from the instigator — it must be computed and
+ * applied manually. The search is scoped to the nearest modal dialog ancestor
+ * (if any): searching the whole document would let it pick a tab stop outside
+ * an open modal panel, which the app's own modal focus-trap then immediately
+ * yanks focus back from — landing on the trap's container div, which has no
+ * visible focus styling, making focus appear to vanish. Falls back to
+ * computing the tab stop from the menu element itself if the instigator is
+ * gone, so focus still lands on a real neighbouring control rather than
+ * jumping to an unrelated part of the app.
+ *
+ * @param {HTMLElement|null} instigator - DOM node of the trigger button, may no longer exist.
+ * @param {boolean}          isShiftTab - Whether Shift+Tab (move backwards) was pressed.
+ * @param {HTMLElement}      fallbackEl - Menu element to anchor from if the instigator is gone.
+ */
+const focusAdjacentTabStop = (instigator, isShiftTab, fallbackEl) => {
+  const anchor = instigator ?? fallbackEl
+  const root = anchor.closest?.(MODAL_DIALOG_SELECTOR) ?? document
+  const nextStop = findTabStop({ el: anchor, direction: isShiftTab ? 'prev' : 'next', root })
+  nextStop?.focus()
+}
+
 const handleMenuEnter = (event, { items, index, disabledButtons, activateCtx, instigator, dispatch, viewportRef, setIsOpen }) => {
   event.preventDefault()
   const item = items[index]
@@ -98,7 +147,7 @@ const handleMenuEnter = (event, { items, index, disabledButtons, activateCtx, in
     }
     activateItem(event, item, activateCtx)
     if (keepFocus) {
-      instigator.focus()
+      focusInstigatorOrViewport(instigator, viewportRef)
       setIsOpen(false)
       return
     }
@@ -122,7 +171,7 @@ const handleMenuSpace = (event, { items, index, disabledButtons, activateCtx, in
   activateItem(event, item, activateCtx)
   if (!(item.isPressed !== undefined || item.pressedWhen)) {
     if (keepFocus) {
-      instigator.focus()
+      focusInstigatorOrViewport(instigator, viewportRef)
     } else {
       requestAnimationFrame(() => viewportRef.current?.focus())
     }
@@ -149,11 +198,9 @@ const handleMenuSpace = (event, { items, index, disabledButtons, activateCtx, in
  * @returns {Function} onKeyDown handler for the menu element.
  */
 const createMenuKeyDownHandler = ({ items, visibleIndices, index, setIndex, disabledButtons, instigator, setIsOpen, activateCtx, dispatch, viewportRef }) => {
-  const closeAndFocus = (event, preventDefault = false) => {
-    if (preventDefault && event?.preventDefault) {
-      event.preventDefault()
-    }
-    instigator.focus()
+  const closeAndFocus = (event) => {
+    event.preventDefault()
+    focusInstigatorOrViewport(instigator, viewportRef)
     setIsOpen(false)
   }
 
@@ -179,11 +226,13 @@ const createMenuKeyDownHandler = ({ items, visibleIndices, index, setIndex, disa
 
   return (event) => {
     if (['Escape', 'Esc'].includes(event.key)) {
-      closeAndFocus(event, true)
+      closeAndFocus(event)
       return
     }
     if (event.key === 'Tab') {
-      closeAndFocus(event)
+      event.preventDefault()
+      focusAdjacentTabStop(instigator, event.shiftKey, event.currentTarget)
+      setIsOpen(false)
       return
     }
     if (['ArrowDown', 'ArrowUp'].includes(event.key)) {
@@ -195,7 +244,7 @@ const createMenuKeyDownHandler = ({ items, visibleIndices, index, setIndex, disa
       return
     }
     if (event.key === 'End' && visibleIndices.length) {
-      setIndex(visibleIndices[visibleIndices.length - 1])
+      setIndex(visibleIndices[visibleIndices.length - 1]) // NOSONAR .length - 1 used instead of .at(-1) for wider browser support
       return
     }
     if (event.key === 'Enter') {
@@ -274,7 +323,7 @@ export const usePopupMenu = ({
     setIsOpen(false)
     activateItem(event, item, activateCtx)
     if (keepFocus) {
-      instigator.focus()
+      focusInstigatorOrViewport(instigator, viewportRef)
     } else {
       viewportRef.current?.focus()
     }
@@ -285,7 +334,7 @@ export const usePopupMenu = ({
     if (startPos === 'first') {
       setIndex(visibleIndices[0] ?? -1)
     } else if (startPos === 'last') {
-      setIndex(visibleIndices[visibleIndices.length - 1] ?? -1)
+      setIndex(visibleIndices[visibleIndices.length - 1] ?? -1) // NOSONAR .length - 1 used instead of .at(-1) for wider browser support
     } else {
       // No action
     }

src/utils/findNextTabStop.js

@@ -1,7 +1,7 @@
-export function findTabStop ({ el, direction }) {
-  const focusableEls = document.querySelectorAll('input, button, select, textarea, a[href]')
+export function findTabStop ({ el, direction, root = document }) {
+  const focusableEls = root.querySelectorAll('input, button, select, textarea, a[href]')
   const list = Array.prototype.filter.call(focusableEls, item => {
-    return item.tabIndex >= 0
+    return item.tabIndex >= 0 && !!item.offsetParent
   })
   const index = list.indexOf(el)
   return list[direction === 'next' ? index + 1 : index - 1] || list[0]

src/utils/findNextTabStop.test.js

@@ -7,9 +7,10 @@ import { findTabStop } from './findNextTabStop.js'
 // --- MOCK SETUP ---
 // Helper to create mock elements. The default value (tabIndex = 0)
 // is covered below by creating 'el1' without the second argument.
-const createMockElement = (id, tabIndex = 0) => ({
+const createMockElement = (id, tabIndex = 0, offsetParent = {}) => ({
   id,
   tabIndex,
+  offsetParent,
   // Required so Jest knows which element is which in the array lookup
   toString: () => id
 })
@@ -74,4 +75,27 @@ describe('findTabStop', () => {
     // Assert: It wraps around to the first element, el1.
     expect(result).toBe(el1)
   })
+
+  // Test 4: Covers: skipping elements with no offsetParent (hidden, e.g. wrong-breakpoint duplicates).
+  it('should skip hidden elements (no offsetParent) even when tabIndex is valid', () => {
+    const el1 = createMockElement('el1', 0)
+    const hiddenEl = createMockElement('hidden', 0, null)
+    const el3 = createMockElement('el3', 0)
+
+    mockFocusableElements([el1, hiddenEl, el3])
+
+    const result = findTabStop({ el: el1, direction: 'next' })
+
+    expect(result).toBe(el3)
+  })
+
+  it('accepts a custom root to scope the search', () => {
+    const el1 = createMockElement('el1')
+    const scopedRoot = { querySelectorAll: jest.fn(() => [el1]) }
+
+    findTabStop({ el: el1, direction: 'next', root: scopedRoot })
+
+    expect(scopedRoot.querySelectorAll).toHaveBeenCalledTimes(1)
+    expect(document.querySelectorAll).not.toHaveBeenCalled()
+  })
 })