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


  <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


  // <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 @@
   <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".)

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.

TimothyGu force-pushed the document-open-s2 branch from 72fe881 to 4fc7e01 Compare August 20, 2018 22:21

annevk added the topic: document.open() label Aug 21, 2018

annevk reviewed Aug 21, 2018

View reviewed changes

TimothyGu mentioned this pull request Aug 21, 2018

Tracking issue for document.open() overhaul #3818

Closed

19 tasks

domenic mentioned this pull request Aug 21, 2018

Allow overload resolution to "round down"? whatwg/webidl#581

Open

domenic reviewed Aug 21, 2018

View reviewed changes

TimothyGu force-pushed the document-open-s2 branch from 4dde366 to 5e81626 Compare August 21, 2018 22:01

domenic approved these changes Aug 21, 2018

View reviewed changes

TimothyGu mentioned this pull request Aug 22, 2018

HTML: document.open() and reload web-platform-tests/wpt#12555

Merged

TimothyGu mentioned this pull request Aug 22, 2018

HTML: document.open() and history entry creation web-platform-tests/wpt#12634

Closed

TimothyGu mentioned this pull request Aug 22, 2018

HTML: document.open() and document's URL web-platform-tests/wpt#12636

Merged

TimothyGu added the needs tests Moving the issue forward requires someone to write tests label Aug 23, 2018

TimothyGu mentioned this pull request Aug 23, 2018

HTML: add test for history.state after document.open() web-platform-tests/wpt#12650

Merged

TimothyGu removed the needs tests Moving the issue forward requires someone to write tests label Aug 23, 2018

TimothyGu force-pushed the document-open-s2 branch 3 times, most recently from 5c865f5 to c241269 Compare August 23, 2018 18:31

TimothyGu added 4 commits August 23, 2018 14:31

Editorial: Factor out history.push/replaceState() steps

51f52de

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

Editorial: Remove parameters from document.open() references

2e193aa

This effectively reverts parts of 8f2816a.

TimothyGu force-pushed the document-open-s2 branch from c241269 to 2e193aa Compare August 23, 2018 18:32

domenic merged commit 288fc16 into whatwg:master Aug 23, 2018

jdm mentioned this pull request Aug 23, 2018

Update document.open implementation to match spec simplification servo/servo#21445

Closed

TimothyGu deleted the document-open-s2 branch August 23, 2018 20:21

TimothyGu mentioned this pull request Aug 28, 2018

HTML: document.open() and history entry creation web-platform-tests/wpt#12724

Merged

domenic mentioned this pull request Jan 11, 2019

Why does document.open not change the document URL for not-fully-active documents? #4286

Closed

annevk mentioned this pull request May 28, 2019

Fallback base URL computation for a document with about:srcdoc but not an iframe srcdoc document #3989

Closed

domenic mentioned this pull request Apr 5, 2021

document.open() URL updates can cause recursive loading problems on history traversal #6556

Open


		<hr id="history-1">

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

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

document.open() simplifications, part 2 #3946

document.open() simplifications, part 2 #3946

Uh oh!

Conversation

TimothyGu commented Aug 20, 2018 • edited by pr-preview bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

annevk left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

domenic left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

domenic left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

3 participants

TimothyGu commented Aug 20, 2018 •

edited by pr-preview bot

Loading

annevk left a comment •

edited

Loading