search

floating-ui / react-popper

🍿⚛Official React library to use Popper, the positioning library
https://popper.js.org/react-popper/
MIT License
2.51k stars 225 forks source link

Incorrect positions with placement `auto` #375

Open jquense opened 4 years ago

jquense commented 4 years ago

Reproduction demo

https://codesandbox.io/s/react-popper-v2x-issue-template-vb3kt?file=/src/index.js

Steps to reproduce the problem

  1. ensure the codesandbox has enough room along both axis' so that auto move it to the side as well as bottom/top
  2. notice an axis while scrolling change calculates the wrong offsets (further scrolling fixes it)

The issue is that the new arrow position makes the popper longer/shorter/wider/etc

What went wrong?

This behavior makes sense to me abstractly, position changes require the popper to render with the new position in order to be the correct size. What i don't understand is why i don't usually see this with the non react version, and is there a general why to handle this? In React-overlays we have an effect that checks if the placement changed in state, and triggers a forceUpdate but this leads to situations where it loops indefinitely in narrow cases where the new placement also causes a placement change.

Any other comments?

It seems like modifiers do something to avoid these loops, but i'm not sure if the same thing is possible to do outside of them in react-land. Also how Tippy seem to avoid this?

jquense commented 4 years ago

Realizing the reason this works, is that most examples use offsets instead of padding to make room for the arrow yeah?

atomiks commented 4 years ago

The issue is that the new arrow position makes the popper longer/shorter/wider/etc

You can't use margin to offset the popper because it changes its size which causes problems like the flip loop. You can only use the offset modifier.

It also happens in the core version, see https://popper.js.org/docs/v2/migration-guide/#5-ensure-your-popper-and-arrow-box-size-is-constant-for-all-placements

jquense commented 4 years ago

I know that margin doesn't work, the example here doesn't use margin. The unclear part here is you also can't use padding for accomplish this, which isn't really covered anywhere (it'd be hard to warn about for obvious reasons i know).

Doing markup like the code sandbox is very common, and not obviously an example of using margin to offset the popper, tho that is what is happening in practice. Overall it seems like the real restriction is that the popper element should not change dimensions because of a placement?

atomiks commented 4 years ago

the example here doesn't use margin

It does? Not on the popper itself but an inner element that mimics padding. That's the same thing as far as Popper is concerned with this issue (changing its dimensions based on the placement).

Screen Shot 2020-07-13 at 4 38 36 pm

Overall it seems like the real restriction is that the popper element should not change dimensions because of a placement?

Correct, you can't conditionally change the popper's size based on the placement, it's a limitation

jquense commented 4 years ago

It does? Not on the popper itself but an inner element that mimics padding.

Right, that was my (pedantic) point the popper element does not use margin, I see now that it's effectively the same thing using padding on the inside. I'm not trying to be difficult, i'm sharing how my brain thought about this. reading https://popper.js.org/docs/v2/migration-guide/#4-remove-all-css-margins and seeing the console warnings and it wasn't obvious to me that "padding on an inner element" was actually the same thing until I wrote out the issue and something clicked in my head. In retrospective duh.

So I guess i'm just suggesting that the docs/warning aren't as being as clear as they could be about what the happy path is for styling.