LinkPrefetchObserver: listen for complementary events

[seanpdoyle]

Prior to this commit, the LinkPrefetchObserver only listened for mouseleave events to clear the PrefetchCache instance. Not only were mouseenter events excluded, but the mouseleave event listeners were attached directly to the <a> element with a { once: true } option.

While unlikely, its was for those event listeners to never be removed if a mouseleave were to not fire. Similarly, during touchstart events the event listener were added, but never removed since there wasn't a a complementary mouseleave event firing to remove it.

This commit makes two changes to the event listeners:

1. extract the addEventListener calls to a loop, looping over mouseenter and touchstart event names
2. define complementary events for both mouseenter and touchstart

By moving the cancellation logic out of individual event listeners and into an this.eventTarget-wide scope, we limit the risk of leaking listeners. Similarly, we only ever instantiate one per event-pairing.

To track the <a> element reference, define both a this.#tryToCancelPrefetchRequest method and a this.#linkToPrefetch property to hold the reference to the <a> element in question.

[seanpdoyle]

@afcapel this line is the crux of this PR. Adding listeners directly to the <a> without removing them poses a risk of leaking memory from the instances. Additionally, there isn't a parallel pattern for touchstart (and touchend) events, so I'm not sure that cleanup is currently occurring on touch devices.

The rest of the changes in this diff support that setup-and-teardown.

Once this is merged, adding support for focusin and focusout events would be fairly trivial.

[seanpdoyle]

@afcapel if this is going to be enabled by default, I think this change is important to consider for touch devices.

[davidalejandroaguilar]

Adding listeners directly to the < a > without removing them poses a risk of leaking memory from the instances.

When once is true the listener would be automatically removed when invoked. So there wouldn't be a risk of memory leaks. Or am I missing something?

[davidalejandroaguilar]

Ahh just read the description, got it. Sorry, was linked directly to this comment from #1167 😃

[afcapel]

@seanpdoyle I don't think touchcancel is mimicking mouseleave here. touchcancel means that a touch point has been interrupted in an unexpected manner, but a touch point could leave the target in another ways.

[seanpdoyle]

I think the spirit of the teardown-side of event listening is that unless the click is navigated to 100ms after the setup-side, the prefetching should be reset.

Would touchend be more appropriate? Would mapping observedEvents to an array of listeners be better?

const observedEvents = {
  "mouseenter": ["mouseleave"],
  "touchstart": ["touchcancel", "touchend"],
  "focusin": ["focusout"]
}

[afcapel]

@seanpdoyle what do you think about 5d15bb3 ? It's basically the same idea, but we check if the prefetch request is still relevant on touchmove and touchend events, instead.

[seanpdoyle]

@afcapel I like that approach. I wonder if there's a way to make the event listener callback generic enough to apply to all types.

Maybe something like:

#checkIfPrefetchValid(event) {
  const targets = "targetTouches" in event ?
    [event.target, ...event.targetTouches] :
    [event.target]
  const interactingWithPrefetchedLink = targets.some((target) => target === this.#prefetchedLink)

  if (interactingWithPrefetchedLink) {
    // .. do the cancellation
  }
}

That change could support the idea of observedEvents mapping from "start" to "end" events in a generic enough way to support expanding support for incorporate focus change for keyboard navigators.

[afcapel]

That works, although I'm not sure yet how preload on focus events would work so it's hard to tell if it will generalize properly. We can add the abstraction now or when/if we add support for prefetching on focus events.

[seanpdoyle]

We can add the abstraction now

Regardless of focus support, I think an abstraction that correlates which events start a prefetch with the events that cancel a prefetch has value.

I'm not sure yet how preload on focus events would work

In what way is a focusin event that is not followed by a focusout event 100ms later different than a mouseenter that is not followed by a mouseleave 100ms later? What other facets are there to consider focus keyboard navigation?

[afcapel]

Regardless of focus support, I think an abstraction that correlates which events start a prefetch with the events that cancel a prefetch has value.

Fair enough, If you can update the PR to handle touchend and touchmove I'll merge it 👍 A test exercising the prefetch cancellation would also be useful.

Regarding prefetching on focusin/focusout, as @brunoprietog pointed the keyboard navigation may need a special timing. But honestly, I'm not sure, we'd have to test it on realistic conditions to tell.

[seanpdoyle]

A test exercising the prefetch cancellation would also be useful

Unfortunately, I can't find a way to start then end a touch event in Playwright. The closest that I came to finding a solution was the Locator.tap method, but that ends up definitively tapping the link as a navigation, even when immediately followed by tapping elsewhere in the screen.

[afcapel]

Is this correct for touch events? If the touchend event still has a targetTouch that is the prefetched link, that means we didn't leave the link, right? Same with touchmove?

[seanpdoyle]

Maybe TouchEvent.changedTouches is a better candidate:

• For the touchstart event, it is a list of the touch points that became active with the current event.
• For the touchmove event, it is a list of the touch points that have changed since the last event.
• For the touchend event, it is a list of the touch points that have been removed from the surface (that is, the set of touch points corresponding to fingers no longer touching the surface).

[afcapel]

Not sure if that will work. For example, if a touchend fires and changedTouches points to another element, that doesn't mean the link is not still being tapped. It could be another finger that was removed from the surface. Same with touchmove.

Maybe I'm missing something, but I think we'll have to use two different handlers. For example #checkIfPrefetchValidAfterMouseLeave and #checkIfPrefetchValidAfterTouchChange. The two cases have opposite logic: for mouseleave we left the link if the event target is the link; for touch changes we left the link if none of the targets is the link.

[seanpdoyle]

After more reflection, I realize that I've misread the conditionals in 5d15bb3.

I've pushed up a new commit use Array.some to check a touchend event's changedTouches for the presence of the link, Array.some to check for a touchmove event's targetTouches for the absence of the link, and all other types of events for the target itself.

Even though its a singular conditional and a singular event listener callback, it isn't as general purpose as I originally intended.

With that being acknowledged, I think that if a change accounts for touch events, removes all event listeners that it attaches, and doesn't leak additional listeners, it's worth shipping. I don't have a preference between this PR and 5d15bb3. If it's easier for you to coordinate the necessary steps to merge your fix, I say go for it.

[seanpdoyle]

Closing in favor of #1167.