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

Run getComputedStyle in the same context as the target element #678

Conversation

mckenna
Copy link

@mckenna mckenna commented Aug 31, 2018

I work on an embedded JavaScript app. We inject an iframe into the page that we're embedded on and the JavaScript is inserted and run in the context of that iframe so we can run in isolation from the parent page.

This works well, but there's an issue in Firefox, specifically with getComputedStyle(), where, if you try to run this method in the context of an iframe, it always returns null.

I'm looking to introduce Popper into my embedded app, but because of the bug above, Popper can't position my elements because getComputedStyle returns null. Unfortunately, this issue has been open for almost a decade and is unlikely to be fixed any time soon.

Because the elements themselves live on the parent page, we can work around this by getting the window of the document that the element is embedded in, and run getComputedStyle() in that context.

This PR solves our specific use case, and should work exactly as before for any other supported browser.

Let me know what you think.

@FezVrasta
Copy link
Member

Thank you @mckenna!

@ryanseddon sorry to bother you, but since you worked on (#607) which is pretty similar, may you review this PR and let me know if anything looks off?

@ryanseddon
Copy link

You would not believe this but we had this issue yesterday and tracked it down to this same function and I was about to do a similar PR. So approved!

@FezVrasta FezVrasta merged commit 6d98b8b into floating-ui:master Sep 13, 2018
@mckenna mckenna deleted the p/get-computed-style-within-element-context branch September 13, 2018 10:33
@mckenna
Copy link
Author

mckenna commented Oct 8, 2018

Nice one on merging this 👍 — any idea when you plan to do another release?

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.

None yet

3 participants