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

Add InstantClick behavior #1101

Merged

Conversation

davidalejandroaguilar
Copy link
Contributor

@davidalejandroaguilar davidalejandroaguilar commented Dec 6, 2023

Description

This PR adds InstantClick-like behavior to Turbo. For those not familiar with it, what it does is prefetch links that are likely to be clicked on.

Before visitors click on a link, they hover over that link. Between these two events, 200 ms to 300 ms usually pass by (test yourself here).

The result is effectively instant navigation in most cases.

Implementation

  • An observer is added to the Turbo session. It adds 3 event listeners:
    • mouseover.
      • When the mouse is over a link, it will wait 100ms before starting a fetch request to prefetch it and save the request without awaiting for it to a cache with an expiration of 10 seconds (configurable).
      • This delay is necessary to prevent a flurry of prefetch requests when casually scrolling down the page.
    • touchstart.
      • Same thing as above, but when the user finger touches the screen.
    • turbo:before-fetch-request.
      • When a link is clicked, it will check if the location exists in the cache, and if so, take the fetch request out of it and put it on the event.detail.response.
  • The FetchRequest#perform method will now accept overriding the response internally (not exposed as part of public API) by assigning it to event.detail.response on the turbo:before-fetch-request event added by the observer.
    • The cached response is then used here instead of creating a new fetch request.
    • This cached request will be awaited and #received as usual (most probably by this time, the fetch request promise has already resolved), resulting in an instant navigation.
    • This means that all the usual Turbo behavior still happens (browser history change, snapshot, page update without full reload, etc.).

Configuration

  • The amount of time a link is prefetched again after hovering can be configured via a <meta name="turbo-prefetch-cache-time" content="1000"> tag, in milliseconds, and defaults to 10000.

  • This behavior is disabled by default and can be opted-in via a <meta name="turbo-prefetch" content="true"> tag.

  • You can opt-out a link from being prefetched on hover by adding a data-turbo-prefetch="false" attribute to it.

  • You can also opt-out/in descendants of an element this way (opt-out/in by container).

Notes

On mobile devices, preloading starts on “touchstart”, letting 300 ms (Android) to 450 ms (iOS) for preloading the page.2

  • The concept name I chose for this behavior is "prefetch", because we already have the concept of "preloading" by using data-turbo-preload on specific links we want to preload on page load.
  • A few more differences from the existing preloading concept and implementation, apart from the main difference of preloading on hover vs on initial page load:
    • Preloading awaits each fetch request sequentially in order to manually populate the SnapshotCache, whereas the prefetched ones are not awaited when created, and thus allows for non-blocking concurrent execution of requests.
    • Prefetching doesn't manually populate the SnapshotCache like preloading does, but just lets Turbo do everything automatically by just using the cached fetch request instead of creating a new one. This means it's less intrusive on the internals of Turbo.

Prior art

In no specific order:

  • Oleksandr and Pete from Docuseal (Open Source Document Signing built with Rails and Turbo) (repo), added a Turbo version of InstantClick. The perceived speed benefits are simply amazing. They can be experienced on their website.
    • This implementation is based on this one.
    • I added this to an app of mine, including support for touchstart, and have been running in production for a while with no hiccups.
  • hopsoft's Turbolinks prefetching implementation.
  • phacks' implementation.

Demo

A scaffold Rails app with 0.5 delay on the PostsController#show action.

Noticeable delay:

Screen.Recording.2023-12-05.at.10.08.44.p.m.mov

Instant navigation:

Screen.Recording.2023-12-05.at.10.06.39.p.m.mov

Scrolling down a page with many links

  1. No delay, ends up in a flurry when moving the mouse.
Screen.Recording.2023-12-09.at.2.41.43.a.m.mov
  1. With the recommended 100ms max. Notice how there's no more flurry of requests, yet when we do a small intent pause before clicking, we get a prefetch.
Screen.Recording.2023-12-09.at.2.42.49.a.m.mov

@davidalejandroaguilar davidalejandroaguilar changed the title Add InstantClick behavior to Turbo Add InstantClick behavior Dec 6, 2023
anchor_ prefix is used for all anchors in the tests
@brandondrew
Copy link

This looks awesome—thanks for your work on it!

  • The concept name I chose for this behavior is "prefetch", because we already have the concept of "preloading" by using data-turbo-preload on specific links we want to preload on page load.

    • I'm open to suggestions on this.

Regarding the name, let's consider what ideas are conveyed by the related words:

  • "preload" and
  • "prefetch"

One contains the verb "load" and the other contains "fetch", implying that preloading and prefetching perform different operations.

Both contain "pre" which is a temporal reference, suggesting doing something early, where "early" means before some other significant point in time. Since both have the same temporal reference, it would imply that they both perform their operations at the same early point in time.

However the truth is the opposite:

  • the perform (more or less) the same operation,
  • they perform their operation at different points in time.

Therefore it would be vastly preferable in my opinion if they shared the verb "load" but the new one had a different way of referring to when it performs this loading operation.

The clearest option I can think of is "hoverload". It loads the linked page on hover. I think this will avoid lots of confusion when reading code, where we have to stop and ask ourselves "wait, what's the difference between preloading and prefetching again?"

@davidalejandroaguilar
Copy link
Contributor Author

@brandondrew Thanks for chiming in!

Agree that the naming might lead to confusion and I like hoverload, though this behavior can happen on mouseover (hover), mousedown (start to press mouse button) and eventually touchstart (finger touches screen). So perhaps the prefix hover is no longer appropriate.

Although if we base the name on the default behavior, which is hovering, and think of touching the screen as the equivalent of "hovering" over a link on a mobile device, then it does make sense.

Happy to make the change, though would like to wait for a few more folks to chip in too!

@brandondrew
Copy link

hmm... you're right, it might not be perfect.... but I can't think of anything better so far... 😄

  • mouseload?
  • late_pre_load?
  • triggerload (where hovering or mousing down, or putting your finger on the screen is the "trigger")

Maybe someone else will have a better idea...

@brandondrew
Copy link

brandondrew commented Dec 7, 2023

  • instantload—is not a perfectly accurate description of what actually happens, but it corresponds to "InstantClick" and it does describe the user's experience.

@airblade
Copy link

airblade commented Dec 7, 2023

"PredictiveLoad"?

@adrienpoly
Copy link
Member

Thanks for this proposal that would be a great addition.

I agree that the proposed naming is confusing with the current preload attribute.

What about data-turbo-instant=true

@marcelolx
Copy link

marcelolx commented Dec 7, 2023

Could we build on top of the existing data attribute?

  • data-turbo-preload preloads the link on page load (backward compatibility)
  • data-turbo-preload="page-load" preloads the link on page load (new attribute value for existing way of link preload)
  • data-turbo-preload="on-demand" preloads the link on mouseover/mousedown/touchstart (this PR)
    • Other options that come to mind would be on-interaction or just interaction vs on-demand

I am not sure if that would make the implementation much more complex, but I think if it was built around the existing data attribute, it would avoid a lot of confusion.

Both features preload the links, the difference is when and how, but for most people that are going to use this feature, the only thing that will really matter is when they want to preload the link, and having different data attributes will make that confusing vs same data attribute with different values

@marcelolx
Copy link

I think it is also worth mentioning that similar behavior has been suggested in the original PR that introduced data-turbo-preload #552 (comment)

@afcapel
Copy link
Collaborator

afcapel commented Dec 7, 2023

@davidalejandroaguilar thanks for bring this up, it is something we definitely want to add to Turbo! I'm going to give it a spin in one of our apps and and will come back with feedback after that.

@brandondrew
Copy link

Could we build on top of the existing data attribute?

  • data-turbo-preload preloads the link on page load (backward compatibility)
  • data-turbo-preload="page-load" preloads the link on page load (new attribute value for existing way of link preload)
  • data-turbo-preload="on-demand" preloads the link on mouseover/mousedown/touchstart (this PR)

Okay, this suggestion has a lot of merit, and in many ways makes sense (I like the cohesive API suggested) but I have strong reservations about falling back to preloading everything on pageload for clients that can't handle loading on hover.

To determine whether the question is moot or not, do we know that there are actually any browsers in this situation? Would there actually be people who have to have everything load on page load because their browser can't handle the code in this PR that's been (at some future date) added to Turbo?

If there are no such browsers, then I like this suggestion a lot.

But if there are browsers like that, then I would not want to be forced to make those browsers fall back to preloading everything on pageload. Imagine a gallery home page with 100 links that all go to image-heavy pages. Imagine a user on a slow cellular network opening the gallery home page.

@brandondrew
Copy link

If the attribute is added to each link, then why does this section (below) seem to imply that it is controlled at the page level, and the links are opt-out only?

Configuration

...

  • This behavior is disabled by default and can be opted-in via a <meta name="turbo-prefetch" content="true"> tag.
  • You can opt-out a link from being prefetched on hover by adding a data-turbo-prefetch="false" attribute to it. You can also opt-out descendants of an element this way.

Did I misunderstand that 👆?

IMHO it would be preferable if the developer could also opt-in at the element level.

@brandondrew
Copy link

brandondrew commented Dec 7, 2023

As far as names (if it is given a different name instead of a new value of the preload attribute)

I think @adrienpoly's suggestion might be the catchiest name, and I like it a lot. It describes the user's experience (more or less) but it doesn't give a very clear description to a developer of what's really happening.

@airblade's suggestion is the most accurate description of what it does, but predictiveload feels a little bit too long and awkward to me. On the other hand, if it is determined that there is no problem with adding values to the data-turbo-preload attribute, then data-turbo-preload="predictive" seems perfect, since that's exactly what is happening. It's a predictive preload. This nails the terminology exactly.

(Also, data-turbo-preload="predictive" might be possible to use without falling back to preloading on page load for all of those links.)

My 2¢ in summary:
If a new attribute is introduced, my preference is slightly for for hoverload over instant, but only slightly. If potential problems of falling back to preloading all designated links when the page loads can be resolved, then I think data-turbo-preload="predictive" is absolutely perfect.

@marcelolx
Copy link

but I have strong reservations about falling back to preloading everything on pageload for clients that can't handle loading on hover.

I didn't mean falling back to pageload, what I meant is that data-turbo-preload and data-turbo-preload='pageload' should behave the same since the existing API expects just data-turbo-preload, and then eventually deprecate the option without a value.

I think if someone specifies data-turbo-preload='on-demand' it should only try to do it on those events David outlined in the PR description

@pfeiffer
Copy link
Contributor

pfeiffer commented Dec 7, 2023

A slightly different - but in my opinion way simpler solution in Turbo 'core' - would be to change the existing Preloader to use a MutationObserver observing any links with data-turbo-preload attribute, instead of just prefetching links on DOMContentLoaded.

Implementing this would allow developers to add their own behavior (ie. "instant click", preload only links above the fold, ..) by simply dynamically adding a data-turbo-preload attribute to the links as they see fit, eg on hover or scroll into the viewport or whatever custom logic they'd like.

The behavior described in this PR (hover, viewport, ..) would be very well suited as a separate add-on/package, providing reasonable default behavior as described in the PR description.

This PR introduces code that is almost duplicate of the existing preloading behavior.

There's already a PR (#911) changing the preloader to use MutationObserver (although some comments must be adressed) and also a related PR making the preloader more resilient by @seanpdoyle in #1033. Could it be built on that?

@pfeiffer
Copy link
Contributor

pfeiffer commented Dec 7, 2023

The FetchRequest#perform method will now accept overriding the response internally (not exposed as part of public API) by assigning it to event.detail.response on the turbo:before-fetch-request event added by the observer.

  • The cached response is then used here instead of creating a new fetch request.

I think this would lead to unexpected results and is a breaking change, as least when combined with the Turbo preloaded links, where the behavior is that Turbo displays the preloaded snapshot from the cache while performing a fetch for the url.

In our application, we use the Turbo preload pattern quite extensively - all preloads fetch "skeleton pages" (that are extremely cachable with no database queries made and with a Vary: Sec-Purpose header) and upon navigation, we fetch the full page that contains dynamic content.

This ensures that tapping a link navigates instantly and displays a skeleton page, and then the full page with dynamic and user-specific content is fetched by Turbo.

This PR assumes that the page returned by a prefetch request is the same as a full normally fetched page, which I think is not always the case as described above and is a breaking change from how Turbo behaves today. I also believe the existing preloading behavior of Turbo supports not re-fetching the preloaded page, by setting cache headers (as the subsequent fetch request would be a cache hit in the browser) if that is what you'd want.

@pfeiffer
Copy link
Contributor

pfeiffer commented Dec 7, 2023

Come to think of it, we are actually also doing "insta click" behavior with Turbo today, although it misuses the internal Turbo.session.preloader.preloadURL(..). There was originally a PR (#910) to make the preloadURL(..) public, but a MutationObserver (#911) would also solve this.

I've added a gist here for inspiration: https://gist.github.com/pfeiffer/d53bd40a39ee0586f54303e525c60fe2

@pfeiffer
Copy link
Contributor

pfeiffer commented Dec 7, 2023

The amount of time a link is prefetched again after hovering can be configured via a <meta name="turbo-prefetch-cache-time" content="1000"> tag, in milliseconds, and defaults to 10000.

I'm wondering why this would be needed? Why not rely on normal HTTP caching mechanism and headers to communicate TTL and let fetch handle that?

Comment on lines +12 to +13
hoverTriggerEvent = "mouseenter"
touchTriggerEvent = "touchstart"
Copy link
Contributor

Choose a reason for hiding this comment

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

In addition to mouse and touch events, should the observer also monitor focusin events for keyboard navigators to also benefit from pre-fetching?

Copy link
Contributor

Choose a reason for hiding this comment

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

This would trigger requests all the time when pressing tab, right? Or by browsing the page with the keyboard as a screen reader user does. I think it could be a machine gun of requests in that case.

However, if the focusin event is not considered, screen reader users cannot take advantage of this feature at all.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's a great idea, as @afcapel noted, let's add that on a follow-up PR!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@brunoprietog The delay feature would prevent this.

i.e. It would prevent a flurry of requests from happening if the user is fast navigating links using the Tab key. However, once the user stops at a particular link, we'd prefetch it.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not so sure that value is satisfactory enough, as an average screen reader user navigates more slowly. This is because when focusing on a control, you have to listen to what it contains before making a decision. It could even be seconds. I really think it's more complicated than I thought.

@afcapel
Copy link
Collaborator

afcapel commented Jan 18, 2024

@davidalejandroaguilar I've given this a good try on Basecamp. It's a good use case because it's a large app with plenty of tests and being a bit older it also has more complicated use cases where Turbo needs to play nicely with older JavaScript.

In all those tests, I kept finding cases where some stale content was being rendered. Clearing the cache on Turbo submission helps, but Basecamp 4 has plenty of screens that use older JavaScript and perform destructive actions without Turbo. I also found a case in which the cache was used in a redirection, after a form submission that didn't use Turbo.

But I think there's an easy way to sidestep this problem. Instead of keeping a cache of all the links we've hovered in the last 10 seconds, we can just keep a reference to a single request, for the link we are currently hovering. We clear up any existing cache entry each time we mouseover (or touchstart) a new link.

So let's say this happens:

  1. You hover a link for more than 100ms. That launches a prefetch request.
  2. You move away from the link and perform a destructive action.
  3. Come back to the link and this time click on the link.

The cache entry created in 1. doesn't matter because we clear it up when we re-enter the link in 3. This is still very fast most of the time and avoids stale content problems. The cache may be a bit less efficient if you hover a link, move away and then go back to it. But trying to use the cache in this situation is what's introducing the stale content in the first place.

I've given this a try in fb58b6b and it solves all the stale content problems. With that in place, I think we can merge the PR and make any improvements, for example for keyboard navigation, in other PRs. What do you think?

afcapel and others added 4 commits January 18, 2024 20:07
Instead of maintaining a cache of all the links that have been hovered
in the last 10 seconds.

This solves issues where the user hovers a link, then performs a non-safe
action and then later clicks the link. In this case, we would be showing
stale content from before the action was performed.
This avoids a flurry of requests when casually scrolling down a page
@davidalejandroaguilar
Copy link
Contributor Author

@afcapel That does make sense to me. We should prioritize compatibility with those cases over prefetching optimization. I have pushed that commit.

I also like the Session now only checking if it's visitable and the prefetch observer creating the request to be used later 👍🏼

Would like to note that I have re-added cancelling the prefetch if the link is no longer hovered, otherwise the flurry of requests when casually scrolling comes back.

Finally, I've been testing the new delay default of 100ms and I'm thinking we might want to allow this to be configured. There might be some apps that are fine with having no delay and would prefer to prefetch as soon as possible. However, to avoid further delaying of this PR, we could add that on a subsequent PR if you agree on this.

Otherwise, I think we're in a good shape now for merging. Thanks again for the thorough testing on a big app like Basecamp! 💪🏼

@guillaumebriday
Copy link

Another check that could be done, is the ensure the connection if good before prefetching data

In my controller stimulus-prefetch, I checked if the connection isn't in saveData mode and if it's fast enough to prefetch new pages

See the code here 👉 https://github.com/stimulus-components/stimulus-prefetch/blob/master/src/index.ts#L38-L48

What do you think?

@afcapel
Copy link
Collaborator

afcapel commented Jan 22, 2024

Another check that could be done, is the ensure the connection if good before prefetching data

@guillaumebriday I like the idea 👍 We don't need it for this PR, but it's something we can add later.

…tclick-behavior

* origin/main:
  Keep Trix dynamic styles in the head (hotwired#1133)
@afcapel
Copy link
Collaborator

afcapel commented Jan 22, 2024

Finally, I've been testing the new delay default of 100ms and I'm thinking we might want to allow this to be configured.

I'm OK with that. The menu is omakase but substitutions are still possible.

@afcapel afcapel merged commit 623a9df into hotwired:main Jan 22, 2024
1 check passed
@afcapel
Copy link
Collaborator

afcapel commented Jan 22, 2024

Great work @davidalejandroaguilar 👏

@davidalejandroaguilar davidalejandroaguilar deleted the davidramos-add-instantclick-behavior branch January 22, 2024 11:21
@davidalejandroaguilar
Copy link
Contributor Author

@afcapel Thank you! And thanks again for your time. Excited for all the awesomeness that's coming for everyone on Turbo 8! 🚀

I'll open a follow-up PR for the configurable delay and another one for the connection health check.

@guillaumebriday-pa
Copy link

Nice! Can you release a new beta so we can try it on our apps? 🚀

@afcapel
Copy link
Collaborator

afcapel commented Jan 22, 2024

@guillaumebriday I've just released v8.0.0.beta.3!

@brandondrew
Copy link

Finally, I've been testing the new delay default of 100ms and I'm thinking we might want to allow this to be configured.

I strongly agree with this

afcapel added a commit to pfeiffer/turbo that referenced this pull request Jan 29, 2024
* origin/main:
  Introduce `turbo:{before-,}morph-{element,attribute}` events
  Turbo 8.0.0-beta.4
  Introduce data-turbo-track="dynamic" (hotwired#1140)
  Ensure that the turbo-frame header is not sent when the turbo-frame has a target of _top (hotwired#1138)
  Turbo 8.0.0-beta.3
  Fix attribute name (hotwired#1134)
  Add InstantClick behavior (hotwired#1101)
  Revert hotwired#926. (hotwired#1126)
  Keep Trix dynamic styles in the head (hotwired#1133)
  Remove unused stylesheets when navigating (hotwired#1128)
  Upgrade idiomorph to 0.3.0 (hotwired#1122)
  Debounce page refreshes triggered via Turbo streams
  Update copyright year to 2024 (hotwired#1118)
  Turbo 8.0.0-beta.2
  Set aria-busy on the form element during a form submission (hotwired#1110)
  Dispatch `turbo:before-fetch-{request,response}` during preloading (hotwired#1034)
@Kagayakashi
Copy link

Is it possible to implement caching to prevent multiple requests for a single link? Currently, every time I hover over the same link, a new request is made. Why isn't this request cached? If the concern is that cached data might be outdated, we could consider making a second request upon click. Alternatively, is it better to use a delay? However, if the user moves the mouse over the link too quickly and clicks, wouldn't this result in no request being made?

@davidalejandroaguilar
Copy link
Contributor Author

@Kagayakashi Hey there! 👋🏼 The initial implementation cached requests internally for a configurable duration of 10 seconds.

However, during testing with a big app like Basecamp, @afcapel noticed this approach could lead to stale content, particularly in areas using older Javascript without Turbo. So for now, we opted to prioritize compatibility over optimization. Despite this, we're still able to harness the benefits of HTTP caching!

Regarding the delay, it was added to prevent a flurry of requests when casually scrolling over a list of links. There are demo videos showing a before and after for this on the PR description.

@brandondrew
Copy link

@afcapel noticed this approach could lead to stale content particularly in areas using older Javascript without Turbo.

Why disable it globally, instead of making it configurable?

So for now, we opted to prioritize compatibility over optimization.

Why not do that by making the default configuration as compatible as possible?

Despite this, we're still able to harness the benefits of HTTP caching!

Are you referring to caching by proxy servers? By the browser, by default?

@doits
Copy link

doits commented Apr 18, 2024

Is there any chance to configure this to work like this?

  • Prefetch and cache once on hover, even when mouse is moved out and in again
  • When clicking the link, display the cached version but make a new get request in parallel to update the page (in case something changed)

The cache could be configured longer then, e.g. 1 minute or so, because there will be always a new fetch after visiting the cached link.

The current default behaviour fetches the same page multiple times, just scrolling an moving the mouse over the a page. Is this really a good default choice? I know the discussion was lengthy already, but maybe it can be reconsidered?

(For my case I will simply disable it, but I'd prefer to have a solution that works like above with one cached fetch and a second fetch on page visit.)

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

Successfully merging this pull request may close these issues.

None yet