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

only set attributes via properties when truly necessary #3013

Merged
merged 2 commits into from
Jun 16, 2019
Merged

Conversation

Conduitry
Copy link
Member

Hopefully fixes #1434 for real. Basically, this waaay cuts back on which attributes we set via properties. Thanks to @RedHatter for doing all of the hard work investigating this!

Since many more attributes now use the attr helper to set them, its logic for removing the attribute on null and undefined can be used.

@Conduitry
Copy link
Member Author

From #2935: There's more to do here. At least the draggable and spellcheck attributes should be removed as well, because they're not truly boolean. This needs some more combing.

@RedHatter
Copy link
Contributor

RedHatter commented Jun 12, 2019

I went through all the remaining attributes and double checked they were indeed boolean attributes and a quick perusal of the removed attributes. I apologize for not catching these the first time.

We already know about draggable and spellcheck. sandbox and contextmenu were typos. The marquee element should be removed from loop. hidden should be added back.

That should be all of them.

@Conduitry
Copy link
Member Author

I'm looking at https://html.spec.whatwg.org/#attributes-3 right now, and it seems that there are a few others that ought to be boolean but are not currently in this list: allowpaymentrequest, formnovalidate, hidden (is on the original list in the master branch), itemscope, nomodule, and playsinline. It looks like seamless is in our list but is not in the spec's list of attributes at all (boolean or no). There are also several discrepancies in which attributes are allowed on which elements.

@RedHatter
Copy link
Contributor

I only checked the attributes on the original list but yeah, it looks like those should be on it as well. seamless as well as scoped where proposed a while back but were removed from the specs. They're not supported by any current browser version so we could probably remove them if we want. They were originally proposed as boolean attributes. Additionally the bgsound, command, and keygen elements are part of an old spec. The command element was never supported by any browsers but the other two still have some support.

@Conduitry
Copy link
Member Author

Okay almost there I think. I've just pushed another update - There are two supposedly boolean attributes (which I've marked with comments) where I could not find a corresponding property when I was inspecting the available properties in a browser. Do you know anything about either of these?

@mrkishi
Copy link
Member

mrkishi commented Jun 12, 2019

playsinline only applies to <video>, with the corresponding playsInline property.

There used to be an itemScope IDL reflection but that was deprecated.

@Conduitry
Copy link
Member Author

All right, I'm really hoping we're good now. I've set playsinline on <video> to use playsInline, and I've not added itemscope to the list at all.

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.

Ignore undefined/falsey properties on components and elements
3 participants