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

Avoid document.write in Chrome with iframe linker #9836

Merged
merged 1 commit into from
Nov 15, 2023

Conversation

zbynek
Copy link
Contributor

@zbynek zbynek commented Jun 16, 2023

This PR avoids document.write in Chrome browser. It cannot be simply removed for all browsers for reasons mentioned in the code comment (I verified it's still valid in Firefox, didn't check Safari or IE).

Motivation
Same as #9814 -- document.write is considered bad for perfomance (flagged by Lighthouse performance audits) and completely forbidden in some contexts (chrome extensions, packaged chrome apps).

Testing
This is part of GeoGebra codebase for a few months (via geogebra/geogebra@ac5c880) and we didn't notice any problems.

@niloc132
Copy link
Contributor

I verified it's still valid in Firefox

Does this mean it is still required in Firefox?

Also, is there any way to test rather than looking for a string in user agent? The unfinished linked patch for example is waiting on detecting the presence of currentScript before trying to use it.

@zbynek
Copy link
Contributor Author

zbynek commented Jun 28, 2023

Does this mean it is still required in Firefox?

That's what I meant, sorry for not being clear.

is there any way to test rather than looking for a string in user agent

I don't know. Not even sure if the behaviors in Firefox, IE and Safari that we're trying to avoid here can be tested in the same way.

@jhickman
Copy link
Contributor

The biggest issue I see with excluding the doc.open/write/close is for the compatibility mode potentially being different in the iframe as it is in the parent document. In chrome, if there's no doctype, it will use BackCompat mode and not CSS1Compat (that you get with the <!doctype html>).

I've been investigating other options, such as using the iframe srcdoc attribute, etc, but they seem to be asynchronous.

I was able to use frameDoc.implementation.createDocument(...) with CSS1Compat compatMode, but it doesn't seem to 'attach' it to the iframe.

@zbynek
Copy link
Contributor Author

zbynek commented Aug 15, 2023

@jhickman thanks for checking this. Do you think the compatibility mode matters for the hidden iframe? Since that frame does not render any HTML, only serves as "sandbox" for loading JS, it might not be an issue.

@jhickman
Copy link
Contributor

@zbynek - you may be right; but it would need to be determined if that's the case. I don't imagine the iframe being used as any kind of 'staging' area for HTML content. Will have to trace uses of the 'getInstallLocatioDoc()' call.

@jhickman
Copy link
Contributor

AFAICT, all the uses of getInstallLocationDoc() are only appending <script> elements. I'm unsure of the original reason behind using the document.write in this particular case. Possibly only for the sake of making this nested iframe use the same compatibility mode as the host page?

@zbynek
Copy link
Contributor Author

zbynek commented Aug 17, 2023

I'm unsure of the original reason behind using the document.write in this particular case

I think the main reason is to make sure the document is accessible immediately in all browsers - if you skip document.write FireFox will create a new document asynchronously and any previously appended scripts will get lost. The part with adding a doctype is indeed related to compatibility mode, but only known to fail in IE8 according to 842e1ef

@jhickman
Copy link
Contributor

@zbynek - you may be right. I've been looking into this and can't really find any firm information on it. In a release or two when GWT cuts out IE11 entirely, then we may not even need that bit of code at all and just allow the browser to choose the compatMode of the newly constructed iframe.

Although, we don't necessarily have to avoid using document.write in chrome altogether; maybe just with extensions and other environments (embedded) that have restricted API access. I would almost suggest a separate linker specifically for browser extensions?

@zbynek
Copy link
Contributor Author

zbynek commented Aug 22, 2023

Since document.write also raises a flag in Lighthouse performance reports, I think this is beneficial even when developing standard webapps, not just extensions. But if the concerns outweigh the benefits we can just keep using a custom linker.

@niloc132 niloc132 merged commit 43ca6fb into gwtproject:main Nov 15, 2023
3 checks passed
@zbynek zbynek deleted the no-write-iframe branch November 15, 2023 02:54
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