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 onNavigate lifecycle function, to enable view transitions #9605

Merged
merged 27 commits into from
Aug 29, 2023

Conversation

Rich-Harris
Copy link
Member

@Rich-Harris Rich-Harris commented Apr 5, 2023

Fixes #5689. This adds an onNavigate lifecycle function.

Presently, there are two navigation lifecycle functions — beforeNavigate and afterNavigate. beforeNavigate fires before any navigation, and gives you the ability to (for example) cancel it, while afterNavigate fires once the page has been updated following a navigation (or initial hydration).

Between them, most use cases are covered, but there is one major missing piece: between beforeNavigate and afterNavigate callbacks running, we need to load the code and data for the new route, and that can take a noticeable amount of time. Because of that, we can't use these functions to do some work immediately before a navigation takes effect.

Aside: @oodavid has collected some cases where beforeNavigate is insufficient, and this will require some design work in the near future. This PR shouldn't conflict with that work.

With View Transitions, this is a problem. When you call document.startViewTransition(callback), it takes a screenshot of the current page, and the page is frozen until the promise returned from callback resolves. This user experience is untenable.

onNavigate addresses this problem. Callbacks are called immediately before a navigation is applied; if they return functions, those functions are called once the DOM has updated.

Because the view transition callback isn't called synchronously, we in fact need to delay DOM updates until the screenshot has been taken. For that reason, onNavigate callbacks can return a promise that will delay the navigation from taking effect. This is a footgun — you could return a promise that resolves slowly or even fails to resolve, breaking your app — but in practice I don't expect this to be a problem as long as it's well-documented (and we could add dev-time checks to ensure that promises resolve quickly).

Here's an example — note the sliding nav indicator:

Screen.Recording.2023-04-05.at.9.24.51.AM.mov

This was done by adding the following code to the root layout...

onNavigate(() => {
  if (!document.startViewTransition) return;

  return new Promise((fulfil) => {
    document.startViewTransition(() => new Promise(fulfil));
  });
});

...and the following CSS:

:root {
  /* disable document-level crossfade */
  view-transition-name: none;
}

li[aria-current='page']::before {
  view-transition-name: indicator; /* this name could be anything */
}

If we wanted to control the animation, we could do so like this:

:root::view-transition-group(indicator) {
  animation-duration: 1s;
}

(Note that we need to add the :root selector before the pseudo-element, otherwise Svelte will scope it to the component. We may want to omit ::view-transition-{old,new,group} from the CSS scoping logic in Svelte 4, so that idiomatic uses of the view transitions API are covered.)

I'll admit that this code...

return new Promise((fulfil) => {
  document.startViewTransition(() => new Promise(fulfil));
});

...is absolutely brain-breaking. Perhaps there's a nicer way to do this that I'm not seeing, but if not then we may eventually want to add some higher level utilities around this stuff.

cc @geoffrich who has been investigating the view transitions API more thoroughly than I have, and may have feedback/thoughts.

Please don't delete this checklist! Before submitting the PR, please make sure you do the following:

  • It's really useful if your PR references an issue where it is discussed ahead of time. In many cases, features are absent for a reason. For large changes, please create an RFC: https://github.com/sveltejs/rfcs
  • This message body should clearly illustrate what problems it solves.
  • Ideally, include a test that fails without this PR but passes with it.

Tests

  • Run the tests with pnpm test and lint the project with pnpm lint and pnpm check

Changesets

  • If your PR makes a change that should be noted in one or more packages' changelogs, generate a changeset by running pnpm changeset and following the prompts. Changesets that add features should be minor and those that fix bugs should be patch. Please prefix changeset messages with feat:, fix:, or chore:.

@changeset-bot
Copy link

changeset-bot bot commented Apr 5, 2023

🦋 Changeset detected

Latest commit: fa6bc93

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@sveltejs/kit Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

Rich-Harris and others added 2 commits April 5, 2023 10:10
Co-authored-by: Simon H <5968653+dummdidumm@users.noreply.github.com>
Co-authored-by: Simon H <5968653+dummdidumm@users.noreply.github.com>
@geoffrich
Copy link
Member

Super excited for this - at first glance, this should fix a lot of the issues I had to work around in my SvelteKit + view transition demos. In particular, being able to return a promise will fix a lot of the race conditions I encountered that broke the transition.

Will try to make time tonight to experiment with this branch and my demos and see if anything is missing / doesn't work as expected.

@KoRnFactory
Copy link

This is great!
I was wondering if this new function could be used in a transition: directive like so:

<span transition:native={{duration: 300}}> Moving text </span>
function native(node, params){
  onNavigate(() => {
  // call startViewTransition
  });

  const uniqueId = "svelte-1234";
  node.style.setProperty("view-transition-name", uniqueId);
  
  // somehow generate the styles for animation-duration
  `:root::view-transition-group(`${uniqueId}`) {
  animation-duration: ${params.duration || 200}ms;
  }`
  
  return {} // nothing should be done by classic svelte transitions, we only set the View Transition API
}

Of course this would only make sense for SvelteKit, so it may live in a submodule like @sveltejs/kit/transition, but still leverages the same know-how for the user.

I don't really know how to generate styles in SSR from this function, would that be doable?

current = navigation_result.state;

// reset url before updating page store
if (navigation_result.props.page) {
navigation_result.props.page.url = url;
}

const after_navigate = (
await Promise.all(callbacks.on_navigate.map((fn) => fn(navigation)))
Copy link
Member

Choose a reason for hiding this comment

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

should we consider what happens here if any of these callbacks reject? Or at least document the behavior? I'm not sure if it's the right behavior, but right now a rejected promise will prevent the navigation from completing.

packages/kit/types/ambient.d.ts Outdated Show resolved Hide resolved
@bennymi bennymi mentioned this pull request Apr 6, 2023
21 tasks
@geoffrich
Copy link
Member

Started trying this out with some of my demos and it's definitely a big improvement - I'm not seeing the race conditions I used to see, so preloading on hover works consistently. Though I admit, the suggested code snippet took a long time to wrap my brain around.

return new Promise((fulfil) => {
  document.startViewTransition(() => new Promise(fulfil));
});

I was able to better understand it by splitting it out like this:

onNavigate(async (navigation) => {
		if (!document.startViewTransition) {
			return;
		}

		// by awaiting this promise, we prevent SvelteKit from completing the navigation until the promise resolves
		// this promise is resolved with a function that will resolve the page transition promise
		const resolvePageTransition = await new Promise((fulfil) => {
			document.startViewTransition(async () => {
				await new Promise((res) => fulfil(res));
			});
		});

		// we return that function, which SvelteKit will automatically resolve in afterNavigate
		return () => {
			resolvePageTransition();
		};
	});

Not sure if this is gesturing at the API being too confusing or is just something we have to live with.

@markjaquith
Copy link
Contributor

Though I admit, the suggested code snippet took a long time to wrap my brain around.

It's really rough. If this is the canonical implementation of it, and this is the major use case of this feature, I'd consider abstracting it.

onNavigate(waitForTransition)

@jakearchibald
Copy link

jakearchibald commented Apr 11, 2023

Fwiw, I find the callback-returns-a-callback pattern a little opaque. It isn't always clear what the callback is for.

Instead, could the navigation have a promise that resolves when the navigation completes? Even better, that means it can reject if the navigation fails, and that information is useful to things like view transitions.

onNavigate(async (navigation) => {
  return new Promise((oldStateCaptureResolve) => {
    document.startViewTransition(() => {
      oldStateCaptureResolve();
      await navigation.complete;
    });
  });
});

I'm not against adding something like oldStateCaptured to the transition object, so the above could become:

onNavigate(async (navigation) => {
  const transition = document.startViewTransition(() => navigation.complete);
  await transition.oldStateCaptured;
});

Although you could do that with a helper today:

function startViewTransition(callback) {
  return new Promise((resolve) => {
    const transition = document.startViewTransition(() => {
      resolve(transition);
      return callback();
    });
  });
}

onNavigate(async (navigation) => {
  await startViewTransition(() => navigation.complete);
});

@Rich-Harris Rich-Harris mentioned this pull request Apr 18, 2023
5 tasks
@dummdidumm
Copy link
Member

Because Github hides it, I'll add this comment again:

Feels a bit loose to just return strings in the error. Should we declare a set of consts that are reused? And should it just be "navigation was aborted" or a more techincal/stable-looking thing like NAV_ABORTED?

const NAV_ABORTED = 'NAV_ABORTED';
const NAV_CANCELLED = 'NAV_CANCELLED';

@Rich-Harris
Copy link
Member Author

What would the difference be in practical terms?

I think there is scope to make error messages more structured by using error codes that correspond to pages in the docs, but that's a larger issue — it applies to 'Redirect loop' and 'Can only disable scroll handling...' and 'Cannot use reserved query parameter...' and so on. It would feel weird to come up with something only for this feature. I think we should tackle that separately and not let it block this PR.

@Smirow
Copy link

Smirow commented Aug 31, 2023

Is there a way to catch this error (that was not present in <1.24.0)? As it may happen quite often in some use cases.

@dummdidumm
Copy link
Member

Which error? Please provide more details and possibly open an issue with a reproduction.

@Smirow
Copy link

Smirow commented Aug 31, 2023

The errors discussed above when aborting/cancelling navigation:
https://github.com/sveltejs/kit/pull/9605/files/eff8b4a84da082d579ce9c22867e8be3e33d6d43#r1297641206

They keep triggering when the user cancel the navigation, on multiple form submission (with method='GET') for instance.

@dummdidumm
Copy link
Member

Could you open a new issue with a reproduction and explanation why this error is introducing unwanted behavior?

@Smirow
Copy link

Smirow commented Aug 31, 2023

It isn't much about unwanted behavior than error keep getting logged on the client side whenever the client willingly abort a navigation. I'll open an issue with a repro later.

@@ -1017,7 +1008,10 @@ export function create_client(app, target) {
url = intent?.url || url;

// abort if user navigated during update
if (token !== nav_token) return false;
if (token !== nav_token) {
nav.reject(new Error('navigation was aborted'));

Choose a reason for hiding this comment

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

this is getting thrown when updating query params via goto()

Choose a reason for hiding this comment

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

I think you should open an issue with a reproduction.

Copy link

Choose a reason for hiding this comment

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

This should be fixed by 1.24.1 (tried to update ?) #10666

Choose a reason for hiding this comment

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

Just updated and no more error, thanks

@gterras
Copy link

gterras commented Sep 4, 2023

Can I ask if adding

onNavigate(() => {
  if (!document.startViewTransition) return;

  return new Promise((fulfil) => {
    document.startViewTransition(() => new Promise(fulfil));
  });
});

to the root layout is expected to lower global performance in any way ?

@gtm-nayan
Copy link
Contributor

@gterras The promise inside the startViewTransition in your example will never resolve, so the page will just freeze until the browser thinks it's getting too late and should end the transition

@gterras
Copy link

gterras commented Sep 4, 2023

@gterras The promise inside the startViewTransition in your example will never resolve, so the page will just freeze until the browser thinks it's getting too late and should end the transition

Isn't it the same code as in Rich's opening thread?

@gtm-nayan
Copy link
Contributor

Looks like it, although be aware that there was a change in the onNavigate API after the initial post. The original code could have been Promise.resolve() or maybe document.startViewTransition behaved differently at that point.

You can observe the freeze in this example:

<h1
	on:click={
		() => {
			new Promise(fulfil => document.startViewTransition(_ => new Promise(fulfil)))
		}
	}
>
	Hello {name}!
</h1>

@gterras
Copy link

gterras commented Sep 4, 2023

Looks like it, although be aware that there was a change in the onNavigate API after the initial post. The original code could have been Promise.resolve() or maybe document.startViewTransition behaved differently at that point.

Ok thanks that would explain why I got some slowdowns testing the API. I didn't check the FAQ updated code which is for reference

onNavigate((navigation) => {
    if (!document.startViewTransition) return;

    return new Promise((resolve) => {
        document.startViewTransition(async () => {
            resolve();
            await navigation.complete;
        });
    });
});

So back to my original question, this code isn't expected to slow down anything (provided you don't actually use the transition API in components) right?

@gtm-nayan
Copy link
Contributor

provided you don't actually use the transition API

wdym? Calling that code is using the transition API, so any navigation will have the transition (250ms by default IIRC) applied during which time your page will essentially be showing a non-interactive screenshot of itself.

@gterras
Copy link

gterras commented Sep 5, 2023

provided you don't actually use the transition API

wdym? Calling that code is using the transition API, so any navigation will have the transition (250ms by default IIRC) applied during which time your page will essentially be showing a non-interactive screenshot of itself.

No I mean while disabling any transition behavior (like the default fade) though CSS. Does the very presence of this code, provided you disable anything related to viewtransitions, is subject to even the slightest slowdown? Even a few ms?

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

Successfully merging this pull request may close these issues.

Support for Chrome's Document Transitions