Improve Portal detection for Popover components (#1842)

* improve Popover heuristics for detecting being inside a Portal * improve performance of checks * make it work in Vue * verify behaviour in tests
tailwindlabs · Sep 28, 2022 · b032679 · b032679
1 parent 060f37b
commit b032679
Show file tree

Hide file tree

Showing 4 changed files with 193 additions and 38 deletions.
diff --git a/packages/@headlessui-react/src/components/popover/popover.test.tsx b/packages/@headlessui-react/src/components/popover/popover.test.tsx
@@ -1,4 +1,4 @@
-import React, { createElement, useEffect, useRef, Fragment } from 'react'
+import React, { createElement, useEffect, useRef, Fragment, useState } from 'react'
 import { render } from '@testing-library/react'
 
 import { Popover } from './popover'
@@ -17,6 +17,7 @@ import {
 import { click, press, focus, Keys, MouseButton, shift } from '../../test-utils/interactions'
 import { Portal } from '../portal/portal'
 import { Transition } from '../transitions/transition'
+import ReactDOM from 'react-dom'
 
 jest.mock('../../hooks/use-id')
 
@@ -1634,6 +1635,53 @@ describe('Keyboard interactions', () => {
       })
     )
 
+    it(
+      'should focus the Popover.Button when pressing Shift+Tab when we focus inside the Popover.Panel (heuristc based portal)',
+      suppressConsoleLogs(async () => {
+        function Example() {
+          let [portal, setPortal] = useState<HTMLElement | null>(null)
+
+          return (
+            <Popover>
+              <Popover.Button>Trigger 1</Popover.Button>
+              {portal &&
+                ReactDOM.createPortal(
+                  <Popover.Panel focus>
+                    <a href="/">Link 1</a>
+                    <a href="/">Link 2</a>
+                  </Popover.Panel>,
+                  portal
+                )}
+              <button>Before</button>
+              <div ref={setPortal} />
+              <button>After</button>
+            </Popover>
+          )
+        }
+
+        render(<Example />)
+
+        // Open the popover
+        await click(getPopoverButton())
+
+        // Ensure the popover is open
+        assertPopoverButton({ state: PopoverState.Visible })
+
+        // Ensure the Link 1 is focused
+        assertActiveElement(getByText('Link 1'))
+
+        // Tab out of the Panel
+        await press(shift(Keys.Tab))
+
+        // Ensure the Popover.Button is focused again
+        assertActiveElement(getPopoverButton())
+
+        // Ensure the Popover is closed
+        assertPopoverButton({ state: PopoverState.InvisibleUnmounted })
+        assertPopoverPanel({ state: PopoverState.InvisibleUnmounted })
+      })
+    )
+
     it(
       'should be possible to focus the last item in the Popover.Panel when pressing Shift+Tab on the next Popover.Button',
       suppressConsoleLogs(async () => {
@@ -1905,7 +1953,7 @@ describe('Keyboard interactions', () => {
       })
     )
 
-    xit(
+    it(
       'should close the Popover by pressing `Enter` on a Popover.Button and go to the href of the `a` inside a Popover.Panel',
       suppressConsoleLogs(async () => {
         render(

diff --git a/packages/@headlessui-react/src/components/popover/popover.tsx b/packages/@headlessui-react/src/components/popover/popover.tsx
@@ -220,12 +220,34 @@ let PopoverRoot = forwardRefWithAs(function Popover<
     if (!button) return false
     if (!panel) return false
 
+    // We are part of a different "root" tree, so therefore we can consider it portalled. This is a
+    // heuristic because 3rd party tools could use some form of portal, typically rendered at the
+    // end of the body but we don't have an actual reference to that.
     for (let root of document.querySelectorAll('body > *')) {
       if (Number(root?.contains(button)) ^ Number(root?.contains(panel))) {
         return true
       }
     }
 
+    // Use another heuristic to try and calculate wether or not the focusable elements are near
+    // eachother (aka, following the default focus/tab order from the browser). If they are then it
+    // doesn't really matter if they are portalled or not because we can follow the default tab
+    // order. But if they are not, then we can consider it being portalled so that we can ensure
+    // that tab and shift+tab (hopefully) go to the correct spot.
+    let elements = getFocusableElements()
+    let buttonIdx = elements.indexOf(button)
+
+    let beforeIdx = (buttonIdx + elements.length - 1) % elements.length
+    let afterIdx = (buttonIdx + 1) % elements.length
+
+    let beforeElement = elements[beforeIdx]
+    let afterElement = elements[afterIdx]
+
+    if (!panel.contains(beforeElement) && !panel.contains(afterElement)) {
+      return true
+    }
+
+    // It may or may not be portalled, but we don't really know.
     return false
   }, [button, panel])
 

diff --git a/packages/@headlessui-vue/src/components/popover/popover.test.ts b/packages/@headlessui-vue/src/components/popover/popover.test.ts
@@ -1,4 +1,4 @@
-import { defineComponent, nextTick, ref, watch, h } from 'vue'
+import { defineComponent, nextTick, ref, watch, h, onMounted } from 'vue'
 import { createRenderTemplate, render } from '../../test-utils/vue-testing-library'
 
 import { Popover, PopoverGroup, PopoverButton, PopoverPanel, PopoverOverlay } from './popover'
@@ -85,38 +85,46 @@ describe('Rendering', () => {
           html`
             <PopoverGroup>
               <Popover>
-                <PopoverButton>Trigger 1</PopoverButton>
-                <PopoverPanel>Panel 1</PopoverPanel>
+                <PopoverButton data-trigger="1">Trigger 1</PopoverButton>
+                <PopoverPanel data-panel="1">Panel 1</PopoverPanel>
               </Popover>
               <Popover>
-                <PopoverButton>Trigger 2</PopoverButton>
-                <PopoverPanel>Panel 2</PopoverPanel>
+                <PopoverButton data-trigger="2">Trigger 2</PopoverButton>
+                <PopoverPanel data-panel="2">Panel 2</PopoverPanel>
               </Popover>
             </PopoverGroup>
           `
         )
 
-        assertPopoverButton({ state: PopoverState.InvisibleUnmounted }, getByText('Trigger 1'))
-        assertPopoverButton({ state: PopoverState.InvisibleUnmounted }, getByText('Trigger 2'))
+        function getTrigger(number: number) {
+          return document.querySelector(`[data-trigger="${number}"]`)! as HTMLElement
+        }
 
-        assertPopoverPanel({ state: PopoverState.InvisibleUnmounted }, getByText('Panel 1'))
-        assertPopoverPanel({ state: PopoverState.InvisibleUnmounted }, getByText('Panel 2'))
+        function getPanel(number: number) {
+          return document.querySelector(`[data-panel="${number}"]`)! as HTMLElement
+        }
 
-        await click(getByText('Trigger 1'))
+        assertPopoverButton({ state: PopoverState.InvisibleUnmounted }, getTrigger(1))
+        assertPopoverButton({ state: PopoverState.InvisibleUnmounted }, getTrigger(2))
 
-        assertPopoverButton({ state: PopoverState.Visible }, getByText('Trigger 1'))
-        assertPopoverButton({ state: PopoverState.InvisibleUnmounted }, getByText('Trigger 2'))
+        assertPopoverPanel({ state: PopoverState.InvisibleUnmounted }, getPanel(1))
+        assertPopoverPanel({ state: PopoverState.InvisibleUnmounted }, getPanel(2))
 
-        assertPopoverPanel({ state: PopoverState.Visible }, getByText('Panel 1'))
-        assertPopoverPanel({ state: PopoverState.InvisibleUnmounted }, getByText('Panel 2'))
+        await click(getTrigger(1))
 
-        await click(getByText('Trigger 2'))
+        assertPopoverButton({ state: PopoverState.Visible }, getTrigger(1))
+        assertPopoverButton({ state: PopoverState.InvisibleUnmounted }, getTrigger(2))
 
-        assertPopoverButton({ state: PopoverState.InvisibleUnmounted }, getByText('Trigger 1'))
-        assertPopoverButton({ state: PopoverState.Visible }, getByText('Trigger 2'))
+        assertPopoverPanel({ state: PopoverState.Visible }, getPanel(1))
+        assertPopoverPanel({ state: PopoverState.InvisibleUnmounted }, getPanel(2))
+
+        await click(getTrigger(2))
 
-        assertPopoverPanel({ state: PopoverState.InvisibleUnmounted }, getByText('Panel 1'))
-        assertPopoverPanel({ state: PopoverState.Visible }, getByText('Panel 2'))
+        assertPopoverButton({ state: PopoverState.InvisibleUnmounted }, getTrigger(1))
+        assertPopoverButton({ state: PopoverState.Visible }, getTrigger(2))
+
+        assertPopoverPanel({ state: PopoverState.InvisibleUnmounted }, getPanel(1))
+        assertPopoverPanel({ state: PopoverState.Visible }, getPanel(2))
       })
     )
   })
@@ -1098,47 +1106,55 @@ describe('Keyboard interactions', () => {
           html`
             <PopoverGroup>
               <Popover>
-                <PopoverButton>Trigger 1</PopoverButton>
-                <PopoverPanel>Panel 1</PopoverPanel>
+                <PopoverButton data-trigger="1">Trigger 1</PopoverButton>
+                <PopoverPanel data-panel="1">Panel 1</PopoverPanel>
               </Popover>
 
               <Popover>
-                <PopoverButton>Trigger 2</PopoverButton>
-                <PopoverPanel>Panel 2</PopoverPanel>
+                <PopoverButton data-trigger="2">Trigger 2</PopoverButton>
+                <PopoverPanel data-panel="2">Panel 2</PopoverPanel>
               </Popover>
             </PopoverGroup>
           `
         )
 
+        function getTrigger(number: number) {
+          return document.querySelector(`[data-trigger="${number}"]`)! as HTMLElement
+        }
+
+        function getPanel(number: number) {
+          return document.querySelector(`[data-panel="${number}"]`)! as HTMLElement
+        }
+
         // Focus the button of the first Popover
-        getByText('Trigger 1')?.focus()
+        getTrigger(1)?.focus()
 
         // Verify popover is closed
-        assertPopoverButton({ state: PopoverState.InvisibleUnmounted }, getByText('Trigger 1'))
-        assertPopoverButton({ state: PopoverState.InvisibleUnmounted }, getByText('Trigger 2'))
+        assertPopoverButton({ state: PopoverState.InvisibleUnmounted }, getTrigger(1))
+        assertPopoverButton({ state: PopoverState.InvisibleUnmounted }, getTrigger(2))
 
         // Open popover
-        await click(getByText('Trigger 1'))
+        await click(getTrigger(1))
 
         // Verify popover is open
-        assertPopoverButton({ state: PopoverState.Visible }, getByText('Trigger 1'))
-        assertPopoverButton({ state: PopoverState.InvisibleUnmounted }, getByText('Trigger 2'))
+        assertPopoverButton({ state: PopoverState.Visible }, getTrigger(1))
+        assertPopoverButton({ state: PopoverState.InvisibleUnmounted }, getTrigger(2))
 
-        assertPopoverPanel({ state: PopoverState.Visible }, getByText('Panel 1'))
-        assertPopoverPanel({ state: PopoverState.InvisibleUnmounted }, getByText('Panel 2'))
+        assertPopoverPanel({ state: PopoverState.Visible }, getPanel(1))
+        assertPopoverPanel({ state: PopoverState.InvisibleUnmounted }, getPanel(2))
 
         // Focus the button of the second popover menu
-        getByText('Trigger 2')?.focus()
+        getTrigger(2)?.focus()
 
         // Close popover
         await press(Keys.Escape)
 
         // Verify both popovers are closed
-        assertPopoverButton({ state: PopoverState.InvisibleUnmounted }, getByText('Trigger 1'))
-        assertPopoverButton({ state: PopoverState.InvisibleUnmounted }, getByText('Trigger 2'))
+        assertPopoverButton({ state: PopoverState.InvisibleUnmounted }, getTrigger(1))
+        assertPopoverButton({ state: PopoverState.InvisibleUnmounted }, getTrigger(2))
 
         // Verify the button of the second popover is still focused
-        assertActiveElement(getByText('Trigger 2'))
+        assertActiveElement(getTrigger(2))
       })
     )
   })
@@ -1696,6 +1712,54 @@ describe('Keyboard interactions', () => {
       })
     )
 
+    it(
+      'should focus the Popover.Button when pressing Shift+Tab when we focus inside the Popover.Panel (heuristc based portal)',
+      suppressConsoleLogs(async () => {
+        renderTemplate({
+          template: html`
+            <Popover>
+              <PopoverButton>Trigger 1</PopoverButton>
+              <Teleport v-if="ready" to="#portal">
+                <PopoverPanel focus>
+                  <a href="/">Link 1</a>
+                  <a href="/">Link 2</a>
+                </PopoverPanel>
+              </Teleport>
+              <button>Before</button>
+              <div id="portal" />
+              <button>After</button>
+            </Popover>
+          `,
+          setup() {
+            let ready = ref(false)
+            onMounted(() => {
+              ready.value = true
+            })
+            return { ready }
+          },
+        })
+
+        // Open the popover
+        await click(getPopoverButton())
+
+        // Ensure the popover is open
+        assertPopoverButton({ state: PopoverState.Visible })
+
+        // Ensure the Link 1 is focused
+        assertActiveElement(getByText('Link 1'))
+
+        // Tab out of the Panel
+        await press(shift(Keys.Tab))
+
+        // Ensure the Popover.Button is focused again
+        assertActiveElement(getPopoverButton())
+
+        // Ensure the Popover is closed
+        assertPopoverButton({ state: PopoverState.InvisibleUnmounted })
+        assertPopoverPanel({ state: PopoverState.InvisibleUnmounted })
+      })
+    )
+
     it(
       'should be possible to focus the last item in the PopoverPanel when pressing Shift+Tab on the next PopoverButton',
       suppressConsoleLogs(async () => {
@@ -1981,7 +2045,7 @@ describe('Keyboard interactions', () => {
       })
     )
 
-    xit(
+    it(
       'should close the Popover by pressing `Enter` on a PopoverButton and go to the href of the `a` inside a PopoverPanel',
       suppressConsoleLogs(async () => {
         renderTemplate(

diff --git a/packages/@headlessui-vue/src/components/popover/popover.ts b/packages/@headlessui-vue/src/components/popover/popover.ts
@@ -118,12 +118,33 @@ export let Popover = defineComponent({
       if (!dom(button)) return false
       if (!dom(panel)) return false
 
+      // We are part of a different "root" tree, so therefore we can consider it portalled. This is a
+      // heuristic because 3rd party tools could use some form of portal, typically rendered at the
+      // end of the body but we don't have an actual reference to that.
       for (let root of document.querySelectorAll('body > *')) {
         if (Number(root?.contains(dom(button))) ^ Number(root?.contains(dom(panel)))) {
           return true
         }
       }
 
+      // Use another heuristic to try and calculate wether or not the focusable elements are near
+      // eachother (aka, following the default focus/tab order from the browser). If they are then it
+      // doesn't really matter if they are portalled or not because we can follow the default tab
+      // order. But if they are not, then we can consider it being portalled so that we can ensure
+      // that tab and shift+tab (hopefully) go to the correct spot.
+      let elements = getFocusableElements()
+      let buttonIdx = elements.indexOf(dom(button)!)
+
+      let beforeIdx = (buttonIdx + elements.length - 1) % elements.length
+      let afterIdx = (buttonIdx + 1) % elements.length
+
+      let beforeElement = elements[beforeIdx]
+      let afterElement = elements[afterIdx]
+
+      if (!dom(panel)?.contains(beforeElement) && !dom(panel)?.contains(afterElement)) {
+        return true
+      }
+
       return false
     })