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.

Sign up for GitHub

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

Jump to bottom

Make sure content is loaded when doc is ready #206

Closed
wants to merge 1 commit into from
Closed

Make sure content is loaded when doc is ready #206

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions src/Frame.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,7 @@ export class Frame extends Component {

const doc = this.getDoc();
if (doc && doc.readyState === 'complete') {
this.handleLoad();
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So this seems to break for everyone outside of next.js so I'm wondering what next.js is doing to have to trigger the load event manually?

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This readyState code is here to handle cross browser differences

  • Chromium based browsers don't trigger onload and need the forceUpdate to render and trigger the load event
  • Firefox doesn't use readyState and instead will update contents on iframe load

If we introduce a third call then chromium based browsers throw as it tries to render the contents before srcdoc has injected it (srcdoc is async) so there may be a timing issue here. I'm unsure why this works in next.js based apps in chromium based browsers??

Firefox works fine with as this addition is not a codepath firefox reaches

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I could definitely see this being an async timing issue. My initialContent is fairly large and includes external stylesheets like FontAwesome and stuff, which I could see causing some timing issue if it takes too long to load, especially on a page refresh where external resources might have to reload but other things might be cached and thus trigger things in a different order, but randomly on a later refresh the timing is off in the other direction and it works fine.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My initialContent is fairly large and includes external stylesheets like FontAwesome and stuff, which I could see causing some timing issue if it takes too long to load

Interesting, do you think you could share a test case with your intitialContent?

In some further exploring it seems the addEventListener in compnentDidMount is superfluous and actually triggers a double load event in firefox.

Copy link

@calculuschild calculuschild Jan 6, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would think you could just take @baptisteArno 's test case and apply my initialContent over the top of it.

getInitialState : function() {
return initialContent : `<!DOCTYPE html><html><head>
	<link href="//use.fontawesome.com/releases/v5.15.1/css/all.css" rel="stylesheet" />
	<link href="//fonts.googleapis.com/css?family=Open+Sans:400,300,600,700" rel="stylesheet" type="text/css" />
	<base target=_blank>
	</head><body style='overflow: hidden'><div></div></body></html>`
}

...

<Frame initialContent={this.state.initialContent} > .... </Frame>

You can also try using my repo https://github.com/naturalcrit/homebrewery and just update react-frame-component to see it break. Unfortunately I'm having trouble isolating a smaller version of the app because it's inherited from someone else and I don't know all the ins and outs.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did you mean this PR breaks for everyone not using next.js or the original issue occurs for everyone not using next.js? I can verify this PR fixes the issue for me. I'm using Chrome on node.js, not next.js.

This PR breaks, see the github action tests. It complains about the mountTarget being null as the inititalContent provided to the iframe via srcdoc hasn't rendered yet.

It might be that next.js has aggressive caching and that's why it manifests itself there more than elsewhere

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The onLoad property on the iframe.... should it be onload, lowercase? https://www.w3schools.com/jsref/event_onload.asp

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it's a react thing it'll get normalised to onload when it's written to the DOM

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Gotcha. Like className to class. Ok.

Another possibility. Seems like if a page is in cache, onLoad can potentially trigger right away, before the handler is even assigned to onLoad.
https://stackoverflow.com/questions/19917436/window-onload-not-running-on-page-refresh

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks seems like there could be something related there. This is issue is hard to diagnose as it hasn't been easy for me to reproduce if someone could create a codesandbox.io where it is easily recreated it would help a lot.

this.forceUpdate();
} else {
this.nodeRef.current.addEventListener('load', this.handleLoad);
Expand Down