fix: always call tick() if defined in transitions

[rChaoz]

Closes #12730

Call tick() if it's defined, not if css is undefined.

Note for maintainers: the call to on_finished() used to be wrapped in if (!css), was it intended for it to never be called for CSS transitions? Regardless I left it as-is as I'm not sure + added a TODO, now it's only called when tick is defined instead.

Svelte 5 rewrite

Please note that the Svelte codebase is currently being rewritten for Svelte 5. Changes should target Svelte 5, which lives on the default branch (main).

If your PR concerns Svelte 4 (including updates to svelte.dev.docs), please ensure the base branch is svelte-4 and not main.

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
• Prefix your PR title with feat:, fix:, chore:, or docs:.
• This message body should clearly illustrate what problems it solves.
• Ideally, include a test that fails without this PR but passes with it.

Tests and linting

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

[changeset-bot]

⚠️ No Changeset found

Latest commit: 6bbc386

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

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

[Rich-Harris]

Thank you. To answer the TODO: we do want on_finish to be called when a JS transition completes, just as it's called when a CSS transition completes in the animation.then(...) callback. This callback causes introend and outroend events to be dispatched, for example.

What we don't want is for the event to be called twice, so I've added some code that removes the callback once it's been called.

Right now, that new code isn't actually doing anything though, because of a subtle bug made visible by the test I just added: because the on_finish function calls the abort method on the transition, we never actually get to the end — tick(1, 0) never happens. This means that any work that's supposed to happen once the intro completes will never get done. There's a big more juggling required to make this work, and it's too late here to get into right now so I'll just leave the failing test where it is.

Separately, as a result of this PR we're no longer handling the case where neither css nor tick is provided, meaning the callback would never get called and nodes would never get removed following an outro. We should probably error in that case since it's a broken transition.

[rChaoz]

Separately, as a result of this PR we're no longer handling the case where neither css nor tick is provided, meaning the callback would never get called and nodes would never get removed following an outro. We should probably error in that case since it's a broken transition.

But I think this case should be handled, unless you want breaking changes from Svelte 4, where that is allowed, and used by Skeleton to dynamically disable transitions:

export function dynamicTransition<T extends Transition>(node: Element, dynParams: DynamicTransitionParams<T>): TransitionConfig {
	const { transition, params, enabled } = dynParams;

	if (enabled) return transition(node, params);

	// it's better to just set the `duration` to 0 to prevent flickering
	if ('duration' in params) return transition(node, { duration: 0 });

	// if the transition doesn't provide a `duration` prop, then we'll just return this as a last resort
	return { duration: 0 };
}

[Rich-Harris]

Fair enough. My point is that it was handled prior to this PR — if you modify that example to include a duration, then nothing transitiony happens when you toggle the button, but the element removal is nonetheless delayed.

That's true on main as well, but not in this PR — the transition never completes

[Rich-Harris]

Perhaps the fix here is to remove the completion logic from the if (css) branch, and instead of turning the else into an if (tick) we just always run that logic

[Rich-Harris]

...though that's arguably suboptimal since it means we're adding work on every requestAnimationFrame even when it's unnecessary in many cases. Hmm

[Rich-Harris]

I figured it out: we need to use WAAPI to control tick-based animations as well as css-based ones. Started another PR: #13018