From 845046b443a6b097cc1d04216910d3c79174ff4a Mon Sep 17 00:00:00 2001 From: Alina Andrieieva Date: Thu, 16 Nov 2023 17:56:03 +0200 Subject: [PATCH 1/4] group focus fix --- src/Menu.tsx | 63 ++++++++++++++++++++------------ src/hooks/useAccessibility.ts | 67 +++++++++++++++++------------------ 2 files changed, 73 insertions(+), 57 deletions(-) diff --git a/src/Menu.tsx b/src/Menu.tsx index ef9db196..db18ce31 100644 --- a/src/Menu.tsx +++ b/src/Menu.tsx @@ -2,16 +2,20 @@ import classNames from 'classnames'; import type { CSSMotionProps } from 'rc-motion'; import Overflow from 'rc-overflow'; import useMergedState from 'rc-util/lib/hooks/useMergedState'; +import isEqual from 'rc-util/lib/isEqual'; import warning from 'rc-util/lib/warning'; import * as React from 'react'; import { useImperativeHandle } from 'react'; import { flushSync } from 'react-dom'; -import isEqual from 'rc-util/lib/isEqual'; -import { getMenuId, IdContext } from './context/IdContext'; +import { IdContext } from './context/IdContext'; import MenuContextProvider from './context/MenuContext'; import { PathRegisterContext, PathUserContext } from './context/PathContext'; import PrivateContext from './context/PrivateContext'; -import useAccessibility from './hooks/useAccessibility'; +import { + getFocusableElements, + refreshElements, + useAccessibility, +} from './hooks/useAccessibility'; import useKeyRecords, { OVERFLOW_KEY } from './hooks/useKeyRecords'; import useMemoCallback from './hooks/useMemoCallback'; import useUUID from './hooks/useUUID'; @@ -270,8 +274,9 @@ const Menu = React.forwardRef((props, ref) => { }; // >>>>> Cache & Reset open keys when inlineCollapsed changed - const [inlineCacheOpenKeys, setInlineCacheOpenKeys] = - React.useState(mergedOpenKeys); + const [inlineCacheOpenKeys, setInlineCacheOpenKeys] = React.useState( + mergedOpenKeys, + ); const mountRef = React.useRef(false); @@ -347,10 +352,9 @@ const Menu = React.forwardRef((props, ref) => { [registerPath, unregisterPath], ); - const pathUserContext = React.useMemo( - () => ({ isSubPathKey }), - [isSubPathKey], - ); + const pathUserContext = React.useMemo(() => ({ isSubPathKey }), [ + isSubPathKey, + ]); React.useEffect(() => { refreshOverflowKeys( @@ -378,20 +382,33 @@ const Menu = React.forwardRef((props, ref) => { setMergedActiveKey(undefined); }); - useImperativeHandle(ref, () => ({ - list: containerRef.current, - focus: options => { - const shouldFocusKey = - mergedActiveKey ?? childList.find(node => !node.props.disabled)?.key; - if (shouldFocusKey) { - containerRef.current - ?.querySelector( - `li[data-menu-id='${getMenuId(uuid, shouldFocusKey as string)}']`, - ) - ?.focus?.(options); - } - }, - })); + useImperativeHandle(ref, () => { + return { + list: containerRef.current, + focus: options => { + const elements = new Set(); + const key2element = new Map(); + const element2key = new Map(); + const keys = getKeys(); + + refreshElements(keys, uuid, elements, key2element, element2key); + + const focusableElements = getFocusableElements( + containerRef.current, + elements, + ); + + const shouldFocusKey = + mergedActiveKey ?? element2key.get(focusableElements[0]); + + const elementToFocus = key2element.get(shouldFocusKey); + + if (shouldFocusKey && elementToFocus) { + elementToFocus?.focus?.(options); + } + }, + }; + }); // ======================== Select ======================== // >>>>> Select keys diff --git a/src/hooks/useAccessibility.ts b/src/hooks/useAccessibility.ts index b60b27bf..24f7ac43 100644 --- a/src/hooks/useAccessibility.ts +++ b/src/hooks/useAccessibility.ts @@ -1,9 +1,9 @@ -import * as React from 'react'; +import { getFocusNodeList } from 'rc-util/lib/Dom/focus'; import KeyCode from 'rc-util/lib/KeyCode'; import raf from 'rc-util/lib/raf'; -import { getFocusNodeList } from 'rc-util/lib/Dom/focus'; -import type { MenuMode } from '../interface'; +import * as React from 'react'; import { getMenuId } from '../context/IdContext'; +import type { MenuMode } from '../interface'; // destruct to reduce minify size const { LEFT, RIGHT, UP, DOWN, ENTER, ESC, HOME, END } = KeyCode; @@ -134,7 +134,7 @@ function getFocusElement( /** * Get focusable elements from the element set under provided container */ -function getFocusableElements( +export function getFocusableElements( container: HTMLElement, elements: Set, ) { @@ -181,7 +181,29 @@ function getNextFocusElement( return sameLevelFocusableMenuElementList[focusIndex]; } -export default function useAccessibility( +export const refreshElements = ( + keys: string[], + id: string, + elements: Set, + key2element: Map, + element2key: Map, +) => { + keys.forEach(key => { + const element = document.querySelector( + `[data-menu-id='${getMenuId(id, key)}']`, + ) as HTMLElement; + + if (element) { + elements.add(element); + element2key.set(element, key); + key2element.set(key, element); + } + }); + + return elements; +}; + +export function useAccessibility( mode: MenuMode, activeKey: string, isRtl: boolean, @@ -216,35 +238,12 @@ export default function useAccessibility( const { which } = e; if ([...ArrowKeys, ENTER, ESC, HOME, END].includes(which)) { - // Convert key to elements - let elements: Set; - let key2element: Map; - let element2key: Map; - - // >>> Wrap as function since we use raf for some case - const refreshElements = () => { - elements = new Set(); - key2element = new Map(); - element2key = new Map(); - - const keys = getKeys(); - - keys.forEach(key => { - const element = document.querySelector( - `[data-menu-id='${getMenuId(id, key)}']`, - ) as HTMLElement; - - if (element) { - elements.add(element); - element2key.set(element, key); - key2element.set(key, element); - } - }); - - return elements; - }; + const elements = new Set(); + const key2element = new Map(); + const element2key = new Map(); + const keys = getKeys(); - refreshElements(); + refreshElements(keys, id, elements, key2element, element2key); // First we should find current focused MenuItem/SubMenu element const activeElement = key2element.get(activeKey); @@ -341,7 +340,7 @@ export default function useAccessibility( cleanRaf(); rafRef.current = raf(() => { // Async should resync elements - refreshElements(); + refreshElements(keys, id, elements, key2element, element2key); const controlId = focusMenuElement.getAttribute('aria-controls'); const subQueryContainer = document.getElementById(controlId); From 1055ac589b6d3087f74b3dd78ee7e2885a5bd911 Mon Sep 17 00:00:00 2001 From: Alina Andrieieva Date: Fri, 17 Nov 2023 14:39:57 +0200 Subject: [PATCH 2/4] fixed focus when first item is group or submenu --- src/Menu.tsx | 22 ++++++++++------------ 1 file changed, 10 insertions(+), 12 deletions(-) diff --git a/src/Menu.tsx b/src/Menu.tsx index db18ce31..0a461b9d 100644 --- a/src/Menu.tsx +++ b/src/Menu.tsx @@ -11,11 +11,7 @@ import { IdContext } from './context/IdContext'; import MenuContextProvider from './context/MenuContext'; import { PathRegisterContext, PathUserContext } from './context/PathContext'; import PrivateContext from './context/PrivateContext'; -import { - getFocusableElements, - refreshElements, - useAccessibility, -} from './hooks/useAccessibility'; +import { refreshElements, useAccessibility } from './hooks/useAccessibility'; import useKeyRecords, { OVERFLOW_KEY } from './hooks/useKeyRecords'; import useMemoCallback from './hooks/useMemoCallback'; import useUUID from './hooks/useUUID'; @@ -386,20 +382,22 @@ const Menu = React.forwardRef((props, ref) => { return { list: containerRef.current, focus: options => { + let shouldFocusKey: string; const elements = new Set(); const key2element = new Map(); const element2key = new Map(); const keys = getKeys(); refreshElements(keys, uuid, elements, key2element, element2key); + if (mergedActiveKey) { + shouldFocusKey = mergedActiveKey; + } else { + const firstActiveElement = [...elements].find( + el => !el.ariaDisabled || el.ariaDisabled === 'false', + ); - const focusableElements = getFocusableElements( - containerRef.current, - elements, - ); - - const shouldFocusKey = - mergedActiveKey ?? element2key.get(focusableElements[0]); + shouldFocusKey = element2key.get(firstActiveElement); + } const elementToFocus = key2element.get(shouldFocusKey); From e021ff9dcf5564ee594b9b8692318c82712a139d Mon Sep 17 00:00:00 2001 From: Alina Andrieieva Date: Mon, 20 Nov 2023 12:07:40 +0200 Subject: [PATCH 3/4] added tests for group and submenu focus --- src/Menu.tsx | 21 +++--- tests/Focus.spec.tsx | 175 ++++++++++++++++++++++++++++++++++++++++--- tests/Menu.spec.tsx | 91 +++++++--------------- 3 files changed, 204 insertions(+), 83 deletions(-) diff --git a/src/Menu.tsx b/src/Menu.tsx index 0a461b9d..0733ad87 100644 --- a/src/Menu.tsx +++ b/src/Menu.tsx @@ -11,7 +11,11 @@ import { IdContext } from './context/IdContext'; import MenuContextProvider from './context/MenuContext'; import { PathRegisterContext, PathUserContext } from './context/PathContext'; import PrivateContext from './context/PrivateContext'; -import { refreshElements, useAccessibility } from './hooks/useAccessibility'; +import { + getFocusableElements, + refreshElements, + useAccessibility, +} from './hooks/useAccessibility'; import useKeyRecords, { OVERFLOW_KEY } from './hooks/useKeyRecords'; import useMemoCallback from './hooks/useMemoCallback'; import useUUID from './hooks/useUUID'; @@ -382,22 +386,19 @@ const Menu = React.forwardRef((props, ref) => { return { list: containerRef.current, focus: options => { - let shouldFocusKey: string; const elements = new Set(); const key2element = new Map(); const element2key = new Map(); const keys = getKeys(); refreshElements(keys, uuid, elements, key2element, element2key); - if (mergedActiveKey) { - shouldFocusKey = mergedActiveKey; - } else { - const firstActiveElement = [...elements].find( - el => !el.ariaDisabled || el.ariaDisabled === 'false', - ); + const focusableElements = getFocusableElements( + containerRef.current, + elements, + ); - shouldFocusKey = element2key.get(firstActiveElement); - } + const shouldFocusKey = + mergedActiveKey ?? element2key.get(focusableElements[0]); const elementToFocus = key2element.get(shouldFocusKey); diff --git a/tests/Focus.spec.tsx b/tests/Focus.spec.tsx index f3f94216..5457895f 100644 --- a/tests/Focus.spec.tsx +++ b/tests/Focus.spec.tsx @@ -1,9 +1,20 @@ /* eslint-disable no-undef */ -import { fireEvent, render } from '@testing-library/react'; +import { act, fireEvent, render } from '@testing-library/react'; +import { spyElementPrototypes } from 'rc-util/lib/test/domHook'; import React from 'react'; -import Menu, { MenuItem, SubMenu } from '../src'; +import Menu, { MenuItem, MenuItemGroup, MenuRef, SubMenu } from '../src'; describe('Focus', () => { + beforeAll(() => { + // Mock to force make menu item visible + spyElementPrototypes(HTMLElement, { + offsetParent: { + get() { + return this.parentElement; + }, + }, + }); + }); beforeEach(() => { global.triggerProps = null; @@ -15,13 +26,15 @@ describe('Focus', () => { jest.useRealTimers(); }); - it('Get focus', () => { - const { container } = render( - - - 1 - - , + it('Get focus', async () => { + const { container } = await act(async () => + render( + + + 1 + + , + ), ); // Item focus @@ -34,5 +47,149 @@ describe('Focus', () => { fireEvent.focus(container.querySelector('.rc-menu-submenu-title')); expect(container.querySelector('.rc-menu-submenu-active')).toBeTruthy(); }); + + it('should support focus through ref', async () => { + const menuRef = React.createRef(); + const { getByTestId } = await act(async () => + render( + + + Disabled child + + + Light + + , + ), + ); + + act(() => menuRef.current.focus()); + + const firstFocusableItem = getByTestId('first-focusable'); + expect(document.activeElement).toBe(firstFocusableItem); + expect(firstFocusableItem).toHaveClass('rc-menu-item-active'); + }); + + it('should focus active item through ref', async () => { + const menuRef = React.createRef(); + const { getByTestId } = await act(async () => + render( + + Light + + Cat + + , + ), + ); + act(() => menuRef.current.focus()); + + const activeKey = getByTestId('active-key'); + expect(document.activeElement).toBe(activeKey); + expect(activeKey).toHaveClass('rc-menu-item-active'); + }); + + it('focus moves to the next accessible menu item if the first child is empty group', async () => { + const menuRef = React.createRef(); + const { getByTestId } = await act(async () => + render( + + + + Disabled child + + + Light + + , + ), + ); + + act(() => menuRef.current.focus()); + + const firstFocusableItem = getByTestId('first-focusable'); + expect(document.activeElement).toBe(firstFocusableItem); + expect(firstFocusableItem).toHaveClass('rc-menu-item-active'); + }); + + it('focus moves to the next accessible group item if the first child is non-empty group', async () => { + const menuRef = React.createRef(); + const { getByTestId } = await act(async () => + render( + + + + group-child-1 + + + group-child-2 + + + Light + , + ), + ); + + act(() => menuRef.current.focus()); + + const firstFocusableItem = getByTestId('first-focusable'); + expect(document.activeElement).toBe(firstFocusableItem); + expect(firstFocusableItem).toHaveClass('rc-menu-item-active'); + }); + + it('focus moves to nested group item correctly', async () => { + const menuRef = React.createRef(); + const { getByTestId } = await act(async () => + render( + + + + group-child-1 + + + + nested-group-child-1 + + + nested-group-child-2 + + + group-child-3 + + , + ), + ); + + act(() => menuRef.current.focus()); + + const firstFocusableItem = getByTestId('first-focusable'); + expect(document.activeElement).toBe(firstFocusableItem); + expect(firstFocusableItem).toHaveClass('rc-menu-item-active'); + }); + + it('focus moves to submenu correctly', async () => { + const menuRef = React.createRef(); + const { getByTestId, getByTitle } = await act(async () => + render( + + + Disabled child + + + Submenu child + + Light + , + ), + ); + + act(() => menuRef.current.focus()); + + expect(document.activeElement).toBe(getByTitle('Submenu')); + expect(getByTestId('sub-menu')).toHaveClass('rc-menu-submenu-active'); + }); }); /* eslint-enable */ diff --git a/tests/Menu.spec.tsx b/tests/Menu.spec.tsx index 194a1ff5..a4d35c78 100644 --- a/tests/Menu.spec.tsx +++ b/tests/Menu.spec.tsx @@ -1,4 +1,5 @@ /* eslint-disable no-undef, react/no-multi-comp, react/jsx-curly-brace-presence, max-classes-per-file */ +import type { MenuMode } from '@/interface'; import { fireEvent, render } from '@testing-library/react'; import KeyCode from 'rc-util/lib/KeyCode'; import { resetWarned } from 'rc-util/lib/warning'; @@ -7,7 +8,6 @@ import { act } from 'react-dom/test-utils'; import type { MenuRef } from '../src'; import Menu, { Divider, MenuItem, MenuItemGroup, SubMenu } from '../src'; import { isActive, last } from './util'; -import type { MenuMode } from '@/interface'; jest.mock('@rc-component/trigger', () => { const react = require('react'); @@ -285,7 +285,7 @@ describe('Menu', () => { // don't use selectedKeys as string // it is a compatible feature for https://github.com/ant-design/ant-design/issues/29429 const { container } = render( - + 1 2 , @@ -329,8 +329,8 @@ describe('Menu', () => { it('openKeys should allow to be empty', () => { const { container } = render( { }} - onOpenChange={() => { }} + onClick={() => {}} + onOpenChange={() => {}} openKeys={undefined} selectedKeys={['1']} mode="inline" @@ -703,34 +703,6 @@ describe('Menu', () => { expect(menuRef.current?.list).toBe(container.querySelector('ul')); }); - it('should support focus through ref', () => { - const menuRef = React.createRef(); - const { container } = render( - - - Disabled child - - Light - , - ); - menuRef.current?.focus(); - - expect(document.activeElement).toBe(last(container.querySelectorAll('li'))); - }); - - it('should focus active item through ref', () => { - const menuRef = React.createRef(); - const { container } = render( - - Light - Cat - , - ); - menuRef.current?.focus(); - - expect(document.activeElement).toBe(last(container.querySelectorAll('li'))); - }); - it('should render a divider with role="separator"', () => { const menuRef = React.createRef(); const { container } = render( @@ -747,52 +719,43 @@ describe('Menu', () => { expect(divider).toHaveAttribute('role', 'separator'); }); it('expandIcon should be hidden when setting null or false', () => { - const App = ({expand, subExpand}: {expand?: React.ReactNode, subExpand?: React.ReactNode}) => ( + const App = ({ + expand, + subExpand, + }: { + expand?: React.ReactNode; + subExpand?: React.ReactNode; + }) => ( - + 0-1 0-2 - , - + + , + 0-1 0-2 - , - Cat + + ,Cat ); - + const { container, rerender } = render( , ); - expect(container.querySelectorAll(".rc-menu-submenu-arrow").length).toBe(0); - - rerender( - , - ); - expect(container.querySelectorAll(".rc-menu-submenu-arrow").length).toBe(1); + expect(container.querySelectorAll('.rc-menu-submenu-arrow').length).toBe(0); - rerender( - , - ); - expect(container.querySelectorAll(".rc-menu-submenu-arrow").length).toBe(2); + rerender(); + expect(container.querySelectorAll('.rc-menu-submenu-arrow').length).toBe(1); - rerender( - , - ); - expect(container.querySelectorAll(".rc-menu-submenu-arrow").length).toBe(0); + rerender(); + expect(container.querySelectorAll('.rc-menu-submenu-arrow').length).toBe(2); - rerender( - , - ); - expect(container.querySelectorAll(".rc-menu-submenu-arrow").length).toBe(1); + rerender(); + expect(container.querySelectorAll('.rc-menu-submenu-arrow').length).toBe(0); + rerender(); + expect(container.querySelectorAll('.rc-menu-submenu-arrow').length).toBe(1); }); }); /* eslint-enable */ From d5f24362a6a0dcfd7e5c0217f1025183f32d7457 Mon Sep 17 00:00:00 2001 From: Alina Andrieieva Date: Thu, 23 Nov 2023 16:48:48 +0200 Subject: [PATCH 4/4] moved the maps inside `refreshElements` function --- src/Menu.tsx | 9 ++++----- src/hooks/useAccessibility.ts | 24 ++++++++++-------------- 2 files changed, 14 insertions(+), 19 deletions(-) diff --git a/src/Menu.tsx b/src/Menu.tsx index 0733ad87..e2e86d33 100644 --- a/src/Menu.tsx +++ b/src/Menu.tsx @@ -386,12 +386,11 @@ const Menu = React.forwardRef((props, ref) => { return { list: containerRef.current, focus: options => { - const elements = new Set(); - const key2element = new Map(); - const element2key = new Map(); const keys = getKeys(); - - refreshElements(keys, uuid, elements, key2element, element2key); + const { elements, key2element, element2key } = refreshElements( + keys, + uuid, + ); const focusableElements = getFocusableElements( containerRef.current, elements, diff --git a/src/hooks/useAccessibility.ts b/src/hooks/useAccessibility.ts index 24f7ac43..5cd58e20 100644 --- a/src/hooks/useAccessibility.ts +++ b/src/hooks/useAccessibility.ts @@ -181,13 +181,11 @@ function getNextFocusElement( return sameLevelFocusableMenuElementList[focusIndex]; } -export const refreshElements = ( - keys: string[], - id: string, - elements: Set, - key2element: Map, - element2key: Map, -) => { +export const refreshElements = (keys: string[], id: string) => { + const elements = new Set(); + const key2element = new Map(); + const element2key = new Map(); + keys.forEach(key => { const element = document.querySelector( `[data-menu-id='${getMenuId(id, key)}']`, @@ -200,7 +198,7 @@ export const refreshElements = ( } }); - return elements; + return { elements, key2element, element2key }; }; export function useAccessibility( @@ -238,12 +236,10 @@ export function useAccessibility( const { which } = e; if ([...ArrowKeys, ENTER, ESC, HOME, END].includes(which)) { - const elements = new Set(); - const key2element = new Map(); - const element2key = new Map(); const keys = getKeys(); - refreshElements(keys, id, elements, key2element, element2key); + let refreshedElements = refreshElements(keys, id); + const { elements, key2element, element2key } = refreshedElements; // First we should find current focused MenuItem/SubMenu element const activeElement = key2element.get(activeKey); @@ -340,7 +336,7 @@ export function useAccessibility( cleanRaf(); rafRef.current = raf(() => { // Async should resync elements - refreshElements(keys, id, elements, key2element, element2key); + refreshedElements = refreshElements(keys, id); const controlId = focusMenuElement.getAttribute('aria-controls'); const subQueryContainer = document.getElementById(controlId); @@ -348,7 +344,7 @@ export function useAccessibility( // Get sub focusable menu item const targetElement = getNextFocusElement( subQueryContainer, - elements, + refreshedElements.elements, ); // Focus menu item