transitionWhile doesn't quite work for scroll restoration

[jakearchibald]

A regular navigation happens roughly like this:

1. Begin fetching
2. Time passes
3. Fetch complete
4. Capture & store scroll position
5. Switch to new page
6. Restore scrolling if applicable

transitionWhile doesn't really fit in this model.

If an unsettled promise is passed to transitionWhile (due to pending network activity), it goes like this:

1. Begin fetching
2. transitionWhile called - capture & store scroll position
3. Time passes
4. Fetch complete
5. Alter DOM
6. Promise passed to transitionWhile resolves
7. Restore scrolling if applicable

This isn't ideal, as the user may have scrolled during the fetch, but that change is discarded.

If a fulfilled promise is passed to transitionWhile (due to the page data being available synchronously), it goes like this:

1. Alter DOM
2. transitionWhile called - capture & store scroll position
3. Restore scrolling if applicable

This behaviour is broken, as the new DOM may drastically alter the scroll position. Also, the developer may choose to reset the scroll position after altering the DOM, if the page is conceptually new.

It feels like the API needs to change so it knows when to capture the scroll position.

---

An alternative design:

navigation.addEventListener('navigate', (event) => {
  event.transitionWhile(async (transition) => {
    const response = await fetch(dataURL);
    const data = await response.json();
    transition.captureState();
    await updateDOM(data);
  });
});

Here, transitionWhile takes a callable. It captures scroll and focus state before calling the callable. This solves the synchronous issue.

The developer can call captureState to recapture state at a later time, ideally just before updating the DOM.

Scroll positions are restored when the promise returned by the callable fulfills.

[bathos]

This is similar to the issue that lead me to defer url changes until (the equivalent of) transition completion in our current use of History. When the URL updates on the leading edge, every relative link in the DOM becomes a minesweeper cell for the duration of the transition. Usually too quick to matter, but not guaranteed to be, and especially a hazard for the click-everything-at-least-twice-but-not-quite-fast-enough-for-double-click senior cohort (RIP, grandpa!).

[jakearchibald]

Ohh, I think that's different enough to be its own issue https://github.com/WICG/navigation-api/issues/232

[domenic]

This isn't ideal, as the user may have scrolled during the fetch, but that change is discarded.

This feels like a general symptom of #232. Basically, the current API assumes that everything transitions to the new history entry the moment you call transitionWhile(). In practice this can be an issue for some apps. Apps that are non-interactive after transitionWhile() are well-positioned, but apps that allow interaction should probably delay the URL update/state capture/history entry change until they stop allowing interaction. (Which might be, until they have enough data for the next page.) And that needs a new API, per #232.

If a fulfilled promise is passed to transitionWhile (due to the page data being available synchronously), it goes like this:

I don't think this is correct. The following code should be used for sync renders:

navigation.addEventListener("navigate", event => {
  event.transitionWhile(Promise.resolve());
  updateDOM(data);
});

This changes the URL, and captures the state, before updating the DOM.

Whereas, if there's an immediately-fulfilled promise involved, then the usual flow of

navigation.addEventListener("navigate", event => {
  event.transitionWhile(async () => {
    await doRouting();
    await updateDOM();
  });
});

where doRouting() returns an immediately-fulfilled promise, becomes

1. Begin routing
2. transitionWhile called - capture & store scroll position
3. Alter DOM (possibly async)
4. Promise passed to transitionWhile resolves
5. Restore scrolling if applicable

An alternative design:

If possible I'd like to avoid decoupling state capture and changing the history entry, since those are pretty coupled in current implementations and spec architecture.

[jakearchibald]

Some stuff from internal discussion:

Having to change how you call this API whether it's a sync navigation or not seems like a gotcha, and restricts how you can write your routing logic. Having to add a seemingly redundant await 0 or whatever seems hacky too (and doesn't appear to work in the current implementation https://static-misc-3.glitch.me/nav-api/scroll-restore-sync/).

If transitionWhile takes a callback, that at least guarantees scroll position can be captured before the DOM change.

apps that allow interaction should probably delay the URL update/state capture/history entry change until they stop allowing interaction

Allowing interactivity should be the norm, as it is with MPA. Otherwise we're getting into sync XHR territory 😄

[bathos]

I’d never considered the option of disabling interactivity during a transition at all admittedly. Is that a common practice currently? My first thought is that it seems like it could add a kinda “user-hostile” risk to situations where things don’t go as planned — like, if there’s a bug or network issue leading to a very long transition, I’d expect to be able to continue having the option to click another link personally and if this interaction produced no response, it could give the impression that the app itself had “frozen”. Is this what you mean by sync XHR territory @jakearchibald?

[jakearchibald]

Exactly. Sync XHR blocks JS, therefore blocks all interactivity until the fetch completes.

[jakearchibald]

If we move to a model where a callable is passed to transitionWhile, it might be better to move the callback to the option object.

event.transitionWhile({
  focusReset: 'manual',
  async handler() {
    // …
  },
});

This means that options that impact the behaviour of the callback can appear before the callback in the source.

(it's a pet hate of mine that things like setTimeout and addEventListener end up having important details lots-of-scrolling away)

[bathos]

move to a model where a callable is passed

@jakearchibald iiuc that’s already the (intended) model (from an ES POV) because of the intention to define new Promise<T> conversion steps in Web IDL that would call callable input, something like Promise.resolve(IsCallable(n) ? n() : n):

• Maybe allow respondWith to take a promise-returning function?
• Allow functions to be used as promise inputs

[domenic]

So what's going on here is a bit subtle which makes the fix a bit subtle too.

Basically the scroll position is captured when the navigate event is totally finished firing. This is not the same as:

• When the first line of code in an IIAFE runs, using the event.transitionWhile((async () => { ... })()) pattern.
• When transitionWhile() itself is called.

This is most obviously seen in the case of multiple navigate events. But also, in the case of user-initiated navigations, microtasks will run first, before control returns to the part of the spec that performs the URL and history update steps and thus captures the current scroll position.

So indeed, to work around the current situation you need to find a way to delay your DOM-update code until after navigate and any associated microtasks are all done firing. await Promise.resolve() will not suffice (for user-initiated navigations); you need await promiseDelay(0) or similar. Ick.

This also means that the initial ideas floated here, of having transitionWhile() take a handler function which it runs, doesn't necessarily work.

• If we run that handler immediately, like is proposed in https://github.com/whatwg/webidl/issues/1020, we just recreate the same problem. It's too easy to put sync code into the handler that mutates the DOM, which will run before state is captured.
• If we run the handler "as part of transitionWhile() after it does any interesting work", which is I think what @jakearchibald was gesturing at, this doesn't actually do anything. Because the interesting work done by transitionWhile() doesn't include capturing scroll position etc. It just stores some stuff for later processing after the navigate event is completely done.

But, if we have it take a handler which only runs after navigate has finished firing and the URL and history update steps have taken place, this works. It also seems nicer in a number of ways:

• If some other navigate listener calls e.preventDefault(), your handler will never run. You don't even need to worry about AbortSignals.
• This totally avoids the sync/async split in the IIAFE pattern, where the sync part runs before commit and the async part runs after. Which has caused other problems, e.g. https://github.com/WICG/navigation-api/issues/124#issuecomment-869804711.

I'm unsure on the exact migration path here. It still feels like "transition while this promise is ongoing" is reasonable, but I admit I haven't seen it too much in the demos we've built, and maybe it's just a footgun by encouraging you to run promise-producing code before commit.

The options I can think of are:

1. Introduce an overload between promises and functions, where the function version behaves rather differently.
   • This is not forward-compatible with https://github.com/whatwg/webidl/issues/1020, if we do that.
   • This would require custom IDL bindings in spec and implementation since promises can't be overloaded usually.
2. Introduce an overload between (promise, dictionary) and (dictionary-with-handler)
   • This would similarly require custom IDL bindings
3. Same as (1), but also phase out the promise overload over time.
4. Same as (2), but also phase out the (promise, dictionary) overload over time.
5. Introduce a new method that takes a dictionary
   • This also might allow us to avoid the common-in-transitional-code pattern of event.transitionWhile(Promise.resolve()). You'd just call event.newMethod() and it'd convert the navigation.
   • The hard part here is figuring out the new name. We previously bikeshedded in https://github.com/WICG/navigation-api/issues/94#issuecomment-854929003 for some time.
6. Same as (5), but also phase out transitionWhile()

(I don't think we'd particularly want to introduce a new method that takes a (function, dictionary) since as @jakearchibald mentions it's a bit annoying.)

Thoughts welcome on which path would be best.

[jakearchibald]

4 seems fine. I like the idea of 6 because I'm still worried that 'transition' is a naming clash with animated transitions. The only naming suggestion I have is intercept.

[domenic]

Should we allow

event.intercept(async () => {
  // ...
});

as an overload? Or just

event.intercept({
  async handler() {
    // ...
  }
});

? The overload makes a lot of short code snippets in the README and tests look nicer, but I'm not sure that it would matter in the real world...

[domenic]

I guess

event.intercept({ async handler() {
  // ...
} });

isn't so bad.

[domenic]

Accidentally merged what was supposed to be a PR to main; reopening.

New question: should we rename navigation.transition to navigation.ongoing or navigation.ongoingIntercept or something? That would solve https://github.com/WICG/shared-element-transitions/issues/143 entirely.

[domenic]

If we did that we'd probably also have to rename the two "after-transition" enum values. I don't really like that. I still feel like we have a good claim to the name "transition" here, even though we're not a CSS transition.

[jakearchibald]

I still don't like transition as a name, but I understand if you don't want to change it.

I could make a Twitter poll and see how devs feel about either naming?

[jakearchibald]

The overload seems ok.

[VicGUTT]

Playing around with the Navigation API and I found myself writing intercept in a few places, prior to seeing this discussion. I must admit I also share the same concerns as Jake regarding clashes/possible confusions with the shared element transition API. But I also don't think "transition" reflects as well as "intercept" what the method does.

Sample code

```ts export default class Router { constructor() { this.onNavigationAttempt = this.onNavigationAttempt.bind(this); window.navigation.addEventListener('navigate', this.onNavigationAttempt); } public shouldInterceptNavigation(e: NavigateEvent): boolean { return e.canTransition && !e.hashChange; } protected onNavigationAttempt(e: NavigateEvent): void { if (!this.shouldInterceptNavigation(e)) { return; } this.interceptNavigation(e); } protected interceptNavigation(e: NavigateEvent): void { e.transitionWhile( this.asyncStuff(e).then(() => { this.onNavigationIntercepted(e); }) ); } protected onNavigationIntercepted(e: NavigateEvent): void { // } } ```