Skip to content

[Popper] Fix potential issues with non-measured positioning #541

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

Merged
merged 1 commit into from
Mar 8, 2021

Conversation

benoitgrelard
Copy link
Contributor

@benoitgrelard benoitgrelard commented Mar 8, 2021

Closes #540

The issue is kinda intricate but I will try and give a summary here…

  • For a popper to position itself it needs a few measurements (anchor react, popper size)
  • Until these measurements are available, the popper needs to be measurable, but not visible, nor impact the flow/layout
  • For this, we return some default styles where it is positioned fixed, out of the flow, and also had pointer-events: none to avoid catching any events
  • That said, we can see here that some events were still caught (including hover)
  • This is because DropdownMenu and others use DismissableLayer and sometimes set disabledOutsidePointerEvents: true
  • This in turn puts pointerEvents: 'none' on document.body and pointerEvents: 'auto' on the content part, so in this case it puts it on what ends up being the primitive wrapped by the outer popper div
  • So even though in the popper we say prevent pointer events, it gets re-enabled

@benoitgrelard benoitgrelard requested a review from jjenzz as a code owner March 8, 2021 14:13
@@ -359,7 +359,7 @@ const UNMEASURED_POPPER_STYLES: CSS.Properties = {
top: 0,
left: 0,
opacity: 0,
pointerEvents: 'none',
transform: 'translate3d(0, -200%, 0)',
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Whilst writing the recap in the issue description, I'm still wondering whether it makes more sense to actually apply the pointerEvents properly as we actually have what it takes to do so, and it seems like a more precise fix for the situation described.

It's that different from what we already do there:
https://github.com/radix-ui/primitives/blob/main/packages/react/popper/src/Popper.tsx#L100-L104

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems we created a conflict of issues because the element is on the page during measuring phase when it shouldn't be. If it's off the page, it can't get pointer events so pointerEvents: none wouldn't really matter.

I'm happy with either approach tho tbh 🙂

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I suppose that's true haha
So I guess from that standpoint this fix is just as legitimate then. Let's roll with it.

@benoitgrelard benoitgrelard merged commit 76743aa into main Mar 8, 2021
@benoitgrelard benoitgrelard deleted the 540-fix-popper-default-position branch March 8, 2021 15:10
benoitgrelard added a commit that referenced this pull request Mar 22, 2021
# New features

Breaking changes are indicated with a 🔥 icon.

[Tooltip] Add custom duration support (#550, #551, #554, #558)
[Tooltip] Add unmount animation support (#558)
[Toggle] Rename `ToggleButton` primitive to `Toggle` and `toggled` to `pressed` (#546) 🔥
[ToggleGroup] New primitive! (#376) 🎉

# Fixes

[ContextMenu] Ensure you can open a context menu when one is already open (#565)
[Popper] Fix potential issues with non-measured positioning (#541)
[Presence] Fix unmount hanging (#548)
@benoitgrelard benoitgrelard mentioned this pull request Jul 19, 2022
8 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[DropdownMenu] first item is sometimes selected when opening menu
2 participants