From 7a132fde8bb2894e2875b584696d0b59421d44b4 Mon Sep 17 00:00:00 2001 From: Lingfan Gao Date: Wed, 4 Jan 2023 16:28:27 +0100 Subject: [PATCH 1/5] fix: Tab should focus thewindow if trigger is the last focusable element Removes the explicit tab handling in the Menu. We know that React effects are sync so we always focus the menu trigger when the menu is closed. Since the browser only performs the default behaviour for Tab once events have fully bubbled up the window, the browser will move focus to the next tabbable element before/after the trigger if needed. If tab was not pressed, then we simply focus the trigger as expected. --- .../src/components/Menu/Menu.cy.tsx | 22 +++++++++++ .../src/components/Menu/useMenu.tsx | 38 +++++-------------- .../components/MenuPopover/useMenuPopover.ts | 1 - 3 files changed, 31 insertions(+), 30 deletions(-) diff --git a/packages/react-components/react-menu/src/components/Menu/Menu.cy.tsx b/packages/react-components/react-menu/src/components/Menu/Menu.cy.tsx index 916adec9696e6..a801a9e5a93d5 100644 --- a/packages/react-components/react-menu/src/components/Menu/Menu.cy.tsx +++ b/packages/react-components/react-menu/src/components/Menu/Menu.cy.tsx @@ -931,9 +931,31 @@ describe(`Nested Menus`, () => { .get(menuSelector) .eq(1) .realPress('Tab'); + cy.contains('After').should('be.focused').get(menuSelector).should('not.exist'); }); + it('should move focus out of document if menu trigger is the last focusable element in DOM', () => { + mount( + <> + + , + ); + + cy.get(menuTriggerSelector) + .click() + .get(menuSelector) + .within(() => { + cy.get(menuItemSelector).eq(4).type('{rightarrow}'); + }) + .get(menuSelector) + .eq(1) + .realPress('Tab'); + + cy.realPress(['Shift', 'Tab']); + cy.get(menuTriggerSelector).should('be.focused'); + }); + it('should be able to shift tab to previous element after the root trigger', () => { mount( <> diff --git a/packages/react-components/react-menu/src/components/Menu/useMenu.tsx b/packages/react-components/react-menu/src/components/Menu/useMenu.tsx index 542bdaca534d1..9d223cb348496 100644 --- a/packages/react-components/react-menu/src/components/Menu/useMenu.tsx +++ b/packages/react-components/react-menu/src/components/Menu/useMenu.tsx @@ -14,7 +14,6 @@ import { useMenuContext_unstable } from '../../contexts/menuContext'; import { MENU_ENTER_EVENT, useOnMenuMouseEnter } from '../../utils/index'; import { useIsSubmenu } from '../../utils/useIsSubmenu'; import type { MenuOpenChangeData, MenuOpenEvent, MenuProps, MenuState } from './Menu.types'; -import { Tab } from '@fluentui/keyboard-keys'; /** * Create the state required to render Menu. @@ -159,8 +158,6 @@ const useMenuOpenState = ( const onOpenChange: MenuProps['onOpenChange'] = useEventCallback((e, data) => state.onOpenChange?.(e, data)); const shouldHandleCloseRef = React.useRef(false); - const shouldHandleTabRef = React.useRef(false); - const pressedShiftRef = React.useRef(false); const setOpenTimeout = React.useRef(0); const enteringTriggerRef = React.useRef(false); @@ -182,13 +179,6 @@ const useMenuOpenState = ( shouldHandleCloseRef.current = true; } - if (e.type === 'keydown') { - if ((e as React.KeyboardEvent).key === Tab) { - shouldHandleTabRef.current = true; - pressedShiftRef.current = (e as React.KeyboardEvent).shiftKey; - } - } - if (data.bubble) { parentSetOpen(e, { ...data }); } @@ -261,39 +251,29 @@ const useMenuOpenState = ( }, []); // Manage focus for open state - const { findFirstFocusable, findNextFocusable, findPrevFocusable } = useFocusFinders(); + const { findFirstFocusable } = useFocusFinders(); const focusFirst = React.useCallback(() => { const firstFocusable = findFirstFocusable(state.menuPopoverRef.current); firstFocusable?.focus(); }, [findFirstFocusable, state.menuPopoverRef]); - const focusAfterMenuTrigger = React.useCallback(() => { - const nextFocusable = findNextFocusable(state.triggerRef.current); - nextFocusable?.focus(); - }, [findNextFocusable, state.triggerRef]); - - const focusBeforeMenuTrigger = React.useCallback(() => { - const prevFocusable = findPrevFocusable(state.triggerRef.current); - prevFocusable?.focus(); - }, [findPrevFocusable, state.triggerRef]); - React.useEffect(() => { if (open) { focusFirst(); } else { if (shouldHandleCloseRef.current) { - if (shouldHandleTabRef.current && !state.isSubmenu) { - pressedShiftRef.current ? focusBeforeMenuTrigger() : focusAfterMenuTrigger(); - } else { - state.triggerRef.current?.focus(); - } + // We know that React effects are sync so we focus the trigger here + // after any event handler (event handlers will call setState). + // Since the browser only performs the default behaviour for Tab once + // events have fully bubbled up the window, the browser will move + // focus to the next tabbable element before/after the trigger if needed. + // If tab was not pressed, then we simply focus the trigger as expected. + state.triggerRef.current?.focus(); } } shouldHandleCloseRef.current = false; - shouldHandleTabRef.current = false; - pressedShiftRef.current = false; - }, [state.triggerRef, state.isSubmenu, open, focusFirst, focusAfterMenuTrigger, focusBeforeMenuTrigger]); + }, [state.triggerRef, state.isSubmenu, open, focusFirst]); return [open, setOpen] as const; }; diff --git a/packages/react-components/react-menu/src/components/MenuPopover/useMenuPopover.ts b/packages/react-components/react-menu/src/components/MenuPopover/useMenuPopover.ts index 6f8b02bc52ebd..dbc2a5ab1ebbe 100644 --- a/packages/react-components/react-menu/src/components/MenuPopover/useMenuPopover.ts +++ b/packages/react-components/react-menu/src/components/MenuPopover/useMenuPopover.ts @@ -85,7 +85,6 @@ export const useMenuPopover_unstable = (props: MenuPopoverProps, ref: React.Ref< if (key === Tab) { setOpen(event, { open: false, keyboard: true, type: 'menuPopoverKeyDown', event }); - event.preventDefault(); } onKeyDownOriginal?.(event); From 43662ae91204fbd005ad086123f9120665ac44b4 Mon Sep 17 00:00:00 2001 From: Lingfan Gao Date: Wed, 4 Jan 2023 16:32:34 +0100 Subject: [PATCH 2/5] changefile --- ...ui-react-menu-9787aa9c-cdf8-41b8-a9d6-c8d7e1f3c502.json | 7 +++++++ 1 file changed, 7 insertions(+) create mode 100644 change/@fluentui-react-menu-9787aa9c-cdf8-41b8-a9d6-c8d7e1f3c502.json diff --git a/change/@fluentui-react-menu-9787aa9c-cdf8-41b8-a9d6-c8d7e1f3c502.json b/change/@fluentui-react-menu-9787aa9c-cdf8-41b8-a9d6-c8d7e1f3c502.json new file mode 100644 index 0000000000000..464d83d37b7e8 --- /dev/null +++ b/change/@fluentui-react-menu-9787aa9c-cdf8-41b8-a9d6-c8d7e1f3c502.json @@ -0,0 +1,7 @@ +{ + "type": "patch", + "comment": "fix: Tab should focus thewindow if trigger is the last focusable element", + "packageName": "@fluentui/react-menu", + "email": "lingfangao@hotmail.com", + "dependentChangeType": "patch" +} From 41f19c3d044d454793196789690124c9ebf268b6 Mon Sep 17 00:00:00 2001 From: ling1726 Date: Thu, 5 Jan 2023 11:51:11 +0100 Subject: [PATCH 3/5] Update change/@fluentui-react-menu-9787aa9c-cdf8-41b8-a9d6-c8d7e1f3c502.json Co-authored-by: Oleksandr Fediashov --- ...luentui-react-menu-9787aa9c-cdf8-41b8-a9d6-c8d7e1f3c502.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/change/@fluentui-react-menu-9787aa9c-cdf8-41b8-a9d6-c8d7e1f3c502.json b/change/@fluentui-react-menu-9787aa9c-cdf8-41b8-a9d6-c8d7e1f3c502.json index 464d83d37b7e8..bd32cff23f6a1 100644 --- a/change/@fluentui-react-menu-9787aa9c-cdf8-41b8-a9d6-c8d7e1f3c502.json +++ b/change/@fluentui-react-menu-9787aa9c-cdf8-41b8-a9d6-c8d7e1f3c502.json @@ -1,6 +1,6 @@ { "type": "patch", - "comment": "fix: Tab should focus thewindow if trigger is the last focusable element", + "comment": "fix: Tab should focus the window if trigger is the last focusable element", "packageName": "@fluentui/react-menu", "email": "lingfangao@hotmail.com", "dependentChangeType": "patch" From 82b53f68ac7fb15aaa70dc4a2805a328f0886083 Mon Sep 17 00:00:00 2001 From: ling1726 Date: Thu, 5 Jan 2023 11:51:15 +0100 Subject: [PATCH 4/5] Update packages/react-components/react-menu/src/components/Menu/Menu.cy.tsx Co-authored-by: Oleksandr Fediashov --- .../react-menu/src/components/Menu/Menu.cy.tsx | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/packages/react-components/react-menu/src/components/Menu/Menu.cy.tsx b/packages/react-components/react-menu/src/components/Menu/Menu.cy.tsx index a801a9e5a93d5..4f438e161c5cb 100644 --- a/packages/react-components/react-menu/src/components/Menu/Menu.cy.tsx +++ b/packages/react-components/react-menu/src/components/Menu/Menu.cy.tsx @@ -937,9 +937,7 @@ describe(`Nested Menus`, () => { it('should move focus out of document if menu trigger is the last focusable element in DOM', () => { mount( - <> - - , + , ); cy.get(menuTriggerSelector) From 0fe939d6d41d8a62734b54bf2a2b08e998ec275b Mon Sep 17 00:00:00 2001 From: ling1726 Date: Thu, 5 Jan 2023 13:53:12 +0100 Subject: [PATCH 5/5] Update packages/react-components/react-menu/src/components/Menu/useMenu.tsx --- .../react-menu/src/components/Menu/useMenu.tsx | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/packages/react-components/react-menu/src/components/Menu/useMenu.tsx b/packages/react-components/react-menu/src/components/Menu/useMenu.tsx index 9d223cb348496..a8492c9de0d05 100644 --- a/packages/react-components/react-menu/src/components/Menu/useMenu.tsx +++ b/packages/react-components/react-menu/src/components/Menu/useMenu.tsx @@ -263,11 +263,11 @@ const useMenuOpenState = ( } else { if (shouldHandleCloseRef.current) { // We know that React effects are sync so we focus the trigger here - // after any event handler (event handlers will call setState). - // Since the browser only performs the default behaviour for Tab once - // events have fully bubbled up the window, the browser will move + // after any event handler (event handlers will update state and re-render). + // Since the browser only performs the default behaviour for the Tab key once + // keyboard events have fully bubbled up the window, the browser will move // focus to the next tabbable element before/after the trigger if needed. - // If tab was not pressed, then we simply focus the trigger as expected. + // If the Tab key was not pressed, focus will remain on the trigger as expected. state.triggerRef.current?.focus(); } }