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

Block Editor: Mark last change as persistent on save #36887

Merged
merged 2 commits into from
Dec 4, 2021

Conversation

zaguiini
Copy link
Member

@zaguiini zaguiini commented Nov 25, 2021

Description

Fixes #36417.

It makes sense to check the last change as persistent once the "Save" button is clicked, even if the attributes themselves didn't change. Not doing so results in the save button not being enabled if editing the same attribute after clicking "Save" once.

Let me know if there's a better way of performing such action or if I'm missing something.

How has this been tested?

  1. Enable the full site editor
  2. Navigate to full site editor
  3. Select a block and try to adjust padding width without making other changes
  4. Save the changes
  5. Return to the same block. Select it and try to adjust the padding width without making other changes.
  6. Verify that option to update doesn't reset

Screenshots

Before

2021-11-11 12 26 23

After

2021-11-11 12 21 59

Types of changes

Bug fix

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • I've tested my changes with keyboard and screen readers.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.
  • I've updated all React Native files affected by any refactorings/renamings in this PR (please manually search all *.native.js files for terms that need renaming or removal).

@zaguiini
Copy link
Member Author

cc @youknowriad.

@@ -159,6 +164,8 @@ export default function EntitiesSavedStates( { close } ) {
siteItemsToSave
);
}

__unstableMarkLastChangeAsPersistent();
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe instead of dispatching this here, we could do so inside the close callback so we don't dispatch this action on every instance of EntitiesSavedState.

But on the other hand, it makes sense to dispatch this action here since it's part of editing save.

@zaguiini zaguiini force-pushed the fix/saving-block-editor-attributes branch from 2751efb to 45088d8 Compare November 25, 2021 19:05
@Copons Copons added [Feature] Block settings menu The block settings screen [Type] Bug An existing feature does not function as intended labels Nov 25, 2021
@youknowriad
Copy link
Contributor

Do you think we can add an e2e test about this?

Does this happen only in the site editor? What about the post editor (it uses a different component for saving by default)?

cc @ellatrix and @mcsf who might know more about "persistence"/"undo"

@zaguiini
Copy link
Member Author

zaguiini commented Nov 26, 2021

Do you think we can add an e2e test about this?

Of course Done.

We might even do some refactoring removing other markAsPersistent calls that happen on Rich Editor for example, but I'll need to double check that and will probably happen in a follow-up PR.

Does this happen only in the site editor? What about the post editor (it uses a different component for saving by default)?

Only on the site editor. I could not replicate the behavior on the post editor.

Copy link
Contributor

@jeyip jeyip left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks reasonable to me. It sidesteps a lot of issues with our first approach, and it gets the save feature working again for block settings attributes.

+1 from me, but we should wait for Ella or Miguel to chime in. Like Riad mentioned, they have more experience with and levels and dirty checking.

@zaguiini
Copy link
Member Author

zaguiini commented Dec 2, 2021

It's almost a week without additional feedback, so we're assuming the solution is good enough. This is affecting FSE users. We’re aiming to merge this really early tomorrow, but wanted to leave a comment so that it doesn’t take anyone by surprise.

cc @youknowriad, @ellatrix, @ntwb.

@mcsf
Copy link
Contributor

mcsf commented Dec 2, 2021

I apologise for not commenting earlier, but it took me a while to dig into the problem.

Does this happen only in the site editor? What about the post editor (it uses a different component for saving by default)?

I think the post-editor question is crucial, and we should be able to answer it with confidence. It's an important clue that markLastChangeAsPersistent is used so little throughout Gutenberg: you'll mostly see a couple of RichText-specific usages, which we can ignore, and thus we are left with essentially one call in the editor store's savePost action:

// Make sure that any edits after saving create an undo level and are
// considered for change detection.
if ( ! options.isAutosave ) {
yield controls.dispatch(
blockEditorStore,
'__unstableMarkLastChangeAsPersistent'
);
}

The Site Editor doesn't have an equivalent action. In fact, the edit-site store generally shies away from creating a "site" abstraction comparable to "post", and instead operates on collections of entities (templates, posts, etc.), heavily interacting with the core store.

To get back to the point of this PR, I was initially suspicious of tucking away such important behaviour in a UI component like EntitiesSavedStates. But, considering the relationship between edit-site and core, there is currently no good place in the stores for this functionality — specifically, because entities in the core store exist independently of editor state.

I think that, in the future, we should keep all this delicate logic in some place that's central to the whole Site Editor to avoid bad integrity bugs, race conditions, etc. But, right now, I think this PR will do. One last thing: please thoroughly test saving, editing, undoing not just in the Site Editor, but in the Post Editor too — I don't expect overlap based on this PR's changes, but just to be safe.

@jeyip
Copy link
Contributor

jeyip commented Dec 3, 2021

Thanks for digging into this @mcsf! Much appreciated.

I think that, in the future, we should keep all this delicate logic in some place that's central to the whole Site Editor to avoid bad integrity bugs, race conditions, etc. But, right now, I think this PR will do. One last thing: please thoroughly test saving, editing, undoing not just in the Site Editor, but in the Post Editor too — I don't expect overlap based on this PR's changes, but just to be safe.

I'm looking into everything you described now. I'll be sure to leave detailed testing notes of my findings. 👍

@jeyip
Copy link
Contributor

jeyip commented Dec 3, 2021

Testing

Post Editor Testing Steps + Requirements

  1. Add "Hello world!" Paragraph block
  2. Upload an image block
  3. Add image caption
  4. Add "Test nested paragraph" Paragraph block inside a group block
  5. Add a "Test link paragraph" Paragraph block and add an external link
  6. Add a columns block with three columns
  7. Add a "Test verse" Verse block inside a group block inside column 1
  8. Add an image block inside column 2
  9. Add a "Test column paragraph" Paragraph block inside column 3
  10. Add an empty paragraph block using the default inline block appender
  11. Select the "Test nested paragraph" Paragraph block
  12. Modify block settings color options (background color and text color)
  13. Modify block settings typography options (line height, appearance, letter spacing)
  14. Select the "Test link paragraph" Paragraph block
  15. Modify block settings color options (background color)
  16. Modify advanced block settings options (HTML anchor and Additional CSS classes)
  17. Open the post sidebar settings and modify options (post visibility, publish date, "stick to the top of the blog" toggle, assigned categories, assigned template)

Undo and Redo

  • Images, paragraphs, verses, columns, groups, videos, block patterns
  • Nested blocks
  • Block settings color options background color and text color (link color didn't appear to be available in post editor?)
  • Block settings typography options like line height, appearance, letter spacing, letter case, and drop cap
  • Advanced Block settings like HTML anchor or Additional CSS classes
  • Post sidebar settings like post visibility, publish date, "stick to the top of the blog" toggle, assigned categories, and assigned template (We cannot redo post sidebar settings consistently. It's a bug that's present in trunk. I've noted it below)
  • Continually clicking undo eventually leads to a blank post (Sometimes undoing immediately after publishing a post doesn't have any effect. It's a bug that's present in trunk. I've noted it below)
  • Continually clicking redo eventually restores the final state of the post

Save

  • Can save final state of post (no undos)
  • Can save partially undone post (some undos)
  • Can save completely undone post

Edit

  • Can edit partially undone state and see that redo restores edits

Site Editor Testing Steps + Requirements

  1. Add "Hello world!" Paragraph block
  2. Upload an image block
  3. Add image caption
  4. Add "Test nested paragraph" Paragraph block inside a group block
  5. Add a "Test link paragraph" Paragraph block and add an external link
  6. Add a columns block with three columns
  7. Add a "Test verse" Verse block inside a group block inside column 1
  8. Add an image block inside column 2
  9. Add a "Test column paragraph" Paragraph block inside column 3
  10. Add an empty paragraph block using the default inline block appender
  11. Select the "Test nested paragraph" Paragraph block
  12. Modify block settings color options (background color and text color)
  13. Modify block settings typography options (line height, appearance, letter spacing)
  14. Select the "Test link paragraph" Paragraph block
  15. Modify block settings color options (background color and link color)
  16. Modify advanced block settings options (HTML anchor and Additional CSS classes)
  17. Open the post sidebar settings and modify options (post visibility, publish date, "stick to the top of the blog" toggle, assigned categories, assigned template)

Undo and Redo

  • Images, paragraphs, verses, columns, groups, videos, block patterns
  • Nested blocks
  • Block settings color options background color, link color, and text color
  • Block settings typography options like line height, appearance, letter spacing, letter case, and drop cap
  • Advanced Block settings like HTML anchor or Additional CSS classes
  • Post sidebar settings like post visibility, publish date, "stick to the top of the blog" toggle, assigned categories, and assigned template
  • Continually clicking undo eventually leads to a blank post
  • Continually clicking redo eventually restores the final state of the post

Save

  • Can save final state of post (no undos)
  • Can save partially undone post (some undos)
  • Can save completely undone post

Edit

  • Can edit partially undone state and see that redo restores edits
  • Can save new edits

Browsers

  • Chrome

Notes

There are a variety of existing undo and redo bugs on Gutenberg trunk.

  • Undoing and redoing a site logo block with an uploaded image does not restore uploaded image. It restores an empty site logo block.
  • Undoing a site tagline block removes text character by character instead of in larger "chunks"
  • Undoing and redoing a site tagline does not restore the site tagline block (cannot reproduce)
  • Cannot redo block settings font-size when changes are made alongside other typography updates (line spacing, line height, etc.)
  • Cannot redo post editor post sidebar settings updates consistently
  • Undoing immediately after publishing a post in the post editor sometimes does not work. Undo levels appear to be completely reset.

@jeyip
Copy link
Contributor

jeyip commented Dec 3, 2021

@mcsf The undo/redo functionality appears to be in a rough state on its own in trunk, but from what I can tell, we're not introducing any regressions with this change.

I've catalogued any bugs that I've found in trunk here and plan to make a separate issue to keep track of the problems. With all of that being said, everything still looks good to me.

@jeyip jeyip merged commit d4e57dc into WordPress:trunk Dec 4, 2021
@github-actions github-actions bot added this to the Gutenberg 12.2 milestone Dec 4, 2021
@zaguiini zaguiini deleted the fix/saving-block-editor-attributes branch December 7, 2021 17:24
@noisysocks noisysocks added Backport to WP 6.7 Beta/RC Pull request that needs to be backported to the WordPress major release that's currently in beta Backport to WP Minor Release Pull request that needs to be backported to a WordPress minor release and removed Backport to WP 6.7 Beta/RC Pull request that needs to be backported to the WordPress major release that's currently in beta Backport to WP Minor Release Pull request that needs to be backported to a WordPress minor release labels Jan 20, 2022
noisysocks pushed a commit that referenced this pull request Jan 24, 2022
* Mark last change as persistent when saving changes
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Block settings menu The block settings screen [Type] Bug An existing feature does not function as intended
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Full Site Editing: Cannot persist changes to certain block settings
6 participants