Skip to content

Commit b032679

Browse files
RobinMalfaitRobinMalfait
authored
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
1 parent 060f37b commit b032679

File tree

4 files changed

+193
-38
lines changed

4 files changed

+193
-38
lines changed

‎packages/@headlessui-react/src/components/popover/popover.test.tsx‎

Lines changed: 50 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
import React, { createElement, useEffect, useRef, Fragment } from 'react'
1+
import React, { createElement, useEffect, useRef, Fragment, useState } from 'react'
22
import { render } from '@testing-library/react'
33

44
import { Popover } from './popover'
@@ -17,6 +17,7 @@ import {
1717
import { click, press, focus, Keys, MouseButton, shift } from '../../test-utils/interactions'
1818
import { Portal } from '../portal/portal'
1919
import { Transition } from '../transitions/transition'
20+
import ReactDOM from 'react-dom'
2021

2122
jest.mock('../../hooks/use-id')
2223

@@ -1634,6 +1635,53 @@ describe('Keyboard interactions', () => {
16341635
})
16351636
)
16361637

1638+
it(
1639+
'should focus the Popover.Button when pressing Shift+Tab when we focus inside the Popover.Panel (heuristc based portal)',
1640+
suppressConsoleLogs(async () => {
1641+
function Example() {
1642+
let [portal, setPortal] = useState<HTMLElement | null>(null)
1643+
1644+
return (
1645+
<Popover>
1646+
<Popover.Button>Trigger 1</Popover.Button>
1647+
{portal &&
1648+
ReactDOM.createPortal(
1649+
<Popover.Panel focus>
1650+
<a href="/">Link 1</a>
1651+
<a href="/">Link 2</a>
1652+
</Popover.Panel>,
1653+
portal
1654+
)}
1655+
<button>Before</button>
1656+
<div ref={setPortal} />
1657+
<button>After</button>
1658+
</Popover>
1659+
)
1660+
}
1661+
1662+
render(<Example />)
1663+
1664+
// Open the popover
1665+
await click(getPopoverButton())
1666+
1667+
// Ensure the popover is open
1668+
assertPopoverButton({ state: PopoverState.Visible })
1669+
1670+
// Ensure the Link 1 is focused
1671+
assertActiveElement(getByText('Link 1'))
1672+
1673+
// Tab out of the Panel
1674+
await press(shift(Keys.Tab))
1675+
1676+
// Ensure the Popover.Button is focused again
1677+
assertActiveElement(getPopoverButton())
1678+
1679+
// Ensure the Popover is closed
1680+
assertPopoverButton({ state: PopoverState.InvisibleUnmounted })
1681+
assertPopoverPanel({ state: PopoverState.InvisibleUnmounted })
1682+
})
1683+
)
1684+
16371685
it(
16381686
'should be possible to focus the last item in the Popover.Panel when pressing Shift+Tab on the next Popover.Button',
16391687
suppressConsoleLogs(async () => {
@@ -1905,7 +1953,7 @@ describe('Keyboard interactions', () => {
19051953
})
19061954
)
19071955

1908-
xit(
1956+
it(
19091957
'should close the Popover by pressing `Enter` on a Popover.Button and go to the href of the `a` inside a Popover.Panel',
19101958
suppressConsoleLogs(async () => {
19111959
render(

‎packages/@headlessui-react/src/components/popover/popover.tsx‎

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -220,12 +220,34 @@ let PopoverRoot = forwardRefWithAs(function Popover<
220220
if (!button) return false
221221
if (!panel) return false
222222

223+
// We are part of a different "root" tree, so therefore we can consider it portalled. This is a
224+
// heuristic because 3rd party tools could use some form of portal, typically rendered at the
225+
// end of the body but we don't have an actual reference to that.
223226
for (let root of document.querySelectorAll('body > *')) {
224227
if (Number(root?.contains(button)) ^ Number(root?.contains(panel))) {
225228
return true
226229
}
227230
}
228231

232+
// Use another heuristic to try and calculate wether or not the focusable elements are near
233+
// eachother (aka, following the default focus/tab order from the browser). If they are then it
234+
// doesn't really matter if they are portalled or not because we can follow the default tab
235+
// order. But if they are not, then we can consider it being portalled so that we can ensure
236+
// that tab and shift+tab (hopefully) go to the correct spot.
237+
let elements = getFocusableElements()
238+
let buttonIdx = elements.indexOf(button)
239+
240+
let beforeIdx = (buttonIdx + elements.length - 1) % elements.length
241+
let afterIdx = (buttonIdx + 1) % elements.length
242+
243+
let beforeElement = elements[beforeIdx]
244+
let afterElement = elements[afterIdx]
245+
246+
if (!panel.contains(beforeElement) && !panel.contains(afterElement)) {
247+
return true
248+
}
249+
250+
// It may or may not be portalled, but we don't really know.
229251
return false
230252
}, [button, panel])
231253

‎packages/@headlessui-vue/src/components/popover/popover.test.ts‎

Lines changed: 100 additions & 36 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
import { defineComponent, nextTick, ref, watch, h } from 'vue'
1+
import { defineComponent, nextTick, ref, watch, h, onMounted } from 'vue'
22
import { createRenderTemplate, render } from '../../test-utils/vue-testing-library'
33

44
import { Popover, PopoverGroup, PopoverButton, PopoverPanel, PopoverOverlay } from './popover'
@@ -85,38 +85,46 @@ describe('Rendering', () => {
8585
html`
8686
<PopoverGroup>
8787
<Popover>
88-
<PopoverButton>Trigger 1</PopoverButton>
89-
<PopoverPanel>Panel 1</PopoverPanel>
88+
<PopoverButton data-trigger="1">Trigger 1</PopoverButton>
89+
<PopoverPanel data-panel="1">Panel 1</PopoverPanel>
9090
</Popover>
9191
<Popover>
92-
<PopoverButton>Trigger 2</PopoverButton>
93-
<PopoverPanel>Panel 2</PopoverPanel>
92+
<PopoverButton data-trigger="2">Trigger 2</PopoverButton>
93+
<PopoverPanel data-panel="2">Panel 2</PopoverPanel>
9494
</Popover>
9595
</PopoverGroup>
9696
`
9797
)
9898

99-
assertPopoverButton({ state: PopoverState.InvisibleUnmounted }, getByText('Trigger 1'))
100-
assertPopoverButton({ state: PopoverState.InvisibleUnmounted }, getByText('Trigger 2'))
99+
function getTrigger(number: number) {
100+
return document.querySelector(`[data-trigger="${number}"]`)! as HTMLElement
101+
}
101102

102-
assertPopoverPanel({ state: PopoverState.InvisibleUnmounted }, getByText('Panel 1'))
103-
assertPopoverPanel({ state: PopoverState.InvisibleUnmounted }, getByText('Panel 2'))
103+
function getPanel(number: number) {
104+
return document.querySelector(`[data-panel="${number}"]`)! as HTMLElement
105+
}
104106

105-
await click(getByText('Trigger 1'))
107+
assertPopoverButton({ state: PopoverState.InvisibleUnmounted }, getTrigger(1))
108+
assertPopoverButton({ state: PopoverState.InvisibleUnmounted }, getTrigger(2))
106109

107-
assertPopoverButton({ state: PopoverState.Visible }, getByText('Trigger 1'))
108-
assertPopoverButton({ state: PopoverState.InvisibleUnmounted }, getByText('Trigger 2'))
110+
assertPopoverPanel({ state: PopoverState.InvisibleUnmounted }, getPanel(1))
111+
assertPopoverPanel({ state: PopoverState.InvisibleUnmounted }, getPanel(2))
109112

110-
assertPopoverPanel({ state: PopoverState.Visible }, getByText('Panel 1'))
111-
assertPopoverPanel({ state: PopoverState.InvisibleUnmounted }, getByText('Panel 2'))
113+
await click(getTrigger(1))
112114

113-
await click(getByText('Trigger 2'))
115+
assertPopoverButton({ state: PopoverState.Visible }, getTrigger(1))
116+
assertPopoverButton({ state: PopoverState.InvisibleUnmounted }, getTrigger(2))
114117

115-
assertPopoverButton({ state: PopoverState.InvisibleUnmounted }, getByText('Trigger 1'))
116-
assertPopoverButton({ state: PopoverState.Visible }, getByText('Trigger 2'))
118+
assertPopoverPanel({ state: PopoverState.Visible }, getPanel(1))
119+
assertPopoverPanel({ state: PopoverState.InvisibleUnmounted }, getPanel(2))
120+
121+
await click(getTrigger(2))
117122

118-
assertPopoverPanel({ state: PopoverState.InvisibleUnmounted }, getByText('Panel 1'))
119-
assertPopoverPanel({ state: PopoverState.Visible }, getByText('Panel 2'))
123+
assertPopoverButton({ state: PopoverState.InvisibleUnmounted }, getTrigger(1))
124+
assertPopoverButton({ state: PopoverState.Visible }, getTrigger(2))
125+
126+
assertPopoverPanel({ state: PopoverState.InvisibleUnmounted }, getPanel(1))
127+
assertPopoverPanel({ state: PopoverState.Visible }, getPanel(2))
120128
})
121129
)
122130
})
@@ -1098,47 +1106,55 @@ describe('Keyboard interactions', () => {
10981106
html`
10991107
<PopoverGroup>
11001108
<Popover>
1101-
<PopoverButton>Trigger 1</PopoverButton>
1102-
<PopoverPanel>Panel 1</PopoverPanel>
1109+
<PopoverButton data-trigger="1">Trigger 1</PopoverButton>
1110+
<PopoverPanel data-panel="1">Panel 1</PopoverPanel>
11031111
</Popover>
11041112
11051113
<Popover>
1106-
<PopoverButton>Trigger 2</PopoverButton>
1107-
<PopoverPanel>Panel 2</PopoverPanel>
1114+
<PopoverButton data-trigger="2">Trigger 2</PopoverButton>
1115+
<PopoverPanel data-panel="2">Panel 2</PopoverPanel>
11081116
</Popover>
11091117
</PopoverGroup>
11101118
`
11111119
)
11121120

1121+
function getTrigger(number: number) {
1122+
return document.querySelector(`[data-trigger="${number}"]`)! as HTMLElement
1123+
}
1124+
1125+
function getPanel(number: number) {
1126+
return document.querySelector(`[data-panel="${number}"]`)! as HTMLElement
1127+
}
1128+
11131129
// Focus the button of the first Popover
1114-
getByText('Trigger 1')?.focus()
1130+
getTrigger(1)?.focus()
11151131

11161132
// Verify popover is closed
1117-
assertPopoverButton({ state: PopoverState.InvisibleUnmounted }, getByText('Trigger 1'))
1118-
assertPopoverButton({ state: PopoverState.InvisibleUnmounted }, getByText('Trigger 2'))
1133+
assertPopoverButton({ state: PopoverState.InvisibleUnmounted }, getTrigger(1))
1134+
assertPopoverButton({ state: PopoverState.InvisibleUnmounted }, getTrigger(2))
11191135

11201136
// Open popover
1121-
await click(getByText('Trigger 1'))
1137+
await click(getTrigger(1))
11221138

11231139
// Verify popover is open
1124-
assertPopoverButton({ state: PopoverState.Visible }, getByText('Trigger 1'))
1125-
assertPopoverButton({ state: PopoverState.InvisibleUnmounted }, getByText('Trigger 2'))
1140+
assertPopoverButton({ state: PopoverState.Visible }, getTrigger(1))
1141+
assertPopoverButton({ state: PopoverState.InvisibleUnmounted }, getTrigger(2))
11261142

1127-
assertPopoverPanel({ state: PopoverState.Visible }, getByText('Panel 1'))
1128-
assertPopoverPanel({ state: PopoverState.InvisibleUnmounted }, getByText('Panel 2'))
1143+
assertPopoverPanel({ state: PopoverState.Visible }, getPanel(1))
1144+
assertPopoverPanel({ state: PopoverState.InvisibleUnmounted }, getPanel(2))
11291145

11301146
// Focus the button of the second popover menu
1131-
getByText('Trigger 2')?.focus()
1147+
getTrigger(2)?.focus()
11321148

11331149
// Close popover
11341150
await press(Keys.Escape)
11351151

11361152
// Verify both popovers are closed
1137-
assertPopoverButton({ state: PopoverState.InvisibleUnmounted }, getByText('Trigger 1'))
1138-
assertPopoverButton({ state: PopoverState.InvisibleUnmounted }, getByText('Trigger 2'))
1153+
assertPopoverButton({ state: PopoverState.InvisibleUnmounted }, getTrigger(1))
1154+
assertPopoverButton({ state: PopoverState.InvisibleUnmounted }, getTrigger(2))
11391155

11401156
// Verify the button of the second popover is still focused
1141-
assertActiveElement(getByText('Trigger 2'))
1157+
assertActiveElement(getTrigger(2))
11421158
})
11431159
)
11441160
})
@@ -1696,6 +1712,54 @@ describe('Keyboard interactions', () => {
16961712
})
16971713
)
16981714

1715+
it(
1716+
'should focus the Popover.Button when pressing Shift+Tab when we focus inside the Popover.Panel (heuristc based portal)',
1717+
suppressConsoleLogs(async () => {
1718+
renderTemplate({
1719+
template: html`
1720+
<Popover>
1721+
<PopoverButton>Trigger 1</PopoverButton>
1722+
<Teleport v-if="ready" to="#portal">
1723+
<PopoverPanel focus>
1724+
<a href="/">Link 1</a>
1725+
<a href="/">Link 2</a>
1726+
</PopoverPanel>
1727+
</Teleport>
1728+
<button>Before</button>
1729+
<div id="portal" />
1730+
<button>After</button>
1731+
</Popover>
1732+
`,
1733+
setup() {
1734+
let ready = ref(false)
1735+
onMounted(() => {
1736+
ready.value = true
1737+
})
1738+
return { ready }
1739+
},
1740+
})
1741+
1742+
// Open the popover
1743+
await click(getPopoverButton())
1744+
1745+
// Ensure the popover is open
1746+
assertPopoverButton({ state: PopoverState.Visible })
1747+
1748+
// Ensure the Link 1 is focused
1749+
assertActiveElement(getByText('Link 1'))
1750+
1751+
// Tab out of the Panel
1752+
await press(shift(Keys.Tab))
1753+
1754+
// Ensure the Popover.Button is focused again
1755+
assertActiveElement(getPopoverButton())
1756+
1757+
// Ensure the Popover is closed
1758+
assertPopoverButton({ state: PopoverState.InvisibleUnmounted })
1759+
assertPopoverPanel({ state: PopoverState.InvisibleUnmounted })
1760+
})
1761+
)
1762+
16991763
it(
17001764
'should be possible to focus the last item in the PopoverPanel when pressing Shift+Tab on the next PopoverButton',
17011765
suppressConsoleLogs(async () => {
@@ -1981,7 +2045,7 @@ describe('Keyboard interactions', () => {
19812045
})
19822046
)
19832047

1984-
xit(
2048+
it(
19852049
'should close the Popover by pressing `Enter` on a PopoverButton and go to the href of the `a` inside a PopoverPanel',
19862050
suppressConsoleLogs(async () => {
19872051
renderTemplate(

‎packages/@headlessui-vue/src/components/popover/popover.ts‎

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -118,12 +118,33 @@ export let Popover = defineComponent({
118118
if (!dom(button)) return false
119119
if (!dom(panel)) return false
120120

121+
// We are part of a different "root" tree, so therefore we can consider it portalled. This is a
122+
// heuristic because 3rd party tools could use some form of portal, typically rendered at the
123+
// end of the body but we don't have an actual reference to that.
121124
for (let root of document.querySelectorAll('body > *')) {
122125
if (Number(root?.contains(dom(button))) ^ Number(root?.contains(dom(panel)))) {
123126
return true
124127
}
125128
}
126129

130+
// Use another heuristic to try and calculate wether or not the focusable elements are near
131+
// eachother (aka, following the default focus/tab order from the browser). If they are then it
132+
// doesn't really matter if they are portalled or not because we can follow the default tab
133+
// order. But if they are not, then we can consider it being portalled so that we can ensure
134+
// that tab and shift+tab (hopefully) go to the correct spot.
135+
let elements = getFocusableElements()
136+
let buttonIdx = elements.indexOf(dom(button)!)
137+
138+
let beforeIdx = (buttonIdx + elements.length - 1) % elements.length
139+
let afterIdx = (buttonIdx + 1) % elements.length
140+
141+
let beforeElement = elements[beforeIdx]
142+
let afterElement = elements[afterIdx]
143+
144+
if (!dom(panel)?.contains(beforeElement) && !dom(panel)?.contains(afterElement)) {
145+
return true
146+
}
147+
127148
return false
128149
})
129150

0 commit comments

Comments
 (0)