Update the navigation API when a same-document reload occurs #10989
Conversation
This performs the subset of the "URL and history update steps" that are applicable to reload navigations, in the "inner `navigate` event firing algorithm". Closes whatwg#10621
| <ol> | ||
| <li><p><span>Update the navigation API entries for a same-document navigation</span> given | ||
| <var>document</var>'s <span>relevant global object</span>'s <span | ||
| data-x="window-navigation-api">navigation API</span>, <var>navigable</var>'s <span |
There was a problem hiding this comment.
You can just use the navigation variable here.
| <var>document</var>'s <span>relevant global object</span>'s <span | ||
| data-x="window-navigation-api">navigation API</span>, <var>navigable</var>'s <span | ||
| data-x="nav-active-history-entry">active session history entry</span>, and "<span | ||
| data-x="dom-NavigationType-reload">reload</span>".</p></li> |
| <li><p><span data-x="tn-append-session-history-sync-nav-steps">Append the following session | ||
| history synchronous navigation steps</span> involving <var>navigable</var> to | ||
| <var>navigable</var>'s <span data-x="nav-traversable">traversable navigable</span>: | ||
| <span>Apply the reload history step</span> to <var>navigable</var> given |
| <li><p><span data-x="tn-append-session-history-sync-nav-steps">Append the following session | ||
| history synchronous navigation steps</span> involving <var>navigable</var> to | ||
| <var>navigable</var>'s <span data-x="nav-traversable">traversable navigable</span>: | ||
| <span>Apply the reload history step</span> to <var>navigable</var> given |
There was a problem hiding this comment.
We need to apply the reload history step to the traversable, not the navigable. So probably it's best to pull out the traversable into a separate variable, like https://html.spec.whatwg.org/#reload does.
| history synchronous navigation steps</span> involving <var>navigable</var> to | ||
| <var>navigable</var>'s <span data-x="nav-traversable">traversable navigable</span>: | ||
| <span>Apply the reload history step</span> to <var>navigable</var> given | ||
| <var>userInvolvement</var>.</p></li> |
There was a problem hiding this comment.
Upon reviewing "apply the history step" in detail, I'm no longer sure we need to perform it.
Note that we're not setting the "reload pending" flag. This is probably good, because "reload pending" kicks off stuff like actually re-fetching the document. But I think it ends up meaning that "apply the history step" is a no-op.
Rough check:
- Step 3 is non-applicable for reloads
- Step 6 and 7 set changingNavigables and nonchangingNavigablesThatStillNeedUpdates to the empty set because "reload pending" is not set. This is maybe a bug, but maybe fine: it depends on if we actually need to do anything to changingNavigables.
- Step 8 would "set the ongoing navigation" if changingNavigables were non-empty. Maybe we should be doing that, since it cancels other navigations?
- Step 12 for actual reloads would do the actual fetch. It's good that we skip this.
- The afterDocumentPopulated sub-algorithm also seems unnecessary to run for our case.
- Step 14 job processing seems mostly inapplicable.
- 14.12.3 is maybe interesting. It performs update document for history step application, which would perform "update the navigation API entries for a same-document navigation" again, and fire popstate.
- I don't think we want to fire popstate for same-document reloads? It might be worth checking what Chrome does.
- I found this TODO which might be worth taking care of to get more test coverage here.
- It is correct to skip the update to
navigation.activationfor our case.
If you could double-check my logic here, then probably we should delete this step (ignoring my above nits) and maybe add a note explaining that it's OK for same-document reloads to skip "apply the reload history step", because all the session history synchronization would be a no-op for reloads that don't actually create a new Document.
There was a problem hiding this comment.
- 14.12.3 is maybe interesting. It performs update document for history step application, which would perform "update the navigation API entries for a same-document navigation" again, and fire popstate.
- I don't think we want to fire popstate for same-document reloads? It might be worth checking what Chrome does.
Chrome currently does fire popstate for intercepted reloads. It's a bit dodgy since we're not actually popping any state from history but rather injecting some new state to an intercepted reload. But perhaps this could be seen as modifying an existing history entry and then "popping" it. Not sure where this leaves us with the direction of this PR, I think the safest approach and closest to the chromium implementation is to run those steps and have a lot of them skipped rather than try to extract the minimal set of substeps.
There was a problem hiding this comment.
Got it. In that case the PR as-is doesn't match Chromium. Some possibilities:
- Keep running "apply the history step"
- Modify step 6 to add the navigable to changingNavigables, despite it not having the actual reload flag set. This will ensure we do something instead of nothing in step 14.
- Ensure 14.12.2 does not happen (that would cause a VisiblityStateEntry to be queued)
- Modify "update document for history step application" to not perform "update the navigation API entries for a same-document navigation" again in this case, somehow.
- Note that this only works if Chromium's popstate firing is async... if it's sync, then "apply the history step" is just not correct.
- Extract something more minimal
- Don't run "apply the history step"
- But do run something that fires popstate
- Maybe just a specific step?
- Maybe try to run "update document for history step application"?
- In this case documentsEntryChanged is false (I am pretty sure), so all of step 6 is skipped, but we still want to run step 6.4.3, so that could be tricky.
- Change Chromium to not run popstate.
This performs the subset of the "URL and history update steps" that are applicable to reload navigations, in the "inner
navigateevent firing algorithm".Closes #10621
(See WHATWG Working Mode: Changes for more details.)
/nav-history-apis.html ( diff )