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: add check for is attribute #15086

Merged
merged 6 commits into from
Jan 22, 2025
Merged

fix: add check for is attribute #15086

merged 6 commits into from
Jan 22, 2025

Conversation

edoardocavazza
Copy link
Contributor

@edoardocavazza edoardocavazza commented Jan 22, 2025

Fixes #15085

Add a check for is attribute in is_custom_element_node function.

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 Jan 22, 2025

🦋 Changeset detected

Latest commit: 57137e6

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

@dummdidumm
Copy link
Member

Thank you! Could you add a test to runtime-runes for this? You can look at runtime-runes/custom-element-slot-in-snippet to see how you can model the test

@edoardocavazza
Copy link
Contributor Author

Hello @dummdidumm, I added the test but I am unable to run pnpm lint 🤷‍♂️ .

Copy link
Contributor

Playground

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

@edoardocavazza
Copy link
Contributor Author

Sure, I'll take a look

@edoardocavazza
Copy link
Contributor Author

Hi @paoloricciuti, I managed to extend html validation to custom elements with is attribute. I had to refactor some stuff, please let me know if this can work for you.

@@ -47,6 +47,7 @@ export function SvelteElement(node, context) {
'$.push_element',
b.id('$$payload'),
tag,
b.literal(false),
Copy link
Member

Choose a reason for hiding this comment

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

Couldn't this also be a custom element?

<svelte:element tag={tag} is="my-element" />

i wonder if we need to add extra checks for the SvelteElement case (before it was using the name so it was probably fine)?

@dummdidumm
Copy link
Member

I think we need to revert the changes to validation. When you create a custom element that should be used in place of a regular element, then you do that to mimic the semantics of that element. So <a is="my-a">..</a> is still <a> - and from a quick test the browser seems to see it that way, too, because it still reorders invalid element trees even if they have is=".." on them.

@edoardocavazza
Copy link
Contributor Author

@dummdidumm You are probably right 🤔 I can easily revert the last commit, let me know how to proceed.

@dummdidumm
Copy link
Member

yeah just reverting that last commit sounds good 👍 (sorry for the back and forth)

@paoloricciuti
Copy link
Member

And sorry for interjecting and make everybody waste time 😅

@edoardocavazza
Copy link
Contributor Author

Done!

sorry for the back and forth

And sorry for interjecting and make everybody waste time 😅

No problem at all!

Copy link
Member

@dummdidumm dummdidumm left a comment

Choose a reason for hiding this comment

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

thank you!

@dummdidumm dummdidumm merged commit d17f5c7 into sveltejs:main Jan 22, 2025
10 checks passed
This was referenced Jan 22, 2025
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.

Different behavior for custom elements that extends builtin element.
3 participants