Block Editor: Assign Provider isSyncingOutcomingValue only when blocks changing

[aduth]

Fixes #14950

This pull request seeks to resolve an issue where Undo may not behave as expected in response to creation of explicit undo levels. Additional debugging is available at #14950 (comment) and #14950 (comment) .

Implementation Notes:

As a controlled input, the BlockEditorProvider component assigns an instance variable to avoid resetting its own state when it knows blocks values are changing (when it is the source of those changes). However, because this was also assigned in response to the creation of an explicit persistence level, even when blocks did not change, it wasn't always correctly unset. The reason for this is that while normally the componentDidUpdate is called immediately after the value is assigned, because the blocks value doesn't actually change, it is not unassigned until the next change occurs. For this reason, by the time the user presses Undo, the instance value is still assigned and it (wrongly) skips resetting the blocks state with value reflecting the Undo.

The changes here simply assign the instance value only if blocks are actually changing, to better represent its intention to account for an expected componentDidUpdate in response to its own changed blocks state.

Testing Instructions:

Repeat Steps to Reproduce from #14950, verifying the Undo result is correct.

Ensure end-to-end tests pass:

npm run build && npm run test-e2e packages/e2e-tests/specs/undo.test.js

[ellatrix]

Does it matter whether or not the input happens at the start or end?

[aduth]

Does it matter whether or not the input happens at the start or end?

I guess it doesn't matter. More on the point of the actual interaction of appending " World" to make "Hello World" 🤷‍♂️

[ellatrix]

Looks good in my testing. I also confirmed that the e2e test would fail when running against master.

[ellatrix]

There are two failing e2e tests though.

[aduth]

I'm struggling with the E2E failures. They pass for me locally, and the specific failures don't really make a whole lot of sense (though relevant in being "Undo" failures). I thought maybe something with the setTimeout and a race condition. Tried some weird hacks to try to monkey-patch the timeout to flush them immediately, but doesn't seem to work:

await page.evaluate( () => {
	let id = 0;

	window.timeouts = [];
	window.setTimeout = ( fn ) => {
		window.timeouts[ id ] = fn;
		return id++;
	};

	window.clearTimeout = ( idToRemove ) => {
		delete window.timeouts[ idToRemove ];
	};
} );

// ...

await page.evaluate( () => {
	window.timeouts.forEach( ( fn ) => fn() );
} );

Going to try restarting it once more. Not great obviously to have intermittent failures in tests being introduced, but curious just to learn whether it is flakey, or reliably failing.

[aduth]

Possible issue here being that we don't actually wait for the save to happen. I'm going to swap this out with using createNewPost to provide the content.

[aduth]

The fact that failures occur unpredictably in the middle of the revised texts might seem to indicate that some unexpected state change is occurring outside the user input to disrupt the state flattening behavior, which compares only the current and previous actions.

    Expected: "original"
    Received: "modoriginal"

My guess is this is the fetch of reusable blocks, similar to what was seen with #14766. It may in-fact be resolved by #14915. I might try to cherry-pick that commit into this branch temporarily just to see if it makes a difference.

[aduth]

There was one intermittent build failure after the cherry-picked commit for an unrelated test suite (block-deletion.test.js), but it's passing now the second time around.

In retrospect, this makes sense: When the reusable blocks are received, it causes a change in state. #14916 helped to ensure that this change isn't surfaced to the editor to be considered in creating an undo level or in marking the post as dirty, but for the block editor's own comparison of subsequent actions, the receive action will "break the chain" of updating the same block attribute.

I sense this is likely the reason the Undo end-to-end tests have been generally unstable lately.

There's a trade-off, but I'm generally inclined to merge #14915 and then rebase and merge this pull request. The trade-off is:

• With Editor: Trigger reusable blocks autocomplete when options generated #14915, autocomplete won't fetch (and won't display results of) reusable blocks until the user enters the slash character. It's likely those results would be available by the time they finish typing their search query, and technically the same issue exists already, except that the fetch is triggered immediately upon focus of the paragraph block, so the likelihood of the race condition is lowered (but still present).
• Without Editor: Trigger reusable blocks autocomplete when options generated #14915, this "breaking the chain" behavior will persist, meaning that Undo levels may be unpredictable not just for tests, but also affecting users, in response to merely focusing the paragraph block (and the subsequent receiving of reusable blocks).

As noted at #14910 (comment), the long-term solution for this is at least partly reflected in #7119, where reusable blocks state ideally does not need to coexist in the top-level block editor state.

The alternative is: This pull request is safe/correct on its own, but the tests are not stable. Therefore, we could opt to not merge #14915, then simply remove the tests here but merge the remainder of changes to resolve #14950.

[aduth]

I rebased the branch to the minimal, safe fix of #14950.

If following #14955 (comment) , #14915 can be merged, then tests cherry-picked back in from commit b99908f.

[youknowriad]

Thanks for the fix

[aduth]

If following #14955 (comment) , #14915 can be merged, then tests cherry-picked back in from commit b99908f.

#14915 was since merged, so I proposed a follow-up pull request at #15049 to introduce the test case.