-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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.
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
Improve explicitness of session history spec #6039
Conversation
|
||
<p>Entries that have had their <code>Document</code> objects discarded must, for the purposes of |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This paragraph just seems totally wrong. Restoration is handled explicitly. I basically deleted it. Am I missing something?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
<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> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think so. That's handled by "persistent user state" (which specifically mentions form control values).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we just delete it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, that could also be the case. It's just totally not handled by the spec :(.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Will merge Monday if folks don't have any extra review before then. Proposed commit message:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry. Hopefully better late than never.
<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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess this "any other entries" bit is one of those ambient things we'll need to fix later?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need to consider the "browsing context switch needed" param here (or browsing context equality)? Or is replacement still expected in this case?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah yeah, it can be handled in #5767
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> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be the joint session history, but I guess we'll fix that later?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, just preserving existing mistakes. Interesting find.
|
||
<ol> | ||
<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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another place that should be joint session history, but this PR doesn't make it worse.
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing "is"
<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> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
|
||
<p>Entries that have had their <code>Document</code> objects discarded must, for the purposes of |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"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.
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 toDocument
, orHistory
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 )