Fix autosave while editing a post using the Text Mode editor

[noisysocks]

Description

Fixes #2978.

Text that was typed into the Text Mode editor would be wiped away if the post was autosaved. Here's what was happening:

1. A user switches to Text Mode and makes changes. The modified content is stored in editor.present.edits.content. Note that editor.present.blocksByUid is not modified until the text area is blurred.
2. After some time, the post is autosaved. When the successful save request finishes, REQUEST_POST_UPDATE_SUCCESS is dispatched.
3. REQUEST_POST_UPDATE_SUCCESS dispatches RESET_POST, which clears out the edits in editor.present.edits.
4. Now that editor.present.edits.content is undefined, getEditedPostContent( state ) returns a serialisation of the blocks in editor.present.blocksByUid. But, since the editor was never blurred, this does not reflect the recent changes that were made! We therefore lose those changes.

I've fixed this by patching PostTextEditor so that it copies value into local state when focused. This means that the contents of the text field won't change if value changes because of a dispatch.

How Has This Been Tested?

1. Write a new post in Gutenberg
2. Switch to Text Mode
3. Type in a bunch of stuff
4. Wait for autosave to kick in—your changes should remain there
5. Refresh the page—your changes should remain there
6. Switch back to Visual Mode—your changes should appear

[noisysocks]

This fixes the bug but I find it a bit weird that there are moments where editor.present.blocksByUid does not reflect what's currently stored in currentPost.content.

I'm considering changing PostTextEditor so that it calls resetBlocks( parse( content ) ) when the text field is changed instead of blurred. This way, the two subtrees of our editor state remain in sync. Debouncing the change event should keep the editor feeling responsive.

Thoughts?

[youknowriad]

I'm considering changing PostTextEditor so that it calls resetBlocks( parse( content ) )

Yes, this is a hard problem to solve with constant syncing. We tried to avoid this for performance reasons. Maybe it's fine I don't know, we have to try for long posts as well. cc @aduth

[aduth]

Previously it would be an issue because parsing an incomplete text would probably alter the serialized output of the reset blocks. Using state to ensure that the textarea value isn't clobbered should remedy this as far as the user is concerned, but the parsed data is probably still often out of sync with what the user is typing.

[aduth]

FWIW I find these sorts of function names aren't very informative and encourage developers to not clearly separate the responsibilities of functions. As an outsider looking at this function later, I'd have no idea what it's intending to do, at least not without reading through and interpreting the logic (and who has time for that?). If it were named setStateValue with a DocBlock explaining how we track a local copy of the value to avoid clobbering edits from global state, I'd feel much more informed.

There's a few places where I've tried to set more explicit patterns for some of these common behaviors. One in particular is handling onKeyDown, since we usually want distinct functionality to take place on individual key codes. While maybe a bit extreme (especially since it only handles as a single key code at the moment), NavigableToolbar is one such example:

gutenberg/editor/components/navigable-toolbar/index.js

Lines 32 to 34 in 4bac4fe

	this.switchOnKeyDown = cond( [
	[ matchesProperty( [ 'keyCode' ], ESCAPE ), this.focusSelection ],
	] );

gutenberg/editor/components/navigable-toolbar/index.js

Lines 51 to 73 in 4bac4fe

	/**
	* Programmatically shifts focus to the element where the current selection
	* exists, if there is a selection.
	*/
	focusSelection() {
	// Ensure that a selection exists.
	const selection = getSelection();
	if ( ! selection ) {
	return;
	}
	// Focus node may be a text node, which cannot be focused directly.
	// Find its parent element instead.
	const { focusNode } = selection;
	let focusElement = focusNode;
	if ( focusElement.nodeType !== Node.ELEMENT_NODE ) {
	focusElement = focusElement.parentElement;
	}
	if ( focusElement ) {
	focusElement.focus();
	}
	}

gutenberg/editor/components/navigable-toolbar/index.js

Line 83 in 4bac4fe

onKeyDown={ this.switchOnKeyDown }

(And yes, arguments can be made that _.cond is not a very obvious / readable utility, but better abstractions could exist. The general point stands all the same)

[noisysocks]

👍 noted. I've moved away from 'handle-' method names in c4035be.

[aduth]

Do we need to track isDirty? Can't we just check if ( this.state.value !== this.props.value ) { ?

[noisysocks]

Since we change the global value by dispatching EDIT_POST in our onChange handler,
this.state.value !== this.props.value will always evaluate to false.

[noisysocks]

Using state to ensure that the textarea value isn't clobbered should remedy this as far as the user is concerned, but the parsed data is probably still often out of sync with what the user is typing.

Makes sense. Let's stick with this approach for now, then.

[noisysocks]

Are we good to go ahead and get this in?

[youknowriad]

What about external updates, in the current implementation they are ignored completely, I think an option could be:

• If we receive new content and the editor is not dirty, replace the state value if existant

• If we receive new content and the editor is dirty, an option would be to compare the values and if different show a notice saying if you want to revert changes (new external updates) you can.

The second point can probably be done later, I see its value especially for plugins that could update the content independently from the UI. Granted we don't have this use case right now.

[noisysocks]

If we receive new content and the editor is not dirty, replace the state value if existant

I've implemented this in af209c2 and tested it by:

1. Running this in the DevTools console:

setTimeout( () => { wp.data.dispatch( 'core/editor' ).editPost( { content: '<!-- wp:paragraph -->\n<p>hey there! How are you?</p>\n<!-- /wp:paragraph -->\n\n<!-- wp:paragraph -->\n<p>Pretty well, thank you</p>\n<!-- /wp:paragraph -->' } ) }, 2000 )

2. Quickly focusing the text area with my mouse. If I don't type anything, the contents will change after 2 seconds

The second point can probably be done later, I see its value especially for plugins that could update the content independently from the UI. Granted we don't have this use case right now.

I agree that this can be done later since it's not (yet) common that the post is edited externally.

Thanks for reviewing! ❤️