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

Boolean attributes #629

Merged
merged 1 commit into from Jul 18, 2018
Merged

Conversation

frenzzy
Copy link
Contributor

@frenzzy frenzzy commented Mar 1, 2018

Fixes #628

With this change we may rich consistent behavior between client and server rendering for reserved DOM attributes such as spellcheck or draggable, see:

before

<input spellcheck="false" /> // => <input spellcheck="true" /> <== Oh NO!
<input spellcheck={false} /> // => <input spellcheck="false" />

after

<input spellcheck="false" /> // => <input spellcheck="false" /> <== Oh YES!
<input spellcheck={false} /> // => <input spellcheck="false" />

SSR

<input spellcheck="false" /> // => <input spellcheck="false" />
<input spellcheck={false} /> // => <input />

@codecov
Copy link

codecov bot commented Mar 1, 2018

Codecov Report

Merging #629 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@          Coverage Diff          @@
##           master   #629   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files           1      1           
  Lines         166    166           
  Branches       52     52           
=====================================
  Hits          166    166
Impacted Files Coverage Δ
src/index.js 100% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ebdfc2c...23778ee. Read the comment docs.

@jorgebucaran
Copy link
Owner

jorgebucaran commented Mar 1, 2018

How is this different to #592?

@frenzzy
Copy link
Contributor Author

frenzzy commented Mar 1, 2018

#592 behavior:

// before
<input spellcheck="false" data-x="false" /> // => <input spellcheck="true" data-x="false" />
<input spellcheck={false} data-x={false} /> // => <input spellcheck="false" />

// after
<input spellcheck="false" data-x="false" /> // => <input spellcheck="true" data-x="false" />
<input spellcheck={false} data-x={false} /> // => <input spellcheck="false" data-x="false" />

// SSR
<input spellcheck="false" data-x="false" /> // => <input spellcheck="false" data-x="false" />
<input spellcheck={false} data-x={false} /> // => <input />

so, this PR is about reserved DOM attributes while #592 is about custom DOM attributes.

src/index.js Outdated
@@ -153,7 +153,7 @@ export function app(state, actions, view, container) {
typeof value === "function" ||
(name in element && name !== "list" && !isSVG)
) {
element[name] = value == null ? "" : value
element[name] = value == null || value === "false" ? "" : value
Copy link
Contributor Author

@frenzzy frenzzy Mar 2, 2018

Choose a reason for hiding this comment

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

or we can use regex here: /^false$/i.test(value)
it will be more robust (users can use False, FALSE etc) but slower

<input spellcheck="false" /> // => <input spellcheck="false" />
<input spellcheck="False" /> // => <input spellcheck="true" />
<input spellcheck="FALSE" /> // => <input spellcheck="true" />
// vs regexp
<input spellcheck="false" /> // => <input spellcheck="false" />
<input spellcheck="False" /> // => <input spellcheck="false" />
<input spellcheck="FALSE" /> // => <input spellcheck="false" />

Copy link
Contributor

Choose a reason for hiding this comment

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

HTML spec allow different cases ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, HTML case insensitive

@jorgebucaran
Copy link
Owner

Unrelated but related. Adding it here before I forget. preactjs/preact#983

@jorgebucaran
Copy link
Owner

@frenzzy But why not use {false}?

@frenzzy
Copy link
Contributor Author

frenzzy commented Mar 8, 2018

@frenzzy But why not use {false}?

Because false does't render attribute at all:

<input data-example={false} /> // renders <input />

Also "false" already works correctly for most of attributes:

<input data-example="false" /> // renders <input data-example="false" />

The problem only in bunch of DOM attributes such as spellcheck, dragging etc.

After this change "false" not longer converted to "true" for SOME attributes.
Which makes "false" value consistent between different attributes and server-side rendering.

@jorgebucaran
Copy link
Owner

jorgebucaran commented Mar 8, 2018

@frenzzy Why does "false" work for some attributes and not for others? I am trying to leave this information documented here for our record.

@frenzzy
Copy link
Contributor Author

frenzzy commented Mar 8, 2018

@frenzzy Why does "false" work for some attributes and not for others?

Because of the way how we modify attributes (props vs attributes):
https://github.com/hyperapp/hyperapp/blob/fa8fdd373539c386c2bca45dfd185ba5047f6f1a/src/index.js#L163

const el = document.createElement('input')

el.spellcheck = false                   // => <input spellcheck="false" />
el.spellcheck = 'false'                 // => <input spellcheck="true" /> !!! Noooo...
el.setAttribute('spellcheck', false)    // => <input spellcheck="false" />
el.setAttribute('spellcheck', 'false')  // => <input spellcheck="false" />
el.setAttribute('data-custom', false)   // => <input data-custom="false" />
el.setAttribute('data-custom', 'false') // => <input data-custom="false" />

see also #634

@frenzzy frenzzy force-pushed the falsey-attributes branch 2 times, most recently from a02b0d9 to 0b88944 Compare March 9, 2018 09:32
@frenzzy
Copy link
Contributor Author

frenzzy commented Mar 13, 2018

Looks like hyperapp has problems with all kind of boolean attribute values "true"/"false", "yes"/"no" and "on"/"off", for example:

// jsx                     => html
<div translate="yes" /> // => <div translate="yes"></div>
<div translate="no" />  // => <div translate="yes"></div> <= No!

Probably related: facebook/react#10589

@jorgebucaran
Copy link
Owner

jorgebucaran commented Mar 13, 2018

I think we should make only {true} or {false} work and doing that, fix that horrible "yes", "no", "on", "off", etc., API.

@frenzzy frenzzy force-pushed the falsey-attributes branch 2 times, most recently from b8fef98 to b8d26a2 Compare March 16, 2018 09:37
@frenzzy frenzzy force-pushed the falsey-attributes branch 2 times, most recently from de86cf8 to ef44361 Compare March 29, 2018 08:46
@frenzzy
Copy link
Contributor Author

frenzzy commented Mar 29, 2018

@frenzzy frenzzy changed the title Falsey attributes Boolean attributes Mar 29, 2018
@frenzzy
Copy link
Contributor Author

frenzzy commented Mar 29, 2018

I think we should make only {true} or {false} work and doing that, fix that horrible "yes", "no", "on", "off", etc., API.

  1. It is not obvious for user why "false" does not work properly for some attributes. He writes "false" hyperapp renders "true" - WTF!?
  2. It is not possible to render some attributes in isomorphic manner. If use {false}, server will not render it, if use "false", hyperapp will change it to "true".
  3. User should always keep in mind list of attributes which works incorrectly, see https://codepen.io/frenzzy/pen/Zxrrzz/left/?editors=0010

@frenzzy frenzzy force-pushed the falsey-attributes branch 3 times, most recently from bda9768 to 11c99a0 Compare March 31, 2018 06:58
@jorgebucaran
Copy link
Owner

Let's wait after 2.0. Then let's figure out what to do.

src/index.js Outdated
} else if (
name in element &&
name !== "list" &&
!(typeof value === "string" && typeof element[name] === "boolean") &&
Copy link
Owner

Choose a reason for hiding this comment

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

@frenzzy What about only typeof value === "string"?

So, removing typeof element[name] === "boolean".

Copy link
Owner

Choose a reason for hiding this comment

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

Nevermind :D

@jorgebucaran jorgebucaran merged commit aa2ce46 into jorgebucaran:master Jul 18, 2018
@frenzzy frenzzy deleted the falsey-attributes branch July 18, 2018 10:17
@jorgebucaran
Copy link
Owner

Thanks, @frenzzy! 1.2.8 🎉

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.

None yet

3 participants