Improve explicitness of session history spec by domenic · Pull Request #6039 · whatwg/html

domenic · 2020-10-07T22:27:57Z

This is mostly an editorial change, although some of it crosses the line into bugfixes. (E.g., pushState/replaceState would previously create session history entries without a document object.)

The intent of this pull request is to introduce explicit fields for every item in the session history entry, as the first step toward updating them as in #5767. /cc @jakearchibald. This does mean that several of these will soon change shape, but I think it will be easier to proceed from this basis than from the current spec, because now we can find all cross references more reliably. As such, I'm especially interested in review feedback wherein you preview the spec and find some reference to a session history entry's items that is not properly linked.

Along the way, this includes a number of clarifications and fixes as I kept touching old-and-crusty areas of the spec.

This PR definitely leaves some things unrigorous. The goal is an overall improvement in rigor, not perfection. Notable problems I noticed, and only fixed in certain locations where it was particularly easy:

These sections of the spec often refers to ambient "browsing context", "session history", or "current entry". Ideally, they should be passed in a browsing context, and always get that browsing context's session history, and that session history's current entry. (Similar problems occurred with things like "active document" and "the top-level browsing context", but they were rarer.)
Going from History object to Document, or History object to "session history", is still very imprecise.

I'm interested in fixing these over time, but what I have here seems like a reasonable stopping point to try to land something.

The commits are not meaningful here, and I suggest using the "Files" tab to review. I don't want to squash them yet though as I did find them helpful while editing.

/browsing-the-web.html ( diff )
/history.html ( diff )
/index.html ( diff )
/window-object.html ( diff )

This one was easy to hunt down

domenic · 2020-10-07T22:29:17Z

+  <p>Apart from these restrictions, this standard does not specify when user agents should discard
+  an entry's <span data-x="she-document">document</span>, versus keeping it cached.</p>

-  <p>Entries that have had their <code>Document</code> objects discarded must, for the purposes of


This paragraph just seems totally wrong. Restoration is handled explicitly. I basically deleted it. Am I missing something?

"any other entries that shared the same Document object with it must share the new object as well" is an important bit of hand-waving. Else 5 contiguous entries that shared the same document could become 5 separate documents.

annevk

This generally looks fine and given that the current state is a mess I don't really object to trying to improve it through iteration. I think it would be good for at least one other person to review though. Maybe @rakina or @jakearchibald?

annevk · 2020-10-08T08:20:15Z

+    <p><dfn data-x="she-form-data">form data</dfn></p>
+
+    <p class="XXX">This is never explicitly used. Probably it should be used by some part of the
+    spec dealing with form (re)submission?</p>


I think this is for when you reopen a closed tab. That the browser can restore what you filled in the forms. Might also be applicable to navigation.

I don't think so. That's handled by "persistent user state" (which specifically mentions form control values).

Should we just delete it?

I'm gonna delete it. Leaving it lying around, unreferenced, is not going to really help us or any readers. Especially given how confusing it is with persisted user state.

My best guess is that at some point form data and persisted user state were stored, or at least discussed, separately. Then at some point form data got rolled into persisted user state (thus all the special care to talk about form data in sections dealing with persisted user state), but Hixie forgot to update this sentence.

Hmm, so the other thing I can think of is that when you go back in history and you get this prompt about whether you want to resubmit form data. So form data indicates you need to do a POST request to get at the URL.

Yeah, that could also be the case. It's just totally not handled by the spec :(.

In particular, I think it's related to

In the case of non-idempotent methods (e.g., HTTP POST), the user agent should prompt the user to confirm the operation first, since otherwise transactions (e.g., purchases or database modifications) could be repeated.

I'm not sure whether that means to remove it or not, but I suspect all implementations do something here like the model would suggest.

domenic · 2020-10-09T18:19:41Z

Will merge Monday if folks don't have any extra review before then. Proposed commit message:

Improve explicitness of the session history spec

This is mostly an editorial change, which introduces explicit <dfn>s and
links for the various items of a session history entry. However, it also
includes some bugfixes and substantial clarifications. Notable ones:

* Removes the mention of "form data" as being part of a session history
  entry. This was never referenced elsewhere in the spec. It may have
  something to do with form resubmission upon history traversal (which
  seems to be unspecified), or maybe it was subsumed into "persisted
  user state". For now it's best removed.

* Removes a paragraph saying that entries with discarded documents
  should act as if the documents were not discarded. This seems totally
  wrong; re-creation of documents is handled explicitly in the spec.

* Fixes the "URL and history update steps" to not create document-less
  session history entries.

* Symmetrizes and more tightly couples scroll position
  saving/restoration and saving/restoration of other,
  implementation-defined, persisted user state.

* Rewords all the discussion of contiguous, document-sharing session
  history entries for improved clarity.

* Updates all history-related IDL constructs to modern style, including
  adding a "state" value for history.state to return, and creating
  "shared history push/replace state steps" for history.pushState() and
  history.replaceState() to call into.

* Reduces some of the implicit variable-passing, notably for the
  "navigate to a fragment" algorithm.

This helps set the stage for #5767, but does not yet make any of the
changes proposed there.

jakearchibald

Sorry. Hopefully better late than never.

jakearchibald · 2020-10-15T09:41:46Z

-       <li><p>Replace the <code>Document</code> of the entry being updated, and any other entries
-       that referenced the same document as that entry, with the new <code>Document</code>.</p></li>
+       <li><p>Replace the <span data-x="she-document">document</span> of the <span>current
+       entry</span>, and any other entries that reference the same <span


I guess this "any other entries" bit is one of those ambient things we'll need to fix later?

I think what's needed here (at least in the current framework) is just to state which "session history" is being operated on. Right now it's just "current entry", but of what session history? It's pretty clear from context, but would be better if precise, indeed.

jakearchibald · 2020-10-15T09:53:13Z

-     data-x="concept-url-equals">equals</span> the <span>browsing context</span>'s <span>active
-     document</span>'s <span data-x="concept-document-url">URL</span></dt>
+     data-x="concept-url-equals">equals</span> <var>newDocument</var>'s <span
+     data-x="concept-document-url">URL</span></dt>


Do we need to consider the "browsing context switch needed" param here (or browsing context equality)? Or is replacement still expected in this case?

I think we do, but that'd be done as part of the normative changes for #5767, not the editorial changes in this PR. Unless I'm missing something?

Ah yeah, it can be handled in #5767

jakearchibald · 2020-10-15T10:07:09Z

-        the last entry in the session history, then no entries are removed.</p>
+        <p>Remove all the entries in the <span>session history</span> after the <span>current
+        entry</span>. If the <span>current entry</span> is the last entry in the session history,
+        then no entries are removed.</p>


This should be the joint session history, but I guess we'll fix that later?

Yeah, just preserving existing mistakes. Interesting find.

jakearchibald · 2020-10-15T10:28:08Z

   <li>
    <p>If <var>historyHandling</var> is not "<code data-x="hh-replace">replace</code>", then remove
-    all the entries in the <span>browsing context</span>'s <span>session history</span> after the
+    all the entries in <var>browsingContext</var>'s <span>session history</span> after the


Another place that should be joint session history, but this PR doesn't make it worse.

jakearchibald · 2020-10-15T10:32:05Z

-      object is "<code data-x="">complete</code>", then <span>queue a global task</span> on the
-      <span>DOM manipulation task source</span> given <var>entry</var>'s <code>Document</code>'s
-      <span>relevant global object</span> to run the following subsubsteps:</p>
+      <p>If <var>newDocument</var>'s <span>current document readiness</span> "<code


Missing "is"

jakearchibald · 2020-10-15T11:08:10Z

-  <div w-nodev>
+  <ul>
+   <li><p>They must not discard the <span data-x="she-document">document</span> of the <span>current
+   entry</span>.</p></li>


Nit: Is it worth wording this in a way that makes it clear that this might be a side-effect. As in, I could feel like I'm removing another entry's document, but it just so happens that the current entry has the same document.

jakearchibald · 2020-10-15T11:15:35Z

+  <p>Apart from these restrictions, this standard does not specify when user agents should discard
+  an entry's <span data-x="she-document">document</span>, versus keeping it cached.</p>

-  <p>Entries that have had their <code>Document</code> objects discarded must, for the purposes of


"any other entries that shared the same Document object with it must share the new object as well" is an important bit of hand-waving. Else 5 contiguous entries that shared the same document could become 5 separate documents.

This fixes several related issues surrounding the "update the session history with the new page" and "traverse the history" algorithms. * A mistaken attempted refactoring in #6039 got the wrong Document for the newDocument variable in "traverse the history". To fix this, we explicitly pass in the new document to the algorithm from all its call sites. (Discussed in #6197.) * As discussed more extensively in #6197, the same-URL and entry update/reload conditions in "update the session history with the new page" were not correctly triggering the new-document clause in "traverse the history", despite replacing the document. This was partially due to #6039, although the phrasing before #6039 was extremely ambiguous, so #6039 was mostly a transition from "unclear" to "clearly wrong". * This fixes the "easy bug" discussed in #6202, where the same-URL case was using the wrong URL to determine sameness. That bug was also introduced in #6039. The harder bug, as well as the action-at-a-distance nature of the same-URL check, remain tracked in #6202. * For years, the spec has required deleting the future session history entries after performing a navigation with history handling of "replace", e.g. via location.replace(). This is not correct; a "replace" navigation does not delete future session history entries. Instead it just replaces the current entry, leaving future entries alone. * The latter point makes the handling of same-URL navigations almost identical to "replace" navigations (including non-same-URL "replace" navigations). This change makes this clear, by using the same text for both, but adding a pointer to #6213 which highlights the fact that some browsers treat them slightly differently (and some treat them the same). * Finally, this adds or modifies a few assertions to check that we're in the "default" history handling behavior, so it's clearer to the reader what the non-exceptional path through the spec is. Closes #6197.

domenic added 3 commits October 7, 2020 16:59

Rewrite intro section, with some fixes, and explicit fields

b96495a

Fix all references to scroll restoration state

fe44946

This one was easy to hunt down

Try to fix other references

93f4974

domenic added clarification Standard could be clearer topic: history labels Oct 7, 2020

domenic requested a review from annevk October 7, 2020 22:27

domenic commented Oct 7, 2020

View reviewed changes

Comment thread source Outdated

annevk approved these changes Oct 8, 2020

View reviewed changes

rakina reviewed Oct 8, 2020

View reviewed changes

Comment thread source

IDL improvements

75f23ff

domenic mentioned this pull request Oct 8, 2020

Should "traverse a history" check Document salvagability? #6045

Closed

domenic added 6 commits October 8, 2020 14:50

Revamp persisted state saving/restoration

75a7c52

Modernize "URL and history update steps" and pushState/replaceState

01dfbc6

Stop writing directly to history.state

531f543

Delete form data

c0a0292

Remove "for the Window interface"

0ccf745

Typo

0bdfe96

domenic merged commit e694724 into master Oct 14, 2020

domenic deleted the structured-session-history-entry branch October 14, 2020 16:31

jakearchibald reviewed Oct 15, 2020

View reviewed changes

domfarolino mentioned this pull request Dec 5, 2020

Updating the session history seems pretty broken #6197

Closed

6 tasks

Uh oh!

Conversation

domenic commented Oct 7, 2020 • edited by pr-preview Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

annevk left a comment

Choose a reason for hiding this comment

Uh oh!

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!

Uh oh!

Uh oh!

Uh oh!

domenic commented Oct 9, 2020

Uh oh!

jakearchibald 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!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

4 participants

domenic commented Oct 7, 2020 •

edited by pr-preview Bot

Loading