Merely selecting a block after load creates undo level

[ellatrix]

To Reproduce

1. Create a new post.
2. Create a new paragraph with some text.
3. Save the post.
4. Reload the page.
5. Select the paragraph block.
6. Observe that the undo button is not disabled any longer.

Expected behavior

I don't expect block selection to create undo levels.

Additional context

I see the following actions when I select a block:

{type: "FETCH_REUSABLE_BLOCKS", id: undefined}
{type: "SELECT_BLOCK", initialPosition: null, clientId: "adb9ef30-61f7-45c3-b785-01fc5ca630e3"}
{type: "RECEIVE_REUSABLE_BLOCKS", results: Array(1)}
{type: "UPDATE_SETTINGS", settings: {…}}
{type: "RECEIVE_BLOCKS", blocks: Array(1)}
{type: "RESET_EDITOR_BLOCKS", blocks: Array(1), shouldCreateUndoLevel: false}
{type: "FETCH_REUSABLE_BLOCKS_SUCCESS", id: undefined}

So it looks like it has something to do with reusable blocks fetching? Cc @noisysocks @aduth.

[ellatrix]

The action that grabs my attention is RESET_EDITOR_BLOCKS. Why are blocks being reset after block selection? The user didn't do anything that should trigger a reset?

[aduth]

I'd need to spend some time investigating to understand, but on hunch I think it's related to one of either reusable blocks triggering RECEIVE_BLOCKS, or as part of the refactor of a separate block editor module and how it treats "persistent" changes.

gutenberg/packages/block-editor/src/store/reducer.js

Lines 178 to 240 in 495f54c

	/**
	* Higher-order reducer intended to augment the blocks reducer, assigning an
	* `isPersistentChange` property value corresponding to whether a change in
	* state can be considered as persistent. All changes are considered persistent
	* except when updating the same block attribute as in the previous action.
	*
	* @param {Function} reducer Original reducer function.
	*
	* @return {Function} Enhanced reducer function.
	*/
	function withPersistentBlockChange( reducer ) {
	let lastAction;
	/**
	* Set of action types for which a blocks state change should be considered
	* non-persistent.
	*
	* @type {Set}
	*/
	const IGNORED_ACTION_TYPES = new Set( [
	'RECEIVE_BLOCKS',
	] );
	return ( state, action ) => {
	let nextState = reducer( state, action );
	const isExplicitPersistentChange = action.type === 'MARK_LAST_CHANGE_AS_PERSISTENT';
	// Defer to previous state value (or default) unless changing or
	// explicitly marking as persistent.
	if ( state === nextState && ! isExplicitPersistentChange ) {
	return {
	...nextState,
	isPersistentChange: get( state, [ 'isPersistentChange' ], true ),
	};
	}
	// Some state changes should not be considered persistent, namely those
	// which are not a direct result of user interaction.
	const isIgnoredActionType = IGNORED_ACTION_TYPES.has( action.type );
	if ( isIgnoredActionType ) {
	return {
	...nextState,
	isPersistentChange: false,
	};
	}
	nextState = {
	...nextState,
	isPersistentChange: (
	isExplicitPersistentChange ||
	! isUpdatingSameBlockAttribute( action, lastAction )
	),
	};
	// In comparing against the previous action, consider only those which
	// would have qualified as one which would have been ignored or not
	// have resulted in a changed state.
	lastAction = action;
	return nextState;
	};
	}

Clicking on a paragraph triggers autocomplete to do the fetch for reusable blocks so that it can populate results in the slash command.

[aduth]

I seem to be observing that that this "dirties" the post as well, which was intended to be explicitly not the case with the implementation of #13088 . If confirmed, I'd suggest it be prioritized for a fix to be included with the WordPress 5.2 release. As a regression, it should be reasonable to expect and to implement a test as part of the change-detection.test.js suite.

[aduth]

There may be separate issues here between how the RESET_EDITOR_BLOCKS impacts undo history and how it impacts change detection.

For the original issue describing undo history, this may in-fact be the expected behavior, or at least there's a specific condition which forces that an undo history entry be created when history is currently empty, even if the action (RESET_EDITOR_BLOCKS) wouldn't otherwise create one.

gutenberg/packages/editor/src/utils/with-history/index.js

Lines 122 to 129 in bb3b77a

	if (
	shouldCreateUndoLevel ||
	! past.length ||
	! shouldOverwriteState( action, previousAction )
	) {
	nextPast = [ ...past, present ];
	lastActionToSubmit = action;
	}

...specifically the ! past.length condition, introduced with #4956, including some discussion at #4956 (comment) .

---

For change detection, it may be a matter of configuring withChangeDetection to either reset at or explicitly ignore the RESET_EDITOR_BLOCKS action:

gutenberg/packages/editor/src/store/reducer.js

Lines 144 to 148 in bb3b77a

	// Track whether changes exist, resetting at each post save. Relies on
	// editor initialization firing post reset as an effect.
	blocks: withChangeDetection( {
	resetTypes: [ 'SETUP_EDITOR_STATE', 'REQUEST_POST_UPDATE_START' ],
	} )( ( state = { value: [] }, action ) => {

Looking at the implementation prior to changes in #13088, we seemed to include at least a few ignoreTypes:

gutenberg/packages/editor/src/store/reducer.js

Lines 411 to 416 in 4be0a36

	// Track whether changes exist, resetting at each post save. Relies on
	// editor initialization firing post reset as an effect.
	withChangeDetection( {
	resetTypes: [ 'SETUP_EDITOR_STATE', 'REQUEST_POST_UPDATE_START' ],
	ignoreTypes: [ 'RECEIVE_BLOCKS', 'RESET_POST', 'UPDATE_POST' ],
	} ),

It's not immediately clear to me why these were dropped (cc @youknowriad ).

[aduth]

...specifically the ! past.length condition, introduced with #4956, including some discussion at #4956 (comment) .

As an action item, I might propose to consider whether the ! past.length condition could be removed or, at the very least, updated to force the next action to behave as to merge ("overwrite") state. This latter approach doesn't resolve the issue that if, immediately after selecting the block, the user were to press Undo, the RESET_EDITOR_BLOCKS action would be inadvertently undone.

Looking at the implementation prior to changes in #13088, we seemed to include at least a few ignoreTypes: [...] It's not immediately clear to me why these were dropped

I will plan to explore a solution here, specifically seeking to restore the removed ignoreTypes.

[aduth]

One possible action which can incidentally serve as a solution: Rather than trigger to fetch reusable blocks immediately upon block selection, the fetch should only occur after some initial typing (i.e. when the autocomplete results are presented). This would help avoid the scenario where the fetch results are the first action dispatched to the editor store.

[aduth]

Looking at the implementation prior to changes in #13088, we seemed to include at least a few ignoreTypes: [...] It's not immediately clear to me why these were dropped

I will plan to explore a solution here, specifically seeking to restore the removed ignoreTypes.

Spending more time looking at this, I understand now why ignoreTypes is no longer applied for this action type, since from the editor state, REPLACE_EDITOR_BLOCKS is the sole way it tracks blocks and changes in this value are necessarily tracked to support the unsaved changes prompt.

I'd wondered then if it would be worth tying this to the shouldCreateUndoLevel property of the action dispatched. While this is workable, it doesn't seem the appropriate solution given that the property corresponds to onInput callbacks from BlockEditorProvider, something which seems a change worth considering (a branch with this proposal can be seen at fix/reset-editor-block-change-detection).

Rather, it seems the issue is that we shouldn't expect RESET_EDITOR_BLOCKS to ever occur in editor as a result of the reusable blocks being received. The way this seems to be happening is that receiving the reusable blocks triggers a receiveBlocks action in the block-editor module, where presumably this should be treated as effectively equivalent to a "syncing incoming value" operation and not bubble back up to editor by its onChange or onInput (but it does).

[aduth]

While related, I've split off the problem of change detection to its own issue at #14910, which I've discovered to be affecting more flows than just the block selection (opening an inserter or editing a post containing a reusable block can also trigger the prompt).

[aduth]

One of #14915 and #14916 should resolve this; the latter being the more "durable" solution.