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

fix: transform everything that is not a selector inside :global #14577

Merged
merged 4 commits into from
Dec 10, 2024

Conversation

paoloricciuti
Copy link
Member

Closes #14568

I'm not entirely sure if there's a better way: i think keyframes are the only thing transformed that are not selectors but this should make sure that we transform everything, we just don't touch the selectors.

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.
  • If this PR changes code within packages/svelte/src, add a changeset (npx changeset).

Tests and linting

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

Copy link

changeset-bot bot commented Dec 5, 2024

🦋 Changeset detected

Latest commit: ed4f62d

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

preview: https://svelte-dev-git-preview-svelte-14577-svelte.vercel.app/

this is an automated message

Copy link
Contributor

github-actions bot commented Dec 5, 2024

Playground

pnpm add https://pkg.pr.new/svelte@14577

@dummdidumm
Copy link
Member

If it's only the keyframes, what about creating a zimmerframe walker just for that, and call that in this case?

const keyframes = {
  WhatEverSelectorIsNeededForThat(node) {
    if (needsToScopeSelector) { doIt }
  }
}

// ...
if (node.metadata.is_global_block) {
  // ...
  walk(node.block, null, keyframes)
}

@paoloricciuti
Copy link
Member Author

If it's only the keyframes, what about creating a zimmerframe walker just for that, and call that in this case?

const keyframes = {
  WhatEverSelectorIsNeededForThat(node) {
    if (needsToScopeSelector) { doIt }
  }
}

// ...
if (node.metadata.is_global_block) {
  // ...
  walk(node.block, null, keyframes)
}

That's a good idea but i wonder if it's not better to do this like this so that it's not an "edge case" and even if there's something else that get's transformed it will be transformed anyway.

@paoloricciuti
Copy link
Member Author

I think I wrote my answer very bad lol. The point is: this will technically transform everything that is not a selector. If we specifically target key frames we will fix this bug but we might discover new edge cases or, if we add new transforms in the future, we have to add it to the special case. Considering this is not to hairy maybe is the better solution? I'm a bit thorned

Copy link

@7antra 7antra left a comment

Choose a reason for hiding this comment

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

Thanks

@dummdidumm dummdidumm merged commit ab1f7f4 into main Dec 10, 2024
11 checks passed
@dummdidumm dummdidumm deleted the fix-global-transform branch December 10, 2024 13:58
@github-actions github-actions bot mentioned this pull request Dec 10, 2024
@paoloricciuti
Copy link
Member Author

This should lay some ground to also fix #14616

@ai
Copy link
Contributor

ai commented Dec 24, 2024

Seems like this PR broke disabling animation isolation #14713

@paoloricciuti
Copy link
Member Author

Seems like this PR broke disabling animation isolation #14713

Yup, technically there are workaround to that but we need to fix it

@ai
Copy link
Contributor

ai commented Dec 24, 2024

@paoloricciuti what is a way to prevent it? Use :global() in selector for each rule?

@paoloricciuti
Copy link
Member Author

@paoloricciuti what is a way to prevent it? Use :global() in selector for each rule?

Yeah...but I think the important bit is just the one with the animation

@paoloricciuti
Copy link
Member Author

Anyway I'll try to fix this asap

@ai
Copy link
Contributor

ai commented Dec 24, 2024

@paoloricciuti I try to draft some fix if it could help #14822

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.

CSS animation property has differing behaviour when using :global vs :global()
5 participants