Skip to content

Commit

Permalink
Don’t unmount portal targets used by other portals (#3144)
Browse files Browse the repository at this point in the history
* Refactor

* Don’t unmount portal targets used by other portals

* Add test

* Update changelog
  • Loading branch information
thecrypticace committed Apr 26, 2024
1 parent 722ad2d commit 1724106
Show file tree
Hide file tree
Showing 3 changed files with 83 additions and 3 deletions.
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

0 comments on commit 1724106

Please sign in to comment.