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 the beforeunload event #10778

Merged
merged 4 commits into from
Aug 17, 2018

Conversation

annevk
Copy link
Member

@annevk annevk commented May 2, 2018

@annevk
Copy link
Member Author

annevk commented May 2, 2018

I wasn't quite sure if you could automate this. Suggestions welcome. It seems that Edge and Firefox do fire the beforeunload event for document.open(), but Chrome and Safari do not. (Edge is a bit surprising here, as it generally seems to follow Chrome and Safari.)

If we could remove

Prompt to unload document. If the user refused to allow the document to be unloaded, then return document.

that would be quite nice.

cc @bzbarsky @jeisinger

Copy link
Member

@foolip foolip left a comment

Choose a reason for hiding this comment

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

LGTM if my comments don't lead to any non-trivial changes.

@@ -0,0 +1,12 @@
async_test(t => {
const frame = document.body.appendChild(document.createElement("iframe"));
t.add_cleanup(() => frame.remove());
Copy link
Member

Choose a reason for hiding this comment

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

This doesn't help much when there's a single test, and leaving it in might make debugging easier.

Copy link
Member

Choose a reason for hiding this comment

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

We don't want the test to time out, and having frame.remove() here is the easiest way to ensure that the load event is fired on the top-level Window no matter the outcome of the test.

Copy link
Member

Choose a reason for hiding this comment

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

Ah, I assumed it was copypasta from something with multiple tests. Never mind me.

frame.contentWindow.onbeforeunload = t.unreached_func("beforeunload should not be fired");
frame.contentDocument.open();
t.step_timeout(t.step_func_done(() => {
frame.contentDocument.close();
Copy link
Member

Choose a reason for hiding this comment

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

Can you add a comment describing why it's helpful to invoke document.close() here? I don't quite see why that would fire the beforeunload event, which is the only thing that would change the outcome of the test...

Copy link
Member

Choose a reason for hiding this comment

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

Sure. The intent here is to test that open() does not fire the beforeunload event. The close() here is merely to fire the load event on the document and unblock testharness from thinking that the page has not yet loaded.

Copy link
Member

Choose a reason for hiding this comment

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

Added a comment. LMK if that works.

TimothyGu added a commit to TimothyGu/html that referenced this pull request Aug 14, 2018
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 <[email protected]>
TimothyGu added a commit to TimothyGu/html that referenced this pull request Aug 14, 2018
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 <[email protected]>
@TimothyGu TimothyGu force-pushed the annevk/document-open-steps-beforeunload branch from db2ff6b to 7d4d95f Compare August 14, 2018 19:18
domenic pushed a commit to whatwg/html that referenced this pull request Aug 16, 2018
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.
@@ -0,0 +1,12 @@
async_test(t => {
const frame = document.body.appendChild(document.createElement("iframe"));
t.add_cleanup(() => frame.remove());
Copy link
Member

Choose a reason for hiding this comment

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

Ah, I assumed it was copypasta from something with multiple tests. Never mind me.

@TimothyGu TimothyGu merged commit eafe4bd into master Aug 17, 2018
@TimothyGu TimothyGu deleted the annevk/document-open-steps-beforeunload branch August 17, 2018 14:06
mustaqahmed pushed a commit to mustaqahmed/html that referenced this pull request Feb 15, 2019
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.
mustaqahmed pushed a commit to mustaqahmed/html that referenced this pull request Feb 15, 2019
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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants