diff --git a/src/App/components/MapButton/MapButton.jsx b/src/App/components/MapButton/MapButton.jsx index c08ae8c5..fe9237f1 100755 --- a/src/App/components/MapButton/MapButton.jsx +++ b/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, diff --git a/src/App/components/MapButton/MapButton.module.scss b/src/App/components/MapButton/MapButton.module.scss index 408cfb0d..44f09c48 100755 --- a/src/App/components/MapButton/MapButton.module.scss +++ b/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); } diff --git a/src/App/components/MapButton/MapButton.test.jsx b/src/App/components/MapButton/MapButton.test.jsx index 0d88cccf..f29dc877 100755 --- a/src/App/components/MapButton/MapButton.test.jsx +++ b/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', () => { diff --git a/src/App/components/PopupMenu/PopupMenu.test.jsx b/src/App/components/PopupMenu/PopupMenu.test.jsx index 1316c7c9..cc19f3cc 100644 --- a/src/App/components/PopupMenu/PopupMenu.test.jsx +++ b/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') diff --git a/src/App/components/PopupMenu/usePopupMenu.js b/src/App/components/PopupMenu/usePopupMenu.js index c71b700b..414e75a9 100644 --- a/src/App/components/PopupMenu/usePopupMenu.js +++ b/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 } diff --git a/src/utils/findNextTabStop.js b/src/utils/findNextTabStop.js index 6797f27d..4c937de2 100755 --- a/src/utils/findNextTabStop.js +++ b/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] diff --git a/src/utils/findNextTabStop.test.js b/src/utils/findNextTabStop.test.js index a26ed890..9d83ab77 100644 --- a/src/utils/findNextTabStop.test.js +++ b/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() + }) })