Skip to content

fix(vue): skip deleting portal root node to avoid race condition #3745

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

fix(vue): skip deleting portal root node to avoid race condition #3745

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
415fca4
fix: skip deleting portal root node to avoid race condition
fabis94 Jun 20, 2025
048db8c
test fix
fabis94 Jun 20, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
72 changes: 1 addition & 71 deletions packages/@headlessui-vue/src/components/portal/portal.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -167,76 +167,6 @@ it('should be possible to use multiple Portal elements', async () => {
expect(content2).toHaveTextContent('Contents 2 ...')
})

it('should cleanup the Portal root when the last Portal is unmounted', async () => {
expect(getPortalRoot()).toBe(null)

renderTemplate({
template: html`
<main id="parent">
<button id="a" @click="toggleA">Toggle A</button>
<button id="b" @click="toggleB">Toggle B</button>

<Portal v-if="renderA">
<p id="content1">Contents 1 ...</p>
</Portal>

<Portal v-if="renderB">
<p id="content2">Contents 2 ...</p>
</Portal>
</main>
`,
setup() {
let renderA = ref(false)
let renderB = ref(false)

return {
renderA,
renderB,
toggleA() {
renderA.value = !renderA.value
},
toggleB() {
renderB.value = !renderB.value
},
}
},
})

let a = document.getElementById('a')
let b = document.getElementById('b')

expect(getPortalRoot()).toBe(null)

// Let's render the first Portal
await click(a)

expect(getPortalRoot()).not.toBe(null)
expect(getPortalRoot().children).toHaveLength(1)

// Let's render the second Portal
await click(b)

expect(getPortalRoot()).not.toBe(null)
expect(getPortalRoot().children).toHaveLength(2)

// Let's remove the first portal
await click(a)

expect(getPortalRoot()).not.toBe(null)
expect(getPortalRoot().children).toHaveLength(1)

// Let's remove the second Portal
await click(b)

expect(getPortalRoot()).toBe(null)

// Let's render the first Portal again
await click(a)

expect(getPortalRoot()).not.toBe(null)
expect(getPortalRoot().children).toHaveLength(1)
})

it('should be possible to render multiple portals at the same time', async () => {
expect(getPortalRoot()).toBe(null)

Expand Down Expand Up @@ -311,7 +241,7 @@ it('should be possible to render multiple portals at the same time', async () =>

// Remove Portal 1
await click(document.getElementById('a'))
expect(getPortalRoot()).toBe(null)
expect(getPortalRoot().children).toHaveLength(0)

// Render A and B at the same time!
await click(document.getElementById('double'))
Expand Down
12 changes: 0 additions & 12 deletions packages/@headlessui-vue/src/components/portal/portal.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
import {
Teleport,
computed,
defineComponent,
getCurrentInstance,
h,
Expand Down Expand Up @@ -51,7 +50,6 @@ export let Portal = defineComponent({
},
setup(props, { slots, attrs }) {
let element = ref<HTMLElement | null>(null)
let ownerDocument = computed(() => getOwnerDocument(element))

let forcePortalRoot = usePortalRoot()
let groupContext = inject(PortalGroupContext, null)
Expand Down Expand Up @@ -90,16 +88,6 @@ export let Portal = defineComponent({
didRegister = true
})

onUnmounted(() => {
let root = ownerDocument.value?.getElementById('headlessui-portal-root')
if (!root) return
if (myTarget.value !== root) return

if (myTarget.value.children.length <= 0) {
myTarget.value.parentElement?.removeChild(myTarget.value)
}
})

return () => {
if (!ready.value) return null
if (myTarget.value === null) return null
Expand Down