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.

Sign up for GitHub

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

Jump to bottom

HTML: document.open() and reload #12555

Merged

TimothyGu merged 4 commits into master from timothygu/document-open-reload

Aug 23, 2018

Member

TimothyGu commented Aug 17, 2018 •

edited

Loading

For whatwg/html#3946.

wpt-pr-bot added the html label

wpt-pr-bot requested review from annevk, domenic, foolip, jdm, jgraham, sideshowbarker, zcorpan and zqzhang

August 17, 2018 21:30

TimothyGu mentioned this pull request

document.open() simplifications, part 2 whatwg/html#3946

Merged

TimothyGu added a commit to TimothyGu/html that referenced this pull request


          document.open() simplifications, part 2

4fc7e01

The current behavior of adding a new entry to the session history is
removed, with the "replace" parameter behavior made the only option. To
fix whatwg#3885, we now reuse history.replaceState()'s history model, which
keeps the document's URL and the history entry URL in sync.

At the same time, we restrict the history state update (including
setting the document's URL) to be only run with fully active documents.
Firefox already bails out for non-fully active documents (including
those that are active documents), and it's not expected that there would
be much usage of the URL of non-fully active documents anyway.

This allows us to additionally remove the seldom implemented "replace"
parameter in document.open(), whose behavior is now the default
(aligning with with Chrome and Safari). The IDL is modified accordingly.

This PR also removes the "overridden reload" algorithm and related
concepts, which were previously only implemented in Firefox and Edge,
causing developer confusion evident in
https://bugzilla.mozilla.org/show_bug.cgi?id=556002.

Tests: web-platform-tests/wpt#12555
Tests: … (more coming soon)

Fixes whatwg#3564.
Fixes whatwg#3885.

domenic reviewed

View reviewed changes

Member

domenic left a comment

Logic looks good, if mind-bending. Some ideas on improving clarity.

html/webappapis/dynamic-markup-insertion/opening-the-input-stream/reload.window.js

		@@ -0,0 +1,53 @@
		// This test tests for the nonexistence of a reload override buffer, which is

Member

domenic Aug 21, 2018

Name should be -historical, I think.

Member Author

TimothyGu Aug 21, 2018

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

Member

domenic Aug 21, 2018

Fair enough, we can leave it as-is.

html/webappapis/dynamic-markup-insertion/opening-the-input-stream/reload.window.js Outdated

+              // This test tests for the nonexistence of a reload override buffer, which 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 URL.

Member

domenic Aug 21, 2018

"the URL" -> "the document's URL" or "the document's current URL"?

html/webappapis/dynamic-markup-insertion/opening-the-input-stream/reload.window.js Outdated

+                  t.add_cleanup(() => { win.close(); });
+                  win.addEventListener("load", t.step_func(() => {
+                    t.step_timeout(() => {

Member

domenic Aug 21, 2018

What's the setTimeout for? Needs a comment.

Member Author

TimothyGu Aug 21, 2018 •

edited

Loading

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

Member Author

TimothyGu Aug 21, 2018

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

html/webappapis/dynamic-markup-insertion/opening-the-input-stream/reload.window.js Outdated

+              // 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.

Member

domenic Aug 21, 2018

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]?

Member Author

TimothyGu Aug 21, 2018

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

html/webappapis/dynamic-markup-insertion/opening-the-input-stream/reload.window.js Outdated

+              // parent. To work around that, we use window.open() which does not suffer from
+              // that restriction.
+              if (!opener) {

Member

domenic Aug 21, 2018

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

TimothyGu added 3 commits

August 21, 2018 16:48


          HTML: document.open() and reload

bff6011


          comment revision

4efa609


          Add some more assertions for URL

d8ef463

Member Author

TimothyGu commented Aug 21, 2018

@domenic comments should be fixed. PTAL.

TimothyGu force-pushed the timothygu/document-open-reload branch from 91473e6 to d8ef463 Compare

August 21, 2018 21:34

domenic approved these changes

View reviewed changes

TimothyGu added a commit to TimothyGu/html that referenced this pull request


          document.open() simplifications, part 2

19b17d6

The current behavior of adding a new entry to the session history is
removed, with the "replace" parameter behavior made the only option. To
fix whatwg#3885, we now reuse history.replaceState()'s history model, which
keeps the document's URL and the history entry URL in sync.

At the same time, we restrict the history state update (including
setting the document's URL) to be only run with fully active documents.
Firefox already bails out for non-fully active documents (including
those that are active documents), and it's not expected that there would
be much usage of the URL of non-fully active documents anyway.

This allows us to additionally remove the seldom implemented "replace"
parameter in document.open(), whose behavior is now the default
(aligning with with Chrome and Safari). The IDL is modified accordingly.

This PR also removes the "overridden reload" algorithm and related
concepts, which were previously only implemented in Firefox and Edge,
causing developer confusion evident in
https://bugzilla.mozilla.org/show_bug.cgi?id=556002.

Tests: web-platform-tests/wpt#12555
Tests: … (more coming soon)

Fixes whatwg#3564.
Fixes whatwg#3885.

foolip mentioned this pull request

Modify source path for wpt test of Accept-CH #12542

Merged

TimothyGu mentioned this pull request

Tracking issue for document.open() overhaul whatwg/html#3818

Closed

19 tasks

TimothyGu self-assigned this

foolip mentioned this pull request

./wpt check-stability fails with ManifestVersionMismatch if older MANIFEST.json is present #12635

Closed

Member

foolip commented Aug 22, 2018

Note that this PR is affected by #12635. Rebasing might fix it, but it's not fixed yet so no guarantees.

Member Author

TimothyGu commented Aug 22, 2018

Purging the cache fixed it.


          Add text content assertions; fix URL checks

b5c413e

TimothyGu added a commit to TimothyGu/html that referenced this pull request


          document.open() simplifications, part 2

f11424a

The current behavior of adding a new entry to the session history is
removed, with the "replace" parameter behavior made the only option. To
fix whatwg#3885, we now reuse history.replaceState()'s history model, which
keeps the document's URL and the history entry URL in sync.

At the same time, we restrict the history state update (including
setting the document's URL) to be only run with fully active documents.
Firefox already bails out for non-fully active documents (including
those that are active documents), and it's not expected that there would
be much usage of the URL of non-fully active documents anyway.

This allows us to additionally remove the seldom implemented "replace"
parameter in document.open(), whose behavior is now the default
(aligning with with Chrome and Safari). The IDL is modified accordingly.

Tests: web-platform-tests/wpt#12555
Tests: web-platform-tests/wpt#12634
Tests: web-platform-tests/wpt#12636
Tests: web-platform-tests/wpt#12650

Fixes whatwg#3564.
Fixes whatwg#3885.

TimothyGu added a commit to TimothyGu/html that referenced this pull request


          document.open() simplifications, part 2

d484c6c

The current behavior of adding a new entry to the session history is
removed, with the "replace" parameter behavior made the only option. To
fix whatwg#3885, we now reuse history.replaceState()'s history model, which
keeps the document's URL and the history entry URL in sync.

At the same time, we restrict the history state update (including
setting the document's URL) to be only run with fully active documents.
Firefox already bails out for non-fully active documents (including
those that are active documents), and it's not expected that there would
be much usage of the URL of non-fully active documents anyway.

This allows us to additionally remove the seldom implemented "replace"
parameter in document.open(), whose behavior is now the default
(aligning with with Chrome and Safari). The IDL is modified accordingly.

Tests: web-platform-tests/wpt#12555
Tests: web-platform-tests/wpt#12634
Tests: web-platform-tests/wpt#12636
Tests: web-platform-tests/wpt#12650

Fixes whatwg#3564.
Fixes whatwg#3885.

TimothyGu added a commit to TimothyGu/html that referenced this pull request


          document.open() simplifications, part 2

a896854

The current behavior of adding a new entry to the session history is
removed, with the "replace" parameter behavior made the only option. To
fix whatwg#3885, we now reuse history.replaceState()'s history model, which
keeps the document's URL and the history entry URL in sync.

At the same time, we restrict the history state update (including
setting the document's URL) to be only run with fully active documents.
Firefox already bails out for non-fully active documents (including
those that are active documents), and it's not expected that there would
be much usage of the URL of non-fully active documents anyway.

This allows us to additionally remove the seldom implemented "replace"
parameter in document.open(), whose behavior is now the default
(aligning with with Chrome and Safari). The IDL is modified accordingly.

Tests: web-platform-tests/wpt#12555
Tests: web-platform-tests/wpt#12634
Tests: web-platform-tests/wpt#12636
Tests: web-platform-tests/wpt#12650

Fixes whatwg#3564.
Fixes whatwg#3885.

TimothyGu added a commit to TimothyGu/html that referenced this pull request


          Remove overridden reload concept

38efcee

Or: document.open() simplifications, part 1.9.

This behavior is only implemented in Firefox and Edge and has
contributed to developer confusion. See
https://bugzilla.mozilla.org/show_bug.cgi?id=556002.

This is a part of the effort to renovate document.open(). See
whatwg#3818 for context.

Tests: web-platform-tests/wpt#12555

TimothyGu added a commit to TimothyGu/html that referenced this pull request


          document.open() simplifications, part 2

b6e1c2b

The current behavior of adding a new entry to the session history is
removed, with the "replace" parameter behavior made the only option. To
fix whatwg#3885, we now reuse history.replaceState()'s history model, which
keeps the document's URL and the history entry URL in sync.

At the same time, we restrict the history state update (including
setting the document's URL) to be only run with fully active documents.
Firefox already bails out for non-fully active documents (including
those that are active documents), and it's not expected that there would
be much usage of the URL of non-fully active documents anyway.

This allows us to additionally remove the seldom implemented "replace"
parameter in document.open(), whose behavior is now the default
(aligning with with Chrome and Safari). The IDL is modified accordingly.

Tests: web-platform-tests/wpt#12555
Tests: web-platform-tests/wpt#12634
Tests: web-platform-tests/wpt#12636
Tests: web-platform-tests/wpt#12650

Fixes whatwg#3564.
Fixes whatwg#3885.

domenic pushed a commit to whatwg/html that referenced this pull request


          Remove overridden reload concept

6440cca

Or: document.open() simplifications, part 1.9.

This behavior is only implemented in Firefox and Edge and has
contributed to developer confusion. See
https://bugzilla.mozilla.org/show_bug.cgi?id=556002.

This is a part of the effort to renovate document.open(). See
#3818 for context.

Tests: web-platform-tests/wpt#12555

domenic pushed a commit to whatwg/html that referenced this pull request


          document.open() simplifications, part 2

ae7cf0c

The current behavior of adding a new entry to the session history is
removed, with the "replace" parameter behavior made the only option. To
fix #3885, we now reuse history.replaceState()'s history model, which
keeps the document's URL and the history entry URL in sync.

At the same time, we restrict the history state update (including
setting the document's URL) to be only run with fully active documents.
Firefox already bails out for non-fully active documents (including
those that are active documents), and it's not expected that there would
be much usage of the URL of non-fully active documents anyway.

This allows us to additionally remove the seldom implemented "replace"
parameter in document.open(), whose behavior is now the default
(aligning with with Chrome and Safari). The IDL is modified accordingly.

Tests: web-platform-tests/wpt#12555
Tests: web-platform-tests/wpt#12634
Tests: web-platform-tests/wpt#12636
Tests: web-platform-tests/wpt#12650

Fixes #3564.
Fixes #3885.

TimothyGu merged commit 8b0c29b into master

TimothyGu deleted the timothygu/document-open-reload branch

August 23, 2018 19:37

zcorpan pushed a commit that referenced this pull request


          HTML: document.open() and reload (#12555)

b94bf23

For whatwg/html#3946.

mustaqahmed pushed a commit to mustaqahmed/html that referenced this pull request


          Remove overridden reload concept

Or: document.open() simplifications, part 1.9.

This behavior is only implemented in Firefox and Edge and has
contributed to developer confusion. See
https://bugzilla.mozilla.org/show_bug.cgi?id=556002.

This is a part of the effort to renovate document.open(). See
whatwg#3818 for context.

Tests: web-platform-tests/wpt#12555

mustaqahmed pushed a commit to mustaqahmed/html that referenced this pull request


          document.open() simplifications, part 2

003168e

The current behavior of adding a new entry to the session history is
removed, with the "replace" parameter behavior made the only option. To
fix whatwg#3885, we now reuse history.replaceState()'s history model, which
keeps the document's URL and the history entry URL in sync.

At the same time, we restrict the history state update (including
setting the document's URL) to be only run with fully active documents.
Firefox already bails out for non-fully active documents (including
those that are active documents), and it's not expected that there would
be much usage of the URL of non-fully active documents anyway.

This allows us to additionally remove the seldom implemented "replace"
parameter in document.open(), whose behavior is now the default
(aligning with with Chrome and Safari). The IDL is modified accordingly.

Tests: web-platform-tests/wpt#12555
Tests: web-platform-tests/wpt#12634
Tests: web-platform-tests/wpt#12636
Tests: web-platform-tests/wpt#12650

Fixes whatwg#3564.
Fixes whatwg#3885.

mustaqahmed pushed a commit to mustaqahmed/html that referenced this pull request


          Remove overridden reload concept

ca5117e

Or: document.open() simplifications, part 1.9.

This behavior is only implemented in Firefox and Edge and has
contributed to developer confusion. See
https://bugzilla.mozilla.org/show_bug.cgi?id=556002.

This is a part of the effort to renovate document.open(). See
whatwg#3818 for context.

Tests: web-platform-tests/wpt#12555

mustaqahmed pushed a commit to mustaqahmed/html that referenced this pull request


          document.open() simplifications, part 2

352c62e

The current behavior of adding a new entry to the session history is
removed, with the "replace" parameter behavior made the only option. To
fix whatwg#3885, we now reuse history.replaceState()'s history model, which
keeps the document's URL and the history entry URL in sync.

At the same time, we restrict the history state update (including
setting the document's URL) to be only run with fully active documents.
Firefox already bails out for non-fully active documents (including
those that are active documents), and it's not expected that there would
be much usage of the URL of non-fully active documents anyway.

This allows us to additionally remove the seldom implemented "replace"
parameter in document.open(), whose behavior is now the default
(aligning with with Chrome and Safari). The IDL is modified accordingly.

Tests: web-platform-tests/wpt#12555
Tests: web-platform-tests/wpt#12634
Tests: web-platform-tests/wpt#12636
Tests: web-platform-tests/wpt#12650

Fixes whatwg#3564.
Fixes whatwg#3885.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

domenic domenic approved these changes

annevk Awaiting requested review from annevk

foolip Awaiting requested review from foolip

jdm Awaiting requested review from jdm

jgraham Awaiting requested review from jgraham

sideshowbarker Awaiting requested review from sideshowbarker

zcorpan Awaiting requested review from zcorpan

zqzhang Awaiting requested review from zqzhang

Labels

html