Skip to content

fix: add check for is attribute#15086

Merged
dummdidumm merged 6 commits intosveltejs:mainfrom
edoardocavazza:main
Jan 22, 2025
Merged

fix: add check for is attribute#15086
dummdidumm merged 6 commits intosveltejs:mainfrom
edoardocavazza:main

Conversation

@edoardocavazza
Copy link
Copy Markdown
Contributor

@edoardocavazza edoardocavazza commented Jan 22, 2025 •
edited
Loading

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

@changeset-bot
Copy link
Copy Markdown

changeset-bot Bot commented Jan 22, 2025 •
edited
Loading

🦋 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
Copy Markdown
Member

dummdidumm commented Jan 22, 2025

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
Copy Markdown
Contributor Author

edoardocavazza commented Jan 22, 2025

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

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Jan 22, 2025

Playground

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

@edoardocavazza
Copy link
Copy Markdown
Contributor Author

edoardocavazza commented Jan 22, 2025

Sure, I'll take a look

@edoardocavazza
Copy link
Copy Markdown
Contributor Author

edoardocavazza commented Jan 22, 2025

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.

paoloricciuti
paoloricciuti reviewed Jan 22, 2025
View reviewed changes
Comment thread packages/svelte/src/compiler/phases/3-transform/server/visitors/SvelteElement.js Outdated
'$.push_element',
b.id('$$payload'),
tag,
b.literal(false),
Copy link
Copy Markdown
Member

@paoloricciuti paoloricciuti Jan 22, 2025

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

dummdidumm commented Jan 22, 2025

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
Copy Markdown
Contributor Author

edoardocavazza commented Jan 22, 2025

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

@dummdidumm
Copy link
Copy Markdown
Member

dummdidumm commented Jan 22, 2025

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

@paoloricciuti
Copy link
Copy Markdown
Member

paoloricciuti commented Jan 22, 2025

And sorry for interjecting and make everybody waste time 😅

@edoardocavazza
Copy link
Copy Markdown
Contributor Author

edoardocavazza commented Jan 22, 2025

Done!

sorry for the back and forth

And sorry for interjecting and make everybody waste time 😅

No problem at all!

dummdidumm
dummdidumm approved these changes Jan 22, 2025
View reviewed changes
Copy link
Copy Markdown
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
This was referenced Jan 22, 2025
Merged
Closed
@dummdidumm dummdidumm mentioned this pull request Aug 18, 2025
Merged
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@paoloricciuti paoloricciuti paoloricciuti left review comments

@dummdidumm dummdidumm dummdidumm approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Different behavior for custom elements that extends builtin element.

3 participants

@edoardocavazza @dummdidumm @paoloricciuti