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

Incorrect position on mount #354

Closed
ejdzipi opened this issue Apr 15, 2020 · 12 comments
Closed

Incorrect position on mount #354

ejdzipi opened this issue Apr 15, 2020 · 12 comments

Comments

@ejdzipi
Copy link

ejdzipi commented Apr 15, 2020

After updating to version 2.2 and above, popper is incorrectly positioned on first mount. Simple scroll or resize event will fix it.

Reproduction demo

https://codesandbox.io/s/gifted-ishizaka-3mxdv

Steps to reproduce the problem

  1. In provided codesandbox demo, open styles.css and remove min-height on line 4

What is the expected behavior?

Popper should be placed correctly

What went wrong?

Popper is misplaced, window resize will cause popper refresh and it will be corrected

Any other comments?

incorrect position after mount: (purple is arrow element, just to demonstrate position)
1

after resize, position is corrected
2

Packages versions

  • @popperjs/core: 2.3.2
  • react-popper: 2.2.2
@atomiks
Copy link
Collaborator

atomiks commented Apr 15, 2020

I can't reproduce it in Chrome or FF. In Safari the layout shifts but updates aren't triggered by default when the DOM layout changes. Is that the issue?

@ejdzipi
Copy link
Author

ejdzipi commented Apr 15, 2020

I am using Chrome 81.0.4044.92 64bit on Win10. Also tried Firefox 75.0 and issue is present.
Here is screencast:
popper

@atomiks
Copy link
Collaborator

atomiks commented Apr 15, 2020

I see, this is expected behavior. The popper doesn't update due to layout changes by default, only scroll/resize. It should've happened with any version in the past though.

@ejdzipi
Copy link
Author

ejdzipi commented Apr 15, 2020

But it is causing issues with arrow position in my other project. This is how it looked like on the first mount before using hooks version:
2 1 0
After implementing hooks, it looks like this:
2 2 2

@atomiks
Copy link
Collaborator

atomiks commented Apr 15, 2020

You are probably passing an arrow with useRef, not using useState, right? You need to use callback refs not a ref object.

If not, try to create a CodeSandbox reproduction that matches the screenshot

@ejdzipi
Copy link
Author

ejdzipi commented Apr 15, 2020

I have updated the sandbox: https://codesandbox.io/s/gifted-ishizaka-3mxdv
The first load is showing displaced arrow:
firstload
Can you please check it out?

@atomiks
Copy link
Collaborator

atomiks commented Apr 15, 2020

https://codesandbox.io/s/frosty-breeze-3525h?file=/src/styles.css

two problems:

  1. you arrow needs to have a width & height specified
  2. you need to use a ternary for the className, since it's doing arrowundefined currently

@ejdzipi
Copy link
Author

ejdzipi commented Apr 16, 2020

I have implemented it in my project, based on your example (added width, height, corrected classnames) but the problem persists. I was unable to reproduce it on codesandbox, but i found that if i scroll page at least 1px down, everything works correct. Problem is shown only on the first page load, when content is not scrolled.

@ejdzipi
Copy link
Author

ejdzipi commented Apr 20, 2020

I have created another demo, which reproduces the problem. First, i have bootstrapped simple react app from create-react-app, then i have added popper, react-popper and emotion for css-in-js. I have implemented popper example using hooks and the problem persists.
Repo URL: https://github.com/ejdzipi/cra-popper-arrow-issue
@atomiks can you please take another look on this? I have added width&height for arrow element, ref is managed with useState, code is taken from this library's readme. I have no clue how to fix it. Thanks.
image

@FezVrasta
Copy link
Member

FezVrasta commented Apr 20, 2020

the arrow needs to have same width and height, and its position needs to be absolute, otherwise before Popper has a chance to position it, the arrow will stretch the tooltip.

https://codesandbox.io/s/lingering-waterfall-qr9y2?file=/src/Popper.js

@ejdzipi
Copy link
Author

ejdzipi commented Apr 20, 2020

Thank you, finally i understand it 🎉 Could this information be added to library documentation? I believe that it might be beneficial for others, if there is information that arrow element should have defined size and absolute position.

@HendrikThePendric
Copy link

HendrikThePendric commented Jun 3, 2020

I agree with @ejdzipi that it would be beneficial to add this to the docs. I started out having position: absolute; on the arrow element but removed that because I noticed that the styles.arrow also contained this style. Later on I noticed there was some misplacement going on which must have been caused by the popper being stretched by the arrow in the initial render cycle.

Alternatively, you could consider omitting position: 'absolute' from styles.arrow because that would make it much more obvious what is going wrong.

HendrikThePendric added a commit to dhis2/ui that referenced this issue Jun 9, 2020
Commit history (before squashing)
- fix(popover): add position absolute to arrow to prevent misplacement (See floating-ui/react-popper#354)
- test(popover): add e2e tests and stories for arrow positions
- chore(popover): fix typo in feature file and test
- refactor(popper): replace e2e story decorator by helper component (This prepares the story for the new e2e test which are not going to be compatible with the decorator logic)
- fix(popper): use correct sharedPropType name
- test(popper): prepare e2e stories about different types of references
- test(popper): add e2e test and stories for different types of references
- chore(popper): stop using `getPositionsBySelectors` helper in new tests
- chore(popover): stop using `getPositionsBySelectors` helper in new tests
- chore(popover): fix typo in feature step definition
HendrikThePendric added a commit to dhis2/ui that referenced this issue Jun 9, 2020
Commit history (before squashing)
- fix(popover): add position absolute to arrow to prevent misplacement (See floating-ui/react-popper#354)
- test(popover): add e2e tests and stories for arrow positions
- chore(popover): fix typo in feature file and test
- refactor(popper): replace e2e story decorator by helper component (This prepares the story for the new e2e test which are not going to be compatible with the decorator logic)
- fix(popper): use correct sharedPropType name
- test(popper): prepare e2e stories about different types of references
- test(popper): add e2e test and stories for different types of references
- chore(popper): stop using `getPositionsBySelectors` helper in new tests
- chore(popover): stop using `getPositionsBySelectors` helper in new tests
- chore(popover): fix typo in feature step definition
HendrikThePendric added a commit to dhis2/ui that referenced this issue Jun 9, 2020
Commit history (before squashing)
- fix(popover): add position absolute to arrow to prevent misplacement (See floating-ui/react-popper#354)
- test(popover): add e2e tests and stories for arrow positions
- chore(popover): fix typo in feature file and test
- refactor(popper): replace e2e story decorator by helper component (This prepares the story for the new e2e test which are not going to be compatible with the decorator logic)
- fix(popper): use correct sharedPropType name
- test(popper): prepare e2e stories about different types of references
- test(popper): add e2e test and stories for different types of references
- chore(popper): stop using `getPositionsBySelectors` helper in new tests
- chore(popover): stop using `getPositionsBySelectors` helper in new tests
- chore(popover): fix typo in feature step definition
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

No branches or pull requests

4 participants