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

Does not work with v1.14.2 of Popper.js #122

Closed
Maistho opened this issue Apr 3, 2018 · 20 comments · Fixed by floating-ui/floating-ui#599
Closed

Does not work with v1.14.2 of Popper.js #122

Maistho opened this issue Apr 3, 2018 · 20 comments · Fixed by floating-ui/floating-ui#599

Comments

@Maistho
Copy link

Maistho commented Apr 3, 2018

CodePen demo

I could for some reason not get it to show the error in the codepen when I tried earlier. Not sure exactly why.

But you can see the error here: https://reactstrap-error-tooltip.firebaseapp.com/

And the git repo is here: https://github.com/Maistho/reactstrap-tooltip-error

(I don't have the time right now, but I will try to create a new example that works in the codepen and without reactstrap tomorrow)

Steps to reproduce the problem

  1. Create a new app with create-react-app
  2. add bootstrap and reactstrap
  3. Add a tooltip
  4. Tooltip does not appear in correct position

What is the expected behavior?

Tooltip should appear in correct position

What went wrong?

Seems like Popper.js changes the DOM after react-popper has written styles, and by doing that it overwrites the styles (but doesn't add them after)

Any other comments?

See this issue for more information: reactstrap/reactstrap#938

And it's definitely related to this piece of code: floating-ui/floating-ui@7bfdedf

@ericgio
Copy link

ericgio commented Apr 3, 2018

I'm seeing the same issue. All the correct styles are being passed down to my popper component and I can see a flicker where it's correctly positioned for a split second, but then Popper.js seems to overwrite the values.

@TLadd
Copy link

TLadd commented Apr 4, 2018

Created a smaller case here using react-popper directly: https://github.com/TLadd/popper-test

import * as React from "react";
import ReactDOM from "react-dom";
import { Manager, Target, Popper } from "react-popper";

ReactDOM.render(
  <div
    style={{
      width: 500,
      height: 500,
      margin: "0 auto",
      paddingTop: 250,
      textAlign: "center"
    }}
  >
    <Manager>
      <Target>
        {({ targetProps }) => <div {...targetProps}>Target Box</div>}
      </Target>
      <Popper placement="top">
        {({ popperProps }) => <div {...popperProps}>Popper Content</div>}
      </Popper>
    </Manager>
  </div>,
  document.getElementById("root")
);

screen shot 2018-04-04 at 12 13 15 am

Locking popper.js to 1.14.1 renders correctly.
screen shot 2018-04-04 at 12 18 20 am

@ablamunits
Copy link

We started seeing this issue as well with the latest version of popper.js 1.14.2. Installing popper.js version 1.14.1 in our project resolves the issue.

@FezVrasta
Copy link
Member

I'm not sure what's the right way to fix this... Within Popper.js we need to reset those styles instantly to properly get some measurements, but doing so react-popper can't rely on the deferred React DOM manipulations.

@atomiks do you think could we reset those styles only when the update is triggered by a resize event? This would at least reduce the problem but I'm not sure if it would fix it completely.

FezVrasta pushed a commit to floating-ui/floating-ui that referenced this issue Apr 4, 2018
@TheSharpieOne
Copy link
Contributor

I wonder if react-popper should clone the styles when the data is passed to it's modifier since Popper.js seems to mutate the values of the styles. That way it has its own copy of the most recent styles. Or is it too late by the time the modifiers run?

@Maistho
Copy link
Author

Maistho commented Apr 4, 2018

@TheSharpieOne I don't think that would work since Popper.js modifies the DOM directly.

@FezVrasta
Copy link
Member

I don't think there's anything that react-popper can do

@Maistho
Copy link
Author

Maistho commented Apr 4, 2018

@FezVrasta I think that as long as react-popper can re-apply the styles after the DOM styles has been removed by Popper.js it should be fine. Could Popper.js perhaps add a hook that is run whenever the styles would be applied regularily?

@TheSharpieOne
Copy link
Contributor

Oh, I see, react-popper is applying the styles correctly, but after the render Popper.js does its modification directly to the DOM. Wouldn't that affect more than react-popper's usage, or is it because react-popper has applyStyles set to false?

FezVrasta pushed a commit to floating-ui/floating-ui that referenced this issue Apr 4, 2018
FezVrasta pushed a commit to floating-ui/floating-ui that referenced this issue Apr 4, 2018
@FezVrasta
Copy link
Member

May you guys test this version and tell me if it fixes your problem?

floating-ui/floating-ui#599

@TLadd
Copy link

TLadd commented Apr 4, 2018

Pulled and linked the popper.js branch down and it does fix the problem I was seeing.

@FezVrasta
Copy link
Member

Great! Going to release it soon

FezVrasta added a commit to floating-ui/floating-ui that referenced this issue Apr 4, 2018
@grumd
Copy link

grumd commented Apr 4, 2018

@FezVrasta by soon do you mean today? :)

@FezVrasta
Copy link
Member

https://github.com/FezVrasta/popper.js/releases/tag/v1.14.3

@albinekb
Copy link

albinekb commented Apr 4, 2018

Thanks @FezVrasta, that was a quick fix! 🎉

@grumd
Copy link

grumd commented Apr 4, 2018

@FezVrasta Thanks a lot! I spent several hours today trying to figure out the reason it broke, and you already fixed it, that was fast

@FezVrasta
Copy link
Member

You're welcome! And with this, thanks to @atomiks, we also fixed a long standing bug that almost made Popper.js useless 😅

@atomiks
Copy link
Collaborator

atomiks commented Apr 4, 2018

Sorry for the fix breaking everyone's projects 😞, at least now we've fixed the long-standing issue and thankfully react-popper could also be fixed.

@grumd
Copy link

grumd commented Apr 4, 2018

@atomiks I'm out of the loop here, can you please point me to the issue you fixed in 1.14.2? Just curious

@atomiks
Copy link
Collaborator

atomiks commented Apr 4, 2018

@grumd see floating-ui/floating-ui#251

Basically the popper would overflow the document boundaries on resize and more critically on update if the reference was close to the edge, causing it to not show fully.

(Update the CodePen to use 1.14.1 to see the difference when resizing the browser window)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

9 participants