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

Number and Boolean block attributes stored as an HTML attribute should be parsed properly #13965

Open
wants to merge 3 commits into
base: master
from

Conversation

@davilera
Copy link
Contributor

davilera commented Feb 20, 2019

Description

Closes #13949.

This pull requests fixes two issues I identified while storing a block attribute in an HTML attribute:

  • Boolean attributes such as data-show-button="true|false" didn't work, as the source code didn't parse the "false" value as false. This was first reported here: #13949.
  • Number/integer attributes such as data-height="50" couldn't be saved in an attribute either.

How has this been tested?

I created a simple block and stored boolean and number attributes in HTML attributes. Before the PR, Gutenberg complained about expected and actual values not matching. With this PR, Gutenberg pulls the proper values and things work as expected.

Types of changes

I think both changes are Breaking changes, as they change Gutenberg's behaviour regarding Boolean and Number HTML attributes.

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.
…d`. Updated tests to match the new behaviour.
@aduth

This comment has been minimized.

Copy link
Member

aduth commented Feb 20, 2019

Related: #10338 (Where most type coercion was removed)

I think both changes are Breaking changes, as they change Gutenberg's behaviour regarding Boolean and Number HTML attributes.

With that in mind, do you have an idea for how it could be rolled out?

Minding that today, the option remains to simply use the comment serialization offerings to preserve intended type (i.e. omit source).

@davilera

This comment has been minimized.

Copy link
Contributor Author

davilera commented Feb 20, 2019

There are multiple discussions here.

First, there's #9634. I personally think that Gutenberg's management of undefined attributes makes sense: when loading a block, if an attribute is undefined, Gutenberg should load the default value (if any). From my perspective, undefined should always* mean "try to load the default value".

#10338, on the other hand, proposes a more ambitious solution. I don't feel qualified enough for discussing type coercion in general...

From my point of view, storing primitive values such as a string, a number, or a boolean in an HTML attribute should be possible. My PR addresses numbers and booleans only and I think does so in a predictable and understandable way:

  1. The behaviour of boolean attributes is in line to what we have, but with a semantic fix: "false" should be transformed into false. This is something I'd assume all developers would expect... and I honestly don't think anyone would write something like attribute="false" and then expect attribute to be true.
  2. A number (or integer) attribute is parsed as their string counterparts are, but cast to a Number.

I know this PR proposes some Breaking changes, but I don't think any developer is currently using number or boolean attributes as described here, because such attributes don't work with the current version of Gutenberg and the behaviour a developer would get today is completely counter-intuitive.

* The only exception to this rule are boolean attributes. The mere presence of a boolean attribute (e.g. draggable in <input draggable>) should be transformed into a true value... but this doesn't change with my PR.

@aduth

This comment has been minimized.

Copy link
Member

aduth commented Feb 20, 2019

* The only exception to this rule are boolean attributes. The mere presence of a boolean attribute (e.g. draggable in <input draggable>) should be transformed into a true value... but this doesn't change with my PR.

It does, though. Assigning a value of 'false' would have previously been interpreted as true, whereas with these changes it's interpreted as false.

Given the inline examples provided in toBooleanAttributeMatcher, considering something like:

	// <input disabled="false">
	// - Value:       `'false'`
	// - Transformed: `true`

Aligns to the actual DOM behavior: https://codepen.io/aduth/pen/PVgpeG

I know this PR proposes some Breaking changes, but I don't think any developer is currently using number or boolean attributes as described here, because such attributes don't work with the current version of Gutenberg and the behaviour a developer would get today is completely counter-intuitive.

I bring it up because largely there's no established protocol for how to introduce breaking changes. It would be one thing if it were something classified as a bug, but it caught my attention being self-described as breaking.

@davilera

This comment has been minimized.

Copy link
Contributor Author

davilera commented Feb 20, 2019

Given the inline examples provided in toBooleanAttributeMatcher, considering something like <input disabled="false"> align to the actual DOM behavior.

Indeed. According to the spec:

Some attributes play the role of boolean variables. Their appearance in the start tag of an element implies that the value of the attribute is "true". Their absence implies a value of "false".

In my opinion, though, something like <input disabled="false"> is misleading and a really, really bad practice. The HTML value in the disabled HTML attribute doesn't have any effect on the disabled property.

But look at this other example: https://codepen.io/anon/pen/jdRwdr. An attribute such as data-enabled is not an HTML boolean attribute, and so it's attribute is interpreted as a string that I can then use to my need.

it caught my attention being self-described as breaking

I described this as a breaking change because if there was one single developer that had ever written something like <input enabled="false">, he might be expecting the property enabled to be true. But, honestly, I don't think anyone would do that, as it's completely counter-intuitive.

Maybe this should only apply to attributes named data-*?

How do you feel about the proposed PR? Does it make sense to move ahead with such changes? Should anybody else join the discussion?

@youknowriad youknowriad removed this from the 5.3 (Gutenberg) milestone Mar 18, 2019
@paaljoachim

This comment has been minimized.

Copy link

paaljoachim commented Sep 9, 2019

It seems this PR is at the moment stuck as it needs a decision on what to do with it.
@noisysocks

@pascalknecht

This comment has been minimized.

Copy link

pascalknecht commented Sep 9, 2019

@paaljoachim @davilera I also have problems with this currently. Any solutions in sight?

@paaljoachim

This comment has been minimized.

Copy link

paaljoachim commented Sep 9, 2019

What you @pascalknecht and/or @davilera can do is bring it up during the Open Floor in the core editor chat on Wednesday. As it is a good way to get immediate feedback on an issue/PR.

Editor Weekly Chat Wednesday, September 11, 2019, 03:00 PM GMT+2 in #core-editor
See the sidebar of https://make.wordpress.org/core/

@davilera

This comment has been minimized.

Copy link
Contributor Author

davilera commented Sep 18, 2019

Sounds good to me, @paaljoachim. I couldn't attend last week, but I can today.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

6 participants
You can’t perform that action at this time.