Fix undo on paragraph split by jorgefilipecosta · Pull Request #6964 · WordPress/gutenberg

jorgefilipecosta · 2018-05-25T20:50:20Z

Description

Splitting was not a single operation but two operations insert blocks + setAttributes. That made undo behave in a strange way.
In this PR we make onSplit a single operation using onReplace (props to @iseulde for this idea).
onReplace by default selects the first block. That was not what we wanted in this use case so a new prop was added that allows choosing the bock we want to select.
To accomplish this onReplace was changed to not clone the blocks because cloning changes the UID.
I think we should remove clone from onReplace anyway even if we find an alternative approach to the PR. When a user asks for some blocks to be replaced I think the expectation is that the new blocks the user set are the used ones and the UID's saved in the state are the same that the user passed. In PR #6325 I felt the same need and I applied the same change to onReplace.

We did not added more test cases yet if we find this approach something we want to pursue I will add them.

How has this been tested?

Add a paragraph write some text, press enter in the middle, see the paragraphs were divided. Press undo see that undo works as expected and the paragraphs are joined again.
Try to do some testing to verify removing clone fro onReplace does not cause unexpected behaviors.

Screenshots

Before:

After:

ellatrix · 2018-05-28T11:55:26Z

Seeing an error on splitting:

wp-tinymce.php?ver=4.9.6:2 Uncaught TypeError: Cannot read property 'startContainer' of null
    at Xy (wp-tinymce.php?ver=4.9.6:2)
    at Yy (wp-tinymce.php?ver=4.9.6:2)
    at Object.getStart (wp-tinymce.php?ver=4.9.6:2)
    at Fh (wp-tinymce.php?ver=4.9.6:2)
    at YC.<anonymous> (wp-tinymce.php?ver=4.9.6:2)
    at zm.c.fire (wp-tinymce.php?ver=4.9.6:2)
    at YC.fire (wp-tinymce.php?ver=4.9.6:2)
    at Object.t (wp-tinymce.php?ver=4.9.6:2)
    at C (wp-tinymce.php?ver=4.9.6:2)
    at HTMLParagraphElement.d (wp-tinymce.php?ver=4.9.6:2)

Otherwise I think this is looking nice.

ellatrix · 2018-05-28T11:56:02Z

editor/store/actions.js

 * @return {Object} Action object.
 */
-export function replaceBlocks( uids, blocks ) {
+export function replaceBlocks( uids, blocks, blockToSelectUid ) {


This looks good to me but would love to know what @youknowriad thinks. :)

youknowriad · 2018-05-28T12:15:56Z

editor/components/block-list/block.js

+				{ ...block, layout }
 			) );
-			replaceBlocks( [ ownProps.uid ], blocks );
+			replaceBlocks( [ ownProps.uid ], blocks, blockToSelectUid );


Why don't we just call selectBlock or something like that right after? It seems weird to tie the replacement of blocks with selection?

Now the action object contains the property selectBlock, unfortunately, I was not able to use it in the parameter name because we already have a function with that name.

I think you misunderstood my remark here. For me we shouldn't add this new parameter here and just call another action right after selectBlock( blockToSelectUid ).

Hi @youknowriad I missed your suggestion at first but now I updated the code to follow it :)

youknowriad · 2018-05-28T12:17:39Z

editor/store/reducer.js

+				get( action.blocks, [ 0, 'uid' ], null );

 			return {
 				...state,


I know it was already the case before but I feel the REPLACE_BLOCKS action should not touch the selection at all unless the currently selected block is being replaced.

Hi @youknowriad, that's exactly the current behavior. We have a condition at the start that guarantees that if the currently selected block is not replaced we don't change the selection:

if ( action.uids.indexOf( state.start ) === -1 ) { return state; }

youknowriad · 2018-05-28T12:19:43Z

core-blocks/paragraph/index.js

 								}
+								const replaceArrayStart = before ?
+									[ createBlock( name, { ...attributes, content: before } ) ] :
+									[];


There's a subtle difference here with the previous behavior. We're creating a new block with a new UID which resets all other properties of the blocks and cause a remount instead of a rerender. Maybe it's fine, just noting.

That's true we change the uid and we have a remount but I think doing that is ok.

Could we reuse the block that is being replaced in the reducer if the name is the same?

We had already an onReplace in the paragraph with logic to reuse attribute when replacing with the same block I added one line there that also reuses the UID. I think it simplifies the things and removes the need for any TinyMCE hacks.

jorgefilipecosta · 2018-05-29T10:29:11Z

Hi @iseulde,

Seeing an error on splitting:

I added some checks that should avoid this error, thank you for catching it.

@iseulde @youknowriad, thank you for the reviews the feedback was addressed.

ellatrix · 2018-06-04T17:58:14Z

editor/components/rich-text/index.js

 			rect = getRectangleFromRange( this.editor.selection.getRng() );
 		}
 		const focusPosition = this.getFocusPosition( rect );
+		if ( ! focusPosition ) {


When would this fail?

ellatrix · 2018-06-04T17:59:18Z

editor/components/rich-text/index.js

 	 */
 	getFocusPosition( position ) {
+		// if the node was removed or replaced return immediately.
+		if ( ! this.containerRef.current ) {


I'm surprised this even happens. Maybe worth finding out the root cause of this and comment. It might even be the same as #6981.

It was not related to #6981 I added it was a patch and it did not fix the problem here. But now that we reuse the UID this problem does not happen and all the changes to TinyMCE here reverted.

ellatrix · 2018-06-04T18:00:52Z

@jorgefilipecosta Have you tried this? #6964 (comment) I think that would make the code a bit cleaner too. :)

ellatrix · 2018-06-12T17:27:29Z

core-blocks/paragraph/index.js

 	}

-	onReplace( blocks ) {
+	onReplace( blocks, blockToSelect ) {


I see this change only being applied to the paragraph block. Should it be generalised to all blocks that have an onSplit prop, such as headings?

aduth · 2018-09-13T19:04:31Z

Is this still desired to be continued? Is it better represented in the alternative proposal discussed in #8119 ?

jorgefilipecosta · 2018-09-13T19:07:07Z

Closing this in favor of #8119.

jorgefilipecosta added the [Type] Bug An existing feature does not function as intended label May 25, 2018

jorgefilipecosta self-assigned this May 25, 2018

jorgefilipecosta force-pushed the fix/on-split-undo branch from d069e99 to 5f16dca Compare May 25, 2018 20:51

ellatrix reviewed May 28, 2018

View reviewed changes

youknowriad reviewed May 28, 2018

View reviewed changes

jorgefilipecosta force-pushed the fix/on-split-undo branch from 5f16dca to 7cc2438 Compare May 29, 2018 10:21

jorgefilipecosta force-pushed the fix/on-split-undo branch from 7cc2438 to 0004562 Compare June 4, 2018 11:20

ellatrix reviewed Jun 4, 2018

View reviewed changes

Fix undo on paragraph split

9272fd5

jorgefilipecosta force-pushed the fix/on-split-undo branch from 0004562 to 9272fd5 Compare June 5, 2018 15:56

ellatrix reviewed Jun 12, 2018

View reviewed changes

aduth added the [Status] Stale Gives the original author opportunity to update before closing. Can be reopened as needed. label Sep 13, 2018

jorgefilipecosta closed this Sep 13, 2018

jorgefilipecosta deleted the fix/on-split-undo branch September 13, 2018 19:07

Conversation

jorgefilipecosta commented May 25, 2018

Description

How has this been tested?

Screenshots

Uh oh!

ellatrix commented May 28, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

youknowriad May 29, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jorgefilipecosta Jun 5, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jorgefilipecosta commented May 29, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ellatrix commented Jun 4, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aduth commented Sep 13, 2018

Uh oh!

jorgefilipecosta commented Sep 13, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

youknowriad May 29, 2018 •

edited

Loading

jorgefilipecosta Jun 5, 2018 •

edited

Loading