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

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

Merged
merged 16 commits into from
Feb 16, 2018
Merged
Show file tree
Hide file tree
Changes from 12 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
120 changes: 56 additions & 64 deletions blocks/rich-text/index.js
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
/**
* External dependencies
*/
import tinymce from 'tinymce';
import classnames from 'classnames';
import {
last,
Expand Down Expand Up @@ -131,22 +130,23 @@ export class RichText extends Component {
this.getSettings = this.getSettings.bind( this );
this.onSetup = this.onSetup.bind( this );
this.onChange = this.onChange.bind( this );
this.onInput = this.onChange.bind( this, false );
this.onNewBlock = this.onNewBlock.bind( this );
this.onNodeChange = this.onNodeChange.bind( this );
this.onKeyDown = this.onKeyDown.bind( this );
this.onKeyUp = this.onKeyUp.bind( this );
this.changeFormats = this.changeFormats.bind( this );
this.onSelectionChange = this.onSelectionChange.bind( this );
this.maybePropagateUndo = this.maybePropagateUndo.bind( this );
this.onPropagateUndo = this.onPropagateUndo.bind( this );
this.onPastePreProcess = this.onPastePreProcess.bind( this );
this.onPaste = this.onPaste.bind( this );
this.onAddUndo = this.onAddUndo.bind( this );
this.onCreateUndoLevel = this.onCreateUndoLevel.bind( this );

this.state = {
formats: {},
empty: ! value || ! value.length,
selectedNodeId: 0,
};

this.isEmpty = ! value || ! value.length;
}

/**
Expand All @@ -161,6 +161,9 @@ export class RichText extends Component {
return ( this.props.getSettings || identity )( {
...settings,
forced_root_block: this.props.multiline || false,
// Allow TinyMCE to keep one undo level for comparing changes.
// Prevent it otherwise from accumulating any history.
custom_undo_redo_levels: 1,
} );
}

Expand All @@ -180,16 +183,15 @@ export class RichText extends Component {
} );

editor.on( 'init', this.onInit );
editor.on( 'focusout', this.onChange );
editor.on( 'NewBlock', this.onNewBlock );
editor.on( 'nodechange', this.onNodeChange );
editor.on( 'keydown', this.onKeyDown );
editor.on( 'keyup', this.onKeyUp );
editor.on( 'selectionChange', this.onSelectionChange );
editor.on( 'BeforeExecCommand', this.maybePropagateUndo );
editor.on( 'BeforeExecCommand', this.onPropagateUndo );
editor.on( 'PastePreProcess', this.onPastePreProcess, true /* Add before core handlers */ );
editor.on( 'paste', this.onPaste, true /* Add before core handlers */ );
editor.on( 'input', this.onInput );
editor.on( 'input', this.onChange );
editor.on( 'addundo', this.onAddUndo );

patterns.apply( this, [ editor ] );

Expand Down Expand Up @@ -242,42 +244,23 @@ export class RichText extends Component {
} );
}

/**
* Handles the global selection change event.
*/
onSelectionChange() {
const isActive = document.activeElement === this.editor.getBody();
// We must check this because selectionChange is a global event.
if ( ! isActive ) {
return;
}

const isEmpty = tinymce.DOM.isEmpty( this.editor.getBody() );
if ( this.state.empty !== isEmpty ) {
this.setState( { empty: isEmpty } );
}
}

/**
* Handles an undo event from tinyMCE.
*
* When user attempts Undo when empty Undo stack, propagate undo
* action to context handler. The compromise here is that: TinyMCE
* handles Undo until change, at which point `editor.save` resets
* history. If no history exists, let context handler have a turn.
* Defer in case an immediate undo causes TinyMCE to be destroyed,
* if other undo behaviors test presence of an input field.
*
* @param {UndoEvent} event The undo event as triggered by tinyMCE.
* @param {UndoEvent} event The undo event as triggered by TinyMCE.
*/
maybePropagateUndo( event ) {
const { onUndo } = this.context;
if ( onUndo && event.command === 'Undo' && ! this.editor.undoManager.hasUndo() ) {
onPropagateUndo( event ) {
const { onUndo, onRedo } = this.context;
const { command } = event;

if ( command === 'Undo' && onUndo ) {
defer( onUndo );
Copy link
Member

@aduth aduth Feb 13, 2018

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

😰

Copy link
Member Author

Choose a reason for hiding this comment

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

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

event.preventDefault();
}

// We could return false here to stop other TinyMCE event handlers
// from running, but we assume TinyMCE won't do anything on an
// empty undo stack anyways.
if ( command === 'Redo' && onRedo ) {
defer( onRedo );
event.preventDefault();
}
}

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

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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

if ( checkIfDirty && ! this.editor.isDirty() ) {
Copy link
Member

Choose a reason for hiding this comment

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

I imagine this should no longer be necessary:

this.editor.setDirty( true );

Copy link
Member Author

Choose a reason for hiding this comment

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

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.


onChange() {
this.isEmpty = this.editor.dom.isEmpty( this.editor.getBody() );
this.savedContent = this.isEmpty ? [] : this.getContent();
Copy link
Member

Choose a reason for hiding this comment

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

Unrelated: Is this check necessary anymore since getContent will try to filter out the empty nodes and assign an empty array instead?

This is all in an effort to avoid the this.isEmpty instance variable altogether, since it should be trivial to calculate at any point as _.empty( this.props.value )

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not sure. I'm thinking there could be more cases where something would get inserted by MCE or the browser where the field looks empty but isn't actually. Maybe @youknowriad knows more about the introduction of this check here. In any case, this sounds like a good thing to reconsider in a separate PR.

this.props.onChange( this.savedContent );
}

onAddUndo( { lastLevel } ) {
if ( ! lastLevel ) {
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

@ellatrix ellatrix Feb 16, 2018

Choose a reason for hiding this comment

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

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

return;
}
const isEmpty = tinymce.DOM.isEmpty( this.editor.getBody() );
this.savedContent = isEmpty ? [] : this.getContent();
this.props.onChange( this.savedContent );
this.editor.save();

this.onCreateUndoLevel();
}

onCreateUndoLevel() {
// Always ensure the content is up-to-date.
this.onChange();
this.context.onCreateUndoLevel();
}

/**
Expand Down Expand Up @@ -547,7 +541,7 @@ export class RichText extends Component {
return;
}

this.onChange( false );
this.onCreateUndoLevel();

const forward = event.keyCode === DELETE;

Expand Down Expand Up @@ -586,6 +580,7 @@ export class RichText extends Component {
}

event.preventDefault();
this.onCreateUndoLevel();

const childNodes = Array.from( rootNode.childNodes );
const index = dom.nodeIndex( selectedNode );
Expand All @@ -594,10 +589,10 @@ export class RichText extends Component {
const beforeElement = nodeListToReact( beforeNodes, createTinyMCEElement );
const afterElement = nodeListToReact( afterNodes, createTinyMCEElement );

this.setContent( beforeElement );
this.props.onSplit( beforeElement, afterElement );
} else {
event.preventDefault();
this.onCreateUndoLevel();
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

See also #7482

Copy link
Member Author

Choose a reason for hiding this comment

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

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


if ( event.shiftKey || ! this.props.onSplit ) {
this.editor.execCommand( 'InsertLineBreak', false, event );
Expand All @@ -614,8 +609,10 @@ export class RichText extends Component {
* @param {number} keyCode The key code that has been pressed on the keyboard.
*/
onKeyUp( { keyCode } ) {
// The input event does not fire when the whole field is selected and
// BACKSPACE is pressed.
if ( keyCode === BACKSPACE ) {
this.onSelectionChange();
this.onChange();
Copy link
Member

Choose a reason for hiding this comment

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

Previously we were only invoking onSelectionChange to keep empty state in sync. Why do we suddenly need to fire an onChange ?

Copy link
Member Author

Choose a reason for hiding this comment

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

Because similarly to selectionChange, input doesn't seem to fire if you fully select the field and then backspace. Could use more comments.

}
}

Expand Down Expand Up @@ -651,10 +648,8 @@ export class RichText extends Component {

const beforeElement = nodeListToReact( beforeFragment.childNodes, createTinyMCEElement );
const afterElement = nodeListToReact( filterEmptyNodes( afterFragment.childNodes ), createTinyMCEElement );
this.setContent( beforeElement );
Copy link
Member

Choose a reason for hiding this comment

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

I'm not entirely clear why this change was warranted, but in basic usage splitting a paragraph in the middle works as I'd expect.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, before we had to set the content to make sure it was synced by the time props.onSplit happens.

this.props.onSplit( beforeElement, afterElement, ...blocks );
} else {
this.setContent( [] );
this.props.onSplit( [], [], ...blocks );
}
}
Expand Down Expand Up @@ -725,17 +720,6 @@ export class RichText extends Component {
this.setState( { formats, focusPosition, selectedNodeId: this.state.selectedNodeId + 1 } );
}

updateContent() {
const bookmark = this.editor.selection.getBookmark( 2, true );
this.savedContent = this.props.value;
this.setContent( this.savedContent );
this.editor.selection.moveToBookmark( bookmark );

// Saving the editor on updates avoid unecessary onChanges calls
// These calls can make the focus jump
this.editor.save();
}

setContent( content = '' ) {
this.editor.setContent( renderToString( content ) );
}
Expand All @@ -756,7 +740,14 @@ export class RichText extends Component {
this.props.value !== prevProps.value &&
this.props.value !== this.savedContent
) {
this.updateContent();
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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

// Do not trigger a `addUndo` event.
this.editor.undoManager.ignore( () => {
const bookmark = this.editor.selection.getBookmark( 2, true );

this.savedContent = this.props.value;
this.setContent( this.savedContent );
this.editor.selection.moveToBookmark( bookmark );
} );
}
}

Expand Down Expand Up @@ -827,15 +818,14 @@ export class RichText extends Component {
isSelected = false,
formatters,
} = this.props;
const { empty } = this.state;

const ariaProps = pickAriaProps( this.props );

// Generating a key that includes `tagName` ensures that if the tag
// changes, we unmount and destroy the previous TinyMCE element, then
// mount and initialize a new child element in its place.
const key = [ 'editor', Tagname ].join();
const isPlaceholderVisible = placeholder && ( ! isSelected || keepPlaceholderOnFocus ) && empty;
const isPlaceholderVisible = placeholder && ( ! isSelected || keepPlaceholderOnFocus ) && this.isEmpty;
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm thinking this change could potentially create an issue if the block is not rerendered when the this.isEmpty property changes. Do you think we should use forceUpdate when its value changes?

Copy link
Member Author

Choose a reason for hiding this comment

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

Won't this component always rerender if the value props change as well?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, it should unless the parent component is "uncontrolled" or something. Granted we don't use it this way though

Copy link
Member

Choose a reason for hiding this comment

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

As noted above, I'd like if we could avoid assigning isEmpty as an instance of state variable at all, and instead calculate when we need it as empty( this.props.value )

Copy link
Member Author

Choose a reason for hiding this comment

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

Sounds good, but I don't want to go too deep into empty checks in this PR.

const classes = classnames( wrapperClassName, 'blocks-rich-text' );

const formatToolbar = (
Expand Down Expand Up @@ -889,7 +879,9 @@ export class RichText extends Component {

RichText.contextTypes = {
onUndo: noop,
onRedo: noop,
canUserUseUnfilteredHTML: noop,
onCreateUndoLevel: noop,
};

RichText.defaultProps = {
Expand Down
2 changes: 2 additions & 0 deletions blocks/rich-text/provider.js
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,8 @@ class RichTextProvider extends Component {

RichTextProvider.childContextTypes = {
onUndo: noop,
onRedo: noop,
onCreateUndoLevel: noop,
};

export default RichTextProvider;
1 change: 1 addition & 0 deletions blocks/rich-text/test/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -219,6 +219,7 @@ describe( 'RichText', () => {
expect( wrapper.instance().getSettings( settings ) ).toEqual( {
setting: 'hi',
forced_root_block: false,
custom_undo_redo_levels: 1,
} );
} );

Expand Down
6 changes: 5 additions & 1 deletion editor/components/provider/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ import {
/**
* Internal Dependencies
*/
import { setupEditor, undo, initializeMetaBoxState } from '../../store/actions';
import { setupEditor, undo, redo, createUndoLevel, initializeMetaBoxState } from '../../store/actions';
import store from '../../store';

/**
Expand Down Expand Up @@ -105,10 +105,14 @@ class EditorProvider extends Component {
// RichText provider:
//
// - context.onUndo
// - context.onRedo
// - context.onCreateUndoLevel
[
RichTextProvider,
bindActionCreators( {
onUndo: undo,
onRedo: redo,
onCreateUndoLevel: createUndoLevel,
}, this.store.dispatch ),
],

Expand Down
10 changes: 10 additions & 0 deletions editor/store/actions.js
Original file line number Diff line number Diff line change
Expand Up @@ -302,6 +302,16 @@ export function undo() {
return { type: 'UNDO' };
}

/**
* Returns an action object used in signalling that undo history record should
* be created.
*
* @return {Object} Action object.
*/
export function createUndoLevel() {
return { type: 'CREATE_UNDO_LEVEL' };
}

/**
* Returns an action object used in signalling that the blocks
* corresponding to the specified UID set are to be removed.
Expand Down
32 changes: 31 additions & 1 deletion editor/store/reducer.js
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,8 @@ import {
findIndex,
reject,
omitBy,
keys,
isEqual,
} from 'lodash';

/**
Expand Down Expand Up @@ -95,6 +97,31 @@ function getFlattenedBlocks( blocks ) {
return flattenedBlocks;
}

/**
* Option for the history reducer. When the block ID and updated attirbute keys
* are the same as previously, the history reducer should overwrite its present
* state.
*
* @param {Object} action The currently dispatched action.
* @param {Object} previousAction The previously dispatched action.
*
* @return {boolean} Whether or not to overwrite present state.
*/
function 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;
}

/**
* Undoable reducer returning the editor post state, including blocks parsed
* from current HTML markup.
Expand All @@ -115,7 +142,10 @@ export const editor = flow( [
combineReducers,

// Track undo history, starting at editor initialization.
partialRight( withHistory, { resetTypes: [ 'SETUP_NEW_POST', 'SETUP_EDITOR' ] } ),
partialRight( withHistory, {
resetTypes: [ 'SETUP_NEW_POST', 'SETUP_EDITOR' ],
shouldOverwriteState,
} ),

// Track whether changes exist, resetting at each post save. Relies on
// editor initialization firing post reset as an effect.
Expand Down
Loading