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

RichText onChange is erroneously called (and puts editor in dirty state) when wrapped in withFilters HOC #6134

Closed
westonruter opened this issue Apr 12, 2018 · 5 comments · Fixed by #6150
Labels
[Feature] Rich Text Related to the Rich Text component that allows developers to render a contenteditable [Type] Bug An existing feature does not function as intended

Comments

@westonruter
Copy link
Member

Issue Overview

I'm extending blocks to conditionally add a notice, using wp.hooks.addFilter( 'blocks.BlockEdit'… ), for example:

wp.hooks.addFilter(
	'blocks.BlockEdit',
	'example/add-notice',
	function( BlockEdit ) {
		var NewBlockEdit = function( props ) {
			return [
				wp.element.createElement(
					wp.components.Notice,
					{
						status: 'info',
						isDismissible: false,
						key: 'example-notice'
					},
					'This is an example notice.'
				),
				wp.element.createElement(
					BlockEdit,
					_.extend( {}, props, { key: 'example-original-edit' } )
				)
			];
		};
		return NewBlockEdit;
	}
);

In doing this, I've found that when a block uses the RichText component that the overall editor state will become initially dirty after first loading the page, if the component doesn't in fact error due to TinyMCE not being instantiated. When using a production build, normally the editor prop is not initialized yet here and an error is raised:

this.isEmpty = this.editor.dom.isEmpty( this.editor.getBody() );

Even when the editor is initialized, however, the result of calling onChange is that the editor is put into a dirty state, even when no changes have been made by the user. With the above wp.hooks.addFilter( 'blocks.BlockEdit'… ) call in place, if the editor is loaded with a paragraph block, then if you then try to reload the page after making no edits then you'll get prompted with the onbeforeunload “Are you sure?” prompt, as well as the Update button is initially enabled.

In digging through the stack trace it seems the reason is due to the withFilters HOC call to forceUpdate after applying filters:

Down the stack trace this then results in the RichEdit component getting unmounted and componentWillUnmount is called which in turn unconditionally calls onChange even when there are no edits:

componentWillUnmount() {
this.onChange();
}

Steps to Reproduce

  1. Add the above addFilter code to a plugin.
  2. Load a post that contains a paragraph block or image block.
  3. Notice that without making any changes, the initial editor state is dirty when it should be clean.

Possible Solution

Check if this.editor is defined and in a dirty state itself before calling onChange during unmounting?

--- a/blocks/rich-text/index.js
+++ b/blocks/rich-text/index.js
@@ -700,7 +700,9 @@ export class RichText extends Component {
 	}
 
 	componentWillUnmount() {
-		this.onChange();
+		if ( this.editor && this.editor.isDirty() ) {
+			this.onChange();
+		}
 	}
 
 	componentDidUpdate( prevProps ) {
@westonruter westonruter added the [Type] Bug An existing feature does not function as intended label Apr 12, 2018
@gziolo gziolo added the [Feature] Rich Text Related to the Rich Text component that allows developers to render a contenteditable label Apr 12, 2018
@gziolo
Copy link
Member

gziolo commented Apr 12, 2018

I can confirm the issue with unnecessary this.onChange(); calls executed every time I register or unregister a filter. Every RichText dispatches an action when it happens even when there were no changes introduced.

@gziolo
Copy link
Member

gziolo commented Apr 12, 2018

Down the stack trace this then results in the RichEdit component getting unmounted and componentWillUnmount is called which in turn unconditionally calls onChange even when there are no edits:

I can confirm that the proposed change fixes this issue. I don't see any reason why onChange would have to be called in the case where there were no changes introduced by the user. @iseulde or @youknowriad can you confirm that it is the proper way of preventing the unwanted Redux state update?

@youknowriad
Copy link
Contributor

I haven't tested but this issue I see is that we don't "save" the editor when it's not dirty anymore (when we call updateContent or onChange. So this won't work consistently IMO

@ellatrix
Copy link
Member

Also not a fan of adding this.editor.isDirty(). If want to check if the editor is initialized, you can check editor.initialized? Is that what you're wanting to do?

Also not sure if the onChange is really needed on unmount... Everything should be synced whenever there is a change in the editor. At first sight I think it is fine to remove. It seems to be a remnant from when we only synced the value on blur.

@youknowriad
Copy link
Contributor

Also not sure if the onChange is really needed on unmount... Everything should be synced whenever there is a change in the editor. At first sight I think it is fine to remove. It seems to be a remnant from when we only synced the value on blur.

I agree with this statement, but I'd test carefully the different usecases, of splitting/merging/switching etc... :) fast typing + splitting

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Rich Text Related to the Rich Text component that allows developers to render a contenteditable [Type] Bug An existing feature does not function as intended
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants