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

HTML: document.open() and reload #12555

Merged
merged 4 commits into from
Aug 23, 2018
Merged
Changes from 3 commits
Commits
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
Original file line number Diff line number Diff line change
@@ -0,0 +1,66 @@
// This test tests for the nonexistence of a reload override buffer, which is
Copy link
Member

Choose a reason for hiding this comment

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

Name should be -historical, I think.

Copy link
Member Author

Choose a reason for hiding this comment

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

In that case most tests I added should be -historical…

Copy link
Member

Choose a reason for hiding this comment

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

Fair enough, we can leave it as-is.

// used in a previous version of the HTML Standard to make reloads of a
// document.open()'d document load the written-to document rather than doing an
// actual reload of the document's URL.
//
// This test has a somewhat interesting structure compared to the other tests
// in this directory. It eschews the <iframe> structure used by other tests,
// since when the child frame is reloaded it would adopt the URL of the test
// page (the responsible document of the entry settings object), and the spec
// forbids navigation in nested browsing contexts to the same URL as their
// parent. To work around that, we use window.open() which does not suffer from
// that restriction.
Copy link
Member

Choose a reason for hiding this comment

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

Maybe explain that this test file serves two purposes: both as a test file, and as part of the test itself. Then annotate each comment step with something like [opener branch] or [top level branch]?

Copy link
Member Author

Choose a reason for hiding this comment

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

As discussed in person, it's not really clear which parts of the file could be considered which branch. The step numbers should suffice.

//
// In any case, this test as the caller of `document.open()` would be used both
// as the test file and as part of the test file. The `if (!opener)` condition
// controls what role this file plays.

if (!opener) {
Copy link
Member

Choose a reason for hiding this comment

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

Let's add some document.URL preconditions and postconditions to make things clearer.

async_test(t => {
const testURL = document.URL;
const blankURL = new URL("/common/blank.html", document.URL).href;

// 1. Open an auxiliary window.
const win = window.open("/common/blank.html");
t.add_cleanup(() => { win.close(); });

win.addEventListener("load", t.step_func(() => {
// The timeout seems to be necessary for Firefox, which when `load` is
// called may still have an active parser.
t.step_timeout(() => {
Copy link
Member

Choose a reason for hiding this comment

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

What's the setTimeout for? Needs a comment.

Copy link
Member Author

@TimothyGu TimothyGu Aug 21, 2018

Choose a reason for hiding this comment

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

Good question, I think it might no longer be necessary.

Copy link
Member Author

Choose a reason for hiding this comment

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

It seems to be necessary for Firefox. Added a comment.

const doc = win.document;
assert_equals(doc.body.childNodes.length, 0, "precondition");
assert_equals(doc.URL, blankURL, "precondition");

window.onChildLoad = t.step_func(message => {
// 3. The dynamically overwritten content will trigger this function,
// which puts in place the actual test.

assert_equals(message, "Written", "script on written page is executed");
assert_equals(window.document.URL, testURL, "postcondition: after document.write()");
window.onChildLoad = t.step_func_done(message => {
// 6. This function should be called from the if (opener) branch of
// this file. It would throw an assertion error if the overwritten
// content was executed instead.
assert_equals(message, "Done!", "actual test");
assert_equals(window.document.URL, testURL, "postcondition: after reload");
});

// 4. Reload the pop-up window. Because of the doc.open() call, this
// pop-up window will reload to the same URL as this test itself.
win.location.reload();
});

// 2. When it is loaded, dynamically overwrite its content.
assert_equals(doc.open(), doc);
assert_equals(doc.URL, testURL, "postcondition: after document.open()");
doc.write("<p>Content</p><script>opener.onChildLoad('Written');</script>");
doc.close();
}, 100);
}), { once: true });
}, "Reloading a document.open()'d page should reload the URL of the entry realm's responsible document");
} else {
// 5. Since this window is window.open()'d, opener refers to the test window.
// Inform the opener that reload succeeded.
opener.onChildLoad("Done!");
}