Close Menu component when using tab key (#1673)

* menu should not trap focus for tab key

* introduce `focusFrom` focus management utility

This is internal API, and the actual API is not 100% ideal. I started
refactoring this in a separate branch but it got out of hand and touches
a bit more pieces of the codebase that aren't related to this PR at all.

The idea of this function is just so that we can go Next/Previous but
from the given element not from the document.activeElement. This is
important for this feature. We also bolted this ontop of the existing
code which now means that we have this API:

```js
focusIn([], Focus.Previouw, true, DOMNode)
```

Luckily it's internal API only!

* ensure closing via Tab works as expected

Just closing the Menu isn't 100% enough. If we do this, it means that
when the Menu is open, we press shift+tab, then we go to the
Menu.Button because the Menu.Items were the active element.

The other way is also incorrect because it can happen if you have an
`<a>` element as one of the Menu.Item elements then that `<a>` will
receive focus, then the `Menu` will close unmounting the focused `<a>`
and now that element is gone resulting in `document.body` being the
active element.

To fix this, we will make sure that we consider the `Menu` as 1 coherent
component. This means that using `<Tab>` will now go to the next element
after the `<Menu.Button>` once the Menu is closed.

Shift+Tab will go to the element before the `<Menu.Button>` even though
you are currently focused on the `Menu.Items` so depending on the timing
you go to the `Menu.Button` or not.

Considering the Menu as a single component it makes more sense use the
elements before / after the `Menu`

* update changelog

Co-authored-by: Enoch Riese <enoch.riese@gmail.com>

packages/@headlessui-react/CHANGELOG.md

@@ -18,6 +18,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 - Don’t close dialog when opened during mouse up event ([#1667](https://github.com/tailwindlabs/headlessui/pull/1667))
 - Don’t close dialog when drag ends outside dialog ([#1667](https://github.com/tailwindlabs/headlessui/pull/1667))
 - Fix outside clicks to close dialog when nested, unopened dialogs are present ([#1667](https://github.com/tailwindlabs/headlessui/pull/1667))
+- Close `Menu` component when using `tab` key ([#1673](https://github.com/tailwindlabs/headlessui/pull/1673))
 
 ## [1.6.6] - 2022-07-07
 

packages/@headlessui-react/src/components/menu/menu.test.tsx

@@ -1359,7 +1359,7 @@ describe('Keyboard interactions', () => {
 
   describe('`Tab` key', () => {
     it(
-      'should focus trap when we use Tab',
+      'should close when we use Tab',
       suppressConsoleLogs(async () => {
         render(
           <Menu>
@@ -1401,9 +1401,9 @@ describe('Keyboard interactions', () => {
         // Try to tab
         await press(Keys.Tab)
 
-        // Verify it is still open
-        assertMenuButton({ state: MenuState.Visible })
-        assertMenu({ state: MenuState.Visible })
+        // Verify it is closed
+        assertMenuButton({ state: MenuState.InvisibleUnmounted })
+        assertMenu({ state: MenuState.InvisibleUnmounted })
       })
     )
 
@@ -1450,9 +1450,9 @@ describe('Keyboard interactions', () => {
         // Try to Shift+Tab
         await press(shift(Keys.Tab))
 
-        // Verify it is still open
-        assertMenuButton({ state: MenuState.Visible })
-        assertMenu({ state: MenuState.Visible })
+        // Verify it is closed
+        assertMenuButton({ state: MenuState.InvisibleUnmounted })
+        assertMenu({ state: MenuState.InvisibleUnmounted })
       })
     )
   })

packages/@headlessui-react/src/components/menu/menu.tsx

@@ -29,7 +29,13 @@ import { useId } from '../../hooks/use-id'
 import { Keys } from '../keyboard'
 import { Focus, calculateActiveIndex } from '../../utils/calculate-active-index'
 import { isDisabledReactIssue7711 } from '../../utils/bugs'
-import { isFocusableElement, FocusableMode, sortByDomNode } from '../../utils/focus-management'
+import {
+  isFocusableElement,
+  FocusableMode,
+  sortByDomNode,
+  Focus as FocusManagementFocus,
+  focusFrom,
+} from '../../utils/focus-management'
 import { useOutsideClick } from '../../hooks/use-outside-click'
 import { useTreeWalker } from '../../hooks/use-tree-walker'
 import { useOpenClosed, State, OpenClosedProvider } from '../../internal/open-closed'
@@ -492,6 +498,13 @@ let Items = forwardRefWithAs(function Items<TTag extends ElementType = typeof DE
       case Keys.Tab:
         event.preventDefault()
         event.stopPropagation()
+        dispatch({ type: ActionTypes.CloseMenu })
+        disposables().nextFrame(() => {
+          focusFrom(
+            state.buttonRef.current!,
+            event.shiftKey ? FocusManagementFocus.Previous : FocusManagementFocus.Next
+          )
+        })
         break
 
       default:

packages/@headlessui-react/src/utils/focus-management.ts

@@ -129,7 +129,16 @@ export function sortByDomNode<T>(
   })
 }
 
-export function focusIn(container: HTMLElement | HTMLElement[], focus: Focus, sorted = true) {
+export function focusFrom(current: HTMLElement | null, focus: Focus) {
+  return focusIn(getFocusableElements(), focus, true, current)
+}
+
+export function focusIn(
+  container: HTMLElement | HTMLElement[],
+  focus: Focus,
+  sorted = true,
+  active: HTMLElement | null = null
+) {
   let ownerDocument = Array.isArray(container)
     ? container.length > 0
       ? container[0].ownerDocument
@@ -141,7 +150,7 @@ export function focusIn(container: HTMLElement | HTMLElement[], focus: Focus, so
       ? sortByDomNode(container)
       : container
     : getFocusableElements(container)
-  let active = ownerDocument.activeElement as HTMLElement
+  active = active ?? (ownerDocument.activeElement as HTMLElement)
 
   let direction = (() => {
     if (focus & (Focus.First | Focus.Next)) return Direction.Next

packages/@headlessui-vue/CHANGELOG.md

@@ -18,6 +18,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 - Don’t close dialog when opened during mouse up event ([#1667](https://github.com/tailwindlabs/headlessui/pull/1667))
 - Don’t close dialog when drag ends outside dialog ([#1667](https://github.com/tailwindlabs/headlessui/pull/1667))
 - Fix outside clicks to close dialog when nested, unopened dialogs are present ([#1667](https://github.com/tailwindlabs/headlessui/pull/1667))
+- Close `Menu` component when using `tab` key ([#1673](https://github.com/tailwindlabs/headlessui/pull/1673))
 
 ## [1.6.7] - 2022-07-12
 

packages/@headlessui-vue/src/components/menu/menu.test.tsx

@@ -1656,7 +1656,7 @@ describe('Keyboard interactions', () => {
   })
 
   describe('`Tab` key', () => {
-    it('should focus trap when we use Tab', async () => {
+    it('should not focus trap when we use Tab', async () => {
       renderTemplate(jsx`
         <Menu>
           <MenuButton>Trigger</MenuButton>
@@ -1697,12 +1697,12 @@ describe('Keyboard interactions', () => {
       // Try to tab
       await press(Keys.Tab)
 
-      // Verify it is still open
-      assertMenuButton({ state: MenuState.Visible })
-      assertMenu({ state: MenuState.Visible })
+      // Verify it is closed
+      assertMenuButton({ state: MenuState.InvisibleUnmounted })
+      assertMenu({ state: MenuState.InvisibleUnmounted })
     })
 
-    it('should focus trap when we use Shift+Tab', async () => {
+    it('should not focus trap when we use Shift+Tab', async () => {
       renderTemplate(jsx`
         <Menu>
           <MenuButton>Trigger</MenuButton>
@@ -1743,9 +1743,9 @@ describe('Keyboard interactions', () => {
       // Try to Shift+Tab
       await press(shift(Keys.Tab))
 
-      // Verify it is still open
-      assertMenuButton({ state: MenuState.Visible })
-      assertMenu({ state: MenuState.Visible })
+      // Verify it is closed
+      assertMenuButton({ state: MenuState.InvisibleUnmounted })
+      assertMenu({ state: MenuState.InvisibleUnmounted })
     })
   })
 

packages/@headlessui-vue/src/components/menu/menu.ts

@@ -22,7 +22,13 @@ import { useTreeWalker } from '../../hooks/use-tree-walker'
 import { useOpenClosedProvider, State, useOpenClosed } from '../../internal/open-closed'
 import { match } from '../../utils/match'
 import { useResolveButtonType } from '../../hooks/use-resolve-button-type'
-import { FocusableMode, isFocusableElement, sortByDomNode } from '../../utils/focus-management'
+import {
+  FocusableMode,
+  isFocusableElement,
+  sortByDomNode,
+  Focus as FocusManagementFocus,
+  focusFrom,
+} from '../../utils/focus-management'
 import { useOutsideClick } from '../../hooks/use-outside-click'
 
 enum MenuStates {
@@ -413,6 +419,13 @@ export let MenuItems = defineComponent({
         case Keys.Tab:
           event.preventDefault()
           event.stopPropagation()
+          api.closeMenu()
+          nextTick(() =>
+            focusFrom(
+              dom(api.buttonRef),
+              event.shiftKey ? FocusManagementFocus.Previous : FocusManagementFocus.Next
+            )
+          )
           break
 
         default:

packages/@headlessui-vue/src/utils/focus-management.ts

@@ -122,7 +122,16 @@ export function sortByDomNode<T>(
   })
 }
 
-export function focusIn(container: HTMLElement | HTMLElement[], focus: Focus, sorted = true) {
+export function focusFrom(current: HTMLElement | null, focus: Focus) {
+  return focusIn(getFocusableElements(), focus, true, current)
+}
+
+export function focusIn(
+  container: HTMLElement | HTMLElement[],
+  focus: Focus,
+  sorted = true,
+  active: HTMLElement | null = null
+) {
   let ownerDocument =
     (Array.isArray(container)
       ? container.length > 0
@@ -135,7 +144,7 @@ export function focusIn(container: HTMLElement | HTMLElement[], focus: Focus, so
       ? sortByDomNode(container)
       : container
     : getFocusableElements(container)
-  let active = ownerDocument.activeElement as HTMLElement
+  active = active ?? (ownerDocument.activeElement as HTMLElement)
 
   let direction = (() => {
     if (focus & (Focus.First | Focus.Next)) return Direction.Next
@@ -180,15 +189,6 @@ export function focusIn(container: HTMLElement | HTMLElement[], focus: Focus, so
     offset += direction
   } while (next !== ownerDocument.activeElement)
 
-  // This is a little weird, but let me try and explain: There are a few scenario's
-  // in chrome for example where a focused `<a>` tag does not get the default focus
-  // styles and sometimes they do. This highly depends on whether you started by
-  // clicking or by using your keyboard. When you programmatically add focus `anchor.focus()`
-  // then the active element (document.activeElement) is this anchor, which is expected.
-  // However in that case the default focus styles are not applied *unless* you
-  // also add this tabindex.
-  if (!next.hasAttribute('tabindex')) next.setAttribute('tabindex', '0')
-
   // By default if you <Tab> to a text input or a textarea, the browser will
   // select all the text once the focus is inside these DOM Nodes. However,
   // since we are manually moving focus this behaviour is not happening. This
@@ -201,5 +201,14 @@ export function focusIn(container: HTMLElement | HTMLElement[], focus: Focus, so
     next.select()
   }
 
+  // This is a little weird, but let me try and explain: There are a few scenario's
+  // in chrome for example where a focused `<a>` tag does not get the default focus
+  // styles and sometimes they do. This highly depends on whether you started by
+  // clicking or by using your keyboard. When you programmatically add focus `anchor.focus()`
+  // then the active element (document.activeElement) is this anchor, which is expected.
+  // However in that case the default focus styles are not applied *unless* you
+  // also add this tabindex.
+  if (!next.hasAttribute('tabindex')) next.setAttribute('tabindex', '0')
+
   return FocusResult.Success
 }