-
-
Notifications
You must be signed in to change notification settings - Fork 2.8k
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
Make attribute "charset" valid #1863
Conversation
The attribute "charset" was considered an error, but it results in valid HTML because HTML is case-insensitive. In other words, these two lines will be interpreted in the same way: <meta charSet="utf-8" /> <meta charset="utf-8" /> facebook/react#11492 (comment)
@@ -30,6 +30,8 @@ ruleTester.run('no-unknown-property', rule, { | |||
{code: '<App class="bar" />;'}, | |||
{code: '<App for="bar" />;'}, | |||
{code: '<App accept-charset="bar" />;'}, | |||
{code: '<meta charset="utf-8" />;'}, | |||
{code: '<meta charSet="utf-8" />;'}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i think we should be forcing charset
; HTML also supports CHARSET
and cHaRsEt
, but lowercase is the very dominant convention.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That was my initial thought, but charSet
is not an unknown attribute, as the name of the rule implies. That said, I’ll modify the PR, so you decide. 😉
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hmm, that is true. I suppose it’s a slight overreach of this rule to force it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah. As much as I would like for people to write charset
instead of charSet
, raising an error because of that would not be symmetrical with raising an error for acceptcharset
in favor of acceptCharset
, because only the latter results into the correct form of the attribute – accept-charset
.
I'll hold with the changes then until we agree what they should be.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no need, i think this makes sense as-is
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
The attribute
charset
was considered an error in favor ofcharSet
, but both of them result in valid HTML because HTML is case-insensitive. In other words, these two lines will be interpreted in the same way:They will render different HTML (exactly like this JSX suggests), but browsers won't care. If nothing else,
charset
should be the more "correct" form of this attribute because it doesn't suggest a dash (fillOpacity
=fill-opacity
,charSet
≠char-set
).facebook/react#11492 (comment)