Skip to content
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.

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

Don’t unmount portal targets used by other portals #3144

Merged
merged 4 commits into from Apr 26, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions packages/@headlessui-vue/CHANGELOG.md
Expand Up @@ -12,6 +12,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
### Fixed

- Prevent closing the `Combobox` component when clicking inside the scrollbar area ([#3104](https://github.com/tailwindlabs/headlessui/pull/3104))
- Don’t unmount portal targets used by other portals ([#3144](https://github.com/tailwindlabs/headlessui/pull/3144))

## [1.7.20] - 2024-04-15

Expand Down
45 changes: 45 additions & 0 deletions packages/@headlessui-vue/src/components/portal/portal.test.ts
Expand Up @@ -391,3 +391,48 @@ it('should be possible to force the Portal into a specific element using PortalG
`"<div><div><div data-v-app=\\"\\"><main><aside id=\\"group-1\\">A<div data-headlessui-portal=\\"\\">Next to A</div></aside><section id=\\"group-2\\"><span>B</span></section><!--teleport start--><!--teleport end--></main></div></div></div>"`
)
})

it('the root shared by multiple portals should not unmount when they change in the same tick', async () => {
let a = ref(false)
let b = ref(false)

renderTemplate({
template: html`
<main>
<Portal v-if="a">Portal A</Portal>
<Portal v-if="b">Portal B</Portal>
</main>
`,
setup: () => ({ a, b }),
})

await new Promise<void>(nextTick)

let root = () => document.querySelector('#headlessui-portal-root')

// There is no portal root initially because there are no visible portals
expect(root()).toBe(null)

// Show portal A
a.value = true
await new Promise<void>(nextTick)

// There is a portal root now because there is a visible portal
expect(root()).not.toBe(null)

// Swap portal A for portal B
a.value = false
b.value = true

await new Promise<void>(nextTick)

// The portal root is still there because there are still visible portals
expect(root()).not.toBe(null)

// Hide portal B
b.value = false
await new Promise<void>(nextTick)

// The portal root is gone because there are no visible portals
expect(root()).toBe(null)
})
40 changes: 37 additions & 3 deletions packages/@headlessui-vue/src/components/portal/portal.ts
Expand Up @@ -44,6 +44,27 @@ function getPortalRoot(contextElement?: Element | null) {
return ownerDocument.body.appendChild(root)
}

// A counter that keeps track of how many portals are currently using
// a specific portal root. This is used to know when we can safely
// remove the portal root from the DOM.
const counter = new WeakMap<HTMLElement, number>()

function getCount(el: HTMLElement): number {
return counter.get(el) ?? 0
}

function setCount(el: HTMLElement, cb: (val: number) => number): number {
let newCount = cb(getCount(el))

if (newCount <= 0) {
counter.delete(el)
} else {
counter.set(el, newCount)
}

return newCount
}

export let Portal = defineComponent({
name: 'Portal',
props: {
Expand All @@ -63,6 +84,11 @@ export let Portal = defineComponent({
: groupContext.resolveTarget()
)

// Make a note that we are using this element
if (myTarget.value) {
setCount(myTarget.value, (val) => val + 1)
}

let ready = ref(false)
onMounted(() => {
ready.value = true
Expand Down Expand Up @@ -95,9 +121,17 @@ export let Portal = defineComponent({
if (!root) return
if (myTarget.value !== root) return

if (myTarget.value.children.length <= 0) {
myTarget.value.parentElement?.removeChild(myTarget.value)
}
// We no longer need the portal target
let remaining = setCount(myTarget.value, (val) => val - 1)

// However, if another portal is still using the same target
// we should not remove it.
if (remaining) return

// There are still children in the portal, we should not remove it.
if (myTarget.value.children.length > 0) return

myTarget.value.parentElement?.removeChild(myTarget.value)
})

return () => {
Expand Down