document.open() simplifications, part 2 - #3946
Conversation
72fe881 to
4fc7e01
Compare
|
|
||
| </ol> | ||
|
|
||
|
|
There was a problem hiding this comment.
This looks like it removes too many newlines.
There was a problem hiding this comment.
What do you mean? There are still two newlines remaining here (the first one is above this hunk).
There was a problem hiding this comment.
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 was a problem hiding this comment.
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…
| <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 |
There was a problem hiding this comment.
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?)
| <p>Add a <span>session history entry</span> entry to the session history, after the | ||
| <span>current entry</span>, with</p> | ||
|
|
||
| <ul> |
There was a problem hiding this comment.
<ul class="brief"> (with no <p>s) or <dl>, maybe?
There was a problem hiding this comment.
I had that but decided the current style looked slightly better…
| <p>Add a <span>session history entry</span> entry to the session history, after the | ||
| <span>current entry</span>, with</p> | ||
|
|
||
| <ul> |
There was a problem hiding this comment.
Gah, I really wish entries were proper structs with fields.
|
|
||
| <hr id="history-1"> | ||
|
|
||
| <p>The <dfn>history state update steps</dfn>, given a <code>Document</code> object |
There was a problem hiding this comment.
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"?
|
|
||
| // <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 |
There was a problem hiding this comment.
| <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> |
There was a problem hiding this comment.
Should we save the "responsible document" in a variable earlier, since it's used twice?
| @@ -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> | |||
There was a problem hiding this comment.
Any ideas why this still shows up in the diff, despite previous commits removing it?
There was a problem hiding this comment.
Not really, it was removed in this PR.
| 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 |
There was a problem hiding this comment.
Did we forget to clean this up previously? We don't unload any more.
4dde366 to
5e81626
Compare
| <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> |
There was a problem hiding this comment.
While here, let's move this note up and have it talk about the document open steps?
| <span>current entry</span>, with</p> | ||
|
|
||
| <ul> | ||
| <li><p><var>newURL</var> as the <span>URL</span></p></li> |
There was a problem hiding this comment.
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. 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.
5c865f5 to
c241269
Compare
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.
c241269 to
2e193aa
Compare
The current behavior of adding a new entry to the session history is removed, with the
replaceparameter behavior made the only option. To fix #3885, we now reusehistory.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
replaceparameter indocument.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 )