Blocks: Handle pressing backspace at the beginning of blocks #461

youknowriad · 2017-04-20T11:02:49Z

In this PR, I'm handling the backspace behaviour. I've tried to keep the API needed for this as minimal as possible:

A single callback mergeWithPrevious to be called when we hit backspace at the beginning of a block.
A merge function responsible of merging attributes of a block with the same type.

The different use cases are:

If the merge function is not defined in a block, this means the blocks is an "object" block that can not be merged (think image, embed, gallery), thus, the block is removed when we hit backspace in a following block.
If the merge function is defined, we can merge this block with the following one but before doing that we need to match the block types of these two blocks. We transform the following block to the same block type and then we call the merge function to merge them. If the following block can not be switched to the current block, this means these two blocks are not mergeable.

youknowriad · 2017-04-21T09:20:05Z

I could use some 👀 here

aduth

Seems #466 will conflict with some of the ideas here.

aduth · 2017-04-21T13:56:54Z

editor/modes/visual-editor/block.js

+
+		// If the block types can not match, do nothing
+		if ( ! blockWithTheSameType ) {
+			return;


Do we want to remove the previous block here?

Good question. I don't really know the right answer. I think this use-case might not happen as often as we think because I'm assuming the majority of textual blocks could be transformed in between'em but It's a possibility.

I preferred to avoid deletion and do nothing but I don't have strong arguments for one or another.

My main reason for bringing it up was merely as a point of consistency, since it seemed from the above condition that the "unhandleable" case should simply be removed.

I'm not considering the "merge not defined" as "unhandleable" but more like, if you want this block to be removed when we hit backspace, don't define the merge

I'm not considering the "merge not defined" as "unhandleable" but more like, if you want this block to be removed when we hit backspace, don't define the merge

Isn't that the condition above though, not this one?

Oh, I misread your previous message as "if you don't want this block to be removed". Disregard.

This is fine, but we should probably document the behavior somewhere.

I'd very much prefer that we don't silently delete content here. Submitted a PR to change this behavior - #530

aduth · 2017-04-21T14:22:11Z

Seems #466 will conflict with some of the ideas here.

Might not be that bad actually, basically turns from concatenating strings to creating an array of two content members.

aduth · 2017-04-24T12:43:52Z

editor/modes/visual-editor/block.js

 	( dispatch, ownProps ) => ( {
-		onChange( updates ) {
+		onChange( uid, updates ) {


Minor: I don't love that we have some props that accept a uid and others that assume it from the other props. I understand the need for it and don't have a great alternative.

Do you think making the uid mandatory even for callbacks we're using only for the current block (and using _.partial ) is a good move?

I like that idea because it promotes consistency, but I don't like that idea because it loses the sense that the component is meant to be specific to a single block.

aduth · 2017-04-24T12:46:29Z

blocks/components/editable/index.js

+	}
+
+	onKeyDown( event ) {
+		if ( this.props.onMerge && event.keyCode === KEYCODE_BACKSPACE && this.isStartOfEditor() ) {


I don't know if it was intentional, but nice optimization having the heaviest part of the condition performed last and taking advantage of short-circuiting 👍

Intentional :)

aduth · 2017-04-24T12:48:27Z

Do you think this could qualify as a third transforms type: from, to, and now merge?

youknowriad · 2017-04-24T12:53:14Z

Do you think this could qualify as a third transforms type: from, to, and now merge?

I don't have a preference. Merging is not really a transformation but it returns attributes in the same way the transformations do. Answering a question by another one :P, what's your opinion on this? :)

aduth · 2017-04-24T12:58:56Z

I hadn't thought of it 'til today, and only came to mind as an attempt to consider if we could eliminate the merge and use only to and from. Eventually I relented and accepted the need for merge, but the thought process and considering it alike to and from made me think whether merge could be treated in a similar way. I also do not feel strongly; to and from is another nice duality. We maybe also shouldn't treat merge as first-class, since it's quite specific to blocks containing Editable fields (not all blocks).

youknowriad · 2017-04-24T13:55:09Z

element/index.js

@@ -65,3 +66,21 @@ export function renderToString( element ) {

 	return renderToStaticMarkup( element );
 }
+
+
+export function concatValues( value1, value2 ) {


@aduth Concatenating children values is not that easy, can you think of a simpler alternative?

ellatrix · 2017-04-24T13:56:05Z

blocks/components/editable/index.js

@@ -10,6 +10,7 @@ import { Parser as HtmlToReactParser } from 'html-to-react';
 */
 import './style.scss';

+const KEYCODE_BACKSPACE = 8;


There's also window.tinymce.util.VK. It's okay like this I guess, but at some point we may have to use other things from it anyway, like metaKeyPressed.

At some point in the near future we may also consider leveraging the now-preferred event.key property which, funny enough, is better supported in IE back to IE11 than it is in Safari (only available in most recent versions).

ellatrix · 2017-04-24T14:09:30Z

blocks/components/editable/index.js

+		do {
+			const child = element;
+			element = element.parentNode;
+			if ( element.childNodes[ 0 ] !== child ) {


element.firstChild? (It's faster)

element.firstChild? (It's faster)

I agree with the suggestion but now I'm intrigued in what way it would be faster? Seems like the difference is array access at an index, which I wouldn't expect to be slow.

@aduth I honestly have no idea :) If you find out, please let me know. :)

It's also not just array access?

aduth · 2017-04-24T17:45:13Z

element/index.js

@@ -65,3 +66,20 @@ export function renderToString( element ) {

 	return renderToStaticMarkup( element );
 }
+
+export function concatValues( value1, value2 ) {


If we are to drop the element React abstraction, what's to happen to these utility functions?

Is this function effectively concatenating two element trees? Could the name more accurately reflect this? "values" is not a term that means much to me in a React sense.

concatChildren ?

If we are to drop the element React abstraction, what's to happen to these utility functions?

Yep, we should consider moving those to a utils inside the editor folder maybe.

concatChildren ?

I like this, as it seems like if this were a function native to React, it'd be called React.Children.concat

aduth · 2017-04-24T17:47:05Z

element/index.js

@@ -65,3 +66,20 @@ export function renderToString( element ) {

 	return renderToStaticMarkup( element );
 }
+
+export function concatValues( value1, value2 ) {


Is this function effectively concatenating two element trees? Could the name more accurately reflect this? "values" is not a term that means much to me in a React sense.

aduth · 2017-04-24T17:47:51Z

element/index.js

@@ -65,3 +66,20 @@ export function renderToString( element ) {

 	return renderToStaticMarkup( element );
 }
+
+export function concatValues( value1, value2 ) {
+	const toArray = value => Array.isArray( value ) ? Children.toArray( value ) : [ value ];


Is it not enough to use Children.toArray in all cases here?

That was my first guess, but it's not working as expected on string values

aduth · 2017-04-24T17:48:47Z

element/index.js

+export function concatValues( value1, value2 ) {
+	const toArray = value => Array.isArray( value ) ? Children.toArray( value ) : [ value ];
+
+	return toArray( value1 )


Could this function be simplified to...

return [ ...Children.toArray( value1 ), ...Children.toArray( value2 ) ];

?

no, Children.toArray is not enough, and I figured out, I'd reset the keys to avoid any confusion/duplication

aduth · 2017-04-24T17:49:24Z

element/index.js

+				key: index
+			};
+		} );
+}


Evidenced by my confusion on the behavior of this function, would be good to add some JSDoc and test cases.

Yep, and unit tests too :)

aduth · 2017-04-24T17:51:58Z

editor/modes/visual-editor/block.js

+	( state, ownProps ) => {
+		const order = state.blocks.order.indexOf( ownProps.uid );
+		return {
+			previousBlock: order === 0 ? null : state.blocks.byUid[ state.blocks.order[ order - 1 ] ],


Maybe this should be <= 0 to account for the chance that uid is not present in the order array?

Alternatively perhaps the fallback could occur as an || case:

previousBlock: state.blocks.byUid[ state.blocks.order[ state.blocks.order.indexOf( ownProps.uid ) - 1 ] ] || null

(with better formatting)

youknowriad · 2017-04-25T13:07:22Z

Any other concern here?

aduth · 2017-04-25T13:28:59Z

Do we want to address focus issues separately? I'd hoped to be able to split and restore a block cleanly, but appears focus is lost after the merge:

Click in middle of text block
Hit enter twice
Hit backspace twice

Expected: Text block to have same text as in step 1

youknowriad · 2017-04-25T13:30:28Z

@aduth Yeah, I noticed and the fix for this would be to implement the focus offset (at least a special value to specify the end of an Editable, maybe -1) but I was hoping to fix this as a separate PR.

aduth · 2017-04-25T13:36:33Z

I cannot merge two blocks if the second block has no content.

aduth · 2017-04-25T13:36:55Z

Fine to handle separately (though maybe good to create an issue), but we should also handle the forward deletion merge.

youknowriad · 2017-04-25T15:51:14Z

I cannot merge two blocks if the second block has no content.

@aduth This is working for me. Can you detail your use case (what kind of blocks etc...)

aduth · 2017-04-25T18:00:34Z

Error when backspacing on new block paragraph:

Select end of first text block
Press enter twice
Press backspace
Block is eventually removed but an error is logged to the console

Unable to backspace second of two empty headings added to post:

Click inserter at bottom of post
Insert a heading
Repeat steps 1 and 2 to add a second heading
Press backspace while second empty heading is focused
Nothing happens

youknowriad · 2017-04-26T08:32:38Z

Thanks for the details @aduth

Error when backspacing on new block paragraph

This was caused because TinyMCE is handling the keydown internally and we're destroying TinyMCE on our call to onMerge. The fix in d091952 is not ideal, I'm debouncing the onMerge call. Let me know if you have a better idea.

Unable to backspace second of two empty headings added to post

Oh thanks! I was trying to reproduce, by splitting and backspace which was working. Fixed in fbef18c

aduth · 2017-04-26T13:32:08Z

This was caused because TinyMCE is handling the keydown internally and we're destroying TinyMCE on our call to onMerge. The fix in d091952 is not ideal, I'm debouncing the onMerge call. Let me know if you have a better idea.

TinyMCE's internal event handling respects propagation being stopped, so in 5a32acf I changed to test whether the editor was removed after the merge to determine whether to stop.

aduth · 2017-04-26T13:56:10Z

element/index.js

+ *
+ * @return {Array}             The concatenation
+ */
+export function concatChildren( children1, children2 ) {


I was poking around at this function, and had a couple observations:

I think it'd be easy enough and a nice feature to support a spread value of ...childrens, so two or more set of children can be concatenated

I'm still unclear why toArray need exist. You'd mentioned different values, but in my testing Children.toArray seems to handle most everything thrown at it.

I think we might be able to drop an iteration by creating the return value while iterating children (instead of toArray then map)

Do you think something like this would work?

/** * Concatenate two or more React children objects * * @param {...?Object} childrens Set of children to concatenate * @return {Array} The concatenated value */ export function concatChildren( ...childrens ) { return childrens.reduce( ( memo, children, i ) => { Children.forEach( children, ( child, l ) => { memo.push( cloneElement( child, { key: [ i, l ].join() } ) ); } ); return memo; }, [] ); }

I think it'd be easy enough and a nice feature to support a spread value of ...childrens, so two or more set of children can be concatenated

Great idea!

I've tried your version but it's not working. I'm finding the toArray to be necessary. I'm unsure if it may be a problem coming from another place (parser or anything), but you can easily reproduce a bug by backspacing from the start of the second test block "I imagine..."

Huh, yeah, I can reproduce that. Seems to be an issue at the point of calling cloneElement on the string though. This variation seems to work okay for me:

/** * Concatenate two or more React children objects * * @param {...?Object} childrens Set of children to concatenate * @return {Array} The concatenated value */ export function concatChildren( ...childrens ) { return childrens.reduce( ( memo, children, i ) => { Children.forEach( children, ( child, j ) => { if ( 'string' !== typeof child ) { child = cloneElement( child, { key: [ i, j ].join() } ); } memo.push( child ); } ); return memo; }, [] ); }

- Adds a `merge` function to the block API

youknowriad · 2017-04-27T08:36:13Z

@aduth Updated the concatChildren. I'm merging to move forward the other blocked PRs.

youknowriad added [Feature] Blocks Overall functionality of blocks Framework Issues related to broader framework topics, especially as it relates to javascript labels Apr 20, 2017

youknowriad self-assigned this Apr 20, 2017

youknowriad requested review from aduth, ellatrix and nylen April 20, 2017 11:02

youknowriad force-pushed the add/merge-api/backspace branch from caae7d8 to 5f405c4 Compare April 20, 2017 16:33

aduth reviewed Apr 21, 2017

View reviewed changes

youknowriad force-pushed the add/merge-api/backspace branch from 5f405c4 to 188ef3f Compare April 21, 2017 14:40

youknowriad mentioned this pull request Apr 24, 2017

Quote Block: Support Block Switcher #484

Merged

aduth reviewed Apr 24, 2017

View reviewed changes

youknowriad force-pushed the add/merge-api/backspace branch from 188ef3f to 3a03903 Compare April 24, 2017 13:52

youknowriad commented Apr 24, 2017

View reviewed changes

ellatrix reviewed Apr 24, 2017

View reviewed changes

youknowriad force-pushed the add/merge-api/backspace branch from 3a03903 to 37cd1e0 Compare April 24, 2017 13:57

ellatrix reviewed Apr 24, 2017

View reviewed changes

aduth reviewed Apr 24, 2017

View reviewed changes

youknowriad force-pushed the add/merge-api/backspace branch from c2ff037 to 78b6c9a Compare April 25, 2017 08:45

youknowriad mentioned this pull request Apr 25, 2017

Blocks: Handle forward deletion merge #506

Closed

aduth reviewed Apr 26, 2017

View reviewed changes

youknowriad and others added 8 commits April 27, 2017 09:21

Blocks: Handle the backspace at the beginning of blocks

5e4945d

- Adds a `merge` function to the block API

replacing childNodes[ 0 ] with firstChild

7106128

Element: Rename concatChildren helper and add unit tests

d5fc22a

Style: avoid ternary and fallback using ||

f0c34ba

Fix merging two consecutive empty headings

ae4f41a

Fix TinyMCE isCollapsed error when merging two blocks

15b70c1

Stop keydown event propagation if editor removed by merge

129ffd7

Update concatChildren to accomodate multiple children

1b360dc

youknowriad force-pushed the add/merge-api/backspace branch from ff4ee99 to 1b360dc Compare April 27, 2017 08:23

ConcatChildren: Fix unit tests

a2c1065

youknowriad merged commit eb4cb05 into master Apr 27, 2017

youknowriad deleted the add/merge-api/backspace branch April 27, 2017 08:36

aduth mentioned this pull request Mar 29, 2019

Keep styling after spliting a block #14464

Closed

9 tasks

Blocks: Handle pressing backspace at the beginning of blocks #461

Blocks: Handle pressing backspace at the beginning of blocks #461

Conversation

youknowriad commented Apr 20, 2017 • edited Loading

youknowriad commented Apr 21, 2017

aduth left a 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

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

aduth commented Apr 21, 2017

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

aduth commented Apr 24, 2017

youknowriad commented Apr 24, 2017

aduth commented Apr 24, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ellatrix Apr 24, 2017 • 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

Choose a reason for hiding this comment

youknowriad Apr 24, 2017 • 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

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

youknowriad commented Apr 25, 2017

aduth commented Apr 25, 2017

youknowriad commented Apr 25, 2017

aduth commented Apr 25, 2017

aduth commented Apr 25, 2017

youknowriad commented Apr 25, 2017

aduth commented Apr 25, 2017

youknowriad commented Apr 26, 2017

aduth commented Apr 26, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

youknowriad commented Apr 27, 2017

youknowriad commented Apr 20, 2017 •

edited

Loading

ellatrix Apr 24, 2017 •

edited

Loading

youknowriad Apr 24, 2017 •

edited

Loading