-
-
Notifications
You must be signed in to change notification settings - Fork 156
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
Add document.write() fallback #248
base: master
Are you sure you want to change the base?
Conversation
@@ -253,6 +253,27 @@ describe('The Frame Component', () => { | |||
); | |||
}); | |||
|
|||
it.only('should allow setting initialContent via document.write() when required', done => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ryanseddon this test fails because contentDidMount
isn't being triggered when using document.write()
. I had a look through the discussions on #206 and #207 and got quite confused about what needs to be done in this situation 😅 Do you have any pointers to share to resolve this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok so got this working by working backwards and comparing v4.x.x to latest and seeing what the diff was, you might be able to tidy this up and it may have redundant checks
The main thing with test is doc.write was synchronous and didn't need the async done function passed in.
diff --git a/src/Frame.jsx b/src/Frame.jsx
index 151fa4e..91442c4 100644
--- a/src/Frame.jsx
+++ b/src/Frame.jsx
@@ -47,11 +47,20 @@ export class Frame extends Component {
const doc = this.getDoc();
- if (doc) {
- this.nodeRef.current.contentWindow.addEventListener(
- 'DOMContentLoaded',
- this.handleLoad
- );
+ if (!this.props.dangerouslyUseDocWrite) {
+ if (doc) {
+ this.nodeRef.current.contentWindow.addEventListener(
+ 'DOMContentLoaded',
+ this.handleLoad
+ );
+ }
+ } else {
+ // eslint-disable-next-line no-lonely-if
+ if (doc && doc.readyState === 'complete') {
+ this.forceUpdate();
+ } else {
+ this.nodeRef.addEventListener('load', this.handleLoad);
+ }
}
}
@@ -62,6 +71,8 @@ export class Frame extends Component {
'DOMContentLoaded',
this.handleLoad
);
+
+ this.nodeRef.removeEventListener('load', this.handleLoad);
}
getDoc() {
@@ -76,7 +87,7 @@ export class Frame extends Component {
return doc.body.children[0];
}
- setRef = node => {
+ setRef = (node) => {
this.nodeRef.current = node;
const { forwardedRef } = this.props; // eslint-disable-line react/prop-types
@@ -88,10 +99,14 @@ export class Frame extends Component {
};
handleLoad = () => {
- clearInterval(this.loadCheck);
- // Bail update as some browsers will trigger on both DOMContentLoaded & onLoad ala firefox
- if (!this.state.iframeLoaded) {
- this.setState({ iframeLoaded: true });
+ if (this.props.dangerouslyUseDocWrite) {
+ this.forceUpdate();
+ } else {
+ clearInterval(this.loadCheck);
+ // Bail update as some browsers will trigger on both DOMContentLoaded & onLoad ala firefox
+ if (!this.state.iframeLoaded) {
+ this.setState({ iframeLoaded: true });
+ }
}
};
@@ -150,6 +165,7 @@ export class Frame extends Component {
if (!this.props.dangerouslyUseDocWrite) {
props.srcDoc = this.props.initialContent;
+ props.onLoad = this.handleLoad;
}
delete props.head;
@@ -161,8 +177,10 @@ export class Frame extends Component {
delete props.forwardedRef;
return (
- <iframe {...props} ref={this.setRef} onLoad={this.handleLoad}>
- {this.state.iframeLoaded && this.renderFrameContents()}
+ <iframe {...props} ref={this.setRef}>
+ {this.props.dangerouslyUseDocWrite
+ ? this.renderFrameContents()
+ : this.state.iframeLoaded && this.renderFrameContents()}
</iframe>
);
}
diff --git a/test/Frame.spec.jsx b/test/Frame.spec.jsx
index 6e8b784..cb34daa 100644
--- a/test/Frame.spec.jsx
+++ b/test/Frame.spec.jsx
@@ -253,7 +253,7 @@ describe('The Frame Component', () => {
);
});
- it.only('should allow setting initialContent via document.write() when required', done => {
+ it.only('should allow setting initialContent via document.write() when required', () => {
div = document.body.appendChild(document.createElement('div'));
const initialContent =
@@ -261,17 +261,12 @@ describe('The Frame Component', () => {
const renderedContent =
'<html><head><script>console.log("foo");</script></head><body><div><div class="frame-content"></div></div></body></html>';
const frame = ReactDOM.render(
- <Frame
- dangerouslyUseDocWrite
- initialContent={initialContent}
- contentDidMount={() => {
- const doc = ReactDOM.findDOMNode(frame).contentDocument;
- expect(doc.documentElement.outerHTML).to.equal(renderedContent);
- done();
- }}
- />,
+ <Frame dangerouslyUseDocWrite initialContent={initialContent} />,
div
);
+ const doc = ReactDOM.findDOMNode(frame).contentDocument;
+ expect(doc.documentElement.outerHTML).to.equal(renderedContent);
+
});
it('should allow setting mountTarget', done => {
Thanks for the input on this @ryanseddon 🙇 We've got a couple of heavy projects on the go at the moment so this has been sidelined whilst those are underway. I hope to return to it once they're shipped so we can wrap this up 🤞 |
@andrewpye @ryanseddon |
As per #169 (comment), this PR adds a new
dangerouslyUseDocWrite
prop to theFrame
component as an "escape hatch" that allows consumers to fall back to usingdocument.write()
to populate the frame's initial content. This is usually not advisable, but some libraries such as Recaptcha and Google Maps will only work with certain values of the frame's location and/or origin, and the currentsrcdoc
approach for initial content population is incompatible with these libraries.