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: Fix undo level inconsistencies. #17259

Merged
merged 7 commits into from Sep 16, 2019

Conversation

@epiqueras
Copy link
Contributor

commented Aug 29, 2019

Description

This PR fixes a couple of undo level related inconsistencies that surfaced after merging #16932.

In the Block Editor, withPersistentBlockChange marks any change to an attribute different than the last, as persistent. However, it considers all actions for this comparison, so an explicit persistence change triggered by a timeout like the one that fires after not typing for a while, would break this "chain of comparison", and the next change would be marked as persistent even if targeted at the same attribute as the one before the explicit persistence change. This resulted in scenarios where typing "Hello", waiting a bit, then typing "World", would result in 4 undo levels instead of 3. Namely, there would be an extra unnecessary undo level in the middle, requiring an extra click to undo. This PR fixes this by ignoring explicit persistence changes when comparing the last two attribute changes.

In the Editor, the decision was made to consider initial edits as dirtying operations that create an undo level and enable saving functionality. For some reason, the server sends an empty initial edit with empty strings for the title, excerpt, and content of new posts. This results in another unnecessary undo level. This PR fixes this by ignoring initial edits that would not change any values of the post.

How has this been tested?

It was verified that typing two words with a pause in between does not create an undo level and that new posts don't load with an undo level.

Types of Changes

Bug Fix: Stop inserting unnecessary undo levels between words an on empty posts.

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.
@ellatrix

This comment has been minimized.

Copy link
Member

commented Aug 30, 2019

@epiqueras I added some e2e tests that should pass, and do pass before #16932. They are still failing with these changes though. I think one undo level has been removed, but for some reason there's still one being created with the first letter.

@epiqueras

This comment has been minimized.

Copy link
Contributor Author

commented Aug 30, 2019

That's because the Block Editor is marking it as persistent (calling onInput). That was somehow ignored before. Could we make the Block Editor not do that?

@ellatrix

This comment has been minimized.

Copy link
Member

commented Aug 30, 2019

Sure. Where is it doing that?

@epiqueras

This comment has been minimized.

Copy link
Contributor Author

commented Aug 30, 2019

with the first letter.

When you type that first character.

@ellatrix

This comment has been minimized.

Copy link
Member

commented Sep 2, 2019

@epiqueras I meant in the code :)

@epiqueras

This comment has been minimized.

Copy link
Contributor Author

commented Sep 2, 2019

No idea, I thought you might know, because of your RichText/writing flow expertise.

@ellatrix

This comment has been minimized.

Copy link
Member

commented Sep 3, 2019

@epiqueras RichText doesn't mark the first character as persistent. It continuously calls onChange and createUndoLevel after 1s of no typing. The latter should eventually move somewhere else, so it only calls onChange. The only other case it creates an undo level is when it automatically changes the content based on an input rule, which doesn't seem to be the case here.

@epiqueras

This comment has been minimized.

Copy link
Contributor Author

commented Sep 3, 2019

So, the issue is probably coming from somewhere else in the block editor.

In your tests, you are also seeing the first character passed through onInput right?

@epiqueras

This comment has been minimized.

Copy link
Contributor Author

commented Sep 3, 2019

@ellatrix

Well, it follows this logic:

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

The first character's edit is not updating the same attribute as the last edit right?

How should we go about this?

I think the logic behind MARK_LAST_CHANGE_AS_PERSISTENT is kind of flawed. It should be a part of the next edit, not a standalone action. So when you type and stop for a while, the next edit will mark the last edit as persistent. With the current flow, the last edit gets marked as persistent, and the next character that comes in is also marked as persistent.

Also, do you have an example of a release where this worked correctly?

@ellatrix

This comment has been minimized.

Copy link
Member

commented Sep 5, 2019

git checkout 256a6745354c80a63de53c62dc0ec107eed00c30 (the commit before merging #16932). Everything worked fine there.

@ellatrix

This comment has been minimized.

Copy link
Member

commented Sep 15, 2019

Should we not make sure this all works before the last Gutenberg release of WP 5.3?

@epiqueras epiqueras force-pushed the fix/undo-level-inconsistencies branch 2 times, most recently from d195dae to a40ce33 Sep 15, 2019
@epiqueras

This comment has been minimized.

Copy link
Contributor Author

commented Sep 16, 2019

Should we not make sure this all works before the last Gutenberg release of WP 5.3?

@ellatrix I think we can merge this now with the latest changes?

@ellatrix ellatrix force-pushed the fix/undo-level-inconsistencies branch from a40ce33 to e816e71 Sep 16, 2019
@ellatrix

This comment has been minimized.

Copy link
Member

commented Sep 16, 2019

Rebased the branch as I was getting some PHP errors with the current checkout of WP core.

@ellatrix

This comment has been minimized.

Copy link
Member

commented Sep 16, 2019

Looks way better now.

Still seeing some inconsistencies.

  1. Major: Create a new post. Create a new paragraph and type something in it. Save the draft. Undo the last change. The typing isn't undone. Instead, it seem like the saving is "undone"? The "Save Draft" button appears again, even though the content didn't change.
  2. Minor: When I open the demo content, the undo button is active, even though I didn't touch the content. This wasn't the case before.
Copy link
Member

left a comment

Functionally it looks good. Would be great if the last e2e test case that I added could be fixed. Codewise I don't have any objections, but would be great if someone familiar with the core-data package could provide a second review.

} );

const comparisonUndoEdits = Object.values( action.meta.undo.edits ).filter( ( edit ) => typeof edit !== 'function' );

This comment has been minimized.

Copy link
@ellatrix

ellatrix Sep 16, 2019

Member

What is this checking for? Would be great to inline comment.

This comment has been minimized.

Copy link
@epiqueras

epiqueras Sep 16, 2019

Author Contributor

That the last two edits are not the same.

This comment has been minimized.

Copy link
@ellatrix

ellatrix Sep 16, 2019

Member

I'm more wondering what the filter is for. :)

This comment has been minimized.

Copy link
@epiqueras

epiqueras Sep 16, 2019

Author Contributor

When an edit is a function it's an optimization to avoid running some expensive operation. We can't rely on the function references being the same so we opt out of comparing them here.

This comment has been minimized.

Copy link
@mcsf

mcsf Sep 16, 2019

Contributor

That could use an inline comment.

This comment has been minimized.

Copy link
@epiqueras

epiqueras Sep 16, 2019

Author Contributor

I'm working on it with another fix.

This comment has been minimized.

Copy link
@epiqueras

epiqueras Sep 16, 2019

Author Contributor
edits &&
Object.keys( edits ).some(
( key ) =>
edits[ key ] !== ( has( post, [ key, 'raw' ] ) ? post[ key ].raw : post[ key ] )

This comment has been minimized.

Copy link
@ellatrix

ellatrix Sep 16, 2019

Member

The raw key seems very API specific. Given all the API abstractions, is this the right place to check for the raw key?

This comment has been minimized.

Copy link
@epiqueras

epiqueras Sep 16, 2019

Author Contributor

Yeah, it's not ideal. I think we should deprecate initial edits if possible.

@ellatrix

This comment has been minimized.

Copy link
Member

commented Sep 16, 2019

I think the logic behind MARK_LAST_CHANGE_AS_PERSISTENT is kind of flawed. It should be a part of the next edit, not a standalone action. So when you type and stop for a while, the next edit will mark the last edit as persistent. With the current flow, the last edit gets marked as persistent, and the next character that comes in is also marked as persistent.

So this is all fine in the end? Anything that should be changed?

@ellatrix

This comment has been minimized.

Copy link
Member

commented Sep 16, 2019

Since this seems more or less ready, and fixing important issues, I'd suggest we include it in Gutenberg 6.5.

@ellatrix

This comment has been minimized.

Copy link
Member

commented Sep 16, 2019

420ead2 adds a test case for the third item listed in this comment: #16932 (comment). This PR fixes the issue.

@youknowriad

This comment has been minimized.

Copy link
Contributor

commented Sep 16, 2019

For the demo content being dirty. That was a decision we made on purpose after some discussions with @jasmussen and @epiqueras

@ellatrix

This comment has been minimized.

Copy link
Member

commented Sep 16, 2019

Ok sure, I agree, that's just a minor thing.

@ellatrix

This comment has been minimized.

Copy link
Member

commented Sep 16, 2019

I'm fine with reverting the test case introduced in d1817b6 for now if it would take too long to fix. It's good to have the other fixes merged.

@ellatrix

This comment has been minimized.

Copy link
Member

commented Sep 16, 2019

With that test case I meant the first item in #17259 (comment), not the demo content one. Would still be great to fix for 5.3 separately.

Copy link
Member

left a comment

In my opinion, feel free to merge. :)

@epiqueras epiqueras merged commit 5fa136b into master Sep 16, 2019
4 of 7 checks passed
4 of 7 checks passed
pull-request-automation
Details
Header rules - gutenberg-playground No header rules processed
Details
Pages changed - gutenberg-playground All files already uploaded
Details
Redirect rules - gutenberg-playground No redirect rules processed
Details
Mixed content - gutenberg-playground No mixed content detected
Details
Travis CI - Pull Request Build Passed
Details
netlify/gutenberg-playground/deploy-preview Deploy preview ready!
Details
@epiqueras epiqueras deleted the fix/undo-level-inconsistencies branch Sep 16, 2019
@epiqueras epiqueras referenced this pull request Sep 16, 2019
5 of 5 tasks complete
@epiqueras

This comment has been minimized.

Copy link
Contributor Author

commented Sep 16, 2019

@ellatrix This fixes the other issue: #17452.

dd32 pushed a commit to dd32/gutenberg that referenced this pull request Sep 27, 2019
* Block Editor: Skip explicit persistent marking actions when comparing against the last action in the persistent block change HOR.

* Editor: Ignore initial edits that wouldn't change post values.

* Add e2e tests that should pass

* Core Data: Fix undo approach.

* Add e2e test case for undo level creation on save

* Add passing e2e that would fail before

* Revert d1817b6
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.