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

chore: refactor HTML validation #11969

Closed
wants to merge 1 commit into from
Closed

chore: refactor HTML validation #11969

wants to merge 1 commit into from

Conversation

Rich-Harris
Copy link
Member

Follow-up to #11947. Right now our HTML validation (for checking which elements are allowed inside which other elements) isn't great — we have special handling for <p> (why?) and interactive elements, and #11947 added some logic for preventing <form><form></form></form> that doesn't prevent <form><Form></Form></form>.

This PR is a naive attempt to fix it by having a single place to define disallowed children/parents that can be used in multiple places (parsing, validation, SSR) rather than having a bunch of special-case logic. Unfortunately it doesn't quite work, because it prevents children from having certain descendant elements, when the required logic is a bit more subtle. For example one test fails because an <li> has a valid descendant (not child) <li>. Examples of nuances that aren't captured by our current validation or this PR:

  • <ul> and <ol> can only contain <li>, <script> or <template> — we don't have the concept of an allowlist of direct children
  • <li> must be a direct child of <ul> or <ol> or <menu> — we don't have the concept of an allowlist of direct parents
  • <table> can contain <tr> directly — the intermediate <tbody> is automatically created if absent. we only insist on it because we're not sophisticated enough to handle the resulting hydration complexities
  • some errors (<p> inside <p>) result in broken DOM, others (<h1> inside <dd>) are invalid but do not result in broken DOM

I also wonder if an error is appropriate. In one app I converted to Svelte 5, I hit an error immediately because a <button> contained another <button> — this is incorrect, but the app had been working just fine. For people who aren't using SSR, this case is a potential accessibility pitfall but it won't result in a hydration mismatch, and so maybe a warning is actually more appropriate?

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 Jun 8, 2024

⚠️ No Changeset found

Latest commit: 52056cf

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@trueadm
Copy link
Contributor

trueadm commented Jun 9, 2024

I also wonder if an error is appropriate. In one app I converted to Svelte 5, I hit an error immediately because a contained another

The browser might try and patch this when we clone the template too. So it could also break when we clone the template. Furthermore, buttons within buttons are a major issue for screen readers and other accessibility tooling – where JAWS will skip the inner button entirely.

@Rich-Harris Rich-Harris marked this pull request as draft July 3, 2024 16:36
@dummdidumm
Copy link
Member

Closing in favor of #12618

@dummdidumm dummdidumm closed this Jul 26, 2024
@dummdidumm dummdidumm deleted the html-validation branch July 26, 2024 12:16
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.

3 participants