document.open() simplifications, part 2 #3946

TimothyGu · 2018-08-20T19:32:43Z

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.

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: web-platform-tests/wpt#12634
Tests: web-platform-tests/wpt#12636
Tests: web-platform-tests/wpt#12650

Fixes #3564.
Fixes #3885.

/browsing-the-web.html ( diff )
/custom-elements.html ( diff )
/dom.html ( diff )
/dynamic-markup-insertion.html ( diff )
/history.html ( diff )
/infrastructure.html ( diff )
/origin.html ( diff )
/parsing.html ( diff )
/webappapis.html ( diff )

annevk

~~So it was my understanding that Chrome and Safari don't alter history at all for this method. Should we really go there?~~ Yes, see #3885.

annevk · 2018-08-21T07:27:20Z

source

-
-  </ol>
-
-


This looks like it removes too many newlines.

What do you mean? There are still two newlines remaining here (the first one is above this hunk).

But there used to be three, unless I'm missing something. Perhaps two is more consistent with content I can't see in this diff though.

There's little consistency within the spec and I see places with one, two, three, or four consecutive empty lines. However I think two seems to remain the most popular for the sections surround this one…

domenic

Found some things to tweak!

domenic · 2018-08-21T17:26:58Z

source

-    <p class="note">An <span data-x="override URL">override URL</span> is set when <span
-    data-x="javascript protocol">dereferencing a <code>javascript:</code> URL</span> and when
-    performing <span>an overridden reload</span>.</p>
+    <p><dfn data-x="set the document's address">Setting the document's address</dfn>: Any


Maybe as a follow-up, I hope we can move this into "Initializing a new Document object", and use something actually concrete instead of "the URL that was originally to be fetched". (Especially since that seems to imply the request URL, not the response URL, which can't be right?)

domenic · 2018-08-21T17:33:31Z

source

+      <p>Add a <span>session history entry</span> entry to the session history, after the
+      <span>current entry</span>, with</p>
+
+      <ul>


<ul class="brief"> (with no <p>s) or <dl>, maybe?

I had that but decided the current style looked slightly better…

domenic · 2018-08-21T17:34:29Z

source

+      <p>Add a <span>session history entry</span> entry to the session history, after the
+      <span>current entry</span>, with</p>
+
+      <ul>


Gah, I really wish entries were proper structs with fields.

domenic · 2018-08-21T17:46:44Z

source

@@ -80542,6 +80542,85 @@ interface <dfn>History</dfn> {

  <hr id="history-1">

+  <p>The <dfn>history state update steps</dfn>, given a <code>Document</code> object


I'm concerned the use of "state" here is a bit too narrow. Also this doesn't communicate how we ideally only want to change the document URL via this algorithm. Maybe something like "URL and history update steps"?

domenic · 2018-08-21T18:07:50Z

source

@@ -9168,7 +9168,7 @@ partial interface <dfn id="document" data-lt="">Document</dfn> {
  readonly attribute <span>HTMLOrSVGScriptElement</span>? <span data-x="dom-document-currentScript">currentScript</span>; // classic scripts in a document tree only

  // <span>dynamic markup insertion</span>
-  [<span>CEReactions</span>] <span>Document</span> <span data-x="dom-document-open">open</span>(optional DOMString type, optional DOMString replace = ""); // type is ignored
+  [<span>CEReactions</span>] <span>Document</span> <span data-x="dom-document-open">open</span>(optional DOMString type, optional DOMString replace); // both type and replace are ignored


We should change the name of the arguments to e.g. unused1, unused2, or historical1, historical2. This will involve changing all the references throughout the document, sad to say.

We should update the note that says "The type argument is ignored" to explain that both are ignored, but that we can't remove the arguments because it would affect Web IDL overload resolution for historical code, causing it to throw. With a link to whatwg/webidl#581.

And we should link this IDL comment to that note.

domenic · 2018-08-21T18:09:21Z

source

+   <li><p>If <var>document</var> is <span>fully active</span>, then run the <span>history state
+   update steps</span> with <var>document</var> and the <span
+   data-x="concept-document-url">URL</span> of the <span>responsible document</span> specified by
+   the <span>entry settings object</span>.</p></li>


Should we save the "responsible document" in a variable earlier, since it's used twice?

domenic · 2018-08-21T18:11:56Z

source

@@ -90985,14 +90971,6 @@ document.body.appendChild(frame)</code></pre>
   <li><p>Remove any earlier entries whose <code>Document</code> object is
   <var>document</var>.</p></li>


Any ideas why this still shows up in the diff, despite previous commits removing it?

Not really, it was removed in this PR.

domenic · 2018-08-21T18:12:43Z

source

-     If the user <span>refused to allow the document to be unloaded</span>, then return. Otherwise,
-     the <span>insertion point</span> will point at just before the end of the (empty) <span>input
+     <li><p>Run the <span>document open steps</span> with <var>document</var>. If the user
+     <span>refused to allow the document to be unloaded</span>, then return. Otherwise, the


Did we forget to clean this up previously? We don't unload any more.

domenic

LGTM with nits!

domenic · 2018-08-21T22:54:33Z

source

-  <span>completely loaded</span>.</p>
+  <p class="note">The <code data-x="dom-document-open">document.open()</code> method does not affect
+  whether a <code>Document</code> is <span>ready for post-load tasks</span> or <span>completely
+  loaded</span>.</p>


While here, let's move this note up and have it talk about the document open steps?

domenic · 2018-08-21T22:57:20Z

source

+      <span>current entry</span>, with</p>
+
+      <ul>
+       <li><p><var>newURL</var> as the <span>URL</span></p></li>


So for such lists they should be semicolon-delimited, with a period at the end. I.e. they're a part of one big sentence. (Which, yes, is split over several "paragraphs".)

For whatwg/html#3946.

For whatwg/html#3946. Adapts the basic test in #10817 for a number of advanced scenarios. Co-authored-by: Anne van Kesteren <annevk@annevk.nl>

No browser currently passes the 010.html test, and it is now removed. For whatwg/html#3946.

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

The resultant URL and history state update steps will be used in document.open().

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.

This effectively reverts parts of 8f2816a.

For whatwg/html#3946.

For whatwg/html#3946. Adapts the basic test in #10817 for a number of advanced scenarios regarding document activity. Co-authored-by: Anne van Kesteren <annevk@annevk.nl>

For whatwg/html#3946.

For whatwg/html#3946. Adapts the basic test in #10817 for a number of advanced scenarios regarding document activity. Co-authored-by: Anne van Kesteren <annevk@annevk.nl>

For whatwg/html#3946.

The 010.html test has been removed for a multitude of reasons: - It does not seem to work as intended in any browser. (I.e., no browser passes the test.) - The concept it allegedly tests (salvageability) is no longer in the specification for document.open(). For whatwg/html#3946.

…only Automatic update from web-platform-testsHTML: document.open() and reload (#12555) For whatwg/html#3946. -- wpt-commits: 8b0c29bfa35d03c3db9027af796abcdb08b7c6dc wpt-pr: 12555

…, a=testonly Automatic update from web-platform-testsHTML: document.open() and document's URL (#12636) For whatwg/html#3946. Adapts the basic test in web-platform-tests/wpt#10817 for a number of advanced scenarios regarding document activity. Co-authored-by: Anne van Kesteren <annevk@annevk.nl> -- wpt-commits: 312d71258a26bfa420f7eef100df4ef92d961483 wpt-pr: 12636

…ocument.open(), a=testonly Automatic update from web-platform-testsHTML: add test for history.state after document.open() (#12650) For whatwg/html#3946. -- wpt-commits: e57d136af9eef24a9f3b54dffcf9c7ca74331f09 wpt-pr: 12650

…only Automatic update from web-platform-testsHTML: document.open() and reload (#12555) For whatwg/html#3946. -- wpt-commits: 8b0c29bfa35d03c3db9027af796abcdb08b7c6dc wpt-pr: 12555

…, a=testonly Automatic update from web-platform-testsHTML: document.open() and document's URL (#12636) For whatwg/html#3946. Adapts the basic test in web-platform-tests/wpt#10817 for a number of advanced scenarios regarding document activity. Co-authored-by: Anne van Kesteren <annevk@annevk.nl> -- wpt-commits: 312d71258a26bfa420f7eef100df4ef92d961483 wpt-pr: 12636

…ocument.open(), a=testonly Automatic update from web-platform-testsHTML: add test for history.state after document.open() (#12650) For whatwg/html#3946. -- wpt-commits: e57d136af9eef24a9f3b54dffcf9c7ca74331f09 wpt-pr: 12650

…creation, a=testonly Automatic update from web-platform-testsHTML: document.open() and history entry creation (#12724) The 010.html test has been removed for a multitude of reasons: - It does not seem to work as intended in any browser. (I.e., no browser passes the test.) - The concept it allegedly tests (salvageability) is no longer in the specification for document.open(). For whatwg/html#3946. -- wpt-commits: f23bb9e37fdf694021519b0e7ce2c282fa026ea5 wpt-pr: 12724

…only Automatic update from web-platform-testsHTML: document.open() and reload (#12555) For whatwg/html#3946. -- wpt-commits: 8b0c29bfa35d03c3db9027af796abcdb08b7c6dc wpt-pr: 12555 UltraBlame original commit: 2e120ceb2b8b1c22b178afaa22ed6d418cab9770

…, a=testonly Automatic update from web-platform-testsHTML: document.open() and document's URL (#12636) For whatwg/html#3946. Adapts the basic test in web-platform-tests/wpt#10817 for a number of advanced scenarios regarding document activity. Co-authored-by: Anne van Kesteren <annevkannevk.nl> -- wpt-commits: 312d71258a26bfa420f7eef100df4ef92d961483 wpt-pr: 12636 UltraBlame original commit: 0aaff791b401b17dd832464bc94688c474a08ebc

…ocument.open(), a=testonly Automatic update from web-platform-testsHTML: add test for history.state after document.open() (#12650) For whatwg/html#3946. -- wpt-commits: e57d136af9eef24a9f3b54dffcf9c7ca74331f09 wpt-pr: 12650 UltraBlame original commit: 33593424f5016a2a7dae3460c0e6c791e9aaa7a7

…creation, a=testonly Automatic update from web-platform-testsHTML: document.open() and history entry creation (#12724) The 010.html test has been removed for a multitude of reasons: - It does not seem to work as intended in any browser. (I.e., no browser passes the test.) - The concept it allegedly tests (salvageability) is no longer in the specification for document.open(). For whatwg/html#3946. -- wpt-commits: f23bb9e37fdf694021519b0e7ce2c282fa026ea5 wpt-pr: 12724 UltraBlame original commit: 3296434f41a427cd1ba5309c5d40eb3b383d0431