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.

Sign up for GitHub

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

Jump to bottom

HTML Parser fails if you have the same attribute twice #5966

Closed
7 tasks done
surma opened this issue Dec 5, 2021 · 11 comments · Fixed by #9678
Closed
7 tasks done

HTML Parser fails if you have the same attribute twice #5966

surma opened this issue Dec 5, 2021 · 11 comments · Fixed by #9678
Labels
bug: upstream Bug in a dependency of Vite feat: html p3-minor-bug An edge case that only affects very specific usage (priority)

Comments

@surma
Copy link

surma commented Dec 5, 2021

Describe the bug

It seems that if you have the same attribute twice, the HTML parser to fail. (Tested it with class=, data-a= and x=. It fails for all of them.)

Reproduction

(Repro on Stackblitz.)

<!DOCTYPE html>
<div class="a" class="b"></div>

Output:

[vite] Internal server error: Unable to parse {"file":"/index.html","line":2,"column":16}
1  |  <!DOCTYPE html>
2  |  <div class="a" class="b"></div>
   |                 ^
3  |  
      at traverseHtml (/home/projects/vitejs-vite-wv2wbb/node_modules/vite/dist/node/chunks/dep-e0fe87f8.js:21240:15)
      at async devHtmlHook (/home/projects/vitejs-vite-wv2wbb/node_modules/vite/dist/node/chunks/dep-e0fe87f8.js:57001:5)
      at async applyHtmlTransforms (/home/projects/vitejs-vite-wv2wbb/node_modules/vite/dist/node/chunks/dep-e0fe87f8.js:21550:21)
      at async viteIndexHtmlMiddleware (/home/projects/vitejs-vite-wv2wbb/node_modules/vite/dist/node/chunks/dep-e0fe87f8.js:57065:28)

System Info

Stackblitz.

Used Package Manager

npm

Logs

No response

Validations
@Shinigami92
Copy link
Member

Should we throw another error such as duplicate attribute? Maybe we are using a parser for html and we can bring this down to their repository so we "fix" it for all their users 🤔 (Hadn't looked into the code)

@Shinigami92
Copy link
Member

Shinigami92 commented Dec 7, 2021 •
edited

Maybe this will be fixed/improved by #5777

@surma
Copy link
Author

surma commented Dec 7, 2021

I wouldn’t expect this to error at all. Duplicate attributes are perfectly valid HTML.

@Niputi
Copy link
Contributor

Niputi commented Dec 7, 2021

it will error because vue/compiler-dom is checking if the html is valid and it doesn't allow duplicate attributes.
we might need to open a issue at vue or change to another tool to validate

@Niputi Niputi added the bug: upstream Bug in a dependency of Vite label Dec 7, 2021
@patak-dev
Copy link
Member

patak-dev commented Dec 7, 2021 •
edited

There was already an issue but was closed as in Vue it is preferred to error on duplicates vuejs/core#4883
Agree with @surma that there shouldnt be an error. We need to open a feat request in vue-next for an option to avoid this error in Vite

@nurulhudaapon
Copy link
Contributor

I think probably it should still at least warn for this kind of low level error since duplicate attributes are still not valid syntax in HTML.

Can just have list of error codes of the Vue HTML parser for which Vite just warns.

Reference: https://www.w3.org/TR/WCAG20-TECHS/H94.html

@surma
Copy link
Author

surma commented Dec 7, 2021 •
edited

duplicate attributes are still not valid syntax in HTML.

That’s not true. As far as I can tell from the HTML parser spec, it’s definitely not a syntactic error and I can’t find anything about it being a semantic error either. (edit: I was wrong, see below)

What you linked to is a WCAG note, saying:

The objective of this technique is to avoid key errors that are known to cause problems for assistive technologies when they are trying to parse content that has duplicate attributes on the same element.

So while it is definitely advisable to not have duplicate attributes — esp. when they are relevant for assistive technologies — but as far as I can tell, it’s not invalid by any means, and should not cause an error imo.

I have no strong opinions on whether or not to emit a warning, but I worry about it potentially being noise when it’s harmless (e.g. a duplicate class attribute is not problematic).

@Shinigami92
Copy link
Member

I'm with you for class, but I could not tell from my knowledge what the behavior would be for other attributes.
Do they override the previous once?

Do you have currently a real world example / project where you need this? Depending on that, we could estimate the priority of this issue.

Also as @Niputi mentioned, do we already have an issue for this in @vue/compiler-dom?
If not, could you please open one and then link it here?

@Niputi Niputi mentioned this issue Dec 8, 2021
Open
@surma
Copy link
Author

surma commented Dec 8, 2021

If the same attribute is specified multiple times, the first one wins.

I am running into this because I’m using vite to optimize and bundle the output from 11ty. Lots my HTML is programmatically generated and happens to have duplicate attributes.

@patak-dev patak-dev added the p3-minor-bug An edge case that only affects very specific usage (priority) label Dec 8, 2021
@yyx990803
Copy link
Member

yyx990803 commented Dec 13, 2021 •
edited

https://html.spec.whatwg.org/multipage/syntax.html#attributes-2

Quote:

There must never be two or more attributes on the same start tag whose names are an ASCII case-insensitive match for each other.

If your markup contains duplicated attributes, you should fix it. If your tool is generating duplicated attributes, it's a bug in that tool and you should ask that tool to fix it.

That said, browsers do silently allow duplicated attributes, so Vite should not break the build because of it. But still.

@surma
Copy link
Author

surma commented Dec 13, 2021

TIL! I don’t know why I didn’t find that previously. Thanks for providing it.

I have no spec-level leg to stand on, but I would consider Postel’s law here, as all browsers uniformly accept this markup. I’ll leave the final call up to you and whether or not to close this.

@bluwy bluwy mentioned this issue Mar 29, 2022
Closed
7 tasks
@bluwy bluwy removed the p3-minor-bug An edge case that only affects very specific usage (priority) label Jun 26, 2022
@patak-dev patak-dev added the p3-minor-bug An edge case that only affects very specific usage (priority) label Jul 21, 2022
@patak-dev patak-dev mentioned this issue Aug 15, 2022
Merged
11 tasks
@github-actions github-actions bot locked and limited conversation to collaborators Sep 8, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
bug: upstream Bug in a dependency of Vite feat: html p3-minor-bug An edge case that only affects very specific usage (priority)
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

refactor: migrate from vue/compiler-dom to parse5 vitejs/vite
7 participants
@surma @yyx990803 @patak-dev @Niputi @Shinigami92 @bluwy @nurulhudaapon