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

Svelte 5: Portal'd content and multiple elements in same conditional #12440

Closed
techniq opened this issue Jul 14, 2024 · 3 comments
Closed

Svelte 5: Portal'd content and multiple elements in same conditional #12440

techniq opened this issue Jul 14, 2024 · 3 comments
Labels
awaiting submitter needs a reproduction, or clarification

Comments

@techniq
Copy link

techniq commented Jul 14, 2024

Describe the bug

If you portal content with an action, such as a simple:

<script>
  function portal(node) {
    document.body.appendChild(node)
  }
</script>

<div use:portal></div>

or something more robust like Svelte UX's portal and you have more than 1 element in a conditional with the portal'd content, such as

{#if show}
  <div class="backdrop"></div>
  <div class="dialog" use:portal>Dialog</div>
{/if}

then when the item is removed, it will remove more than the portal'd content (usually some subsequent elements).

However, if you put the portal'd content in it's own conditional, it works:

{#if show}
  <div class="backdrop" onclick={() => show = false} role="none"></div>
{/if}
{#if show}
  <div class="dialog" use:portal>Dialog</div>
{/if}

This appears to be the root issue of #12427 and currently affects Dialog and Drawer in Svelte UX (although I might add the workaround soon). See the video in this comment to see the issue visually.

Reproduction

  • Svelte 5 REPL
  • Svelte UX Dialog or or Drawer (or using the Source / Page Source / On this page at the top)

Logs

No response

System Info

Severity

blocking an upgrade

@dummdidumm
Copy link
Member

Since #12215 only the DOM boundaries are stored on effects (which are used internally for rendering aswell), and since version 171, everything between the start and end node is removed. In your case, the end node is removed from its original position in the dom, and therefore the cleanup algorithm fails. As a general rule, don't mess with the DOM boundaries manually - boundary means "first or last node inside a block (like if block, in your example)": Fixed example

Is it possible to do that in your case?

@dummdidumm dummdidumm added the awaiting submitter needs a reproduction, or clarification label Jul 15, 2024
@techniq
Copy link
Author

techniq commented Jul 15, 2024

Since #12215 only the DOM boundaries are stored on effects (which are used internally for rendering aswell), and since version 171, everything between the start and end node is removed. In your case, the end node is removed from its original position in the dom, and therefore the cleanup algorithm fails. As a general rule, don't mess with the DOM boundaries manually - boundary means "first or last node inside a block (like if block, in your example)": Fixed example

Is it possible to do that in your case?

Thanks @dummdidumm. My REPL was a little misleading as I can not use runes just yet as I'm planning for Svelte UX and LayerChart to support Svelte 4 with the Svelte 5 package to ease the transition (for both the users and the maintainers 😉).

With that said, I created a REPL that returns { destroy() { ... } from the action to do the cleanup (which is how Svelte UX's portal action works ATM) which works the same as the $effect() rune, although both require adding the extra <div></div> element.

It looks like there are 2 solutions/workarounds.

1.) Put each element in it's own {#if} block

{#if show}
	<div class="backdrop" onclick={() => show = false} role="none"></div>
{/if}
{#if show}
	<div class="dialog" use:portal>Dialog</div>
{/if}

2.) Make sure the last element is not a portal'd component

{#if show}
	<div class="backdrop" onclick={() => show = false} role="none"></div>
	<div class="dialog" use:portal>Dialog</div>
	<!-- makes Svelte 5 happy -->
	<div></div>
{/if}

I'm fine with either of these workarounds, I mostly wanted to add visibility in case it affected other cases I wasn't aware of.

Before I add one of the workarounds, I thought it was a little surprising what is shown in devtools vs the browser after a Dialog is re-opened (maybe I'm overlooking something). I've recorded a video with an audio track...

CleanShot.2024-07-15.at.08.47.32.2.mp4

You can also see it for yourself by using the PR preview for the docs running against Svelte 5.

Thanks for all your help

@techniq
Copy link
Author

techniq commented Jul 15, 2024

@dummdidumm I opt'd for option 1 (separate {#if}) as a workaround. Thanks for looking into this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
awaiting submitter needs a reproduction, or clarification
Projects
None yet
Development

No branches or pull requests

2 participants