Ensure cite is string when merging quote #10678

ellatrix · 2018-10-17T08:18:34Z

Description

When merging a paragraph with a quote, the citation is undefined and cast to a string.

How has this been tested?

Screenshots

Types of changes

Checklist:

My code is tested.
My code follows the WordPress code style.
My code follows the accessibility standards.
My code has proper inline documentation.

ellatrix · 2018-10-17T08:53:14Z

packages/blocks/src/api/factory.js

@@ -50,6 +50,10 @@ export function createBlock( name, blockAttributes = {}, innerBlocks = [] ) {
 			result[ key ] = schema.default;
 		}

+		if ( schema.source === 'html' && typeof result[ key ] !== 'string' ) {


This is similar to what was done with rich-text source before. It would create() here.

jorgefilipecosta · 2018-10-17T09:01:53Z

packages/blocks/src/api/factory.js

@@ -50,6 +50,10 @@ export function createBlock( name, blockAttributes = {}, innerBlocks = [] ) {
 			result[ key ] = schema.default;
 		}

+		if ( schema.source === 'html' && typeof result[ key ] !== 'string' ) {
+			result[ key ] = '';


Isn't this change a little bit risky? e.g: some block may be comparing the attribute with undefining to check if the value does not exist and that may stop working now.
Also isn't there a risk a block may want to differentiate an HTML attribute being undefined (e.g: the query element was not found ) with an HTML attribute being empty (element exists but its content is empty )?
In this case, I would be inclined to just add a default value of the empty string in the merge function to reduce the risks.

(e.g: the query element was not found )

We currently already pass '' if the query is not found.

Alternative is to add default: '' everywhere.

And do you have a use case for the undefined? Note that ! value will still work.

If we already pass '' if the query was not found I think everything seems fine. I was not aware of that. Sorry, I thought we passed undefined when the query did not match any selector.

gutenberg/packages/blocks/src/api/matchers.js

Lines 20 to 22 in d374e02

if ( ! match ) {

return '';

}

I'd prefer default everywhere or accounting for the undefined value in merges much more than to include ad hoc corrections in the factory.

It's also conflicting messaging when we went out of our way in #10338 to explicitly remove tolerances. If we'd kept the tolerances, I'd rather see something where we normalize undefined of type: 'string' to an empty string, but again still nothing targeting one specific source.

What's the issue with providing the default, aside from tedium / verbosity? I see it as no different (and thus extremely confusing in its consistency) from how we don't provide empty arrays for array types, zero's for number types, or an empty string for any other string types (what's the difference here between why we need an empty string here for assumed HTML stringiness and source: 'attribute'?).

At worst, I'd think we should codify this as a source-level default mapping.

I find it a bit weird that plugin authors will have to provide the default value, and may forget about it. Otherwise we have to set the default in merge functions etc., but since even we forget about it, I think block authors might also forget about it and cause these preventable bugs.

That's all well and good as a goal, but targeting a resolution for something so specific is going to have negative consequences (i.e. bugs) elsewhere in setting people up for mixed expectations for what is and isn't normalized on their behalf.

Okay, then let's change it. I'm not sure what the right solution is here tbh.

I think it's a reasonable expectation on the part of the block author that if they want to assume an attribute is a string (i.e. in concatenating), they should assure that it is one, via default.

This doesn't mean we have to add the default for every source: 'html' attribute, if we're not actually making such an assumption. I think it's also perfectly reasonable to not define the default if the stringiness is not assumed but instead accounted-for in the implementation.

See: #10756

gziolo

This solves the issue and improves merging blocks with Quote block nicely 👍

jorgefilipecosta

Everything looks good in my tests 👍

aduth · 2018-10-18T20:36:40Z

packages/block-library/src/quote/index.js

@@ -197,11 +197,18 @@ export const settings = {
 		);
 	},

-	merge( attributes, attributesToMerge ) {
+	merge( attributes, { value, citation } ) {
+		if ( ! value || value === '<p></p>' ) {


This is interesting to me as well that it be required to test for. I understand it's a consequence of the multiline behavior, but (correct me if I'm wrong) it's still effectively an emptiness check, and as such it seems strange we'd need to fork logic for concatenating the value of an "empty" field. And also strange that, if it is a test of emptiness, that it not be something we could use RichText.isEmpty for.

Alternatively, should we account for this from RichText itself, never allowing '' to become the value, but instead normalizing to--I assume--the empty string? This feels awfully reminiscent of inconsistencies we had ages ago with the RichText array vs. undefined values.

ellatrix · 2018-10-18T20:48:50Z

@aduth sure, we can do this. I’ll have a look tomorrow, maybe together with removing RichText.isEmpty checks altogether... I totally agree with you.

…

On Thu, 18 Oct 2018 at 22:43, Andrew Duthie ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In packages/block-library/src/quote/index.js <#10678 (comment)>: > @@ -197,11 +197,18 @@ export const settings = { ); }, - merge( attributes, attributesToMerge ) { + merge( attributes, { value, citation } ) { + if ( ! value || value === '' ) { This is interesting to me as well that it be required to test for. I understand it's a consequence of the multiline behavior, but (correct me if I'm wrong) it's still effectively an emptiness check, and as such it seems strange we'd need to fork logic for concatenating the value of an "empty" field. And also strange that, if it is a test of emptiness, that it not be something we could use RichText.isEmpty for. Alternatively, should we account for this from RichText itself, never allowing '' to become the value, but instead normalizing to--I assume--the empty string? This feels awfully reminiscent of inconsistencies we had ages ago with the RichText array vs. undefined values. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#10678 (review)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AEfg65vKbsVm_3iCndoEvzhejpjV_rK9ks5umOfhgaJpZM4XjX7z> .

* Ensure cite is string when merging quote * Ensure createBlock returns string for html source * Don't merge empty paragraphs

Ensure cite is string when merging quote

92641ea

ellatrix added the [Feature] Rich Text Related to the Rich Text component that allows developers to render a contenteditable label Oct 17, 2018

ellatrix added this to the 4.0 milestone Oct 17, 2018

ellatrix requested a review from a team October 17, 2018 08:18

gziolo added the [Type] Bug An existing feature does not function as intended label Oct 17, 2018

Ensure createBlock returns string for html source

660d419

ellatrix commented Oct 17, 2018

View reviewed changes

Don't merge empty paragraphs

27c6080

jorgefilipecosta reviewed Oct 17, 2018

View reviewed changes

gziolo approved these changes Oct 17, 2018

View reviewed changes

ellatrix merged commit 1c2282c into master Oct 17, 2018

ellatrix deleted the fix/quote-merge branch October 17, 2018 09:17

jorgefilipecosta reviewed Oct 17, 2018

View reviewed changes

aduth mentioned this pull request Oct 18, 2018

Block API: Remove HTML source string normalization #10756

Merged

aduth reviewed Oct 18, 2018

View reviewed changes

ellatrix mentioned this pull request Oct 18, 2018

RichText: deprecate RichText.isEmpty #10763

Closed

aduth mentioned this pull request Oct 19, 2018

Fix rich text value for nested lists #10799

Merged

4 tasks

mcsf pushed a commit that referenced this pull request Nov 1, 2018

Ensure cite is string when merging quote (#10678)

a8b6e73

* Ensure cite is string when merging quote * Ensure createBlock returns string for html source * Don't merge empty paragraphs

ellatrix mentioned this pull request Feb 28, 2019

Cannot delete all text from multiline RichText field #12880

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Ensure cite is string when merging quote #10678

Ensure cite is string when merging quote #10678

ellatrix commented Oct 17, 2018

ellatrix Oct 17, 2018

jorgefilipecosta Oct 17, 2018

ellatrix Oct 17, 2018

ellatrix Oct 17, 2018

jorgefilipecosta Oct 17, 2018 •

edited

Loading

ellatrix Oct 17, 2018 •

edited

Loading

aduth Oct 17, 2018

ellatrix Oct 17, 2018

aduth Oct 17, 2018

ellatrix Oct 17, 2018

aduth Oct 18, 2018

gziolo left a comment

jorgefilipecosta left a comment

aduth Oct 18, 2018 •

edited

Loading

ellatrix commented Oct 18, 2018 via email

Ensure cite is string when merging quote #10678

Ensure cite is string when merging quote #10678

Conversation

ellatrix commented Oct 17, 2018

Description

How has this been tested?

Screenshots

Types of changes

Checklist:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jorgefilipecosta Oct 17, 2018 • edited Loading

Choose a reason for hiding this comment

ellatrix Oct 17, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gziolo left a comment

Choose a reason for hiding this comment

jorgefilipecosta left a comment

Choose a reason for hiding this comment

aduth Oct 18, 2018 • edited Loading

Choose a reason for hiding this comment

ellatrix commented Oct 18, 2018 via email

jorgefilipecosta Oct 17, 2018 •

edited

Loading

ellatrix Oct 17, 2018 •

edited

Loading

aduth Oct 18, 2018 •

edited

Loading