fix: avoid firing unnecessary preload on focus event

[freshgiammi]

onFocus is also invoked when a button is clicked, which causes the route to get preloaded twice (onMouseEnter + onFocus).

When focusing a link via keyboard navigation, we obviously skip onMouseEnter so it gets correctly preloaded once.
Given that there's no way to distinguish between mouse and keyboard origin for focus events, we can "trap" it by skipping the preload call when a focus event is detected between onMouseDown and onMouseUp: this makes preload only happen once and ensures that the behaviour is the same for both mouse and keyboard navigation.

[nx-cloud]

☁️ Nx Cloud Report

CI is running/has finished running commands for commit cc35b11. As they complete they will appear below. Click to see the status, the terminal output, and the build insights.

📂 See all runs for this CI Pipeline Execution

---

✅ Successfully ran 4 targets

• nx affected --targets=test:format,test:eslint,test:unit,test:build,build --parallel=3
• nx run-many --target=test:e2e:replay
• nx affected --target=test:types --exclude=examples/**
• nx run-many --target=build --exclude=examples/**

---

Sent with 💌 from NxCloud.

[schiller-manuel]

what is the real problem with preloading twice? isn't this cached by router (depending on the stale / gc time) settings?

[freshgiammi]

Yeah, there's a couple points this tries to improve rather than fix:

• There's simply no reason to fire preloadat the same time as navigate (+ it could lead to context issues such as preload: undefined context in loader if navigation happens before beforeLoad is done #1886)
• The behaviour is inconsistent, as it's different related to what happens when manaully preloading+navigating or even focusing a link via keyboard navigation
• If the preload fired from onMouseEnter isn't finished before the user clicks on the link, we just end up racing the load match routine further

It's more an improvement than a solution to a problem, but honestly it was screwing up my debugging on the preload issue because it kept changing the route state when I was not expecting it.

If the preload finishes and the route gets cached sure it probably won't harm, but why the extra work (and inconsistent behaviour)?

[SeanCassiere]

Should this have any event listeners for touch-based actions?

[freshgiammi]

Should this have any event listeners for touch-based actions?

Hm, how do we express intent on mobile? Currently, preload is fired when an user touches a link on mobile. I don't think it makes much sense though since we're directly navigating to the route.

I've tweaked the patch a bit further (and now it fires preload once instead of twice when touching on links), however I'm seeing onMouseEnter events getting fired from touch events and I've failed to trap it since it runs after onTouchDown, onPointerDown and onMouseDown.
I am unable to test it on a real device atm so I don't know if this is just mobile simulator on browser acting up.

[SeanCassiere]

With Tanner's #1907, we'll be deduping the beforeLoad calls so this shouldn't be a problem any further and reduces the need to attaching more event listeners.

Once it goes through, we'll revisit the need for this one.

[SeanCassiere]

This should no longer be needed since the route callbacks are now being deduped.

@freshgiammi thoughts? I think we can close.

[freshgiammi]

This should no longer be needed since the route callbacks are now being deduped.

@freshgiammi thoughts? I think we can close.

I haven't been able to test the router after #1907 was merged. I'll take a look early next week and report back, but from a cursory glance I think this isn't problematic anymore: we're probably fine with doing extra function calls rather than attaching listeners to prevent them now that route matching and loading is deduped.

[freshgiammi]

@SeanCassiere I'm not entirely sure everything has been deduped: the behaviour I see is different on first preload vs consecutive navigation, with loader running multiple times at the same time (in different amounts). Considering the work done on route callback side, improvements should be made there instead of here.
I'll close this PR, try to create replications and open issues instead.

EDIT: Scratch that, it looks like it's an issue with how my router is set up. I can't replicate any of the preloading/context issues I've had in the past.

  defaultPreload: "intent",
  defaultPreloadStaleTime: 0, // Ensure RQ never has stale data 
  defaultPendingMinMs: 0,
  defaultPendingMs: 0,