Skip to content

Commit e3cbcc4

Browse files
committed
fix Portal cleanup issue (#266)
When the last portal is unmounted we will document.body.removeChild(target), but this crashes when something tampered with it. This is reproduceable in the tests. Instead we now ensure that the portal root always exists when mounting. We will also ensure to target.parentElement.removeChild which will ensure that we can reference the parent correctly.
1 parent b3c0cbe commit e3cbcc4

File tree

2 files changed

+147
-12
lines changed

2 files changed

+147
-12
lines changed

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

Lines changed: 134 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,7 @@ import { Portal } from './portal'
66
import { click } from '../../test-utils/interactions'
77

88
function getPortalRoot() {
9-
return document.getElementById('headlessui-portal-root')
9+
return document.getElementById('headlessui-portal-root')!
1010
}
1111

1212
beforeEach(() => {
@@ -140,3 +140,136 @@ it('should cleanup the Portal root when the last Portal is unmounted', async ()
140140
expect(getPortalRoot()).not.toBe(null)
141141
expect(getPortalRoot().childNodes).toHaveLength(1)
142142
})
143+
144+
it('should be possible to render multiple portals at the same time', async () => {
145+
expect(getPortalRoot()).toBe(null)
146+
147+
function Example() {
148+
let [renderA, setRenderA] = useState(true)
149+
let [renderB, setRenderB] = useState(true)
150+
let [renderC, setRenderC] = useState(true)
151+
152+
return (
153+
<main id="parent">
154+
<button id="a" onClick={() => setRenderA(v => !v)}>
155+
Toggle A
156+
</button>
157+
<button id="b" onClick={() => setRenderB(v => !v)}>
158+
Toggle B
159+
</button>
160+
<button id="c" onClick={() => setRenderC(v => !v)}>
161+
Toggle C
162+
</button>
163+
164+
<button
165+
id="double"
166+
onClick={() => {
167+
setRenderA(v => !v)
168+
setRenderB(v => !v)
169+
}}
170+
>
171+
Toggle A & B{' '}
172+
</button>
173+
174+
{renderA && (
175+
<Portal>
176+
<p id="content1">Contents 1 ...</p>
177+
</Portal>
178+
)}
179+
180+
{renderB && (
181+
<Portal>
182+
<p id="content2">Contents 2 ...</p>
183+
</Portal>
184+
)}
185+
186+
{renderC && (
187+
<Portal>
188+
<p id="content3">Contents 3 ...</p>
189+
</Portal>
190+
)}
191+
</main>
192+
)
193+
}
194+
195+
render(<Example />)
196+
197+
expect(getPortalRoot()).not.toBe(null)
198+
expect(getPortalRoot().childNodes).toHaveLength(3)
199+
200+
// Remove Portal 1
201+
await click(document.getElementById('a'))
202+
expect(getPortalRoot().childNodes).toHaveLength(2)
203+
204+
// Remove Portal 2
205+
await click(document.getElementById('b'))
206+
expect(getPortalRoot().childNodes).toHaveLength(1)
207+
208+
// Re-add Portal 1
209+
await click(document.getElementById('a'))
210+
expect(getPortalRoot().childNodes).toHaveLength(2)
211+
212+
// Remove Portal 3
213+
await click(document.getElementById('c'))
214+
expect(getPortalRoot().childNodes).toHaveLength(1)
215+
216+
// Remove Portal 1
217+
await click(document.getElementById('a'))
218+
expect(getPortalRoot()).toBe(null)
219+
220+
// Render A and B at the same time!
221+
await click(document.getElementById('double'))
222+
expect(getPortalRoot().childNodes).toHaveLength(2)
223+
})
224+
225+
it('should be possible to tamper with the modal root and restore correctly', async () => {
226+
expect(getPortalRoot()).toBe(null)
227+
228+
function Example() {
229+
let [renderA, setRenderA] = useState(true)
230+
let [renderB, setRenderB] = useState(true)
231+
232+
return (
233+
<main id="parent">
234+
<button id="a" onClick={() => setRenderA(v => !v)}>
235+
Toggle A
236+
</button>
237+
<button id="b" onClick={() => setRenderB(v => !v)}>
238+
Toggle B
239+
</button>
240+
241+
{renderA && (
242+
<Portal>
243+
<p id="content1">Contents 1 ...</p>
244+
</Portal>
245+
)}
246+
247+
{renderB && (
248+
<Portal>
249+
<p id="content2">Contents 2 ...</p>
250+
</Portal>
251+
)}
252+
</main>
253+
)
254+
}
255+
256+
render(<Example />)
257+
258+
expect(getPortalRoot()).not.toBe(null)
259+
260+
// Tamper tamper
261+
document.body.removeChild(document.getElementById('headlessui-portal-root')!)
262+
263+
// Hide Portal 1 and 2
264+
await click(document.getElementById('a'))
265+
await click(document.getElementById('b'))
266+
267+
expect(getPortalRoot()).toBe(null)
268+
269+
// Re-show Portal 1 and 2
270+
await click(document.getElementById('a'))
271+
await click(document.getElementById('b'))
272+
273+
expect(getPortalRoot()).not.toBe(null)
274+
expect(getPortalRoot().childNodes).toHaveLength(2)
275+
})

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

Lines changed: 13 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,16 @@ import { render } from '../../utils/render'
1313
import { useIsoMorphicEffect } from '../../hooks/use-iso-morphic-effect'
1414
import { StackProvider, useElemenStack } from '../../internal/stack-context'
1515

16+
function resolvePortalRoot() {
17+
if (typeof window === 'undefined') return null
18+
let existingRoot = document.getElementById('headlessui-portal-root')
19+
if (existingRoot) return existingRoot
20+
21+
let root = document.createElement('div')
22+
root.setAttribute('id', 'headlessui-portal-root')
23+
return document.body.appendChild(root)
24+
}
25+
1626
// ---
1727

1828
let DEFAULT_PORTAL_TAG = Fragment
@@ -21,23 +31,15 @@ interface PortalRenderPropArg {}
2131
export function Portal<TTag extends ElementType = typeof DEFAULT_PORTAL_TAG>(
2232
props: Props<TTag, PortalRenderPropArg>
2333
) {
24-
let [target] = useState(() => {
25-
if (typeof window === 'undefined') return null
26-
let existingRoot = document.getElementById('headlessui-portal-root')
27-
if (existingRoot) return existingRoot
28-
29-
let root = document.createElement('div')
30-
root.setAttribute('id', 'headlessui-portal-root')
31-
return document.body.appendChild(root)
32-
})
34+
let [target, setTarget] = useState(resolvePortalRoot)
3335
let [element] = useState<HTMLDivElement | null>(() =>
3436
typeof window === 'undefined' ? null : document.createElement('div')
3537
)
3638

3739
useElemenStack(element)
3840

3941
useIsoMorphicEffect(() => {
40-
if (!target) return
42+
if (!target) return setTarget(resolvePortalRoot())
4143
if (!element) return
4244

4345
target.appendChild(element)
@@ -47,7 +49,7 @@ export function Portal<TTag extends ElementType = typeof DEFAULT_PORTAL_TAG>(
4749
if (!element) return
4850

4951
target.removeChild(element)
50-
if (target.childNodes.length <= 0) document.body.removeChild(target)
52+
if (target.childNodes.length <= 0) target.parentElement?.removeChild(target)
5153
}
5254
}, [target, element])
5355

0 commit comments

Comments
 (0)