WP 5.3b2 / GB 6.6: changing + saving same inspector setting (diff. values) twice is no longer possible

[bvlgn]

Describe the bug
If a user changes the same block setting twice in a row with saving the page in between, the second time the editor won't allow the user to press the Update button (it is in disabled state).
So the second (etc) changes using through the same control won't put the editor in dirty mode.

To reproduce
Steps to reproduce the behavior on WP 5.3 Beta 2 or Gutenberg plugin v6.6:

1. Add add and publish a test page with for example a test paragraph.
2. Select the paragraph
3. Change font-size to large
4. Press the Update button
5. Change font-size to huge
6. Now you see the issue: The Update button is inactive, you cannot save your last change (unless you make another change through another control, e.g. toggle Drop Cap)

Expected behavior
As with WP 5.2: you can change any setting as many times in a row as you want, with the ability to press Update every time.

[mcsf]

E2E test to catch this:

commit a4f32490eebc7cf6a644afb50d37fa899f2a8164
Author: Miguel Fonseca <[email protected]>
Date:   Wed Oct 9 16:13:20 2019 +0100

    Tests: Expect dirty after saving and changing same attribute

diff --git a/packages/e2e-tests/specs/change-detection.test.js b/packages/e2e-tests/specs/change-detection.test.js
index 0f384d7c3..02c5a1ca6 100644
--- a/packages/e2e-tests/specs/change-detection.test.js
+++ b/packages/e2e-tests/specs/change-detection.test.js
@@ -6,6 +6,7 @@ import {
 	createNewPost,
 	pressKeyWithModifier,
 	ensureSidebarOpened,
+	openDocumentSettingsSidebar,
 	publishPost,
 } from '@wordpress/e2e-test-utils';
 
@@ -204,6 +205,50 @@ describe( 'Change detection', () => {
 		expect( hadInterceptedSave ).toBe( false );
 	} );
 
+	it( 'Should mark consecutive edits of a same attribute dirty if there was a save in between', async () => {
+		await clickBlockAppender();
+		await page.keyboard.type( 'Hello, World' );
+
+		// Save and wait for "Saved" to confirm save complete.
+		await Promise.all( [
+			page.waitForSelector( '.editor-post-saved-state.is-saved' ),
+			pressKeyWithModifier( 'primary', 'S' ),
+		] );
+
+		// Assert clean editor
+		expect( await page.$( '.editor-post-saved-state.is-saved' ) )
+			.toBeTruthy();
+
+		// Open the sidebar, switch to block settings
+		await openDocumentSettingsSidebar();
+		await page.click( '.edit-post-sidebar__panel-tab[data-label="Block"]' );
+
+		// Make sure our paragraph is selected
+		await page.click( '[data-type="core/paragraph"]' );
+
+		// In the block's inspector controls, increase the font size
+		await page.select( '.components-select-control__input', 'large' );
+		await page.click( '[data-type="core/paragraph"]' );
+
+		// Assert dirtiness
+		expect( await page.$( '.editor-post-saved-state.is-saved' ) )
+			.toBeNull();
+
+		// Save and wait for "Saved" to confirm save complete.
+		await Promise.all( [
+			page.waitForSelector( '.editor-post-saved-state.is-saved' ),
+			pressKeyWithModifier( 'primary', 'S' ),
+		] );
+
+		// Increase the font size one more level
+		await page.select( '.components-select-control__input', 'larger' );
+		await page.click( '[data-type="core/paragraph"]' );
+
+		// Assert dirtiness
+		expect( await page.$( '.editor-post-saved-state.is-saved' ) )
+			.toBeNull();
+	} );
+
 	it( 'Should prompt if save failed', async () => {
 		await page.type( '.editor-post-title__input', 'Hello World' );

[determin1st]

It works with WP 5.2 + GB 6.6

[bvlgn]

It works with WP 5.2 + GB 6.6

I just tested it again on a new install with WP 5.2.3 with GB 6.6.0 as the only active plugin.
And for me it again didn't work, the bug was as outlined above.

@determin1st did your follow all the steps to reproduce the problem completely, including step 4?

[determin1st]

@BennyVL

I think so.. In my case, Update button never goes to disabled state.
It goes to disabled only when saving.. for very short period of time (im on localhost, chrome v77).

[bvlgn]

But you didn't change any of the Text Settings?
To reproduce the bug you need to change e.g. The Font-size to 'Large', click [Update], change the Size again for example to 'Huge', see if you can press Update or if it is disabled.

[determin1st]

Of course I've changed them (font size) many times trying to "catch" Update disabled...
It never becomes disabled (only at short period of saving as I said before).
Do you have any error messages in F12 devtools console?

[bvlgn]

It never becomes disabled (only at short period of saving as I said before).

After you click the Update button it should always become disabled. If not that would be a whole other bug?

[bvlgn]

Do you have any error messages in F12 devtools console??

No errors or other messages show in the console.

[mcsf]

Bisecting revealed 4944315 (#16932) as the first bad commit. /cc @epiqueras

[epiqueras]

This follows the logic of this reducer:

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

Lines 341 to 351 in 69aef3f

	/**
	* 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 ) {

RichText/text is the only scenario I can think of where that logic makes sense.

Maybe by default everything should be persistent and RichText can opt out and continue to use this "same attribute" behavior?

[mcsf]

RichText/text is the only scenario I can think of where that logic makes sense.

If I understood you correctly, then I think the defined behaviour is fine, and could apply in more places than RichText.

The thing is that a save should reset the state and consider the next changes meaningful.

In other words, this is a bug and I don't think the way to approach it is to change the interface:

Maybe by default everything should be persistent and RichText can opt out and continue to use this "same attribute" behavior?

/cc @ellatrix, if you have thoughts

[epiqueras]

I was saying that it doesn't make sense for anything other than RichText. It makes sense that if you type in a sequence, only certain breakpoints are "persistent" and create undo levels, but when changing a radio option, you probably want all of the changes to be "persistent".

We didn't have the bug described in this issue before, because all changes dirtied the post and "persistent" just meant the edit should create an undo level, but after #16932 it started encompassing whether it should dirty the post or not as well.

The fix of considering the next changes after a save "persistent" won't solve the issue that things like changing the same radio option back to back does not create multiple "persistent" edits with undo levels, but I guess that might not necessarily be a bad thing as you could argue that you want to be able to undo that entire sequence of changes in one go, because toggles/radios are prone to be tested/switched between a lot.

[epiqueras]

Here's the fix: #17888

[mcsf]

I was saying that it doesn't make sense for anything other than RichText. It makes sense that if you type in a sequence, only certain breakpoints are "persistent" and create undo levels, but when changing a radio option, you probably want all of the changes to be "persistent".

I can imagine other controls where you wouldn't want to persist everything: a very granular slider (think colour gradients), or a multi-checkbox field listing many taxonomy terms that you want to select. After ticking a batch of boxes, I wouldn't expect Undo to take me through all my previous ticking steps; idem for the gradient slider.

We may not have all the use cases upfront, but conceptually it does seem to me like something that could interest more consumers than RichText. /cc @mtias

[epiqueras]

@mcsf You are right and those are great examples. I think I am convinced that this is a sensible default. Should we maybe discuss the potential of allowing attributes to opt out using a property in the block settings? I think the majority of attributes will be fine with this behavior, but some will want to opt out.