From f83e9f26aa9315fc036afdb8afeff9c35973f3f6 Mon Sep 17 00:00:00 2001 From: Jon Cullison Date: Mon, 28 Jul 2025 16:46:24 -0400 Subject: [PATCH 1/4] fixes issue with keyboard not working after focused menu item is removed --- src/Menu.tsx | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/src/Menu.tsx b/src/Menu.tsx index 6d1cdf36..43593801 100644 --- a/src/Menu.tsx +++ b/src/Menu.tsx @@ -400,10 +400,11 @@ const Menu = React.forwardRef((props, ref) => { const focusableElements = getFocusableElements(containerRef.current, elements); const shouldFocusKey = - mergedActiveKey ?? - (focusableElements[0] - ? element2key.get(focusableElements[0]) - : childList.find(node => !node.props.disabled)?.key); + mergedActiveKey && keys.includes(mergedActiveKey) + ? mergedActiveKey + : focusableElements[0] + ? element2key.get(focusableElements[0]) + : childList.find(node => !node.props.disabled)?.key; const elementToFocus = key2element.get(shouldFocusKey); From ae911bc20a2138378584dd819b2858892b90f63e Mon Sep 17 00:00:00 2001 From: Jon Cullison Date: Tue, 29 Jul 2025 11:21:58 -0400 Subject: [PATCH 2/4] removes nested ternary expression --- src/Menu.tsx | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-) diff --git a/src/Menu.tsx b/src/Menu.tsx index 43593801..9c08850e 100644 --- a/src/Menu.tsx +++ b/src/Menu.tsx @@ -399,13 +399,14 @@ const Menu = React.forwardRef((props, ref) => { const { elements, key2element, element2key } = refreshElements(keys, uuid); const focusableElements = getFocusableElements(containerRef.current, elements); - const shouldFocusKey = - mergedActiveKey && keys.includes(mergedActiveKey) - ? mergedActiveKey - : focusableElements[0] - ? element2key.get(focusableElements[0]) - : childList.find(node => !node.props.disabled)?.key; - + let shouldFocusKey: string; + if (mergedActiveKey && keys.includes(mergedActiveKey)) { + shouldFocusKey = mergedActiveKey; + } else { + shouldFocusKey = focusableElements[0] + ? element2key.get(focusableElements[0]) + : childList.find(node => !node.props.disabled)?.key; + } const elementToFocus = key2element.get(shouldFocusKey); if (shouldFocusKey && elementToFocus) { From c3d3c383adbee0a6904ebd20cfaf8d33d02ce716 Mon Sep 17 00:00:00 2001 From: Jon Cullison Date: Thu, 28 Aug 2025 15:56:24 -0400 Subject: [PATCH 3/4] adds test --- tests/Menu.spec.tsx | 69 +++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 69 insertions(+) diff --git a/tests/Menu.spec.tsx b/tests/Menu.spec.tsx index 0b8248a4..a724f6b5 100644 --- a/tests/Menu.spec.tsx +++ b/tests/Menu.spec.tsx @@ -8,6 +8,7 @@ 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 { spyElementPrototypes } from '@rc-component/util/lib/test/domHook'; jest.mock('@rc-component/trigger', () => { const react = require('react'); @@ -790,5 +791,73 @@ describe('Menu', () => { expect(catItem).toBe(container.querySelectorAll('li')[1]); expect(nonExistentItem).toBe(null); }); + + it('should correctly handle invalid mergedActiveKey and fallback to focusable elements', async () => { + // Mock to force make menu item visible + spyElementPrototypes(HTMLElement, { + offsetParent: { + get() { + return this.parentElement; + }, + }, + }); + + const menuRef = React.createRef(); + const focusSpy = jest.fn(); + + const TestApp = () => { + const [items, setItems] = React.useState([ + { key: 'item1', label: 'First Item' }, + { key: 'item2', label: 'Second Item' }, + { key: 'item3', label: 'Third Item' }, + ]); + + const removeSecondItem = () => { + setItems(prevItems => prevItems.filter(item => item.key !== 'item2')); + }; + + return ( +
+ + {items.map(item => ( + + {item.label} + + ))} + + +
+ ); + }; + + const { getByTestId } = await act(async () => render()); + + const removeButton = getByTestId('remove-button'); + + // Verify initial state - item2 should be active + expect(getByTestId('item2')).toHaveClass('rc-menu-item-active'); + + // Remove the active item (item2) + await act(async () => { + fireEvent.click(removeButton); + }); + + // Verify the item is removed + expect(() => getByTestId('item2')).toThrow(); + + // mock focus on item 1 to make sure it gets focused + const item1 = getByTestId('item1'); + item1.focus = focusSpy; + + // when we call focus(), it should properly handle the case where + // mergedActiveKey ("item2") no longer exists + await act(async () => { + menuRef.current.focus(); + }); + + expect(focusSpy).toHaveBeenCalled(); + }); }); /* eslint-enable */ From dc04152d728d7ef779bbe9ae54a618fd159ea95a Mon Sep 17 00:00:00 2001 From: Jon Cullison Date: Thu, 28 Aug 2025 16:35:53 -0400 Subject: [PATCH 4/4] address code rabbit feedback --- tests/Menu.spec.tsx | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/tests/Menu.spec.tsx b/tests/Menu.spec.tsx index a724f6b5..3ee9bf37 100644 --- a/tests/Menu.spec.tsx +++ b/tests/Menu.spec.tsx @@ -794,7 +794,7 @@ describe('Menu', () => { it('should correctly handle invalid mergedActiveKey and fallback to focusable elements', async () => { // Mock to force make menu item visible - spyElementPrototypes(HTMLElement, { + const domSpy = spyElementPrototypes(HTMLElement, { offsetParent: { get() { return this.parentElement; @@ -803,7 +803,6 @@ describe('Menu', () => { }); const menuRef = React.createRef(); - const focusSpy = jest.fn(); const TestApp = () => { const [items, setItems] = React.useState([ @@ -832,7 +831,7 @@ describe('Menu', () => { ); }; - const { getByTestId } = await act(async () => render()); + const { getByTestId } = render(); const removeButton = getByTestId('remove-button'); @@ -849,15 +848,16 @@ describe('Menu', () => { // mock focus on item 1 to make sure it gets focused const item1 = getByTestId('item1'); - item1.focus = focusSpy; + const focusSpy = jest.spyOn(item1, 'focus').mockImplementation(() => {}); // when we call focus(), it should properly handle the case where // mergedActiveKey ("item2") no longer exists - await act(async () => { - menuRef.current.focus(); - }); + menuRef.current.focus(); expect(focusSpy).toHaveBeenCalled(); + // cleanup + focusSpy.mockRestore(); + domSpy?.mockRestore?.(); }); }); /* eslint-enable */