-
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
HTML: document.open() and tasks #10818
Conversation
This is not consistently implemented and given that Firefox does not appear to do it at all I hope we can get away without it. Tests: web-platform-tests/wpt#10818.
Some other potential tests:
|
|
8cc338c
to
39aa1e6
Compare
w3c-test:mirror |
// second actually calls document.open() to test if the method call removes | ||
// that specific task from the queue. | ||
|
||
setup({ |
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.
Can you point to the reason this is needed? (the promise rejection test below)
Reason being that this can also mask other problems with the test, so usage should be well motivated. (and is)
t.add_cleanup(() => frame.remove()); | ||
frame.onload = t.step_func(() => { | ||
// Make sure there is no parser. Firefox seems to have an additional | ||
// non-spec-compliant readiness state "uninitialized", so test for the |
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.
Can you file/link to a bug for this?
|
||
async_test(t => { | ||
const frame = document.body.appendChild(document.createElement("iframe")); | ||
// The empty HTML seems to be necessary to cajole Chrome into firing a load |
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.
Is there a bug that this could be linked to?
|
||
taskTest("timeout", (t, frame, open) => { | ||
let happened = false; | ||
// Work around the lint script, since we can't use step_timeout here. |
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 would probably add this to lint.whitelist, even if that means it's possible to accidentally add setTimeout elsewhere in this file. It's a small file, and that this fools the linter is arguably just a bug. (For more granular opt-outs some special comment syntax would be an option.)
taskTest("timeout", (t, frame, open) => { | ||
let happened = false; | ||
// Work around the lint script, since we can't use step_timeout here. | ||
frame.contentWindow['setTimeout'](() => happened = true, 100); |
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.
Rather than setting happened
here, how about just t.step_func_done()
as the callback to pass the test ASAP?
The following test just times out if there isn't a 2nd message event, so I'd let this test also time out. If that's no good, is there any timeout one could pick that doesn't make the test more likely to be flaky on slow runners?
counter++; | ||
assert_less_than_equal(counter, 2); | ||
if (counter == 2) { | ||
t.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.
(This is the test that will time out after 30s or so.)
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.
This didn't seem to timeout in the latest Travis run…
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.
Travis unfortunately doesn't fail unless the tests are flaky, it doesn't help catch any other problem, even totally broken tests. Did you read the logs to see that it didn't time out, or how could you tell?
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.
Oh right, I guess I meant rather that it wasn't flaky.
}); | ||
}); | ||
|
||
taskTest("canvas.toBlob()", (t, frame, open) => { |
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 "queue a task" of toBlob
happens inside "in parallel" so might not have happened when document.open()
is called, is that a problem for this test? Does any browser currently fail 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.
Good point. No browser actually seems to fail this, but I should note that setTimeout
is defined the same way (wait in parallel, and then queue a task), and browsers have very different behavior with regards to that. That's why I think this is a good test.
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.
Interesting, I didn't know setTimeout
did that internally in the spec. I tend to think of "in parallel" as possibly happening on another thread or process (thus requiring "queue a task" to rejoin main thread) and that's certainly not how setTimeout
is implemented. Test LGTM in any case, at worst it'll pass everywhere always :)
const doc = frame.contentDocument; | ||
const marquee = doc.body.appendChild(doc.createElement("marquee")); | ||
open(frame.contentDocument); | ||
marquee.addEventListener("start", t.step_func_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.
I like this one. If you want one which is a bit special in practice try a new audio element and changing the volume attribute to 0.5. That should queue a task to fire volumechange. But implementations do something special to emulate that per-instance task source that media elements are supposed to have, and I wouldn't be surprised if it's different from other things tested here.
Based on the work by Anne van Kesteren in whatwg#3651, but without the parts concerning the session history. Changes to that area will come later (see whatwg#3818). Tests: web-platform-tests/wpt#10773 Tests: web-platform-tests/wpt#10778 Tests: web-platform-tests/wpt#10789 Tests: web-platform-tests/wpt#10815 Tests: web-platform-tests/wpt#10818 Fixes whatwg#1698. Fixes whatwg#3286. Fixes whatwg#3306. Closes whatwg#3665. Co-authored-by: Anne van Kesteren <annevk@annevk.nl>
Based on the work by Anne van Kesteren in whatwg#3651, but without the parts concerning the session history. Changes to that area will come later (see whatwg#3818). Tests: web-platform-tests/wpt#10773 Tests: web-platform-tests/wpt#10778 Tests: web-platform-tests/wpt#10815 Tests: web-platform-tests/wpt#10818 Fixes whatwg#1698. Fixes whatwg#3286. Fixes whatwg#3306. Closes whatwg#3665. Co-authored-by: Anne van Kesteren <annevk@annevk.nl>
In particular, removes the realm creation, document unloading, and tasks removal steps. Based on the work by Anne van Kesteren in #3651, but without the parts concerning the session history. Changes to that area will come later (see #3818). These changes allow us to remove several auxiliary concepts that only existed to support document.open(): - The recycle parameter to the "unload a Document" algorithm - The window parameter to the "set the active document" algorithm Tests: web-platform-tests/wpt#10773 Tests: web-platform-tests/wpt#10778 Tests: web-platform-tests/wpt#10815 Tests: web-platform-tests/wpt#10818 Fixes #1698. Fixes #3286. Fixes #3306. Closes #3665.
1c519a7
to
f07c4e2
Compare
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.
Only remaining comments requiring change are the "link to bug" ones, if you like.
c90163d
to
176e9c0
Compare
176e9c0
to
fcf210e
Compare
In particular, removes the realm creation, document unloading, and tasks removal steps. Based on the work by Anne van Kesteren in whatwg#3651, but without the parts concerning the session history. Changes to that area will come later (see whatwg#3818). These changes allow us to remove several auxiliary concepts that only existed to support document.open(): - The recycle parameter to the "unload a Document" algorithm - The window parameter to the "set the active document" algorithm Tests: web-platform-tests/wpt#10773 Tests: web-platform-tests/wpt#10778 Tests: web-platform-tests/wpt#10815 Tests: web-platform-tests/wpt#10818 Fixes whatwg#1698. Fixes whatwg#3286. Fixes whatwg#3306. Closes whatwg#3665.
In particular, removes the realm creation, document unloading, and tasks removal steps. Based on the work by Anne van Kesteren in whatwg#3651, but without the parts concerning the session history. Changes to that area will come later (see whatwg#3818). These changes allow us to remove several auxiliary concepts that only existed to support document.open(): - The recycle parameter to the "unload a Document" algorithm - The window parameter to the "set the active document" algorithm Tests: web-platform-tests/wpt#10773 Tests: web-platform-tests/wpt#10778 Tests: web-platform-tests/wpt#10815 Tests: web-platform-tests/wpt#10818 Fixes whatwg#1698. Fixes whatwg#3286. Fixes whatwg#3306. Closes whatwg#3665.
For whatwg/html#3651.