Skip to content

Fix position when use anchor css properties #1995

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

Conversation

joaom00
Copy link
Contributor

@joaom00 joaom00 commented Mar 4, 2023

Description

Reset the middleware lifecycle to calculate the right position when using --radix-popper-anchor-width or --radix-popper-anchor-height.

See the behavior in some primitives although works with Select

https://codesandbox.io/p/sandbox/cocky-wood-wgqnuy

Fixes #1968
Fixes #2017

@benoitgrelard
Copy link
Contributor

I can see that fixes the issue, but I'm a little unclear as to why it needs to be in this middleware, or why it happens.
Do you have a bit more understanding here @joaom00?

@joaom00
Copy link
Contributor Author

joaom00 commented Mar 9, 2023

@benoitgrelard My understanding is that when the content is mounted all middlewares use the initial width which would be the total size of the content, let's say 66px, and when the CSS variable is defined in the middleware the width of the floating element changes, let's say 100px now, and then I ask to get the updated width, I check if there has been a change, if so I reset the middleware lifecycle with the new width. If the lifecycle is not reset, all other middlewares will continue to use the stale 66px width value.

In fact, this code is very similar to the one internally in the size middleware, if you want we can set the anchor CSS variables there and remove the anchorCssProperties middleware.

@roryhen
Copy link

roryhen commented Mar 9, 2023

Just to add my experience here. Without knowing the implementation details what it seems like is happening is that it is calculating the position of the PopoverContent (or MenubarContent, DropdownContent, etc.) based on it's intrinsic size. In @joaom00's provided repro, when you toggle the width off for PopoverContent in DevTools for example, you'll see that the content seems to be positioned correctly on toggle, although not the desired matching width to the trigger.

@benoitgrelard
Copy link
Contributor

I see that makes sense.

In fact, this code is very similar to the one internally in the size middleware, if you want we can set the anchor CSS variables there and remove the anchorCssProperties middleware.

If we did this, would this mean we don't need all this extra logic as it would already be invalidated by the internals of the size middleware?

@joaom00
Copy link
Contributor Author

joaom00 commented Mar 10, 2023

If we did this, would this mean we don't need all this extra logic as it would already be invalidated by the internals of the size middleware?

Yes, what I understood is that size() was made to perform any mutation in the size of the floating element

@benoitgrelard
Copy link
Contributor

Let's try that then as I think it might make more sense to group anything size related within that middleware and not have the extra invalidation code as its the same concern.

@joaom00
Copy link
Contributor Author

joaom00 commented Mar 10, 2023

I had to change the position of the arrow middleware to after the size because it was not able to position the arrow correctly

@benoitgrelard benoitgrelard merged commit 0728de9 into radix-ui:main Mar 10, 2023
@joaom00 joaom00 deleted the fix-position-anchor-css-properties branch March 15, 2023 11:55
@andree-gojek
Copy link

This still happens in @radix-ui/react-dropdown-menu

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.

Popover "Constrain the content size" example doesn't align properly [Select] wrong position of content
4 participants