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

Fix spellcheck attr not set to false #1110

Merged
merged 1 commit into from
May 25, 2018
Merged

Fix spellcheck attr not set to false #1110

merged 1 commit into from
May 25, 2018

Conversation

marvinhagemeister
Copy link
Member

I'm not too keen on checking directly against the attribute name, but this seems to be the only case in the spec where the attribute should not be removed if the value is false.

const el = document.createElement("input");

el.removeAttribute("spellCheck");
el.spellcheck // logs `true` :/

el.setAttribute("spellCheck", false);
el.spellcheck // logs `false`

Fixes #651.

@coveralls
Copy link

coveralls commented May 20, 2018

Coverage Status

Coverage remained the same at 100.0% when pulling 836c99d on marvinhagemeister:spellcheck into 7e49a91 on developit:master.

@marvinhagemeister
Copy link
Member Author

Locally, without code-coverage enabled the performance difference is much smaller than in our CI:

test before after
empty diff 87939/s (30 ticks) 88119/s (31 ticks)
repeat diff 10988/s (250 ticks) 11709/s (228 ticks)
large VTree 43701/s (63 ticks) 43082/s (62 ticks)
style/prop mutation 20460/s (134 ticks) 20316/s (133 ticks)
SSR Hydrate 1376/s (1967 ticks) 1315/s (2020 ticks)

src/dom/index.js Outdated
// spellcheck is treated differently than all other boolean values and
// should not be removed when the value is `false`. See:
// https://developer.mozilla.org/en-US/docs/Web/HTML/Element/input#attr-spellcheck
if ((value==null || value===false) && name !== "spellCheck") {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the name is "spellcheck", right? In that case it actually wouldn't fall through to here, but rather would pass the in check on line 82. It seems like the special-casing belongs there:

// replaces L84:
if ((value==null || value===false) && name!='spellcheck') node.removeAttribute(name);

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Went the setAttribute way, but forcing lowercase seems better. Will change that.

@marvinhagemeister
Copy link
Member Author

@developit Updated with your suggestion 🎉

@developit developit merged commit 534eb8f into preactjs:master May 25, 2018
@marvinhagemeister marvinhagemeister deleted the spellcheck branch May 25, 2018 16:17
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