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

Make window & document references iframe-aware #177

Closed
wants to merge 3 commits into from
Closed

Make window & document references iframe-aware #177

wants to merge 3 commits into from

Conversation

acusti
Copy link
Contributor

@acusti acusti commented Jul 18, 2016

This PR uses the DOM element’s ownerDocument rather than the global document or window to make react-draggable work across iframes. In our use case, we have an app being rendered into an iframe with React Frame Component, so the DOM elements are in a separate document from the document where the React elements are being executed.

I chose to replace the instanceOf Node check with a simple duck-typing check (https://github.com/mzabriskie/react-draggable/compare/master...Brandcast:iframe?expand=1#diff-9f4a38358e68eff5cf48eda850801a7aL190), which could be adapted to whatever type of conditional. It would even do the same check, but to work across iframes, it would need to first find the window object relative to e.target. Let me know if you want me to change or update that.

@STRML
Copy link
Collaborator

STRML commented Jul 18, 2016

Interesting - thanks for this PR!

Could you please remove the built files? I prefer PRs to be source-only, then I will rebuild and release in the same commit as the tag.

Regarding the Node check, could we not use domNode.ownerDocument.defaultView.Node as well?

@acusti
Copy link
Contributor Author

acusti commented Jul 18, 2016

Yeah, that’s what I alluded to in the PR comment. I’ll make that change, squash, and force update with that change. Also, I’ll remove the built files.

@acusti
Copy link
Contributor Author

acusti commented Jul 18, 2016

Quick question: what kind of browser support do you aim for? document.defaultView is only available in IE 9 and greater (ref). To support IE 8, I would need to add a fallback to use the non-standard parentWindow (ref), like:

var doc = el.ownerDocument;
var win = 'defaultView' in doc ? doc.defaultView : doc.parentWindow;

@STRML
Copy link
Collaborator

STRML commented Jul 18, 2016

I don't care for IE8 at this point. Some Babel transforms are beginning to break IE8 as well. I don't see the need.

nikolas and others added 3 commits July 19, 2016 08:26
Use the ownerDocument and window of the DOM element being manipulated
rather than the global window and document objects. This makes
react-draggable work with a tool like
https://github.com/ryanseddon/react-frame-component
@acusti
Copy link
Contributor Author

acusti commented Jul 19, 2016

Ok, so I removed the built files and updated the Node check to use the window relative to domNode.

By the way, in our use case, we are also running into #170. But I noticed that #173, which attempts to resolve that issue, relies on the global document.body, which in the interests of keeping draggable compatible across iframes would need to become node.ownerDocument.body.

@STRML STRML changed the title Make react-draggable DOM manipulation relative to elements being manipulated Make window & document references iframe-aware Jul 19, 2016
@acusti
Copy link
Contributor Author

acusti commented Jul 19, 2016

Do you want me to have a go at writing a few tests for this behavior? It should be trivial to pull in the react-frame-component package for the tests and just duplicate some of the draggable tests for components being rendered into the iframe. It might even be enough to just validate the should render with style translate() for DOM nodes test across iframes.

@acusti
Copy link
Contributor Author

acusti commented Jul 19, 2016

Also, I’ll rebase.

@STRML
Copy link
Collaborator

STRML commented Jul 19, 2016

Actually wait a bit - I was actually going to push a few updates to this in just a moment.

@acusti
Copy link
Contributor Author

acusti commented Jul 19, 2016

Sure thing. And thanks!

@STRML
Copy link
Collaborator

STRML commented Jul 19, 2016

Closed in #180 - thought it was easier to do so now with all the changes going through.

Would love a working test though - could you make a new PR for that based off master?

Thanks for the help.

@STRML STRML closed this Jul 19, 2016
@acusti
Copy link
Contributor Author

acusti commented Jul 19, 2016

Definitely. How do you feel about adding the devDependency on react-frame-component? I’d peg it to 0.6.2 to make sure updates to the component couldn’t break your tests.

@STRML
Copy link
Collaborator

STRML commented Jul 19, 2016

Yeah that's fine as a devDep.

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