re-work focus restoration

When we click outside of the Menu or Listbox, we want to restore the focus to the Button, *unless* we clicked on/in an element that is focusable in itself. For example, when the Menu is open and you click in an input field, the input field should stay focused. We should also close the Menu itself at this point.
tailwindlabs · Oct 20, 2020 · fd145d2 · fd145d2
1 parent 19f5542
commit fd145d2
Show file tree

Hide file tree

Showing 8 changed files with 51 additions and 186 deletions.
diff --git a/packages/@headlessui-react/src/components/listbox/listbox.test.tsx b/packages/@headlessui-react/src/components/listbox/listbox.test.tsx
@@ -210,7 +210,7 @@ describe('Rendering', () => {
         assertListboxButton({
           state: ListboxState.InvisibleUnmounted,
           attributes: { id: 'headlessui-listbox-button-1' },
-          textContent: JSON.stringify({ open: false, focused: false }),
+          textContent: JSON.stringify({ open: false }),
         })
         assertListbox({ state: ListboxState.InvisibleUnmounted })
 
@@ -219,7 +219,7 @@ describe('Rendering', () => {
         assertListboxButton({
           state: ListboxState.Visible,
           attributes: { id: 'headlessui-listbox-button-1' },
-          textContent: JSON.stringify({ open: true, focused: false }),
+          textContent: JSON.stringify({ open: true }),
         })
         assertListbox({ state: ListboxState.Visible })
       })
@@ -244,7 +244,7 @@ describe('Rendering', () => {
         assertListboxButton({
           state: ListboxState.InvisibleUnmounted,
           attributes: { id: 'headlessui-listbox-button-1' },
-          textContent: JSON.stringify({ open: false, focused: false }),
+          textContent: JSON.stringify({ open: false }),
         })
         assertListbox({ state: ListboxState.InvisibleUnmounted })
 
@@ -253,7 +253,7 @@ describe('Rendering', () => {
         assertListboxButton({
           state: ListboxState.Visible,
           attributes: { id: 'headlessui-listbox-button-1' },
-          textContent: JSON.stringify({ open: true, focused: false }),
+          textContent: JSON.stringify({ open: true }),
         })
         assertListbox({ state: ListboxState.Visible })
       })
@@ -2896,31 +2896,6 @@ describe('Mouse interactions', () => {
     })
   )
 
-  it('should focus the listbox when you try to focus the button again (when the listbox is already open)', async () => {
-    render(
-      <Listbox value={undefined} onChange={console.log}>
-        <Listbox.Button>Trigger</Listbox.Button>
-        <Listbox.Options>
-          <Listbox.Option value="a">Option A</Listbox.Option>
-          <Listbox.Option value="b">Option B</Listbox.Option>
-          <Listbox.Option value="c">Option C</Listbox.Option>
-        </Listbox.Options>
-      </Listbox>
-    )
-
-    // Open listbox
-    await click(getListboxButton())
-
-    // Verify listbox is focused
-    assertActiveElement(getListbox())
-
-    // Try to Re-focus the button
-    getListboxButton()?.focus()
-
-    // Verify listbox is still focused
-    assertActiveElement(getListbox())
-  })
-
   it(
     'should be a no-op when we click outside of a closed listbox',
     suppressConsoleLogs(async () => {

diff --git a/packages/@headlessui-react/src/components/listbox/listbox.tsx b/packages/@headlessui-react/src/components/listbox/listbox.tsx
@@ -169,11 +169,14 @@ export function Listbox<
   React.useEffect(() => {
     function handler(event: MouseEvent) {
       const target = event.target as HTMLElement
+      const active = document.activeElement
+
       if (listboxState !== ListboxStates.Open) return
       if (buttonRef.current?.contains(target)) return
 
       if (!optionsRef.current?.contains(target)) dispatch({ type: ActionTypes.CloseListbox })
-      if (!event.defaultPrevented) d.nextFrame(() => buttonRef.current?.focus())
+      if (active !== document.body && active?.contains(target)) return // Keep focus on newly clicked/focused element
+      if (!event.defaultPrevented) buttonRef.current?.focus()
     }
 
     window.addEventListener('click', handler)
@@ -195,7 +198,7 @@ export function Listbox<
 // ---
 
 const DEFAULT_BUTTON_TAG = 'button'
-type ButtonRenderPropArg = { open: boolean; focused: boolean }
+type ButtonRenderPropArg = { open: boolean }
 type ButtonPropsWeControl =
   | 'ref'
   | 'id'
@@ -205,8 +208,6 @@ type ButtonPropsWeControl =
   | 'aria-expanded'
   | 'aria-labelledby'
   | 'onKeyDown'
-  | 'onFocus'
-  | 'onBlur'
   | 'onPointerUp'
 
 const Button = forwardRefWithAs(function Button<
@@ -217,7 +218,6 @@ const Button = forwardRefWithAs(function Button<
 ) {
   const [state, dispatch] = useListboxContext([Listbox.name, Button.name].join('.'))
   const buttonRef = useSyncRefs(state.buttonRef, ref)
-  const [focused, setFocused] = React.useState(false)
 
   const id = `headlessui-listbox-button-${useId()}`
   const d = useDisposables()
@@ -268,20 +268,14 @@ const Button = forwardRefWithAs(function Button<
     [dispatch, d, state, props.disabled]
   )
 
-  const handleFocus = React.useCallback(() => {
-    if (state.listboxState === ListboxStates.Open) return state.optionsRef.current?.focus()
-    setFocused(true)
-  }, [state, setFocused])
-
-  const handleBlur = React.useCallback(() => setFocused(false), [setFocused])
   const labelledby = useComputed(() => {
     if (!state.labelRef.current) return undefined
     return [state.labelRef.current.id, id].join(' ')
   }, [state.labelRef.current, id])
 
   const propsBag = React.useMemo<ButtonRenderPropArg>(
-    () => ({ open: state.listboxState === ListboxStates.Open, focused }),
-    [state, focused]
+    () => ({ open: state.listboxState === ListboxStates.Open }),
+    [state]
   )
   const passthroughProps = props
   const propsWeControl = {
@@ -293,8 +287,6 @@ const Button = forwardRefWithAs(function Button<
     'aria-expanded': state.listboxState === ListboxStates.Open ? true : undefined,
     'aria-labelledby': labelledby,
     onKeyDown: handleKeyDown,
-    onFocus: handleFocus,
-    onBlur: handleBlur,
     onPointerUp: handlePointerUp,
   }
 

diff --git a/packages/@headlessui-react/src/components/menu/menu.test.tsx b/packages/@headlessui-react/src/components/menu/menu.test.tsx
@@ -133,7 +133,7 @@ describe('Rendering', () => {
         assertMenuButton({
           state: MenuState.InvisibleUnmounted,
           attributes: { id: 'headlessui-menu-button-1' },
-          textContent: JSON.stringify({ open: false, focused: false }),
+          textContent: JSON.stringify({ open: false }),
         })
         assertMenu({ state: MenuState.InvisibleUnmounted })
 
@@ -142,7 +142,7 @@ describe('Rendering', () => {
         assertMenuButton({
           state: MenuState.Visible,
           attributes: { id: 'headlessui-menu-button-1' },
-          textContent: JSON.stringify({ open: true, focused: false }),
+          textContent: JSON.stringify({ open: true }),
         })
         assertMenu({ state: MenuState.Visible })
       })
@@ -167,7 +167,7 @@ describe('Rendering', () => {
         assertMenuButton({
           state: MenuState.InvisibleUnmounted,
           attributes: { id: 'headlessui-menu-button-1' },
-          textContent: JSON.stringify({ open: false, focused: false }),
+          textContent: JSON.stringify({ open: false }),
         })
         assertMenu({ state: MenuState.InvisibleUnmounted })
 
@@ -176,7 +176,7 @@ describe('Rendering', () => {
         assertMenuButton({
           state: MenuState.Visible,
           attributes: { id: 'headlessui-menu-button-1' },
-          textContent: JSON.stringify({ open: true, focused: false }),
+          textContent: JSON.stringify({ open: true }),
         })
         assertMenu({ state: MenuState.Visible })
       })
@@ -2464,31 +2464,6 @@ describe('Mouse interactions', () => {
     })
   )
 
-  it('should focus the menu when you try to focus the button again (when the menu is already open)', async () => {
-    render(
-      <Menu>
-        <Menu.Button>Trigger</Menu.Button>
-        <Menu.Items>
-          <Menu.Item as="a">Item A</Menu.Item>
-          <Menu.Item as="a">Item B</Menu.Item>
-          <Menu.Item as="a">Item C</Menu.Item>
-        </Menu.Items>
-      </Menu>
-    )
-
-    // Open menu
-    await click(getMenuButton())
-
-    // Verify menu is focused
-    assertActiveElement(getMenu())
-
-    // Try to Re-focus the button
-    getMenuButton()?.focus()
-
-    // Verify menu is still focused
-    assertActiveElement(getMenu())
-  })
-
   it(
     'should be a no-op when we click outside of a closed menu',
     suppressConsoleLogs(async () => {

diff --git a/packages/@headlessui-react/src/components/menu/menu.tsx b/packages/@headlessui-react/src/components/menu/menu.tsx
@@ -151,11 +151,14 @@ export function Menu<TTag extends React.ElementType = typeof DEFAULT_MENU_TAG>(
   React.useEffect(() => {
     function handler(event: MouseEvent) {
       const target = event.target as HTMLElement
+      const active = document.activeElement
+
       if (menuState !== MenuStates.Open) return
       if (buttonRef.current?.contains(target)) return
 
       if (!itemsRef.current?.contains(target)) dispatch({ type: ActionTypes.CloseMenu })
-      if (!event.defaultPrevented) d.nextFrame(() => buttonRef.current?.focus())
+      if (active !== document.body && active?.contains(target)) return // Keep focus on newly clicked/focused element
+      if (!event.defaultPrevented) buttonRef.current?.focus()
     }
 
     window.addEventListener('click', handler)
@@ -174,7 +177,7 @@ export function Menu<TTag extends React.ElementType = typeof DEFAULT_MENU_TAG>(
 // ---
 
 const DEFAULT_BUTTON_TAG = 'button'
-type ButtonRenderPropArg = { open: boolean; focused: boolean }
+type ButtonRenderPropArg = { open: boolean }
 type ButtonPropsWeControl =
   | 'ref'
   | 'id'
@@ -183,8 +186,6 @@ type ButtonPropsWeControl =
   | 'aria-controls'
   | 'aria-expanded'
   | 'onKeyDown'
-  | 'onFocus'
-  | 'onBlur'
   | 'onPointerUp'
 
 const Button = forwardRefWithAs(function Button<
@@ -195,7 +196,6 @@ const Button = forwardRefWithAs(function Button<
 ) {
   const [state, dispatch] = useMenuContext([Menu.name, Button.name].join('.'))
   const buttonRef = useSyncRefs(state.buttonRef, ref)
-  const [focused, setFocused] = React.useState(false)
 
   const id = `headlessui-menu-button-${useId()}`
   const d = useDisposables()
@@ -244,17 +244,7 @@ const Button = forwardRefWithAs(function Button<
     [dispatch, d, state, props.disabled]
   )
 
-  const handleFocus = React.useCallback(() => {
-    if (state.menuState === MenuStates.Open) state.itemsRef.current?.focus()
-    setFocused(true)
-  }, [state, setFocused])
-
-  const handleBlur = React.useCallback(() => setFocused(false), [setFocused])
-
-  const propsBag = React.useMemo(() => ({ open: state.menuState === MenuStates.Open, focused }), [
-    state,
-    focused,
-  ])
+  const propsBag = React.useMemo(() => ({ open: state.menuState === MenuStates.Open }), [state])
   const passthroughProps = props
   const propsWeControl = {
     ref: buttonRef,
@@ -264,8 +254,6 @@ const Button = forwardRefWithAs(function Button<
     'aria-controls': state.itemsRef.current?.id,
     'aria-expanded': state.menuState === MenuStates.Open ? true : undefined,
     onKeyDown: handleKeyDown,
-    onFocus: handleFocus,
-    onBlur: handleBlur,
     onPointerUp: handlePointerUp,
   }
 

diff --git a/packages/@headlessui-vue/src/components/listbox/listbox.test.tsx b/packages/@headlessui-vue/src/components/listbox/listbox.test.tsx
@@ -240,7 +240,7 @@ describe('Rendering', () => {
         assertListboxButton({
           state: ListboxState.InvisibleUnmounted,
           attributes: { id: 'headlessui-listbox-button-1' },
-          textContent: JSON.stringify({ open: false, focused: false }),
+          textContent: JSON.stringify({ open: false }),
         })
         assertListbox({ state: ListboxState.InvisibleUnmounted })
 
@@ -249,7 +249,7 @@ describe('Rendering', () => {
         assertListboxButton({
           state: ListboxState.Visible,
           attributes: { id: 'headlessui-listbox-button-1' },
-          textContent: JSON.stringify({ open: true, focused: false }),
+          textContent: JSON.stringify({ open: true }),
         })
         assertListbox({ state: ListboxState.Visible })
       })
@@ -275,7 +275,7 @@ describe('Rendering', () => {
         assertListboxButton({
           state: ListboxState.InvisibleUnmounted,
           attributes: { id: 'headlessui-listbox-button-1' },
-          textContent: JSON.stringify({ open: false, focused: false }),
+          textContent: JSON.stringify({ open: false }),
         })
         assertListbox({ state: ListboxState.InvisibleUnmounted })
 
@@ -284,7 +284,7 @@ describe('Rendering', () => {
         assertListboxButton({
           state: ListboxState.Visible,
           attributes: { id: 'headlessui-listbox-button-1' },
-          textContent: JSON.stringify({ open: true, focused: false }),
+          textContent: JSON.stringify({ open: true }),
         })
         assertListbox({ state: ListboxState.Visible })
       })
@@ -3111,34 +3111,6 @@ describe('Mouse interactions', () => {
     })
   )
 
-  it('should focus the listbox when you try to focus the button again (when the listbox is already open)', async () => {
-    renderTemplate({
-      template: `
-        <Listbox v-model="value">
-          <ListboxButton>Trigger</ListboxButton>
-          <ListboxOptions>
-            <ListboxOption value="a">Option A</ListboxOption>
-            <ListboxOption value="b">Option B</ListboxOption>
-            <ListboxOption value="c">Option C</ListboxOption>
-          </ListboxOptions>
-        </Listbox>
-      `,
-      setup: () => ({ value: ref(null) }),
-    })
-
-    // Open listbox
-    await click(getListboxButton())
-
-    // Verify listbox is focused
-    assertActiveElement(getListbox())
-
-    // Try to Re-focus the button
-    getListboxButton()?.focus()
-
-    // Verify listbox is still focused
-    assertActiveElement(getListbox())
-  })
-
   it(
     'should be a no-op when we click outside of a closed listbox',
     suppressConsoleLogs(async () => {