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

Navigation: navigate to a fragment updates the navigation api for same-document navigation twice. #9826

Closed
ADKaster opened this issue Oct 2, 2023 · 2 comments · Fixed by #9990

Comments

@ADKaster
Copy link
Contributor

ADKaster commented Oct 2, 2023

What is the issue with the HTML Standard?

In navigate to a fragment, the following steps happen:

  • Step 4 fires a push/replace/reload navigation event with either "push", or "replace"
  • Steps 6-12 create a new Session History Entry and sets the navigable's active session history entry to it

Step 13 then calls update document for history step application


My reading of the steps followed in update for history step application is like so:

  1. Let documentIsNew be true if document's latest entry is null; otherwise false.

documentIsNew will be false. This is a fragment navigation, so the document has already been loaded and populated with an initial session history entry.

  1. Let documentsEntryChanged be true if document's latest entry is not entry; otherwise false.

document's latest entry will be the page we are navigating from, so documentsEntryChanged will be true.

skipping ahead to step 5.5.1, since documentsEntryChanged is true and documentIsNew is false:

5.5.1. Update the navigation API entries for a same-document navigation given navigation, entry, and "traverse".

This is very suspicious, because this is not a traverse, it is a push or replace.

Inside update the navigation api entries for a same document navigation we get to step 3, which checks for "traverse".

3.1 Set navigation's current entry index to the result of getting the navigation API entry index of destinationSHE within navigation.
3.2 Assert: navigation's current entry index is not −1.

The assertion in step 3.2 fails, unless we have previously navigated to this fragment.


Later in navigate to a fragment, step 14 calls update the navigation api entries for a same document navigation with the correct "push" or "reload" state change.

At the end of the navigate to a fragment algorithm we end up finalize a same-document navigation, which ends up calling apply the history step, with the fireNavigateEventOnCommit argument set to false. I have a hard time following the path through that algorithm that we are supposed to take for fragment navigations though, and whether the fact that that argument is ignored is ok? We have already fired our navigate event in step 3 of navigate to a fragment, so it should not be fired by apply the history step.

@domenic
Copy link
Member

domenic commented Oct 3, 2023

Notes so far:

  • We do want to fire popstate and hashchange and do everything else. So it's really only the duplicate "update the navigation API entries for a same-document navigation" that's the problem.
  • We want to update the entries before firing popstate and hashchange. So it's the call to "update the navigation API entries for a same-document navigation" inside "navigate to a fragment" that should be deleted.
  • This means we need to fix the navigationType passed to "update the navigation API entries for a same-document navigation" in "update document for history step application".
    • The current arguments do not have enough information to pass in the correct value here.
    • The obvious solution is to introduce a new navigationType argument.
    • For the "navigate to a fragment" call site, this would be easy to plumb.
    • For the "apply the history step" call site, this would be harder to plumb.
      • We could do it relatively easily by using the wrapper algorithms. That's kind of inelegant; the whole point of "apply the history step" is that you should care less about the navigation type, and just be "applying" things that are set up for you.
      • There might be something clever-er we could do with the information already in the algorithm though.
        • In particular, only the "apply the history step" call site passes entriesForNavigationAPI.

I was hoping we could go an alternate route where we keep the "update the navigation API entries for a same-document navigation" inside "navigate to a fragment", but move it before "update document for history step application". I think this fails, because events fired by "update the navigation API entries for a same-document navigation" would observe the old history.length (and history index).

@pragyamishra56

This comment was marked as spam.

domenic added a commit that referenced this issue Dec 13, 2023
Closes #9826. da3a62e made it easy to pass along the correct navigationType, and thus trigger the proper path through "update the navigation API entries for a same-document navigation".

Also fixes a typographical issue from da3a62e.
domenic added a commit that referenced this issue Dec 13, 2023
Closes #9826. da3a62e made it easy to pass along the correct navigationType, and thus trigger the proper path through "update the navigation API entries for a same-document navigation".

Also fixes a typographical issue from da3a62e.
domenic added a commit that referenced this issue Jan 9, 2024
Closes #9826. da3a62e made it easy to pass along the correct navigationType, and thus trigger the proper path through "update the navigation API entries for a same-document navigation".

Also fixes a typographical issue from da3a62e.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging a pull request may close this issue.

3 participants