Skip to content

Fire navigate events on all windows affected during traversal #129

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

Merged
merged 2 commits into from
Jun 17, 2021

Conversation

domenic
Copy link
Collaborator

@domenic domenic commented Jun 15, 2021

Closes #114. @jakearchibald, can you review and also help with the TODO on figuring out the right step?


Preview | Diff


1. Let |destinationURL| be |destinationEntry|'s [=session history entry/URL=].
1. Let |destinationState| be |destinationEntry|'s [=session history entry/app history state=].
1. Let |isSameDocument| be true if |destinationEntry|'s [=session history entry/document=] is equal to |appHistory|'s [=relevant global object=]'s [=associated Document=]; otherwise false.

Choose a reason for hiding this comment

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

Eventually it might be best to compare the "document state" between the active entry and the destination entry, as this caters for cases where the document is null.

Although, I guess conveniently, the document will never be null in this case.

<div algorithm="apply the history step">
Modify the <a spec="HTML">apply the history step</a> algorithm as follows. Change <var ignore>checkForUserCancellation</var> to |fireBeforeunloadAndNavigate|. Add the |userInvolvement| parameter. Then, insert the following step after step 12 (which assembles |toTraverse| and other lists) but before step 13 (which checks if unloading is user-cancelled):

1. If |fireBeforeunloadAndNavigate| is true and the result of <a>firing traversal `navigate` events</a> given |toTraverse|, <var ignore>step</var>, <var ignore>initiatorToCheck</var>, and |userInvolvement| is false, then return.

Choose a reason for hiding this comment

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

Nit: It seems weird to me when an algorithm returns a boolean like this, but the name of the algorithm doesn't suggest what the return means. That's why I went for "checking if unloading is user-cancelled", but I guess the flip side of that is it isn't clear that the algorithm also fires events.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, I agree it's a bit iffy but I'm modeling it after the existing "fire an event" algorithm.

1. Let |completedTasks| be 0.
1. [=list/For each=] |navigable| of |toTraverse|, [=queue a global task=] on the [=history traversal task source=] given |navigable|'s [=navigable/active document=]'s [=relevant global object=] to run these steps:
1. Let |destinationEntry| be the item in the result of [=navigable/getting the session history entries=] for |navigable| that has the greatest [=session history entry/step=] less than or equal to |step|.
1. If |destinationEntry|'s [=session history entry/document=] is not equal to |navigable|'s [=navigable/active document=], and |initiatorOrigin| is not [=same origin-domain=] with |navigable|'s [=navigable/active document=]'s [=Document/origin=], then abort these steps.

Choose a reason for hiding this comment

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

Again, this could compare document states rather than documents, but it doesn't really matter.

It would matter in cases where both documents could be null, which would be unintentionally equal.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

A navigable's active document is never null, right?

@domenic
Copy link
Collaborator Author

domenic commented Jun 16, 2021

@natechapin, now I'd like your review. This doesn't yet account for the complexities of goTo() around passing info and dealing with the promise return value. However it shows the architecture that we'd use as a base. Roughly, any history traversal (whether via history.go() or using browser UI or, eventually, appHistory.back()):

  1. Queues a task/IPC to the browser process asking for the traversal to happen
  2. The browser process assembles a list of frames that are going to get navigated, and for each of them, maybe fires a navigate event. (Whether the event is fired/is cancelable/is canRespond true is computed at this time and subject to things we discussed previously. Most importantly, only same-origin-domain frames can cancel, and then only if it was programmatically initiated.)
  3. If the event ends up canceled in any of the windows, then abort the history traversal---similar to the user clicking no on a beforeunload prompt.
  4. Now proceed onward to doing beforeunload prompts in all unloaded frames. This might also abort the history traversal.
  5. If the history traversal still goes through, then do the traversals, eventually posting a task/IPC into each renderer process to complete each one.

I realize this isn't how we currently implement things, and might take some work to get there. In particular, currently we don't yet support traversing multiple frames at once (due to "the bug"), and once we do, we probably need extra code to handle beforeunload in multiple frames, so I imagine firing navigate in multiple frames might also be difficult. But does it seem directionally reasonable?

@csreis might also be good to review the above flow, although I'm not sure if he wants to review the actual spec text.

Copy link
Collaborator

@natechapin natechapin left a comment

Choose a reason for hiding this comment

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

This is fine as far as I'm concerned, with the caveat that I'm still not sold on allowing navigate to cancel a cross-document history traversal.

@domenic domenic merged commit e62682f into main Jun 17, 2021
@domenic domenic deleted the navigate-multiple-windows branch June 17, 2021 19:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

navigate events on traversal
3 participants