From 40dc5c4df8cd6db93e35869ec6a8ded87256cd96 Mon Sep 17 00:00:00 2001 From: Constance Chen Date: Thu, 5 May 2022 16:01:51 -0700 Subject: [PATCH] [!!!] Move `tabbable` iteration out of focus logic and into its own method - it shouldn't be tied to the focus call anymore since the focus call no longer occurs after update, and makes more sense as a separate call + updates to logic: - do not run `tabbable` on `children` API since it won't even use the navigation - return early - use `this.backButton` instead for back button focusing, since `children` will no longer have `menuItems` - Check for a valid focusedItem - it's possible for consumers to either pass in bad indices or for `items` to update and the index to no longer exist - Add E2E tests confirming changes & new logic work as expected --- .../context_menu/context_menu_panel.spec.tsx | 53 +++++++++++++++---- .../context_menu/context_menu_panel.tsx | 47 ++++++++++------ 2 files changed, 75 insertions(+), 25 deletions(-) diff --git a/src/components/context_menu/context_menu_panel.spec.tsx b/src/components/context_menu/context_menu_panel.spec.tsx index 1758b91118d..1434c86d8fd 100644 --- a/src/components/context_menu/context_menu_panel.spec.tsx +++ b/src/components/context_menu/context_menu_panel.spec.tsx @@ -42,15 +42,6 @@ describe('EuiContextMenuPanel', () => { cy.focused().should('have.attr', 'class', 'euiContextMenuPanel'); }); - it('sets initial focus from `initialFocusedItemIndex`', () => { - cy.mount( - - {children} - - ); - cy.focused().should('have.attr', 'data-test-subj', 'itemC'); - }); - describe('with `children`', () => { it('ignores arrow key navigation, which only toggles for `items`', () => { cy.mount({children}); @@ -60,6 +51,20 @@ describe('EuiContextMenuPanel', () => { }); describe('with `items`', () => { + it('sets initial focus from `initialFocusedItemIndex`', () => { + cy.mount( + + ); + cy.focused().should('have.attr', 'data-test-subj', 'itemC'); + }); + + it('falls back to the panel if given an invalid `focusedItemIndex`', () => { + cy.mount( + + ); + cy.focused().should('have.attr', 'class', 'euiContextMenuPanel'); + }); + it('focuses and registers any tabbable child as navigable menu items', () => { cy.mount( { cy.realPress('{downarrow}'); cy.focused().should('have.attr', 'data-test-subj', 'itemA'); }); + + it('correctly re-finds navigable menu items if `items` changes', () => { + const DynanicItemsTest = () => { + const [dynamicItems, setDynamicItems] = useState([ + items[0], + items[1], + ]); + const appendItems = () => setDynamicItems(items); + return ( + <> + + + + ); + }; + cy.mount(); + cy.realPress('{downarrow}'); + cy.focused().should('have.attr', 'data-test-subj', 'itemA'); + cy.realPress('{downarrow}'); + cy.focused().should('have.attr', 'data-test-subj', 'itemB'); + cy.realPress('{downarrow}'); + cy.focused().should('have.attr', 'data-test-subj', 'itemA'); + + cy.get('[data-test-subj="appendItems"]').click(); + cy.get('[data-test-subj="itemA"]').click(); + cy.realPress('{uparrow}'); + cy.focused().should('have.attr', 'data-test-subj', 'itemC'); + }); }); describe('with `panels`', () => { diff --git a/src/components/context_menu/context_menu_panel.tsx b/src/components/context_menu/context_menu_panel.tsx index 2bcaedd3653..7fdfbdc5067 100644 --- a/src/components/context_menu/context_menu_panel.tsx +++ b/src/components/context_menu/context_menu_panel.tsx @@ -115,6 +115,19 @@ export class EuiContextMenuPanel extends Component { }; } + // Find all tabbable menu items on both panel init and + // whenever `menuItems` resets when `props.items` changes + findMenuItems = () => { + if (!this.panel) return; + if (!this.props.items?.length) return; // We only need menu items/arrow key navigation for the `items` API + if (this.state.menuItems.length) return; // If we already have menu items, no need to continue + + const tabbableItems = tabbable(this.panel); + if (tabbableItems.length) { + this.setState({ menuItems: tabbableItems }); + } + }; + incrementFocusedItemIndex = (amount: number) => { let nextFocusedItemIndex; @@ -250,26 +263,24 @@ export class EuiContextMenuPanel extends Component { return; } - // If menuItems has been cleared, iterate through and set menuItems from tabbableItems - if (!this.state.menuItems.length && this.panel) { - const tabbableItems = tabbable(this.panel); - if (tabbableItems.length) { - this.setState({ menuItems: tabbableItems }); + // If an item should be focused, focus it (if it exists) + if (this.state.focusedItemIndex != null && this.state.menuItems.length) { + const focusedItem = this.state.menuItems[this.state.focusedItemIndex]; + if (focusedItem) { + focusedItem.focus(); + return this.setState({ tookInitialFocus: true }); } } - if (this.state.menuItems.length) { - // If an item is focused, focus it - if (this.state.focusedItemIndex != null) { - this.state.menuItems[this.state.focusedItemIndex].focus(); - return this.setState({ tookInitialFocus: true }); - } - // Otherwise, if the back button panel title is present, focus it - if (this.props.onClose) { + // Otherwise, if the back button panel title is present, focus it + if (this.backButton) { + // Focus the back button for both `items` and `children` APIs + this.backButton.focus(); + // If `items`, ensure our focused item index is correct + if (this.state.menuItems.length) { this.setState({ focusedItemIndex: 0 }); - this.state.menuItems[0].focus(); - return this.setState({ tookInitialFocus: true }); } + return this.setState({ tookInitialFocus: true }); } // Focus on the panel as a last resort. @@ -291,7 +302,10 @@ export class EuiContextMenuPanel extends Component { } }; - componentDidUpdate() { + componentDidUpdate(_: Props, prevState: State) { + if (prevState.menuItems !== this.state.menuItems) { + this.findMenuItems(); + } // Focus isn't always ready to be taken on mount, so we need to call it // on update as well just in case this.takeInitialFocus(); @@ -387,6 +401,7 @@ export class EuiContextMenuPanel extends Component { this.updateHeight(); this.getInitialPopoverParent(); + this.findMenuItems(); }; render() {