Proposal: history "buffer"/overwrite, sync RichText history records #4956

ellatrix · 2018-02-08T16:00:29Z

Description

This PR proposes:

Overwriting/buffering changes in the history reducer when block attributes are updated and the block id and updated attribute keys are the same. This will fix the cases where history records are added for every single character change.
With this change it is OK for editable to sync continuously.
In addition, we'd like changes RichText instances to create additional history records such as a formatting change, on moving the caret, etc. For this we can use the AddUndo event TinyMCE fires.
Completely propagate undo/redo command from RichText. This is necessary to actually undo the changes in the global state as well, not just RichText and pile up changes in the global state.

How Has This Been Tested?

Try undoing changes in the editor.

Checklist:

My code is tested.
My code follows the WordPress code style.
My code has proper inline documentation.

youknowriad · 2018-02-08T16:26:15Z

blocks/rich-text/index.js

-		if ( this.editor.isDirty() ) {
-			this.fireChange();
+		this.savedContent = this.getContent();
+		this.props.onChange( this.state.empty ? [] : this.savedContent );


Minor: I made a small change in the other PR here, I assign [] to this.savedContent if it's empty to avoid updateContent to be called on the next render.

ellatrix · 2018-02-09T21:57:12Z

I'm thinking instead of creating undo levels on focus/blur, we could somehow check when the block ID changes when updating attributes, and then not overwrite state. A bit similar to #4959.

youknowriad · 2018-02-12T13:45:03Z

I'm thinking instead of creating undo levels on focus/blur, we could somehow check when the block ID changes when updating attributes, and then not overwrite state. A bit similar to #4959.

I like this idea, I think we can improve #4959 do something like this:

If we're updating a different block attribute, we add a new undo state
If we're updating a different block, we add a new undo state
If we're updating the same block but after a long enough timeout (say 10 seconds), we create new undo state
If we're updating the same block and same attribute but in a short timeout, we override the previous state.

What do you think, it should be pretty easy to implement?

ellatrix · 2018-02-12T13:56:10Z

I agree on that except for the last part... I think it's pretty simplistic to just add a timeout for the same field. For small fields such as inputs and small textareas, this is not really needed, and for bigger fields this won't be enough. We should let the RichText component decide when to create history records. TinyMCE will look internally at when formatting is applied, at some special keys, at moving the caret, etc. etc. I'm fine to move this looking over to Gutenberg in e.g. writing flow (or in the future), but setting a timeout won't be enough. For now it seems good to just add a level whenever TinyMCE fires AddUndo, on top of looking at block ID diff.

youknowriad · 2018-02-12T14:06:34Z

For now it seems good to just add a level whenever TinyMCE fires AddUndo, on top of looking at block ID diff.

I also agree with that, I'm just trying to think how this will look like technically speaking. I don't want us to add a prop or something like that for block authors to deal with. If it's possible without it, that would be great 👍

Edit: I guess it's possible to do it with something like bufferTypes in this PR, but maybe something like the forceUndoStateTypes.

Do you want to work on this? I can't update my PR accordingly if necessary

ellatrix · 2018-02-12T14:07:42Z

I don't want us to add a prop or something like that for block authors to deal with. If it's possible without it, that would be great 👍

Yeah, agree there too. :) In this branch, I used context.

ellatrix · 2018-02-12T16:04:47Z

@youknowriad Sorry, didn't see your edit. Yeah, but I guess if we want to look at the action ID and attribute keys, it will have to be a function like you did. Updated the PR with tests. This will now always overwrite the state if the ID and attribute keys are the same. Otherwise an undo level will be added. Undo levels will also be added from the RichText component when needed.

ellatrix · 2018-02-13T10:50:16Z

editor/store/reducer.js

+	partialRight( withHistory, {
+		resetTypes: [ 'SETUP_NEW_POST', 'SETUP_EDITOR' ],
+		shouldOverwriteState( action, previousAction ) {
+			if ( ! includes( [ 'UPDATE_BLOCK_ATTRIBUTES', 'EDIT_POST', 'RESET_POST' ], action.type ) ) {


@aduth I'm adding these action here because they seem to fire after auto saving the post. Ideally saving a post should not interfere with undo levels. Not sure if we should create a separate action for this.

youknowriad · 2018-02-13T11:30:24Z

editor/store/reducer.js

+				return action.uid === previousAction.uid && isEqual( attributes, previousAttributes );
+			}
+
+			return true;


Don't you think it should be false by default? and we'd only overwrite the state if the action is UPDATE_BLOCK_ATTRIBUTES with the same keys?

This would allow us to remove the check you're doing line 123

Hm, then I just have to invert that check?

yes, why can't this function be just:

shouldOverwriteState( action, previousAction ) { if ( previousAction && action.type === 'UPDATE_BLOCK_ATTRIBUTES' && action.type === previousAction.type ) { const attributes = keys( action.attributes ); const previousAttributes = keys( previousAction.attributes ); return action.uid === previousAction.uid && isEqual( attributes, previousAttributes ); } return false; },

It still has checks for other actions too?

no, why do we need to check other actions?

That's why I commented here #4956 (comment). I'm having problems with loading the post and autosave interfering with the undo levels.

Oh! I missed that one sorry.

Thinking about these actions 'EDIT_POST', 'RESET_POST'

I believe these two actions shouldn't overwrite previous state. If you edit a category, a publish date or anything in the post, it should create an undo level right?

same for reset_post which means the post is updated globally somewhere.

Maybe we could add a flag to this action when we want to avoid creating undo levels (like post success actions)?

Yep, it definitely a to do.

youknowriad · 2018-02-13T11:42:50Z

blocks/rich-text/index.js

@@ -373,12 +368,22 @@ export class RichText extends Component {
 	 * Handles any case where the content of the tinyMCE instance has changed.
 	 */
 	onChange() {
-		if ( ! this.editor.isDirty() ) {


The dirty checking was causing an issue when you click the "undo" button because onChange was being called right after the updateContent call. Which is a not necessary call to onChange (It was even causing issues).

I wonder if we should keep it to avoid these unnecessary onChange calls.

Wouldn't this.savedContent checking prevent that?

No, We're not checking it on the onChange handler when the updateContent triggers (clicking undo, splitting paragraphs)

Okay. Looking into it. Calling save here is not great because it will call the expensive version of getContent... :)

Would this do the job? https://github.com/WordPress/gutenberg/pull/4956/files#diff-766381e792acf9d4c2505f3aeddcca7eR701

youknowriad · 2018-02-13T11:45:19Z

blocks/rich-text/index.js

 	setContent( content = '' ) {
 		this.editor.setContent( renderToString( content ) );
 	}

 	getContent() {
+		if ( this.state.empty ) {


In #4953 I discovered that sometimes the getContent/onChange is called before the this.state.empty is updated. I think we should not rely on it here and recompute the empty from the content. I think it's performant enough.

I was also wondering if we chould update this state as part of the onChange handler (instead of selectionChange) but could be left as a separate enhancement.

Yeah, this is a good point. Changes through setState are not guaranteed to be applied immediately, so either we need to save it as a class property or just simply add it here. I think the latter sounds great as we can then also remove the selection change event. I can't think of a case where input wouldn't be fired when the field is emptied or filled?

I can't think of a case where input wouldn't be fired when the field is emptied or filled?

Well I discovered that if you make a selection (a word or sentense) and click backspace, it's not triggered. This should be fixed anyway because it's creating a small bug in the side inserter. I was thinking at maybe calling onChange on keydown or something?

Yeah, that's why the keyup handler is there, because selectionChange has the same issue, at least in Chrome. I updated this PR. Nice small performance improvement as well because selectionChange fires waaay more often.

youknowriad · 2018-02-13T11:45:54Z

Left some comments but I like the direction this is taking.

aduth · 2018-02-13T19:15:51Z

editor/utils/with-history/index.js

@@ -63,6 +99,14 @@ export default function withHistory( reducer, options = {} ) {
 			return state;
 		}

+		if ( shouldOverwriteState( action, previousAction ) ) {


Maybe we can consolidate the return paths by assigning past into a variable:

let nextPast = past; if ( ! shouldOverwriteState( action, previousAction ) ) { nextPast = [ ...nextPast, present ]; } return { past: nextPast, present: nextPresent, future: [], };

What would the benefit be? Does it read clearer to you?

I personally find fewer return paths tends to be more clear, yes.

aduth · 2018-02-13T19:16:35Z

blocks/rich-text/index.js

-			// from running, but we assume TinyMCE won't do anything on an
-			// empty undo stack anyways.
+		if ( onUndo && ( command === 'Undo' || command === 'Redo' ) ) {
+			defer( onUndo );


For Redo we'd want to onRedo instead?

Also wondering if we need the defer anymore if we're completely taking over handling of undo history.

Added this. The defer still seems needed. Encountering errors otherwise.

aduth · 2018-02-13T19:19:06Z

editor/store/selectors.js

@@ -95,7 +95,8 @@ export function isSavingMetaBoxes( state ) {
 * @return {boolean} Whether undo history exists.
 */
 export function hasEditorUndo( state ) {
-	return state.editor.past.length > 0;
+	const { past, present } = state.editor;
+	return past.length > 1 || last( past ) !== present;


Aren't we preventing past from including present? When would those be the same?

They should be the same at the moment an undo record is created or at the start of history. This is so that present can be overwritten. Not sure I understand the first question.

aduth · 2018-02-13T19:46:44Z

Per #4008, it's not only block attributes that we'd want to be guarding against excessive undo history (there, the post title and text editor content updates).

ellatrix · 2018-02-15T22:10:42Z

Pushed some more changes. PR makes some good simplifying changes to the RichText component too I think. Note that there will still be actions on init and autosave that will still create undo levels. Let's address those separately then.

Ready for another review I think.

ellatrix · 2018-02-15T22:15:50Z

Conclusion for self: idea was good but implementation ended up a lot simpler than I thought... 🙈

ellatrix · 2018-02-15T22:19:06Z

When splitting a paragraph mid-way and Undo'ing immediately via Cmd-Z, the new block's content simply disappears.

@aduth I see this bug too. The problem here is that there are too many undo levels created still (cross block IDs). This is a bug in master too though, so let's address separately as well.

aduth

This is looking quite good. Left a few minor remarks, but I think we can plan to get this in otherwise.

Nice thing is that with these changes, it starts to become very clear where we're needlessly creating undo history. Found one instance of this with autosave that I'll plan a pull request for shortly.

aduth · 2018-02-15T23:16:47Z

blocks/rich-text/index.js

+	}
+
+	onAddUndo( { lastLevel } ) {
+		if ( ! lastLevel ) {


Might help to include a note and/or reference to the fact that TinyMCE creates an initial undo level, which is why it's checked here.

https://github.com/tinymce/tinymce/blob/a4add29/src/core/main/ts/api/UndoManager.ts#L50-L53

You remind the that there's actually another even in TinyMCE that skips this... I think in previous iteration it was required to catch the initial bookmark, but now that it's no longer setting focus it should be fine to use the change event.

https://github.com/tinymce/tinymce/blob/a4add29/src/core/main/ts/api/UndoManager.ts#L242-L247

aduth · 2018-02-15T23:19:25Z

blocks/rich-text/index.js

@@ -412,14 +395,25 @@ export class RichText extends Component {
 	 *
 	 * @param {boolean} checkIfDirty Check whether the editor is dirty before calling onChange.
 	 */
-	onChange( checkIfDirty = true ) {


Since we're removing the argument, we should also remove the JSDoc tag.

Oh. Surprised there's no listing error for this one.

aduth · 2018-02-15T23:19:55Z

blocks/rich-text/index.js

@@ -412,14 +395,25 @@ export class RichText extends Component {
 	 *
 	 * @param {boolean} checkIfDirty Check whether the editor is dirty before calling onChange.
 	 */
-	onChange( checkIfDirty = true ) {
-		if ( checkIfDirty && ! this.editor.isDirty() ) {


I imagine this should no longer be necessary:

gutenberg/blocks/rich-text/index.js

Line 803 in 9cd9c18

this.editor.setDirty( true );

You may be right. Don't know if it's trying to communicate something else to TinyMCE after handling formatting ourselves. I don't immediately see anything broken by removing it though.

aduth · 2018-02-15T23:21:15Z

blocks/rich-text/index.js

@@ -756,7 +740,14 @@ export class RichText extends Component {
 			this.props.value !== prevProps.value &&
 			this.props.value !== this.savedContent
 		) {
-			this.updateContent();


Curious why this was pulled out of the function and made inline. Ideally we should favor clearly named functions over inline logic in component lifecycle.

Because at the start of this PR it was just one line and then piled up. :) Will move it back to a function.

aduth · 2018-02-15T23:24:04Z

editor/utils/with-history/index.js

+		shouldCreateUndoLevel = ! past.length || shouldCreateUndoLevel;
+
+		if ( ! shouldCreateUndoLevel && shouldOverwriteState( action, previousAction ) ) {
+			nextPast = past;


The idea with the inverse condition in #4956 (comment) is that it avoids wastefully calculating a nextPast above if we don't intend to use it anyways.

let nextPast = past; if ( shouldCreateUndoLevel || ! shouldOverwriteState( action, previousAction ) ) { nextPast = [ ...past, present ]; }

I hadn't thought about that! Thanks for this tip!

ellatrix · 2018-02-16T10:33:30Z

Updated based on the last few comments.

ellatrix · 2018-02-16T10:38:19Z

Alright, merging! 🎉 These changes make me really happy! Thanks for all the help @aduth and @youknowriad!

mcsf · 2018-02-16T13:49:06Z

editor/store/test/reducer.js

+				expect( state.past ).toHaveLength( 2 );
+			} );
+
+			it( 'should not overwrite present history if updating same attributes', () => {


This likely meant to read: should not overwrite present history if updating different attributes. :)

aduth · 2018-06-22T15:01:11Z

blocks/rich-text/index.js

 				this.props.onSplit( beforeElement, afterElement );
 			} else {
 				event.preventDefault();
+				this.onCreateUndoLevel();


Why do we create an undo level here? The behavior of createUndoLevel is such that we'll trigger onChange which in turn calls setAttributes. When we're at the end of a paragraph block and pressing enter (a very common writing flow), we're thus needlessly calling setAttributes when the content of the original paragraph hasn't changed at all (calling twice in fact, the other a separate issue).

We should either:

Remove this line

Document why it's needed with an inline comment

And ideally try to avoid setting content

See also #7482

Unsure why we're adding one before splitting. Seems safe to remove, perhaps with a test ensuring right history behaviour on split.

aduth · 2018-06-28T20:14:42Z

blocks/rich-text/index.js

-		this.editor.save();
+		// Do not trigger a change event coming from the TinyMCE undo manager.
+		// Our global state is already up-to-date.
+		this.editor.undoManager.ignore( () => {


What would be causing an undo level to be added? I added a breakpoint to TinyMCE's internal UndoManager#add and it was never triggered as a result of anything which occurs in the callback of this function.

When content is set, but not by MCE?

But specifically the functions called within this block do not appear to add any undo levels from what I have been able to tell. See also #7620. Could we demonstrate by a failing end-to-end test what we need to be accounting for by its presence if it were to be removed?

Worst case there's also a no_events argument which can be passed with setContent that may be an alternative to what we have here, if we really don't want cascading effects from setting the content.

ellatrix force-pushed the try/undo-buffer branch from ff4ae59 to 8de51ce Compare February 8, 2018 16:10

ellatrix mentioned this pull request Feb 8, 2018

Editable: sync state on input, signal undo levelling #2930

Closed

ellatrix force-pushed the try/undo-buffer branch from 8de51ce to a2b08be Compare February 8, 2018 16:21

ellatrix added the [Status] In Progress Tracking issues with work in progress label Feb 8, 2018

youknowriad reviewed Feb 8, 2018

View reviewed changes

ellatrix force-pushed the try/undo-buffer branch from a2b08be to a27fbd8 Compare February 9, 2018 18:49

youknowriad mentioned this pull request Feb 12, 2018

RichText Trigger onChange on input #4955

Merged

ellatrix force-pushed the try/undo-buffer branch from 80da3cd to 673ffea Compare February 12, 2018 15:58

ellatrix changed the title ~~Proposal: sync RichText state, history buffer, sync RichText history records~~ Proposal: history "buffer"/overwrite, sync RichText history records Feb 12, 2018

ellatrix force-pushed the try/undo-buffer branch 3 times, most recently from 4c517aa to daab823 Compare February 13, 2018 10:24

ellatrix commented Feb 13, 2018

View reviewed changes

ellatrix removed the [Status] In Progress Tracking issues with work in progress label Feb 13, 2018

youknowriad reviewed Feb 13, 2018

View reviewed changes

youknowriad mentioned this pull request Feb 13, 2018

Try a generic undo handler #4959

Closed

youknowriad reviewed Feb 13, 2018

View reviewed changes

ellatrix force-pushed the try/undo-buffer branch from bafd49b to 1ec77c6 Compare February 13, 2018 17:27

aduth reviewed Feb 13, 2018

View reviewed changes

aduth added this to the 2.2 milestone Feb 14, 2018

aduth added the [Component] TinyMCE label Feb 14, 2018

ellatrix force-pushed the try/undo-buffer branch from bd6754f to 2217c1d Compare February 15, 2018 21:56

Leave comment for backspace onChange

9cd9c18

ellatrix requested a review from a team February 15, 2018 22:12

aduth approved these changes Feb 15, 2018

View reviewed changes

This was referenced Feb 15, 2018

State: Set new post status to draft in initialization #5089

Merged

Rich Text: Determine emptiness by value #5091

Merged

ellatrix added 4 commits February 16, 2018 11:16

Use change event instead of addundo which fires during init

bbe1dff

Remove dirty references

2c7d283

Restore updateContent

6d4cd20

Avoid wastefully calculating nextPast

9750e4b

ellatrix merged commit 8897445 into master Feb 16, 2018

mcsf reviewed Feb 16, 2018

View reviewed changes

aduth mentioned this pull request Feb 16, 2018

Rich Text: Unnecessarily dispatching change actions #5103

Closed

aduth mentioned this pull request Feb 28, 2018

Triggering Undo (Ctrl + Z) in Paragraph with inline formatting can move cursor unexpectedly #3524

Closed

ellatrix mentioned this pull request Mar 6, 2018

Fix typo in history tests #5440

Merged

This was referenced Mar 6, 2018

Title Block undo/redo goes letter by letter #4057

Closed

State: Batch updates to title (and other post properties) for undo #5553

Merged

aduth reviewed Jun 22, 2018

View reviewed changes

aduth deleted the try/undo-buffer branch June 22, 2018 15:05

aduth mentioned this pull request Jun 22, 2018

Blocks: Avoid setAttributes on end-of-paragraph split #7482

Merged

aduth reviewed Jun 28, 2018

View reviewed changes

This was referenced Jun 28, 2018

Rich Text: Remove updateContent function #7620

Merged

Rich Text: Remove createUndoLevel for split, merge #7650

Merged

aduth mentioned this pull request Apr 9, 2019

Merely selecting a block after load creates undo level #14766

Closed

ellatrix mentioned this pull request Aug 8, 2019

Record selection changes in history (undo) #16428

Closed

5 tasks

Proposal: history "buffer"/overwrite, sync RichText history records #4956

Proposal: history "buffer"/overwrite, sync RichText history records #4956

Conversation

ellatrix commented Feb 8, 2018 • edited Loading

Description

How Has This Been Tested?

Checklist:

Choose a reason for hiding this comment

ellatrix commented Feb 9, 2018

youknowriad commented Feb 12, 2018 • edited Loading

ellatrix commented Feb 12, 2018 • edited Loading

youknowriad commented Feb 12, 2018 • edited Loading

ellatrix commented Feb 12, 2018

ellatrix commented Feb 12, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

youknowriad Feb 14, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

youknowriad Feb 15, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

youknowriad commented Feb 13, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aduth Feb 13, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aduth commented Feb 13, 2018

ellatrix commented Feb 15, 2018

ellatrix commented Feb 15, 2018

ellatrix commented Feb 15, 2018

aduth left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ellatrix Feb 16, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ellatrix commented Feb 16, 2018

ellatrix commented Feb 16, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aduth Jun 30, 2018 • edited Loading

Choose a reason for hiding this comment

ellatrix commented Feb 8, 2018 •

edited

Loading

youknowriad commented Feb 12, 2018 •

edited

Loading

ellatrix commented Feb 12, 2018 •

edited

Loading

youknowriad commented Feb 12, 2018 •

edited

Loading

youknowriad Feb 14, 2018 •

edited

Loading

youknowriad Feb 15, 2018 •

edited

Loading

aduth Feb 13, 2018 •

edited

Loading

ellatrix Feb 16, 2018 •

edited

Loading

aduth Jun 30, 2018 •

edited

Loading