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.

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

Jump to bottom

Interact Outside Issue When Mounting Tooltip in Popover in separate portals #1077

Closed
anatolzak opened this issue Mar 8, 2024 · 7 comments · Fixed by #1080 or #1192 · May be fixed by patooworld/novel#3
Closed

Interact Outside Issue When Mounting Tooltip in Popover in separate portals #1077

anatolzak opened this issue Mar 8, 2024 · 7 comments · Fixed by #1080 or #1192 · May be fixed by patooworld/novel#3
Labels
bug Something isn't working

Comments

@anatolzak
Copy link
Contributor

Describe the bug

If you have a tooltip inside a popover and you mount the tooltip in a separate portal than the popover, if you click inside the tooltip, the popover closes while the tooltip stays open.

The reason to mount the tooltip in a separate portal is for example to prevent clipping issues if overflow: hidden is applied on the popover.

The reason this happens is because when clicking inside the tooltip, interact outside triggers and closes the tooltip. We should not trigger interact outside when the click happens inside portals.

Screen.Recording.2024-03-08.at.6.13.40.PM.mov

And adding an onOutsideClick to popover with e.preventDefault() doesn't help and introduces the following issues:

  1. Interacting outside the popover doesn't work
  2. Text selection inside the tooltip is disabled
Screen.Recording.2024-03-08.at.6.14.12.PM.mov

Reproduction

https://stackblitz.com/edit/github-kwo5ue?file=src/routes/+page.svelte

Logs

No response

System Info

System:
    OS: Linux 5.0 undefined
    CPU: (6) x64 Intel(R) Core(TM) i9-9880H CPU @ 2.30GHz
    Memory: 0 Bytes / 0 Bytes
    Shell: 1.0 - /bin/jsh
  Binaries:
    Node: 18.18.0 - /usr/local/bin/node
    Yarn: 1.22.19 - /usr/local/bin/yarn
    npm: 10.2.3 - /usr/local/bin/npm
    pnpm: 8.15.3 - /usr/local/bin/pnpm
  npmPackages:
    @melt-ui/svelte: latest => 0.75.3 
    @sveltejs/kit: ^2.0.0 => 2.5.2 
    svelte: ^4.1.2 => 4.2.12

Severity

annoyance
@anatolzak anatolzak added the bug Something isn't working label Mar 8, 2024
@anatolzak
Copy link
Contributor Author

This is fixed in this PR #1064

@anatolzak anatolzak mentioned this issue Mar 8, 2024
Closed
@huntabyte
Copy link
Member

huntabyte commented Mar 9, 2024 •
edited
Loading

We need to have further discussion on this. The PR submitted reintroduces this problem: #1036 which should have never existed as it was a bug (solved by the PR that closed #1036).

The issue with separate portals is that it becomes difficult to relate "stacked" portals, where they are stacked from a visual perspective and functional perspective where the following should be true given the example of a Popover open inside a Dialog:

  • Click events within the popover shouldn't be considered "outside clicks" of the dialog
    • With the proposed change they would be considered "outside" as the popover is now a sibling of the dialog in the DOM
  • Clicking outside the popover into the dialogs overlay to close the popover shouldn't close both the dialog and the popover
  • Pressing the escape key while the popover is open shouldn't close both the dialog and the popover, only the top-most open floating item
    • This one may be easier to solve with some sort of global escape key down registry of sorts

So the question we have to consider:

How do we determine if something is inside of something else that could potentially be portalled and relate those in the appropriate order to properly handle outside clicks, escape key downs, etc?

  • This problem sounds simple at first, but factor in the possibility of conditional rendering of such portal content that can't possibly be displayed or mounted until the highest portal in the tree has already been opened and portalled to a different place in the DOM and now any form of structural relation is broken
  • Actions applied to elements behind a conditional won't trigger until the conditional is true, so no obvious path for a registry of sorts.

@anatolzak
Copy link
Contributor Author

@huntabyte thanks for your feedback! Could you provide an example of conditional rendering that would disrupt stacks designed to track the most recent element for escape key presses and interact-outside interactions?

@huntabyte
Copy link
Member

huntabyte commented Mar 9, 2024 •
edited
Loading

Using the example from your video, if both the tooltip and popover are portalled, they will look something like this

<body>
	<div popover>
	</div>
	<div tooltip>
	</div>
</body>

So the content of the tooltip is no longer "inside" the popover, meaning clicks inside the tooltip would register as outside clicks.

If we had the tooltip behind a conditional, it would never even exist until after the popover has already been portalled and the DOM structure has been tampered with. Meaning there is no simple way to know what popover, dialog, or whatever this tooltip once belonged to. While we could potentially use the trigger, that would mess with custom programmatic open components and the like.

A good way to demonstrate this is that you should be able to apply portal='body' to the various components and you'll see the behavior I'm referring to.

If we were using components, this may be more practical, since we can mount the component without actually rendering any content in it, similar to how radix handles the whole Presence. We're using just actions and attributes here though.

If you truly want to portal it to the body due to an overflow issue or otherwise, you can still do so by setting the portal prop, and then ignoring clicks within that "nested" content from the parent so it doesn't trigger outside clicks.

@anatolzak
Copy link
Contributor Author

@huntabyte and why would the following not work?

  • maintain a stack of opened floating elements
  • each time a floating element is opened, add it to the stack
  • in the outside click event handler, check if the current item is the last item in the stack, and only then proceed with the handler
  • when the element is closed, remove it from the stack

and another stack for escape key presses.

Radix implemented the same concept
https://github.com/radix-ui/primitives/blob/08c0ceb541c0cc2c9610ed04619e252ffbb01cc4/packages/react/dismissable-layer/src/DismissableLayer.tsx#L63

@huntabyte
Copy link
Member

We could do this!

anatolzak added a commit to anatolzak/melt-ui that referenced this issue Mar 10, 2024
@anatolzak
Add sibling portal support melt-ui#1077
076a821
@anatolzak anatolzak mentioned this issue Mar 10, 2024
Merged
@anatolzak
Copy link
Contributor Author

@huntabyte created a PR for what we discussed here #1080

@github-actions github-actions bot mentioned this issue May 4, 2024
Merged
@patooworld patooworld mentioned this issue May 29, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
bug Something isn't working
Projects
None yet
Milestone
No milestone
2 participants
@anatolzak @huntabyte