From 154ac1ee3ac5011f4785c3ced9f330b1d8ea2c2d Mon Sep 17 00:00:00 2001 From: Siddharth Kshetrapal Date: Wed, 24 Aug 2022 15:51:49 +0200 Subject: [PATCH 1/8] Re-enable focus trap --- src/ActionMenu.tsx | 1 - src/stories/ActionMenu/fixtures.stories.tsx | 37 ++++++++++++++++++++- 2 files changed, 36 insertions(+), 2 deletions(-) diff --git a/src/ActionMenu.tsx b/src/ActionMenu.tsx index a88513c7e1e..f45ef03c8c0 100644 --- a/src/ActionMenu.tsx +++ b/src/ActionMenu.tsx @@ -126,7 +126,6 @@ const Overlay: React.FC> = ({children, align={align} overlayProps={overlayProps} focusZoneSettings={{focusOutBehavior: 'wrap'}} - focusTrapSettings={{disabled: true}} >
) } + +export const withinFocusZone = () => { + return ( + <> +

+ In a focus zone, you can navigate between elements with ArrowKeys (not Tab), but, this should not interfere with + keyboard navigation for the menu once opened. +

+

There overlap between keyboard navigation of AnchoredOverlay's focusZone and ActionMenu

+
    +
  • Pressing ArrowDown on menu button will not open the menu, but move focus to the next element
    • +
  • + Pressing Tab on an open menu will close the menu and put the focus back on the anchor instead of the next + element (because focus zone uses ArrowKeys for navigation, not Tab) +
    • +
+ } width="medium"> + + + + open menu + + + Item 1 + Item 2 + + + + + + + + ) +} From 070aba6bc8ca76008ed4521d2aada5041bcebe5c Mon Sep 17 00:00:00 2001 From: Siddharth Kshetrapal Date: Wed, 24 Aug 2022 16:36:11 +0200 Subject: [PATCH 2/8] linting --- src/stories/ActionMenu/fixtures.stories.tsx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/stories/ActionMenu/fixtures.stories.tsx b/src/stories/ActionMenu/fixtures.stories.tsx index 17a3ec0c45f..bf655d373d6 100644 --- a/src/stories/ActionMenu/fixtures.stories.tsx +++ b/src/stories/ActionMenu/fixtures.stories.tsx @@ -797,7 +797,7 @@ export const withinFocusZone = () => { In a focus zone, you can navigate between elements with ArrowKeys (not Tab), but, this should not interfere with keyboard navigation for the menu once opened.

-

There overlap between keyboard navigation of AnchoredOverlay's focusZone and ActionMenu

+

There is overlap between the keyboard navigation of AnchoredOverlay and ActionMenu

  • Pressing ArrowDown on menu button will not open the menu, but move focus to the next element
  • From daa134b6738fe8d5fbf8edc7559046628062ce4f Mon Sep 17 00:00:00 2001 From: Siddharth Kshetrapal Date: Wed, 24 Aug 2022 22:40:02 +0200 Subject: [PATCH 3/8] same syntax --- src/stories/ActionMenu/fixtures.stories.tsx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/stories/ActionMenu/fixtures.stories.tsx b/src/stories/ActionMenu/fixtures.stories.tsx index bf655d373d6..2c967c735fc 100644 --- a/src/stories/ActionMenu/fixtures.stories.tsx +++ b/src/stories/ActionMenu/fixtures.stories.tsx @@ -790,7 +790,7 @@ export function TabTest(): JSX.Element { ) } -export const withinFocusZone = () => { +export function withinFocusZone(): JSX.Element { return ( <>

    From 10939773d6392d821a487303f3266a8dad47f9eb Mon Sep 17 00:00:00 2001 From: Siddharth Kshetrapal Date: Thu, 25 Aug 2022 15:11:18 +0200 Subject: [PATCH 4/8] passing tests --- src/stories/ActionMenu/fixtures.stories.tsx | 43 +++++++++++++++------ 1 file changed, 31 insertions(+), 12 deletions(-) diff --git a/src/stories/ActionMenu/fixtures.stories.tsx b/src/stories/ActionMenu/fixtures.stories.tsx index 2c967c735fc..53050d3c93e 100644 --- a/src/stories/ActionMenu/fixtures.stories.tsx +++ b/src/stories/ActionMenu/fixtures.stories.tsx @@ -791,6 +791,8 @@ export function TabTest(): JSX.Element { } export function withinFocusZone(): JSX.Element { + const [overlayOpen, setOverlayOpen] = React.useState(false) + return ( <>

    @@ -805,19 +807,36 @@ export function withinFocusZone(): JSX.Element { element (because focus zone uses ArrowKeys for navigation, not Tab)

- } width="medium"> + } + width="medium" + open={overlayOpen} + onOpen={() => setOverlayOpen(true)} + onClose={() => setOverlayOpen(false)} + > - - - open menu - - - Item 1 - Item 2 - - - - + + First field + + + + Second field + + open menu + + + Item 1 + Item 2 + Item 3 + + + + + + + Third field + + From 8a874dca700789a148b0fc6f275435d5ec095d54 Mon Sep 17 00:00:00 2001 From: Siddharth Kshetrapal Date: Thu, 25 Aug 2022 15:16:06 +0200 Subject: [PATCH 5/8] Add changeset --- .changeset/actionmenu-reenable-focus-trap.md | 5 +++++ src/stories/ActionMenu/fixtures.stories.tsx | 2 +- 2 files changed, 6 insertions(+), 1 deletion(-) create mode 100644 .changeset/actionmenu-reenable-focus-trap.md diff --git a/.changeset/actionmenu-reenable-focus-trap.md b/.changeset/actionmenu-reenable-focus-trap.md new file mode 100644 index 00000000000..de9c2c9656e --- /dev/null +++ b/.changeset/actionmenu-reenable-focus-trap.md @@ -0,0 +1,5 @@ +--- +'@primer/react': patch +--- + +ActionMenu: Re-enable focus trap. It was disabled in v35.6.0 to improve keyboard navigation, that led to a regression for ActionMenu within focus zones (example: AnchoredOverlay) diff --git a/src/stories/ActionMenu/fixtures.stories.tsx b/src/stories/ActionMenu/fixtures.stories.tsx index 53050d3c93e..a2584962914 100644 --- a/src/stories/ActionMenu/fixtures.stories.tsx +++ b/src/stories/ActionMenu/fixtures.stories.tsx @@ -790,7 +790,7 @@ export function TabTest(): JSX.Element { ) } -export function withinFocusZone(): JSX.Element { +export function WithinFocusZone(): JSX.Element { const [overlayOpen, setOverlayOpen] = React.useState(false) return ( From ee78eee4f27a3d333f44b8d09acf05d4fc718364 Mon Sep 17 00:00:00 2001 From: Siddharth Kshetrapal Date: Thu, 25 Aug 2022 16:08:13 +0200 Subject: [PATCH 6/8] Update actionmenu-reenable-focus-trap.md --- .changeset/actionmenu-reenable-focus-trap.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.changeset/actionmenu-reenable-focus-trap.md b/.changeset/actionmenu-reenable-focus-trap.md index de9c2c9656e..fce95af2c83 100644 --- a/.changeset/actionmenu-reenable-focus-trap.md +++ b/.changeset/actionmenu-reenable-focus-trap.md @@ -2,4 +2,4 @@ '@primer/react': patch --- -ActionMenu: Re-enable focus trap. It was disabled in v35.6.0 to improve keyboard navigation, that led to a regression for ActionMenu within focus zones (example: AnchoredOverlay) +ActionMenu: Fix keyboard navigation for ActionMenu inside Overlay by re-enabling focus trap. It was disabled in v35.6.0, that led to a regression for ActionMenu within focus zones (example: AnchoredOverlay) From 75ae43f8fa146327642fc67d0a4eacf606dbf571 Mon Sep 17 00:00:00 2001 From: Siddharth Kshetrapal Date: Mon, 29 Aug 2022 11:39:46 +0200 Subject: [PATCH 7/8] change key bindings to Tabs in story --- src/stories/ActionMenu/fixtures.stories.tsx | 18 ++++++++---------- 1 file changed, 8 insertions(+), 10 deletions(-) diff --git a/src/stories/ActionMenu/fixtures.stories.tsx b/src/stories/ActionMenu/fixtures.stories.tsx index a2584962914..bce85486048 100644 --- a/src/stories/ActionMenu/fixtures.stories.tsx +++ b/src/stories/ActionMenu/fixtures.stories.tsx @@ -36,6 +36,7 @@ import { IconProps, IssueOpenedIcon } from '@primer/octicons-react' +import {FocusKeys} from '@primer/behaviors' const meta: Meta = { title: 'Composite components/ActionMenu/fixtures', @@ -796,18 +797,15 @@ export function WithinFocusZone(): JSX.Element { return ( <>

- In a focus zone, you can navigate between elements with ArrowKeys (not Tab), but, this should not interfere with - keyboard navigation for the menu once opened. + When ActionMenu is used in a form inside an AnchoredOverlay, it is recommended to use key bindings for Tabs (not + ArrowKeys) +

+

+ TODO: Pressing Tab on an open menu should close the menu and put the focus on the next element instead of the + anchor.

-

There is overlap between the keyboard navigation of AnchoredOverlay and ActionMenu

-
    -
  • Pressing ArrowDown on menu button will not open the menu, but move focus to the next element
    • -
  • - Pressing Tab on an open menu will close the menu and put the focus back on the anchor instead of the next - element (because focus zone uses ArrowKeys for navigation, not Tab) -
    • -
} width="medium" open={overlayOpen} From 38b85c425192895a71074f0807bb192a49f679da Mon Sep 17 00:00:00 2001 From: Siddharth Kshetrapal Date: Mon, 29 Aug 2022 12:15:18 +0200 Subject: [PATCH 8/8] change todo to known bug --- src/stories/ActionMenu/fixtures.stories.tsx | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/stories/ActionMenu/fixtures.stories.tsx b/src/stories/ActionMenu/fixtures.stories.tsx index bce85486048..edd54bdb582 100644 --- a/src/stories/ActionMenu/fixtures.stories.tsx +++ b/src/stories/ActionMenu/fixtures.stories.tsx @@ -801,8 +801,8 @@ export function WithinFocusZone(): JSX.Element { ArrowKeys)

- TODO: Pressing Tab on an open menu should close the menu and put the focus on the next element instead of the - anchor. + Known bug: Pressing Tab on an open menu should close the menu and put the focus on the next element instead of + the anchor.