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

Let developers access the initiator element in visit/fetch events #750

Open
wants to merge 17 commits into
base: main
Choose a base branch
from

Conversation

domchristie
Copy link
Contributor

@domchristie domchristie commented Sep 30, 2022

This builds on top of #733

As @kevinmcconnell discovered in #720, the naïve approach to dispatching events on links/forms in #695 failed with the iOS adapter because DOM elements cannot be sent over postMessage.

This pull request aims to provide a safe way to dispatch events on initiating elements, enabling developers to choose how to handle Visits based on the element that triggered them. (See #99). Prior to sending VisitOptions to the adapter, non-transferable properties are removed. When creating a Visit, the original VisitOptions are reinstated. Ideally the sanitization process would be handled by the iOS adapter (just before calling postMessage), but for backwards compatibility, it needs to done at this level.


Details

First, it performs type-checking on the options sent to an adapter's visitProposedToLocation, ensuring that it conforms to the structuredClone algorithm. Navigator#sanitizeVisitOptionsForTransfer munges the options to satisfy these conditions.

During a Visit proposal, the Visit options temporarily stored in Navigator#proposedVisitOptions and cleared after the Visit has started. There is a bit of back-and-forth, so here's an outline of the relevant method calls:

  1. Navigator#proposeVisit; Navigator#withTransferableVisitOptions stores the proposedVisitOptions, and creates a sanitized version, passing them to…
  2. Session#visitProposedToLocation, which passes the sanitized options to…
  3. Adapter#visitProposedToLocation; the iOS adapter does its thing; the BrowserAdapter calls…
  4. Navigator#startVisit, which creates a Visit with the proposedVisitOptions and starts it
  5. Navigator#withTransferableVisitOptions resets the proposedVisitOptions property

Assuming Visits are created through this process, the initiator should be set. If a Visit is somehow created outside this flow, the initiator defaults to document.documentElement.

There's a potential bug if a Visit proposal errors, and Navigator#startVisit is called outside of this flow e.g. via Back/Forward: the Navigator might create a Visit with old proposedVisitOptions because they're not cleared due to the Visit proposal error. We could fix this using a try/catch/finally but Visit proposal errors would be silenced by the catch. Fixed in 3c3c8e3

Finally, I'm not totally convinced that the navigator is the correct place to store visit options, but it works, and keeps it relatively contained for now.

Resolves #733
Closes #99
Closes #720

@domchristie
Copy link
Contributor Author

An alternative approach to this might be to create a Navigation ID (UUID) that could be dispatched in the details of all events. This would make it possible to track a navigation from turbo:click all the way through to turbo:load, and therefore accurately associate the clicked element in a turbo:visit event.

I've also noticed that a redirected link creates two visits, but there's no accurate way to determine which is the initial visit and which is the redirect. A consistent Navigation ID could help with this. For example, if turbo:visit is dispatched with an ID that matches a previous one, then it's safe to assume it is the redirect.

@domchristie
Copy link
Contributor Author

Any thoughts on this @kevinmcconnell?

@domchristie
Copy link
Contributor Author

With word that Turbo 8 is in the works, do you have any thoughts on this feature? (or have anything similar in the pipeline?) I think it could be a game-changer for customising View Transitions.

/cc @afcapel @packagethief

src/core/types.ts Outdated Show resolved Hide resolved
To successfully communicate with the iOS view, messages need to be of a type supported by the structuredClone algorithm.
The new TransferableVisitOptions type ensures that VisitOptions passed to the adapter conform to this type. The StructuredCloneValue type has been created based on https://developer.mozilla.org/en-US/docs/Web/API/Web_Workers_API/Structured_clone_algorithm#supported_types
@domchristie
Copy link
Contributor Author

Rebased and tidied

@domchristie
Copy link
Contributor Author

Having played around with this implementation, there appears to be some odd side-effects when navigating frames (at least when promoting frame visits to update the URL bar). Unfortunately it triggers an untracked element mismatch and performs a full page reload.

Perhaps the simplest approach would be #695 but as @kevinmcconnell and I discussed in #720 (comment), it would require removing any values that do not conform the the structured clone algorithm at the iOS/Android adapter stage, which would impact backward compatibility.

Are there any plans for this in Turbo 8?

@domchristie
Copy link
Contributor Author

Having played around with this implementation, there appears to be some odd side-effects when navigating frames (at least when promoting frame visits to update the URL bar). Unfortunately it triggers an untracked element mismatch and performs a full page reload.

This appears to be an issue on main: #1047

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
1 participant