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

Svelte 5 : <tr> is invalid inside <table> #9785

Closed
mquandalle opened this issue Dec 5, 2023 · 18 comments
Closed

Svelte 5 : <tr> is invalid inside <table> #9785

mquandalle opened this issue Dec 5, 2023 · 18 comments
Milestone

Comments

@mquandalle
Copy link

Describe the bug

The following markup throws an exception

<table><tr></tr></table>

Reproduction

https://svelte-5-preview.vercel.app/#H4sIAAAAAAAAE6tWSsvMSS1WsoquVspLzE1VslJyLChQ0lEqqSwAcYrLUnNKUoH84vzSomSQiE1JYlJOqp1NSZGdjT6EAAsA1eTmp2SmZaamKFmVFJWm1sbWAgBDd7JdXgAAAA==

Logs

No response

System Info

Svelte 5

Severity

annoyance

@sidharthv96
Copy link

sidharthv96 commented Dec 8, 2023

The check is happening here.

// https://html.spec.whatwg.org/multipage/syntax.html#parsing-main-intr
case 'tr':
return (
tag === 'th' || tag === 'td' || tag === 'style' || tag === 'script' || tag === 'template'
);
// https://html.spec.whatwg.org/multipage/syntax.html#parsing-main-intbody
case 'tbody':
case 'thead':
case 'tfoot':
return tag === 'tr' || tag === 'style' || tag === 'script' || tag === 'template';
// https://html.spec.whatwg.org/multipage/syntax.html#parsing-main-incolgroup
case 'colgroup':
return tag === 'col' || tag === 'template';
// https://html.spec.whatwg.org/multipage/syntax.html#parsing-main-intable
case 'table':
return (
tag === 'caption' ||
tag === 'colgroup' ||
tag === 'tbody' ||
tag === 'tfoot' ||
tag === 'thead' ||
tag === 'style' ||
tag === 'script' ||
tag === 'template'
);

It says in the spec that tr is a valid tag, along with td and th. But a tbody should be inserted as a parent tag automatically.
image

So, is svelte trying to enforce the manual addition of tbody?

I can make the PR to allow tr, td and th if that's a valid case.

@brunnerh
Copy link
Member

brunnerh commented Dec 8, 2023

There is a problem here with hydration. If the SSR generates a table that looks different than the DOM that browser creates via the normalization (e.g. adding a missing tbody), the mismatch can cause the hydration to fail.

The issue was already known, would recommend waiting for maintainer feedback on this.

@mquandalle
Copy link
Author

mquandalle commented Dec 8, 2023

I don't think Svelte should add the missing tbody here. That would be very confusing to have the framework adding tags not present in the source. Since browsers are accepting a <tr> directly inside a <table> svelte should leave the markup unchanged as does Svelte 4 and other UI frameworks.

@7nik
Copy link

7nik commented Jan 9, 2024

This is the result of the limitation of the new way of component rendering - creating all the elements in one go and extracting references to the elements. But when a browser corrects the template structure, it breaks the reference extraction.

Maybe the compiler can just do all the structure correction under the hood. However, it can lead to a confusing experience when a CSS rule doesn't apply to an element or apply to another element because the DOM structure in the source and in the browser differs. So it makes sense to write a markup that will not change its structure.

@mquandalle
Copy link
Author

On MDN <tbody> are omitted in some of the examples https://developer.mozilla.org/en-US/docs/Web/HTML/Element/table#simple_table, so I think it is a reasonable way to write the markup for a table, and Svelte rejecting it as invalid is unexpected.

Although if this is an unavoidable technical limitation of the new rendering model that should be explicitly documented, especially in the migration guide.

@benmccann
Copy link
Member

From @trueadm on an earlier discussion we had about this:

The browser always inserts a <tbody> when you don't put one in. We need to have a <tbody> then to avoid breaking our template mechanic in Svelte 5. There's not a super simple solution here. We either have to try and detect this at runtime or we try and do it statically between modules somehow, but both are pretty difficult. Solid, etc. have the same error. They push for the developer to fix their code and they class this as invalid, as does React.

One possible way would be when we append templates, we can see what the nodes are that we append and check if the parent we're appending to is a <table> and appropriately add a tbody element, and vice versa for when we traverse the elements. This will incur a small overhead cost for checking though, so maybe we can somehow get the compiler to give us a good estimate by looking at the template to see if we're dealing with <tr> elements.

@Rich-Harris Rich-Harris added this to the 5.0 milestone Apr 4, 2024
@7nik
Copy link

7nik commented May 3, 2024

@R3D2 you forgot about <td>/<th>. You cannot add text right to <tr>.

@apokaliptis
Copy link

apokaliptis commented May 14, 2024

Maybe the compiler can just do all the structure correction under the hood. However, it can lead to a confusing experience when a CSS rule doesn't apply to an element or apply to another element because the DOM structure in the source and in the browser differs. So it makes sense to write a markup that will not change its structure.

If the compiler were to also include display: contents; on the automatically inserted <tbody>, then that shouldn't be an issue.

@7nik
Copy link

7nik commented May 14, 2024

It still will be issue for cases like this:

/* style only own rows */
table.mytable > tr:even {
  background: #aaa;
}

And you will be puzzled why it doesn't work on the following markup:

<table class="mytable">
  <tr>
    <td>row 1</td>
  </tr>
  <tr>
    <td>row 2</td>
  </tr>
</table>

@trueadm
Copy link
Contributor

trueadm commented May 14, 2024

Maybe the compiler can just do all the structure correction under the hood. However, it can lead to a confusing experience when a CSS rule doesn't apply to an element or apply to another element because the DOM structure in the source and in the browser differs. So it makes sense to write a markup that will not change its structure.

If the compiler were to also include display: contents; on the automatically inserted <tbody>, then that shouldn't be an issue.

That won't work, the browser will remove the <div> from the table and insert it outside regardless of the display property.

@apokaliptis
Copy link

apokaliptis commented May 14, 2024

It still will be issue for cases like this:

/* style only own rows */
table.mytable > tr:even {
  background: #aaa;
}

Oh, I see. I generally avoid > since I never know when I'm going to have to throw in a random <div> to resolve layout/position issue.

That won't work, the browser will remove the <div> from the table and insert it outside regardless of the display property.

I don't follow. What is the div you're referring to?

@trueadm
Copy link
Contributor

trueadm commented May 14, 2024

Sorry I misread your message. I thought you said div with display contents. Unfortunately, the automatically inserted tbody is done via the browser when we clone the HTML template without it.

@anders-hard
Copy link

I ran into this as well, but in my case I needed to put my th's inside a tr in the thead. Before, the tr wasn't needed there.

@7nik
Copy link

7nik commented May 24, 2024

I wonder why this issue is being kept open. The team can do nothing to prevent the error. So, the best they can do is display a more detailed error message, e.g. X is invalid inside Y. Y can contain only A, B, C, ... or include a link to the docs with a detailed explanation of the error.

@trueadm trueadm closed this as completed Jun 3, 2024
@anders-hard
Copy link

Right, it's a documentation issue so I'd assume it's being added to the breaking changes list :)

@bfontaine
Copy link

It would be nice to have this as a warning when using Svelte 4, so one can start fixing their code right now instead of getting errors about it during the migration.

@scosman
Copy link

scosman commented Oct 31, 2024

Another use case here: emails. We generate emails using svelte templates in the CMSaasStarter boilerplate. You don't get the luxury of modern html/css in email, and need to produce some pretty old/ugly html. I'd like to keep control, without svelte assuming anything about what the browser will do (cause the browser could be Yahoo Mail's render engine...).

scosman added a commit to CriticalMoments/CMSaasStarter that referenced this issue Nov 1, 2024
This isn't ideal as we're diverting from the tested template... but I'll test it.

Should ideally remove if this is fixed: sveltejs/svelte#9785
tanel-n added a commit to tanel-n/IAS0320_proto that referenced this issue Nov 30, 2024
* Deeplink to docs

* Improve blog page, including responsive mobile support

* Update README.md

* Create LICENSE

* Grammar

* design tweaks

* Update README.md

* Update README.md

* Design tweaks

* improve design of admin portal

* Design improvements

* Update README.md

* Update README.md

* Add link from admin console back to marketing

* Compress image

* compress image

* Update deps

* Typo and color

* Update homepage copy

* Home page tweak

* Rename Admin -> User

* Update README.md

* Github Actions build CI (#7)

* New Github Actions build check
* And env vars for build

* Add build status badge, and license badge

* Fix bug in blog caused by upgrading SvelteKit.

* More robust fix for prior bug. Don't want to match prefix or /post matches /post2

* Do I need a perfect pagespeed insights score? No.

Do I want one?

Yes!

* Update README.md

* Update README.md

* npm run format

* Add format check command and CI (#10)

* Add format check command and CI
* Add formatting/prettier status badge

* Fix typescript linting

* Fix all linting errors

* Add linting to CI

* Update README.md with Lint CI and git hook instructions

* Update README devX section for CI and hooks

* Remove non-direct dependency

* Workaround for supabase-community/auth-ui#230

* Update README.md

* Update README.md

* Update README.md

* Update README.md

* Line height tweak

* Spacing tweaks

* fix linting, CI, and ESLint+VSCode setup

* Fix all eslint errors

* update README to fix typos + extension info and finish conversion of repo into TS format

* Fix CriticalMoments/CMSaasStarter#13

We were telling users they had oauth just because they weren't logged in with a password. That's not necessarily true, they can log in via a link (verify email link, magic link).

Make the oauth part of the message conditional on knowing for certain they logged in with oauth.

Also clean up copy and style.

* fix all type errors

* fix bug and naming of form response, plus minor fixups for typechecking PR

* Improved pricing boilerplate with FAQ template and grid template.

Icons were public domain icons.

* Add "check" to README to match CI

* tweak layout and demo content.

* Create SECURITY.md

* Bump undici and @sveltejs/kit

Bumps [undici](https://github.com/nodejs/undici) to 5.28.3 and updates ancestor dependency [@sveltejs/kit](https://github.com/sveltejs/kit/tree/HEAD/packages/kit). These dependencies need to be updated together.


Updates `undici` from 5.26.5 to 5.28.3
- [Release notes](https://github.com/nodejs/undici/releases)
- [Commits](nodejs/undici@v5.26.5...v5.28.3)

Updates `@sveltejs/kit` from 1.30.3 to 1.30.4
- [Release notes](https://github.com/sveltejs/kit/releases)
- [Changelog](https://github.com/sveltejs/kit/blob/@sveltejs/[email protected]/packages/kit/CHANGELOG.md)
- [Commits](https://github.com/sveltejs/kit/commits/@sveltejs/[email protected]/packages/kit)

---
updated-dependencies:
- dependency-name: undici
  dependency-type: indirect
- dependency-name: "@sveltejs/kit"
  dependency-type: direct:development
...

Signed-off-by: dependabot[bot] <[email protected]>

* Account title not changing bug fix

* Contact Us functionality saving to a datanbase

* Add contact us to marketing content

* Cleanup blog engine:

 - Posts into typescript
 - Post type into non-global
 - Remove repeated date parsing code
 - Remove repeated sorting code
 - Proper 404 errors on bad urls

* Fix error page background color.

* Don't pre-render the rss. Need the domain origin which isn't available at pre-render time.

* remove log, and fix layout on big monitors

* Add testing CI.

Yes, I should have more tests built in. But this way is at least setup for testing when I do.

Add dev-ex description to README, and improve setup instructions.

* Update readme

* Update readme

* Fix name of CI test file

* Update homepage: icons, copy, docs links

* Fix for CriticalMoments/CMSaasStarter#25

Smoother setup for initial run of local dev env.

* Add a blog post entry linking to the big blog post explaining how/why we built this template. Includes useful best practices for how to customize this template.

* Weird cloudflare issue isn't serving the compiled JS for this page. I see it rendered, but get expired headers every time.

* Wild bug.

 - Compiled CSR js uses a hash of the page as name
 - DNS based ad blockers (NextDNS) detect that as ad uri, block it

Changing content, should change hash. Testing on branch before merge.

* Bump jose from 4.15.4 to 4.15.5

Bumps [jose](https://github.com/panva/jose) from 4.15.4 to 4.15.5.
- [Release notes](https://github.com/panva/jose/releases)
- [Changelog](https://github.com/panva/jose/blob/v4.15.5/CHANGELOG.md)
- [Commits](panva/jose@v4.15.4...v4.15.5)

---
updated-dependencies:
- dependency-name: jose
  dependency-type: indirect
...

Signed-off-by: dependabot[bot] <[email protected]>

* Cleaner sample blog content. The one fake post wasn't clear it was fake and was confusing.

* Update README.md

* Change tailwind bottom margin to padding for layouts

* Add more meta tags into head on blog page for better facebook and twitter seo

* Update daisyUI and Tailwind

* Updates for daisyUI 4

saadeghi/daisyui#2507

* Run formatting on main

* Add CI for PRs

* Fix theme colors

Matches previous theme colors - see CriticalMoments/CMSaasStarter#38 (comment)

* add cookie warning on pages that create cookies

* Move config into code. People were getting tripped up on ENV var, and we content in code all over. No need for a second content in env concept.

Resolves CriticalMoments/CMSaasStarter#44

* Limit profile field inputs

* Bump github Action version to fix Actions warning

* Add extensions to readme and homepage

* npm audit fix

* migrate to new supabase ssr auth package

* add forgotten lock file update

* add stripe_customers definitions

* Fix a security issue: supabase/supabase#22381

Details:
 - CMSaaSStarted followed the offical SvelteKit Supabase docs here: https://supabase.com/docs/guides/getting-started/tutorials/with-sveltekit
 - Unforunately the server side helper getSession was not validtion the JWT token, which was caught and fixed in the docs here: supabase/supabase#22381
 - There were a few improvements/cleanups since the fix, which we integrated as well, see change history here: https://github.com/supabase/supabase/commits/master/apps/docs/content/guides/getting-started/tutorials/with-sveltekit.mdx

We're now back up to date with the supabase suggested format here: https://supabase.com/docs/guides/getting-started/tutorials/with-sveltekit

* don't put id in strype_customers table type definitions

* fix amr not existing on user object

* Move the pricing FAQ/table into only the pricing page. Link to it from other uses of pricing module.

Cleaner this way.

* Update SEO.

 - Add LD+JSON for the main website, and blog posts
 - Improve descriptions for main page and pricing page

* chore: format all files

* refactor: `$ npm run format`

* Add missing customer table to DB defs. New eslint catches this

* Update deps (and one formatting fix mixed in)

* Fix blog layout.

It previously didn't update title and headers when navigating between blog posts. Made the content reactive to url so now it does.

* Sveltekit 2 migration! (#76)

* Sveltekit 2 migration!

Mostly just running `npx svelte-migrate@latest sveltekit-2` and fixing package conflicts.

Use @supabase/auth-helpers-sveltekit to ^0.11 to avoid supabase/auth-js#888

* Filename correction (README.md) (#79)

pricing.js →  pricing_plans.ts

* Update README.md (#82)

* Update README.md (#86)

Update README.md with header and template docs

* Update README.md

* Update README.md (#91)

* Update README.md

Remove link to image in header

* Update README.md

* Update README.md (#92)

* Update README.md

* Update README.md

* Bump ws from 8.17.0 to 8.17.1 (#93)

Bumps [ws](https://github.com/websockets/ws) from 8.17.0 to 8.17.1.
- [Release notes](https://github.com/websockets/ws/releases)
- [Commits](websockets/ws@8.17.0...8.17.1)

---
updated-dependencies:
- dependency-name: ws
  dependency-type: indirect
...

Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

* Add a vercel deployment button, and cleanup docs a bit (#96)

Add a vercel deployment button, and cleanup docs a bit

* Update Vercel Env Vars (#97)

Remove site name and owner, as these were moved into typescript, like other content.

* Add misspell utility to check for spelling errors

* Fix typos found by new spelling check tool

* Update README.md with vercel hosted demo page

* Improve readme: fixed links, local misspell

* Add admin mailer. (#103)

* Add admin mailer.

Fixes CriticalMoments/CMSaasStarter#83

This was a nightmare
 - Nodemailer worked great, but Cloudflare doesn't allow full node.js and optional imports are a pain
 - Cloudflare a big fat dirty liars. Give an API that can only be tested on server, then once you finally get it working after 20 deploys, it's canceled. https://blog.cloudflare.com/sending-email-from-workers-with-mailchannels/ 

Resend is great: up and running in 5 mins.

* Email Support (#104)

* Add a tool to email folks, mostly users.

Includes
 - templating system using svelte components
 - support for plaintext and html
 - welcome email as demo and template

* Add docs for new email features, and cleanup the welcome email a bit

* Add email to marketing page

bugs:
* Fix new profile logic
* Consider errors in newProfile assessment

* Check email is verified before sending welcome email (#105)

* Check email is verified before sending welcome
* Fix email verification check: works with oauth and email auth

* Clean up the mailer with new email user function that handles verified emails (#106)

* Site Wide Search Feature + Mail cleanup

Clean up the mailer with new email user function that handles verified emails

New site wide search features. Does search in JS for super fast performance. Compiles the search index once on deploy to keep runtime overhead low.

`nom run build` will generate the files needed for search indexing

* Fix async issue that only occures on serverless environments

* Tweaks to search, and an open source card on markting page

* Keyboard support for site search

* Swap fuse for lunr (#112)

* Swap fuse for lunr. Lunr has real inverted search index, and better word matching (price -> pricing).

* Revert "Swap fuse for lunr (#112)"

This reverts commit d9cef83.

Honestly, liked fuse search results more.

* Move search index building into vite plugin, where we can assert it runs AFTER the pages are cached.

* Add json extension for a valid content-type, which also tells Cloudflare to gzip the content: https://developers.cloudflare.com/speed/optimization/content/brotli/content-compression/

Also: give more relevance to title and description over body

* Add ability to unsubscribe from emails (issue 107)

* Fix linting errors

* Fix lint errors

* Use WebsiteName from config in nav bar

* Match file name

login_config.ts

* Fixes CriticalMoments/CMSaasStarter#117

* Cleanup settings design a little. The green buttons were ugly.

Also a few string tweaks.

* Design tweaks

* Made button optional in the settings page

* dynamic settings title

* optionally skip create profile step

* feat: add sitemap.xml

* format

* Adding ./checks.sh for contributors, and easier dev-tool setup

* prerender sitemap

* Update README.md

* Merge master into ssr branch

* Get up to date with the SB SK guide: https://supabase.com/docs/guides/auth/server-side/sveltekit

Supress warnings that are not needed: supabase/auth-js#888 (comment)

* Bit more auth checking. Next step it to get it fully up to https://supabase.com/docs/guides/auth/server-side/sveltekit

* More idiomatic supabase auth following https://supabase.com/docs/guides/auth/server-side/sveltekit

Differences:
 - only run the auth layout under /account and /login, want fast unauthed marketing pages with no JS
 - Be even more explicit in maing getSession safe by not calling it on the server at all

* Don't recall safeGetSession, already in the hook

* Bump deps for secuirty issues

* Inline CSS for faster first page loads.

* update deps

 - Security patch in svelte: https://github.com/CriticalMoments/CMSaasStarter/security/dependabot/13
 - Glob now explicit dev dependency, was implicit before. Fix import format.
 - Update everything while we're at it

* Move glob back to v10. v11 requires node 22, and doesn't support all current maintained/stable node versions

* Fix a redirect loop. We were inconsistent for how we redirected login to account and vice versa. A expired session could enter redirect loop.

Also dont redirect from authHandler. I don't want to leave any page to account when logged in, just /login/*

* Fix sign out page

 - Ugly layout
 - If we tried to render server side it errorer (no goto)
 - It was signing out on load, not on mount

* Fixes CriticalMoments/CMSaasStarter#145

More error logging on unexpected errors.

* Adds a small padding at the bottom to prevent lack of space for descenders

Adds a small padding at the bottom to prevent lack of space for descenders, leading to descenders like "g" and "p" being cut off at the bottom. (Caused by bg-clip-text and text-transparent.)

* Adds small paddings at the bottom to prevent lack of space for descenders

Adds a small padding at the bottom to prevent lack of space for descenders, leading to descenders like "g" and "p" being cut off at the bottom. (Caused by bg-clip-text and text-transparent.)

* Balance the extra space created

Adjust the bottom margin or the top margin of the next element to balance the extra space created by pb

* Bump rollup from 4.21.2 to 4.22.5

Bumps [rollup](https://github.com/rollup/rollup) from 4.21.2 to 4.22.5.
- [Release notes](https://github.com/rollup/rollup/releases)
- [Changelog](https://github.com/rollup/rollup/blob/master/CHANGELOG.md)
- [Commits](rollup/rollup@v4.21.2...v4.22.5)

---
updated-dependencies:
- dependency-name: rollup
  dependency-type: indirect
...

Signed-off-by: dependabot[bot] <[email protected]>

* Bump vite from 5.4.2 to 5.4.8

Bumps [vite](https://github.com/vitejs/vite/tree/HEAD/packages/vite) from 5.4.2 to 5.4.8.
- [Release notes](https://github.com/vitejs/vite/releases)
- [Changelog](https://github.com/vitejs/vite/blob/v5.4.8/packages/vite/CHANGELOG.md)
- [Commits](https://github.com/vitejs/vite/commits/v5.4.8/packages/vite)

---
updated-dependencies:
- dependency-name: vite
  dependency-type: direct:development
...

Signed-off-by: dependabot[bot] <[email protected]>

* Add PostHog analytics to readme

* migration to Svelte 5

* Resolve error in marketing layout

* Fix type error in change password page

* Resolve type error in create profile page

* Update packages and format

* Use import style for Node 23+

* Use TS in one file where we missed it

* Add a tbody.

This isn't ideal as we're diverting from the tested template... but I'll test it.

Should ideally remove if this is fixed: sveltejs/svelte#9785

* Fix svelte check errors

* Fix formatting

* Fix out mailer for svelte 5

* Svelteize the page.

* Move to svelte 5 $effect

* $effect

https://svelte.dev/docs/svelte/v5-migration-guide#Migration-script-run

* Move email templates to handlebars.js

remove tbody tags we added for svelte compatibility

* Fix docs -- correct filenames

* Revert changes made to email template for svelte compat. Using handlebars now

* Don't auto format plaintext email templates

* Remove unneeded exclude

* Upgrade cookie for dependabot warning

* Rename env template for vercel compat

* Fix CriticalMoments/CMSaasStarter#169

* Move analytics to it's own README

Add GA links

* Update README.md

* Bump deps

* update sponsor

---------

Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: scosman <[email protected]>
Co-authored-by: Owen Versteeg <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: mgenovski <[email protected]>
Co-authored-by: Charl <[email protected]>
Co-authored-by: Tom Atkins <[email protected]>
Co-authored-by: Charl Best <[email protected]>
Co-authored-by: David Kizivat <[email protected]>
Co-authored-by: David Kizivat <[email protected]>
Co-authored-by: Jumpei Ogawa <[email protected]>
Co-authored-by: Toon van Ramshorst <[email protected]>
Co-authored-by: Evgenii Neumerzhitckii <[email protected]>
Co-authored-by: Gary Fox <[email protected]>
Co-authored-by: Evan Unit Lim <[email protected]>
Co-authored-by: Louis Deconinck <[email protected]>
Co-authored-by: Jason <[email protected]>
Co-authored-by: elim64 <[email protected]>
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 a pull request may close this issue.