Skip to content
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

Fix same-URL navigation session history handling #6476

Merged
merged 1 commit into from
Mar 12, 2021
Merged

Fix same-URL navigation session history handling #6476

merged 1 commit into from
Mar 12, 2021

Conversation

domenic
Copy link
Member

@domenic domenic commented Mar 10, 2021

Closes #6202. Also tidies up the "update the session history with the new page" algorithm more generally, e.g. to be explicit about what session history is being accessed.

/cc @jakearchibald as this probably conflicts with #6315... sorry about that, but this particular cause was starting to cause some real confusion and I wanted to get it fixed sooner.


/browsing-the-web.html ( diff )

Closes #6202. Also tidies up the "update the session history with the new page" algorithm more generally, e.g. to be explicit about what session history is being accessed.
@rakina
Copy link
Member

rakina commented Mar 11, 2021

The same-URL part LGTM. The javascript: URL part is interesting - in Chrome it's not really treated as a navigation but just as a "document change" (see thread), but maybe the "navigate" function in the spec covers that too? (not sure if the difference matters, but maybe it's important for WICG/navigation-api#63 ?)

@domenic
Copy link
Member Author

domenic commented Mar 11, 2021

The same-URL part LGTM. The javascript: URL part is interesting - in Chrome it's not really treated as a navigation but just as a "document change" (see thread), but maybe the "navigate" function in the spec covers that too?

Yeah, the spec's "navigate" seems to cover such a "document change", by navigating semi-directly to a synethetic response. The real question is what should happen to the user- and web-page-visible session history, and from what I can tell from some manual testing, it should be "replace" behavior.

That said, we've had other problems with this spec approach not matching implementations, e.g. all of the tagged javascript: URL issues. The most direct is that implementations seem to directly modify the body as a string, but creating a synthetic response means creating the body as bytes and then (in theory) passing it through the whole decoding pipeline; that's #1129.

I do think we should keep "navigate" as the entry point for javascript: URLs in the spec, so that, e.g., following a hyperlink can always call "navigate", instead of having to switch on the hyperlink's scheme and dispatch to either navigate or some "document change" algorithm. But there might be room for improvement in terms of how the javascript:-specific path of navigate works.

The thread you linked is a pretty interesting one too. I think it's most related in spec land to the ongoing policy container work: #4926; see the latest comment there on #4926 (comment) .

Copy link
Member

@domfarolino domfarolino left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So we are coalescing the following two conditions:

  • Navigations to the same URL
  • Explicit historyHandling=replace navigations

I vaguely remember (perhaps incorrectly) that one reason we decided to keep them separate is because because browsers might copy session history state on the same-URL navigation case, but we didn't have reason to believe that this took place in the explicit historyHandling=replace case? I really don't know, I just wanted to bring it up in case it was any concern.

Copy link
Member

@rakina rakina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, yeah I agree that having "navigate" as an entry point makes sense. Thank you for the links too!

@domenic
Copy link
Member Author

domenic commented Mar 12, 2021

I vaguely remember (perhaps incorrectly) that one reason we decided to keep them separate is because because browsers might copy session history state on the same-URL navigation case, but we didn't have reason to believe that this took place in the explicit historyHandling=replace case? I really don't know, I just wanted to bring it up in case it was any concern.

Yes, this is exactly why we kept them separate. That's #6213. I kept the XXX box pointing to #6213, but I think if we end up copying over the state then we'd probably do it as an extra condition inside the "replace" case that checks the URLs against each other at that time. See #6213 (comment).

@domenic domenic merged commit 2062a68 into main Mar 12, 2021
@domenic domenic deleted the fix-6202 branch March 12, 2021 00:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

Updating the session history: same-URL case is broken
3 participants