RichText Trigger onChange on input by youknowriad · Pull Request #4955 · WordPress/gutenberg

youknowriad · 2018-02-08T14:35:51Z

This will be necessary to achieve #4951

It also fixes some issues while the save draft link doesn't show up until we focus out of the component.
It may introduce a difference in a way undo/redo works but I believe we should handle those consistently (it's not the case right now, RichText triggers and undo state per word while PlainText it's per character). I'm thinking we could use the withHistory a Reducer enhancer to group similar changes into one undo level.

Testing instructions

Test several writing flow interactions

aduth · 2018-02-08T14:37:35Z

See: #2758

(Particularly bits about undo history)

ellatrix · 2018-02-08T14:58:21Z

I'm currently rebasing a WIP branch that does the undo signalling with a buffer in the history. I'm also leaving out selection now since it would make it more complex. I feel like we should fix undo together this PR.

ellatrix · 2018-02-08T15:05:54Z

blocks/rich-text/index.js

I forgot to add content = '' here 🙈

youknowriad · 2018-02-09T10:41:04Z

So from my testing, performance is good enough here but I'd appreciate that other people try it to confirm

Edit: Erratum: On long post it's not good at all, needs further investigation

aduth · 2018-02-09T15:50:38Z

blocks/rich-text/index.js

 		this.getSettings = this.getSettings.bind( this );
 		this.onSetup = this.onSetup.bind( this );
 		this.onChange = this.onChange.bind( this );
+		this.throttledOnChange = throttle( this.onChange.bind( this ), 500 );


If we can, I'd love if we would avoid throttling. Case in point: We're not cancelling a scheduled change when the component unmounts.

Yep, if the performance fix is enough, I'm fine dropping it.

I tried dropping it but I think performance-wise it's slightly better if we keep it

It seems like it always dispatched change when I type the first character. Should we wait until 500ms passes instead?

@youknowriad What can we do to get rid of this? :)

aduth · 2018-02-09T20:51:25Z

@youknowriad I created a branch off of this one which includes an iteration on rememo to improve per-key selector caching.

https://github.com/WordPress/gutenberg/tree/try/weak-map-cache

See: aduth/rememo#1

youknowriad · 2018-02-10T11:42:43Z

@aduth Awesome, I'm not seeing any performance issues any more 👍

On the performance side, maybe instead of using getBlock selector, we could target block properties directly getBlockName, getBlockAttributes, isValidBlock etc... These granular properties are less likely to change compared to the block object which will avoid unnecessary renders in connect calls.

…Equal

youknowriad · 2018-02-12T11:03:17Z

Rebase this with the changes from #5002 it seems better now.

gziolo · 2018-02-12T11:50:36Z

@youknowriad, see https://github.com/WordPress/gutenberg/pull/5002/files#r167532619, can we bring the polyfill as part of this PR?

gziolo · 2018-02-12T12:05:37Z

blocks/rich-text/index.js

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


Nice refactoring :)

gziolo · 2018-02-12T12:09:19Z

blocks/rich-text/index.js

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


@aduth - should we extract [] to its own shared variable to ensure we have always one variable reference to compare with ===? I wanted to double check - you know better all those perf bits. It might not make any sense in this context :)

gziolo · 2018-02-12T12:11:46Z

blocks/rich-text/index.js

-			this.props.value !== this.savedContent &&
-			! isEqual( this.props.value, prevProps.value ) &&
-			! isEqual( this.props.value, this.savedContent )
+			this.props.value !== this.savedContent


It seems like shallow compare is enough here.

…o button)

gziolo · 2018-02-12T13:23:44Z

I played with cmd + Z and noticed a bit strange behavior, the following screencast contains more details shared as I type:

gziolo · 2018-02-12T13:40:14Z

I also recorded another screencast where you can observe how strangely undo behaves:

I'm doing cmd + Z until undo history gets emptied. What's is very unexpected here is that past items don't get removed until the text is completely removed with undo actions...

gziolo · 2018-02-12T13:47:58Z

I'm still able to type something and close the tab without seeing the prompt to save your changes for my draft:

youknowriad · 2018-02-12T14:01:00Z

@gziolo

I can't seem to reproduce these undo issues though granted that it's not perfect, it's not perfect in master and as I said let's explore those separately in Proposal: history "buffer"/overwrite, sync RichText history records #4956 and Try a generic undo handler #4959
About the last screenshot, this is due to the throttle timeout. I think it's fine for now, I don't expect people to close their tab less than 0.5 seconds after editing the post unless they don't want to save these changes :). I'm also hopeful we could get rid of the throttle entirely at some point when we improve rerendering performance event more.

aduth · 2018-02-12T14:07:19Z

I'd be inclined to omitting the throttle from the start to force our hand in addressing the performance issues. 😅

I'll try to take a pass at these today (non-blocking).

gziolo

I tested in enough. Let's merge it and continue in the follow-up PRs as discussed 👍

ellatrix · 2018-02-12T14:30:37Z

blocks/rich-text/index.js

 			this.updateContent();
 		}
 	}
-


Removed line break by accident?

ellatrix · 2018-02-12T14:31:25Z

blocks/rich-text/index.js

 				return;
 			}

-			this.fireChange();


Why is this removed?

youknowriad · 2018-02-12T14:41:31Z

I don't know, this was part of one of your commits, I can restore :)

ellatrix · 2018-02-12T14:50:11Z

@youknowriad I think I removed that there assuming changes would be in sync all the time, but with the throttle that wouldn't be the case?

aduth · 2018-02-12T19:56:17Z

Noting that in the course of writing a post with a single paragraph containing only 32 words, I accumulated 71 copies of editor state via undo history (and that this will only grow over the course of writing a post in the same session).

aduth · 2018-02-12T19:58:40Z

Something revealed by this change: Should we be debouncing autosave instead of throttling? It feels as though it saves much more frequently now, since it can occur many times while typing within the same paragraph block.

Debouncing isn't great either, since just because I don't stop typing for X amount of time doesn't mean the post should never save during that time.

Or maybe the autosave should just be made less frequent than every 10 seconds?

aduth · 2018-02-12T20:18:11Z

blocks/rich-text/index.js

@@ -149,6 +151,7 @@ export class RichText extends Component {
 		editor.on( 'BeforeExecCommand', this.maybePropagateUndo );


Did we test to see how TinyMCE's undo history interacts with our own?

Are we calling onChange with the correct value after TinyMCE finishes undo?

Should we be bypassing TinyMCE's undo altogether (also preventing it from accumulating any history)?

@aduth I'm proposing these changes in #4956. Good point about preventing TinyMCE accumulating any history at all. We just need the signals, not the store.

ellatrix · 2018-02-13T10:36:30Z

@aduth I don't mind if we save frequently, but it is quite visually distracting ATM. Sounds like it need to be more subtle, similar to Google Docs. We also have to make sure we don't just pile these changes up as revisions, instead we should maybe reuse the last revision and only create a new revision on a user action or at least way less often (each revision is a post in the DB).

aduth · 2018-02-13T13:11:56Z

We also have to make sure we don't just pile these changes up as revisions

Yeah, this is an ongoing issue unfortunately: #3656

youknowriad added the [Feature] Rich Text Related to the Rich Text component that allows developers to render a contenteditable label Feb 8, 2018

youknowriad self-assigned this Feb 8, 2018

youknowriad requested review from ellatrix and mtias February 8, 2018 14:35

youknowriad force-pushed the try/editable-input-onchange branch from 18e97c2 to 571d61d Compare February 8, 2018 14:40

ellatrix reviewed Feb 8, 2018

View reviewed changes

blocks/rich-text/index.js Outdated

Copy link
Copy Markdown

Member

ellatrix Feb 8, 2018

Choose a reason for hiding this comment

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

I forgot to add content = '' here 🙈

youknowriad force-pushed the try/editable-input-onchange branch 2 times, most recently from e6af488 to 87ca673 Compare February 9, 2018 10:23

mtias mentioned this pull request Feb 9, 2018

Collapse the publish sidebar when making edits to a published post #4978

Merged

aduth reviewed Feb 9, 2018

View reviewed changes

aduth mentioned this pull request Feb 9, 2018

Use per-dependants-set WeakMap-based cache aduth/rememo#1

Merged

aduth mentioned this pull request Feb 12, 2018

Framework: Upgrade Rememo dependency to 2.4.0 #5002

Merged

ellatrix and others added 4 commits February 12, 2018 11:58

WIP: try editable onChange on input

94c816f

RichText: Throttle changes for performance reasons

fe6941f

RichText: Optimize performance by using strict equality instead of is…

8127ba2

…Equal

Fix issue when editor is not initialized yet

efff741

youknowriad force-pushed the try/editable-input-onchange branch from 15fa35c to efff741 Compare February 12, 2018 11:00

RichText: cancel throttled functions

6f334f0

gziolo reviewed Feb 12, 2018

View reviewed changes

Check editor dirtyness to avoid unnecessary onChange calls (fixes und…

0728800

…o button)

gziolo approved these changes Feb 12, 2018

View reviewed changes

youknowriad merged commit b4d3132 into master Feb 12, 2018

ellatrix reviewed Feb 12, 2018

View reviewed changes

blocks/rich-text/index.js

this.updateContent();

}

}

Copy link
Copy Markdown

Member

ellatrix Feb 12, 2018

Choose a reason for hiding this comment

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

Removed line break by accident?

ellatrix reviewed Feb 12, 2018

View reviewed changes

blocks/rich-text/index.js

return;

}

this.fireChange();

Copy link
Copy Markdown

Member

ellatrix Feb 12, 2018

Choose a reason for hiding this comment

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

Why is this removed?

ellatrix mentioned this pull request Feb 12, 2018

Restore firing change before RichText merging #5009

Closed

3 tasks

aduth reviewed Feb 12, 2018

View reviewed changes

aduth deleted the try/editable-input-onchange branch February 13, 2018 13:11

youknowriad mentioned this pull request Feb 15, 2018

Editable: Invoke onChange on every input #2758

Closed

This was referenced Feb 15, 2018

getEditedPostAttribute("content") doesn't return edited content of published posts #5073

Closed

Keyboard Shortcuts: Bind Cmd/Ctrl+S as save #2863

Merged

youknowriad mentioned this pull request Feb 16, 2018

Quote Block: Fixing focus transfer between citation and content #5100

Merged

aduth mentioned this pull request Feb 16, 2018

FIX: After saving, ability to save isn't available until you exit a block #2418

Closed

aduth mentioned this pull request Mar 7, 2018

Post isn't marked unsaved on paragraph changes #4589

Closed

aduth mentioned this pull request May 2, 2018

Rich Text: Determine emptiness by value #5091

Merged

aaronrobertshaw mentioned this pull request Mar 16, 2023

Dimensions Panel: Inconsistent menu item positions #49124

Closed

		@@ -149,6 +151,7 @@ export class RichText extends Component {
		editor.on( 'BeforeExecCommand', this.maybePropagateUndo );

Conversation

youknowriad commented Feb 8, 2018

Uh oh!

aduth commented Feb 8, 2018

Uh oh!

ellatrix commented Feb 8, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

youknowriad commented Feb 9, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ellatrix Feb 12, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aduth commented Feb 9, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

youknowriad commented Feb 10, 2018

Uh oh!

youknowriad commented Feb 12, 2018

Uh oh!

gziolo commented Feb 12, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gziolo Feb 12, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gziolo commented Feb 12, 2018

Uh oh!

gziolo commented Feb 12, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gziolo commented Feb 12, 2018

Uh oh!

youknowriad commented Feb 12, 2018

Uh oh!

aduth commented Feb 12, 2018

Uh oh!

gziolo left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

youknowriad commented Feb 12, 2018

Uh oh!

ellatrix commented Feb 12, 2018

Uh oh!

aduth commented Feb 12, 2018

Uh oh!

aduth commented Feb 12, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ellatrix commented Feb 13, 2018

Uh oh!

aduth commented Feb 13, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

youknowriad commented Feb 9, 2018 •

edited

Loading

ellatrix Feb 12, 2018 •

edited

Loading

aduth commented Feb 9, 2018 •

edited

Loading

gziolo Feb 12, 2018 •

edited

Loading

gziolo commented Feb 12, 2018 •

edited

Loading