Skip to content

fix: fix transition breaking the css layout #11166

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

Closed
wants to merge 2 commits into from

Conversation

Xerios
Copy link

@Xerios Xerios commented Apr 14, 2024

Related to #11165 (includes example)

Remove unnecessary fill: "forwards" which prevents any future modification of animated properties. This is discouraged as stated in the official spec. This animation property breaks the layout in a very confusing manner since there's no way to see inside devtools why styles no longer have any effect on the element.

I couldn't find the commit that explains why forwards was used in this case, but fill: 'none' seems to work perfectly fine for transitions and makes it behave as expected.

Alternative approach didn't seem to work, simply using .cancel() at finish doesn't fix the behavior and .commitStyles() applies the styles to the element which is not a desired result.

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

Copy link

changeset-bot bot commented Apr 14, 2024

🦋 Changeset detected

Latest commit: c8c7e7e

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

This PR includes changesets to release 1 package
Name Type
svelte Patch

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
Copy link
Member

Thank you — unfortunately this doesn't work, because of delays. See this demo — when visible becomes true, the <p> will flash for 500ms before the animation starts.

The correct fix is to remove the animation once it's finished, which #11216 does.

@Xerios
Copy link
Author

Xerios commented Apr 19, 2024

I very much felt like my solution wasn't a proper one since I didn't fully explore how this function was used.
This simple PR was mostly made in a rush to because it is hard to pinpoint the bug and could've resulted in people posting various new Issues under suspicion that the bug was something else.

I'm still unsure why adding cancel on finish didn't properly "release" the animated properties in my lazy attempt at fixing this, especially since in my own implementations (outside of svelte) it always worked as expected.

In any case, your solution works properly in my complex transition-heavy nested tree that I've been migrating to svelte5. So I'm happy with the results 🥳

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.

2 participants