From b032679ed088de403cb807a8de37e711c179abb7 Mon Sep 17 00:00:00 2001 From: Robin Malfait Date: Wed, 28 Sep 2022 14:16:59 +0200 Subject: [PATCH] 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 --- .../src/components/popover/popover.test.tsx | 52 ++++++- .../src/components/popover/popover.tsx | 22 +++ .../src/components/popover/popover.test.ts | 136 +++++++++++++----- .../src/components/popover/popover.ts | 21 +++ 4 files changed, 193 insertions(+), 38 deletions(-) diff --git a/packages/@headlessui-react/src/components/popover/popover.test.tsx b/packages/@headlessui-react/src/components/popover/popover.test.tsx index 243d1625af..37da90d7d3 100644 --- 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(null) + + return ( + + Trigger 1 + {portal && + ReactDOM.createPortal( + + Link 1 + Link 2 + , + portal + )} + +
+ + + ) + } + + render() + + // 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 index 69c8ece284..8fdc9b237a 100644 --- 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 index 0f3a7a7af5..4c25e766ed 100644 --- 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` - Trigger 1 - Panel 1 + Trigger 1 + Panel 1 - Trigger 2 - Panel 2 + Trigger 2 + Panel 2 ` ) - 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` - Trigger 1 - Panel 1 + Trigger 1 + Panel 1 - Trigger 2 - Panel 2 + Trigger 2 + Panel 2 ` ) + 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` + + Trigger 1 + + + Link 1 + Link 2 + + + +
+ + + `, + 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 index 61c425c1eb..61ea6c34c6 100644 --- 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 })