Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
BennyVL opened this issue Oct 8, 2019 · 15 comments · Fixed by #17888

Comments

@BennyVL
Copy link

commented Oct 8, 2019

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

This comment has been minimized.

Copy link
Contributor

commented Oct 9, 2019

E2E test to catch this:

commit a4f32490eebc7cf6a644afb50d37fa899f2a8164
Author: Miguel Fonseca <miguelcsf@gmail.com>
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

This comment has been minimized.

Copy link

commented Oct 9, 2019

It works with WP 5.2 + GB 6.6

@BennyVL

This comment has been minimized.

Copy link
Author

commented Oct 9, 2019

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

This comment has been minimized.

Copy link

commented Oct 9, 2019

@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).

EGCjTMhWwAYR4PK

EGCjTMh

@BennyVL

This comment has been minimized.

Copy link
Author

commented Oct 9, 2019

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

This comment has been minimized.

Copy link

commented Oct 9, 2019

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?

@BennyVL

This comment has been minimized.

Copy link
Author

commented Oct 9, 2019

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?

@BennyVL

This comment has been minimized.

Copy link
Author

commented Oct 9, 2019

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

No errors or other messages show in the console.

@mcsf

This comment has been minimized.

Copy link
Contributor

commented Oct 9, 2019

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

@epiqueras

This comment has been minimized.

Copy link
Contributor

commented Oct 9, 2019

This follows the logic of this reducer:

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

This comment has been minimized.

Copy link
Contributor

commented Oct 10, 2019

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

This comment has been minimized.

Copy link
Contributor

commented Oct 10, 2019

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

This comment has been minimized.

Copy link
Contributor

commented Oct 10, 2019

Here's the fix: #17888

@mcsf

This comment has been minimized.

Copy link
Contributor

commented Oct 10, 2019

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

This comment has been minimized.

Copy link
Contributor

commented Oct 10, 2019

@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.

WordPress 5.3 Must Have automation moved this from To do to Done Oct 11, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
5 participants
You can’t perform that action at this time.