-
Notifications
You must be signed in to change notification settings - Fork 3.1k
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
Test moving <script>s between documents #5911
Conversation
Firefox (nightly)Testing web-platform-tests at revision 65891e2 All results1 test ran/html/semantics/scripting-1/the-script-element/moving-between-documents.html
|
Chrome (unstable)Testing web-platform-tests at revision 4a26457 All results1 test ran/html/semantics/scripting-1/the-script-element/moving-between-documents.html
|
Test result summary for classic scripts, with blank cells meaning browsers match the proposed spec:
For module scripts, Edge interestingly starts failing the "Moving to a document where scripting is disabled during fetching" case (via onload + executes), but passing the first one. |
iframe.contentDocument.close(); | ||
|
||
assert_false(window.didExecute, "The script must not have executed in this window"); | ||
assert_equals(iframe.contentWindow.didExecute, undefined, |
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.
Even if the script tested ("s1") is not executed, the test fails on Chromium, because didExecute is false
(not undefined
).
iframe.contentWindow.didExecute
is set to false
at Line 32 and is reset to undefined when document.write() is called on Firefox (but not on Chromium) at Line 33.
Minimized case:
<body>
<script>
const iframe = document.createElement("iframe");
iframe.onload = () => {
iframe.contentWindow.didExecute = false;
iframe.contentDocument.write("<div>");
console.log(iframe.contentWindow.didExecute);
};
document.body.appendChild(iframe);
</script>
</body>
console log: undefined
on Firefox and false
on Chromium.
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.
Ah, because of the implicit document.open() in document.write(), OK. I've uploaded a patch that just uses undefined vs. true instead of false vs. true which should fix this. Please take a look :)
861e061
to
0e5ba75
Compare
Sauce (safari)Testing web-platform-tests at revision 65891e2 All results1 test ran/html/semantics/scripting-1/the-script-element/moving-between-documents.html
|
Sauce (MicrosoftEdge)Testing web-platform-tests at revision 4a26457 Unstable results
All results1 test ran/html/semantics/scripting-1/the-script-element/moving-between-documents.html
|
for (const type of ["text/javascript", "module"]) { | ||
async_test(t => { | ||
t.add_cleanup(() => { | ||
window.didExecute = undefined; |
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.
Maybe also make this remove the iframe? (Same below.)
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.
Happy to do so, but why?
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.
I guess it's fine either way. Just seems a little cleaner to only have the results.
assert_equals(iframe.contentWindow.didExecute, undefined, | ||
"The script must not have executed in the iframe window"); | ||
t.done(); | ||
}, 3000); |
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.
Why a timeout here? Since the parser inserts the script element, wouldn't it simply block on it loading if a user agent doesn't implement the spec?
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.
That's also why the belated onload/onerror handlers seem problematic. They probably need to be in the markup, no?
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.
They should probably be before setting s.src, it's true.
assert_equals(window.didExecute, undefined, "The script must not have executed in this window"); | ||
t.done(); | ||
}, 3000); | ||
}, `${type}: moving to a document where scripting is disabled during fetching`); |
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.
I think using srcdoc with sandbox might be better or good as an alternative test, since it seems likely browsers would check "browsing-context connected" and not just "connected" (and in fact this is what Chrome, Edge, and Safari seem to do).
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.
The definition of scripting-is-disabled means scripting is disabled for browsing-context-less nodes.
There's a separate question as to whether the prepare-a-script algorithm should be using connected vs. BC connected for fetching, but I'd rather keep that separate...
Anyway, happy to also test sandboxed iframes.
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.
Hmm, on second thought, I'm unsure why we'd have such a test since it would always involve creating a new window (and thus settings object). srcdoc="" does not avoid that, as far as I can tell. So it'd be the same as other iframe tests.
If you disagree I'm happy for your to push an additional test showing me what you mean.
This follows the changes in whatwg/html#2673, but also tests the issue at whatwg/html#2137 in favor of the current spec.
…id Firefox vs. others issue
0e5ba75
to
e70aaf6
Compare
Build PASSEDStarted: 2018-02-21 20:54:30 Failing Jobs
View more information about this build on: |
Turns out the timeout is needed at least a bit
Intent to deprecate and remove: https://groups.google.com/a/chromium.org/d/topic/blink-dev/KyB2mwOmjrk/discussion This CL also removes crash tests that require executing scripts moved between documents. Code paths related to scripts moved between documents will be covered by e.g. web-platform-tests/wpt#5911 Bug: 721914 Change-Id: I6acb7182c4a62accd5e5285adb6a77a2cd891465 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1843508 Reviewed-by: Kouhei Ueno <[email protected]> Commit-Queue: Hiroshige Hayashizaki <[email protected]> Cr-Commit-Position: refs/heads/master@{#704503}
Superceded by #19632 |
This follows the changes in whatwg/html#2673, but also tests the issue at whatwg/html#2137 in favor of the current spec.