From 935b7116839661e091802f38c932ab4a7099c090 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E5=8F=B6=E6=96=87=E4=BF=8A?= Date: Fri, 17 Oct 2025 11:30:29 +0800 Subject: [PATCH 1/6] fix: wrong focus behavior when configuring menu selectability --- src/Menu.tsx | 23 +++++++++++++++++------ 1 file changed, 17 insertions(+), 6 deletions(-) diff --git a/src/Menu.tsx b/src/Menu.tsx index b87d2024..bad22cba 100644 --- a/src/Menu.tsx +++ b/src/Menu.tsx @@ -394,14 +394,22 @@ const Menu = React.forwardRef((props, ref) => { const keys = getKeys(); const { elements, key2element, element2key } = refreshElements(keys, uuid); const focusableElements = getFocusableElements(containerRef.current, elements); - + const defaultFocusKey = focusableElements[0] + ? element2key.get(focusableElements[0]) + : childList.find(node => !node.props.disabled)?.key; let shouldFocusKey: string; - if (mergedActiveKey && keys.includes(mergedActiveKey)) { - shouldFocusKey = mergedActiveKey; + // find the item to focus on based on whether it is selectable. + if (selectable) { + // if there is already a selected item, do not apply the focus. + if (!getMergedSelectKeys()?.length) { + if (mergedActiveKey && keys.includes(mergedActiveKey)) { + shouldFocusKey = mergedActiveKey; + } else { + shouldFocusKey = defaultFocusKey; + } + } } else { - shouldFocusKey = focusableElements[0] - ? element2key.get(focusableElements[0]) - : childList.find(node => !node.props.disabled)?.key; + shouldFocusKey = defaultFocusKey; } const elementToFocus = key2element.get(shouldFocusKey); @@ -434,6 +442,9 @@ const Menu = React.forwardRef((props, ref) => { return [internalSelectKeys]; }, [internalSelectKeys]); + function getMergedSelectKeys() { + return mergedSelectKeys; + } // >>>>> Trigger select const triggerSelection = (info: MenuInfo) => { From 27b456edd8c5052917cf3740755473f3a71ebe31 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E5=8F=B6=E6=96=87=E4=BF=8A?= Date: Fri, 17 Oct 2025 14:31:57 +0800 Subject: [PATCH 2/6] fix: focus the first selected item if an item is already selected --- src/Menu.tsx | 17 +++++++++-------- 1 file changed, 9 insertions(+), 8 deletions(-) diff --git a/src/Menu.tsx b/src/Menu.tsx index bad22cba..0c077f8f 100644 --- a/src/Menu.tsx +++ b/src/Menu.tsx @@ -398,15 +398,16 @@ const Menu = React.forwardRef((props, ref) => { ? element2key.get(focusableElements[0]) : childList.find(node => !node.props.disabled)?.key; let shouldFocusKey: string; - // find the item to focus on based on whether it is selectable. + // find the item to focus on based on whether it is selectable if (selectable) { - // if there is already a selected item, do not apply the focus. - if (!getMergedSelectKeys()?.length) { - if (mergedActiveKey && keys.includes(mergedActiveKey)) { - shouldFocusKey = mergedActiveKey; - } else { - shouldFocusKey = defaultFocusKey; - } + const mergedSelectKeys = getMergedSelectKeys(); + // if there is already selected items, select first item to focus + if (mergedSelectKeys.length && keys.includes(mergedSelectKeys[0])) { + shouldFocusKey = mergedSelectKeys[0]; + } else if (mergedActiveKey && keys.includes(mergedActiveKey)) { + shouldFocusKey = mergedActiveKey; + } else { + shouldFocusKey = defaultFocusKey; } } else { shouldFocusKey = defaultFocusKey; From 5a55a013c988ed7f479cf6907c6cfae61b62054f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E5=8F=B6=E6=96=87=E4=BF=8A?= Date: Fri, 17 Oct 2025 16:30:22 +0800 Subject: [PATCH 3/6] test: add unit tests for focus scenarios with and without selection support --- tests/Focus.spec.tsx | 113 +++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 113 insertions(+) diff --git a/tests/Focus.spec.tsx b/tests/Focus.spec.tsx index f4ac1bec..a871b995 100644 --- a/tests/Focus.spec.tsx +++ b/tests/Focus.spec.tsx @@ -1,8 +1,10 @@ /* eslint-disable no-undef */ import { act, fireEvent, render } from '@testing-library/react'; import { spyElementPrototypes } from '@rc-component/util/lib/test/domHook'; +import KeyCode from '@rc-component/util/lib/KeyCode'; import React from 'react'; import Menu, { MenuItem, MenuItemGroup, MenuRef, SubMenu } from '../src'; +import { isActive } from './util'; describe('Focus', () => { beforeAll(() => { @@ -26,6 +28,24 @@ describe('Focus', () => { jest.useRealTimers(); }); + function keyDown(container: HTMLElement, keyCode: number) { + fireEvent.keyDown(container.querySelector('ul.rc-menu-root'), { + which: keyCode, + keyCode, + charCode: keyCode, + }); + + // SubMenu raf need slow than accessibility + for (let i = 0; i < 20; i += 1) { + act(() => { + jest.advanceTimersByTime(10); + }); + } + act(() => { + jest.runAllTimers(); + }); + } + it('Get focus', async () => { const { container } = await act(async () => render( @@ -186,5 +206,98 @@ describe('Focus', () => { expect(document.activeElement).toBe(getByTitle('Submenu')); expect(getByTestId('sub-menu')).toHaveClass('rc-menu-submenu-active'); }); + + it('When selectable is not configured, the focus should move to the first available item instead of keeping the previously focused item', async () => { + const menuRef = React.createRef(); + const items = [ + { key: '0', label: 'First Item' }, + { key: '1', label: 'Second Item' }, + { key: '2', label: 'Third Item' }, + ]; + const TestApp = () => { + return ( +
+ + {items.map(item => ( + + {item.label} + + ))} + +
+ ); + }; + const { getByTestId, container } = render(); + // ================ check keydown ============== + // first item + keyDown(container, KeyCode.DOWN); + isActive(container, 0); + // second item + keyDown(container, KeyCode.DOWN); + isActive(container, 1); + // select second item + keyDown(container, KeyCode.ENTER); + + // mock focus on item 0 to make sure it gets focused + const item0 = getByTestId('0'); + const focusSpy = jest.spyOn(item0, 'focus').mockImplementation(() => {}); + menuRef.current.focus(); + expect(focusSpy).toHaveBeenCalled(); + + // ================ check click ============== + // click third item + const item2 = getByTestId('2'); + fireEvent.click(item2); + menuRef.current.focus(); + expect(focusSpy).toHaveBeenCalled(); + // cleanup + focusSpy.mockRestore(); + }); + it('When selectable is configured, the focus should move to the selected item if there is a selection, else to the first item, not retain on last focused item', async () => { + const menuRef = React.createRef(); + const items = [ + { key: '0', label: 'First Item' }, + { key: '1', label: 'Second Item' }, + { key: '2', label: 'Third Item' }, + ]; + const TestApp = () => { + return ( +
+ + {items.map(item => ( + + {item.label} + + ))} + +
+ ); + }; + const { getByTestId, container } = render(); + // ================ check keydown ============== + // first item + keyDown(container, KeyCode.DOWN); + isActive(container, 0); + // second item + keyDown(container, KeyCode.DOWN); + isActive(container, 1); + // select second item + keyDown(container, KeyCode.ENTER); + // mock focus on item 1 to make sure it gets focused + const item1 = getByTestId('1'); + const focusSpy = jest.spyOn(item1, 'focus').mockImplementation(() => {}); + menuRef.current.focus(); + expect(focusSpy).toHaveBeenCalled(); + + // ================ check click ============== + // click third item + const item2 = getByTestId('2'); + fireEvent.click(item2); + menuRef.current.focus(); + // mock focus on item 2 to make sure it gets focused + expect(focusSpy).toHaveBeenCalled(); + // cleanup + focusSpy.mockRestore(); + }); }); /* eslint-enable */ From 8e6412470ed47345329beb729482ec9ba6366a65 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E5=8F=B6=E6=96=87=E4=BF=8A?= Date: Fri, 17 Oct 2025 17:01:32 +0800 Subject: [PATCH 4/6] test: update unit tests --- tests/Focus.spec.tsx | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/tests/Focus.spec.tsx b/tests/Focus.spec.tsx index a871b995..6c3931ca 100644 --- a/tests/Focus.spec.tsx +++ b/tests/Focus.spec.tsx @@ -292,12 +292,14 @@ describe('Focus', () => { // ================ check click ============== // click third item const item2 = getByTestId('2'); + const focusSpy2 = jest.spyOn(item2, 'focus').mockImplementation(() => {}); fireEvent.click(item2); menuRef.current.focus(); // mock focus on item 2 to make sure it gets focused - expect(focusSpy).toHaveBeenCalled(); + expect(focusSpy2).toHaveBeenCalled(); // cleanup focusSpy.mockRestore(); + focusSpy2.mockRestore(); }); }); /* eslint-enable */ From 759581fbbbb5f30a4ca5a6bb2b987fb793132239 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E5=8F=B6=E6=96=87=E4=BF=8A?= Date: Thu, 23 Oct 2025 13:07:41 +0800 Subject: [PATCH 5/6] feat: optimize code --- src/Menu.tsx | 40 ++++++++--------- tests/Focus.spec.tsx | 102 +++++++++++++++++++++++-------------------- 2 files changed, 73 insertions(+), 69 deletions(-) diff --git a/src/Menu.tsx b/src/Menu.tsx index 0c077f8f..b0b04bb1 100644 --- a/src/Menu.tsx +++ b/src/Menu.tsx @@ -387,6 +387,24 @@ const Menu = React.forwardRef((props, ref) => { setMergedActiveKey(undefined); }); + // ======================== Select ======================== + // >>>>> Select keys + const [internalSelectKeys, setMergedSelectKeys] = useControlledState( + defaultSelectedKeys || [], + selectedKeys, + ); + const mergedSelectKeys = React.useMemo(() => { + if (Array.isArray(internalSelectKeys)) { + return internalSelectKeys; + } + + if (internalSelectKeys === null || internalSelectKeys === undefined) { + return EMPTY_LIST; + } + + return [internalSelectKeys]; + }, [internalSelectKeys]); + // >>>>> accept ref useImperativeHandle(ref, () => { return { list: containerRef.current, @@ -400,7 +418,6 @@ const Menu = React.forwardRef((props, ref) => { let shouldFocusKey: string; // find the item to focus on based on whether it is selectable if (selectable) { - const mergedSelectKeys = getMergedSelectKeys(); // if there is already selected items, select first item to focus if (mergedSelectKeys.length && keys.includes(mergedSelectKeys[0])) { shouldFocusKey = mergedSelectKeys[0]; @@ -426,27 +443,6 @@ const Menu = React.forwardRef((props, ref) => { }; }); - // ======================== Select ======================== - // >>>>> Select keys - const [internalSelectKeys, setMergedSelectKeys] = useControlledState( - defaultSelectedKeys || [], - selectedKeys, - ); - const mergedSelectKeys = React.useMemo(() => { - if (Array.isArray(internalSelectKeys)) { - return internalSelectKeys; - } - - if (internalSelectKeys === null || internalSelectKeys === undefined) { - return EMPTY_LIST; - } - - return [internalSelectKeys]; - }, [internalSelectKeys]); - function getMergedSelectKeys() { - return mergedSelectKeys; - } - // >>>>> Trigger select const triggerSelection = (info: MenuInfo) => { if (selectable) { diff --git a/tests/Focus.spec.tsx b/tests/Focus.spec.tsx index 6c3931ca..e2b9cd8b 100644 --- a/tests/Focus.spec.tsx +++ b/tests/Focus.spec.tsx @@ -228,30 +228,34 @@ describe('Focus', () => { ); }; const { getByTestId, container } = render(); - // ================ check keydown ============== - // first item - keyDown(container, KeyCode.DOWN); - isActive(container, 0); - // second item - keyDown(container, KeyCode.DOWN); - isActive(container, 1); - // select second item - keyDown(container, KeyCode.ENTER); + // let focusSpy = jest; + let focusSpy: jest.SpyInstance | undefined; + try { + // ================ check keydown ============== + // first item + keyDown(container, KeyCode.DOWN); + isActive(container, 0); + // second item + keyDown(container, KeyCode.DOWN); + isActive(container, 1); + // select second item + keyDown(container, KeyCode.ENTER); - // mock focus on item 0 to make sure it gets focused - const item0 = getByTestId('0'); - const focusSpy = jest.spyOn(item0, 'focus').mockImplementation(() => {}); - menuRef.current.focus(); - expect(focusSpy).toHaveBeenCalled(); + // mock focus on item 0 to make sure it gets focused + const item0 = getByTestId('0'); + focusSpy = jest.spyOn(item0, 'focus').mockImplementation(() => {}); + menuRef.current.focus(); + expect(focusSpy).toHaveBeenCalled(); - // ================ check click ============== - // click third item - const item2 = getByTestId('2'); - fireEvent.click(item2); - menuRef.current.focus(); - expect(focusSpy).toHaveBeenCalled(); - // cleanup - focusSpy.mockRestore(); + // ================ check click ============== + // click third item + const item2 = getByTestId('2'); + fireEvent.click(item2); + menuRef.current.focus(); + expect(focusSpy).toHaveBeenCalled(); + } finally { + focusSpy.mockRestore(); + } }); it('When selectable is configured, the focus should move to the selected item if there is a selection, else to the first item, not retain on last focused item', async () => { const menuRef = React.createRef(); @@ -274,32 +278,36 @@ describe('Focus', () => { ); }; const { getByTestId, container } = render(); - // ================ check keydown ============== - // first item - keyDown(container, KeyCode.DOWN); - isActive(container, 0); - // second item - keyDown(container, KeyCode.DOWN); - isActive(container, 1); - // select second item - keyDown(container, KeyCode.ENTER); - // mock focus on item 1 to make sure it gets focused - const item1 = getByTestId('1'); - const focusSpy = jest.spyOn(item1, 'focus').mockImplementation(() => {}); - menuRef.current.focus(); - expect(focusSpy).toHaveBeenCalled(); + let focusSpy: jest.SpyInstance | undefined; + let focusSpy2: jest.SpyInstance | undefined; + try { + // ================ check keydown ============== + // first item + keyDown(container, KeyCode.DOWN); + isActive(container, 0); + // second item + keyDown(container, KeyCode.DOWN); + isActive(container, 1); + // select second item + keyDown(container, KeyCode.ENTER); + // mock focus on item 1 to make sure it gets focused + const item1 = getByTestId('1'); + focusSpy = jest.spyOn(item1, 'focus').mockImplementation(() => {}); + menuRef.current.focus(); + expect(focusSpy).toHaveBeenCalled(); - // ================ check click ============== - // click third item - const item2 = getByTestId('2'); - const focusSpy2 = jest.spyOn(item2, 'focus').mockImplementation(() => {}); - fireEvent.click(item2); - menuRef.current.focus(); - // mock focus on item 2 to make sure it gets focused - expect(focusSpy2).toHaveBeenCalled(); - // cleanup - focusSpy.mockRestore(); - focusSpy2.mockRestore(); + // ================ check click ============== + // click third item + const item2 = getByTestId('2'); + focusSpy2 = jest.spyOn(item2, 'focus').mockImplementation(() => {}); + fireEvent.click(item2); + menuRef.current.focus(); + // mock focus on item 2 to make sure it gets focused + expect(focusSpy2).toHaveBeenCalled(); + } finally { + focusSpy?.mockRestore(); + focusSpy2?.mockRestore(); + } }); }); /* eslint-enable */ From 62360b51ffff29f15af68a87897a60bbcc46a03a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E5=8F=B6=E6=96=87=E4=BF=8A?= Date: Thu, 23 Oct 2025 17:28:29 +0800 Subject: [PATCH 6/6] feat: verify whether the selected item is focusable --- src/Menu.tsx | 28 +++++++++++++--------------- tests/Focus.spec.tsx | 25 +++++++++++++++++++++++-- 2 files changed, 36 insertions(+), 17 deletions(-) diff --git a/src/Menu.tsx b/src/Menu.tsx index b0b04bb1..c8206b39 100644 --- a/src/Menu.tsx +++ b/src/Menu.tsx @@ -409,28 +409,26 @@ const Menu = React.forwardRef((props, ref) => { return { list: containerRef.current, focus: options => { + if (!containerRef.current) { + return; + } const keys = getKeys(); const { elements, key2element, element2key } = refreshElements(keys, uuid); const focusableElements = getFocusableElements(containerRef.current, elements); + const focusableKeys = new Set( + focusableElements.map(el => element2key.get(el)).filter(Boolean), + ); const defaultFocusKey = focusableElements[0] ? element2key.get(focusableElements[0]) : childList.find(node => !node.props.disabled)?.key; - let shouldFocusKey: string; - // find the item to focus on based on whether it is selectable - if (selectable) { - // if there is already selected items, select first item to focus - if (mergedSelectKeys.length && keys.includes(mergedSelectKeys[0])) { - shouldFocusKey = mergedSelectKeys[0]; - } else if (mergedActiveKey && keys.includes(mergedActiveKey)) { - shouldFocusKey = mergedActiveKey; - } else { - shouldFocusKey = defaultFocusKey; - } - } else { - shouldFocusKey = defaultFocusKey; - } - const elementToFocus = key2element.get(shouldFocusKey); + const selectedFocusKey = mergedSelectKeys.find(k => focusableKeys.has(k)); + const activeFocusKey = + mergedActiveKey && key2element.has(mergedActiveKey) ? mergedActiveKey : undefined; + const shouldFocusKey = selectable + ? (selectedFocusKey ?? activeFocusKey ?? defaultFocusKey) + : defaultFocusKey; + const elementToFocus = key2element.get(shouldFocusKey); if (shouldFocusKey && elementToFocus) { elementToFocus?.focus?.(options); } diff --git a/tests/Focus.spec.tsx b/tests/Focus.spec.tsx index e2b9cd8b..33defbde 100644 --- a/tests/Focus.spec.tsx +++ b/tests/Focus.spec.tsx @@ -228,7 +228,6 @@ describe('Focus', () => { ); }; const { getByTestId, container } = render(); - // let focusSpy = jest; let focusSpy: jest.SpyInstance | undefined; try { // ================ check keydown ============== @@ -254,7 +253,7 @@ describe('Focus', () => { menuRef.current.focus(); expect(focusSpy).toHaveBeenCalled(); } finally { - focusSpy.mockRestore(); + focusSpy?.mockRestore(); } }); it('When selectable is configured, the focus should move to the selected item if there is a selection, else to the first item, not retain on last focused item', async () => { @@ -309,5 +308,27 @@ describe('Focus', () => { focusSpy2?.mockRestore(); } }); + it('should fallback when selected item is disabled', () => { + const menuRef = React.createRef(); + const items = [ + { key: '1', label: 'Disabled', disabled: true }, + { key: '2', label: 'Active' }, + { key: '3', label: 'Item 3' }, + ]; + const { getByTestId } = render( + + {items.map(item => ( + + {item.label} + + ))} + , + ); + const item2 = getByTestId('2'); + const focusSpy = jest.spyOn(item2, 'focus').mockImplementation(() => {}); + menuRef.current.focus(); + expect(focusSpy).toHaveBeenCalled(); + focusSpy?.mockRestore(); + }); }); /* eslint-enable */