Undo / Redo inconsistencies when using mouse or keyboard

[afercia]

While investigating on #5468 I've noticed inconsistencies in the way Undo and Redo work, depending if using the mouse or the keyboard. Seems to me this feature is a bit fragile and prone to errors and probably worth some investigation.

I've tried to reduce the inconsistencies I've noticed to a few cases, hopefully easy to reproduce;

1
Undo / Redo using only the mouse:

• add a new post
• click on the "Add title" field and type some text
• select the title text with the mouse and press backspace to delete it
• click on the "Write your story" field and type some text
• click the empty area below (the "sibling inserter") to add a second paragraph and type some text
• click on an empty area to deselect the second paragraph

Now click Undo multiple times through all the history steps and carefully observe what happens:

• everything seems to work, even the previously deleted post title gets restored at some point
• however there's the need to click "Undo" more times than expected: sometimes seems clicking Undo doesn't do anything
• expected: at each "Undo" click, there should be a visible undo step, whether it's removing text or removing a block; instead, for some Undo steps, the UI doesn't show any change

Now click Redo multiple times:

• the post title gets restored and removed again (correct)
• the paragraphs text doesn't get restored
• expected: the paragraphs text to be restored

2
Undo / Redo using only the keyboard:

• add a new post
• just to speed up testing, click on the "Add title" field and type some text
• select the title text with Cmd/Ctrl+A and press backspace to delete it
• press Tab and type some text in the "Write your story" field
• press Enter to create a second paragraph and type some text
• press Tab a few times to tab away from the content area

Now click Undo multiple times:

• as with the mouse, there are some Undo steps that apparently don't do anything
• the Post title doesn't get restored (notice this is different from when using the mouse)
• expected: at some point, the Post title should be restored (and then removed again)

Now click Redo:

• the Post title doesn't get restored
• only the first paragraph text gets restored
• expected: the second paragraph to be restored
• the first paragraph placeholder is visible, so the paragraph text and placeholder are both visible on two layers:
  

3
Using Cmd/Ctrl+z and Cmd/Ctrl+Shift+z:
Instead of clicking Undo and Redo, using the keyboard shortcuts generally produces the same inconsistencies except for:

• when redoing, this time also the second paragraph gets restored
• even in this case, paragraph text and placeholder are both visible:
  

Sorry I can't help further, I don't have the knowledge to understand how Undo/Redo exactly works. If I had to guess, I'd suspect history steps are saved differently when using the mouse or the keyboard. Not sure this can be fully fixed, given how the UI works. However, it would be nice to investigate on some of these cases, especially the ones that are potentially very confusing for users.

[afercia]

For reference: tested on current master 8434cc9 a few days after release 2.4.0.

[gziolo]

I encountered a similar issue when working on improvements for e2e tests in #8301:

It happens only in the headless mode when running e2e tests. Sometimes it creates two undo levels for the first paragraph. When you modify test and add snapshots after each undo action you might see the following:

exports[`undo Should undo to expected level intervals 5`] = `
“<!-- wp:paragraph -->
<p>This</p>
<!-- /wp:paragraph -->”
`;

exports[`undo Should undo to expected level intervals 6`] = `
“<!-- wp:paragraph -->
<p>Thi</p>
<!-- /wp:paragraph -->”
`;

This isn't expected behavior. It uncovers some edge case happening inside the logic which creates undo levels.

Test which might allow to reproduce it: https://github.com/WordPress/gutenberg/blob/master/test/e2e/specs/undo.test.js

You can run it as follows:
PUPPETEER_HEADLESS=false npm run test-e2e test/e2e/specs/undo.test.js

[oldrup]

Forgive me not being more specific, but let me just add, that Ctrl-Z, Ctrl-Y rarely gives me the expected results. Using Gutenberg 3.4 - on production site, woop woop! ;)

[gziolo]

gutenberg/packages/editor/src/components/rich-text/index.js

Lines 403 to 423 in 7447194

	onCreateUndoLevel( event ) {
	// TinyMCE fires a `change` event when the first letter in an instance
	// is typed. This should not create a history record in Gutenberg.
	// https://github.com/tinymce/tinymce/blob/4.7.11/src/core/main/ts/api/UndoManager.ts#L116-L125
	// In other cases TinyMCE won't fire a `change` with at least a previous
	// record present, so this is a reliable check.
	// https://github.com/tinymce/tinymce/blob/4.7.11/src/core/main/ts/api/UndoManager.ts#L272-L275
	if ( event && event.lastLevel === null ) {
	return;
	}
	// Always ensure the content is up-to-date. This is needed because e.g.
	// making something bold will trigger a TinyMCE change event but no
	// input event. Avoid dispatching an action if the original event is
	// blur because the content will already be up-to-date.
	if ( ! event || ! event.originalEvent || event.originalEvent.type !== 'blur' ) {
	this.onChange();
	}
	this.context.onCreateUndoLevel();
	}

@iseulde is it possible that this thing forces editor to create another undo level even when new block wasn't created or blur didn't happen?

In the e2e tes (test/e2e/specs/undo.test.js) it happens the following from time to time:

1. New undo level is created when block are received.
2. New undo level is created when first block is selected.
3. When typing test, sometime undo level is create after I type tes and another one when I hit enter and new block gets created.

When test fails, this is always the reason.

[ellatrix]

I think we should remove the change event handler and replace it with our own logic so we have complete control over it. So that means: deregistering built in shortcuts and reregister them to use our own formatting methods, then create history records for any formatting change and any clicks in the editor (click = typing interruption), and maybe some other things like the ENTER key etc. Planning to have a look at this.

[ellatrix]

Should be fixed after #10650. Please reopen if this is not the case. :)

[afercia]

I see the behavior has improved a lot with regards to RichText, thanks! However, following the steps above, the post title is not restored. Depending on the flow, seems the title is not included in the undo/redo history also in other cases.

GIF:

Also, I understand the switch from the default (empty) block to a paragraph is included in the history, but maybe that could be potentially confusing for users.

[ellatrix]

Yes, I think only blocks are included in the history reducers. Could you make a new issue for this? It seems unrelated to this one.

[aduth]

Yes, I think only blocks are included in the history reducers

Edits (including title) are included.

[ellatrix]

Could you make a new issue for this? It seems unrelated to this one.

This issue has been created: #12075.