diff --git a/blocks/rich-text/index.js b/blocks/rich-text/index.js index e68cec125bb2b..8a94d86794e8b 100644 --- a/blocks/rich-text/index.js +++ b/blocks/rich-text/index.js @@ -1,7 +1,6 @@ /** * External dependencies */ -import tinymce from 'tinymce'; import classnames from 'classnames'; import { last, @@ -131,22 +130,22 @@ 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.onCreateUndoLevel = this.onCreateUndoLevel.bind( this ); this.state = { formats: {}, - empty: ! value || ! value.length, selectedNodeId: 0, }; + + this.isEmpty = ! value || ! value.length; } /** @@ -161,6 +160,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, } ); } @@ -180,16 +182,16 @@ 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 ); + // The change event in TinyMCE fires every time an undo level is added. + editor.on( 'change', this.onCreateUndoLevel ); patterns.apply( this, [ editor ] ); @@ -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 ); + 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(); } } @@ -409,17 +392,18 @@ export class RichText extends Component { /** * Handles any case where the content of the tinyMCE instance has changed. - * - * @param {boolean} checkIfDirty Check whether the editor is dirty before calling onChange. */ - onChange( checkIfDirty = true ) { - if ( checkIfDirty && ! this.editor.isDirty() ) { - return; - } - const isEmpty = tinymce.DOM.isEmpty( this.editor.getBody() ); - this.savedContent = isEmpty ? [] : this.getContent(); + + onChange() { + this.isEmpty = this.editor.dom.isEmpty( this.editor.getBody() ); + this.savedContent = this.isEmpty ? [] : this.getContent(); this.props.onChange( this.savedContent ); - this.editor.save(); + } + + onCreateUndoLevel() { + // Always ensure the content is up-to-date. + this.onChange(); + this.context.onCreateUndoLevel(); } /** @@ -547,7 +531,7 @@ export class RichText extends Component { return; } - this.onChange( false ); + this.onCreateUndoLevel(); const forward = event.keyCode === DELETE; @@ -586,6 +570,7 @@ export class RichText extends Component { } event.preventDefault(); + this.onCreateUndoLevel(); const childNodes = Array.from( rootNode.childNodes ); const index = dom.nodeIndex( selectedNode ); @@ -594,10 +579,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(); if ( event.shiftKey || ! this.props.onSplit ) { this.editor.execCommand( 'InsertLineBreak', false, event ); @@ -614,8 +599,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(); } } @@ -651,10 +638,8 @@ export class RichText extends Component { const beforeElement = nodeListToReact( beforeFragment.childNodes, createTinyMCEElement ); const afterElement = nodeListToReact( filterEmptyNodes( afterFragment.childNodes ), createTinyMCEElement ); - this.setContent( beforeElement ); this.props.onSplit( beforeElement, afterElement, ...blocks ); } else { - this.setContent( [] ); this.props.onSplit( [], [], ...blocks ); } } @@ -726,14 +711,15 @@ export class RichText extends Component { } 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(); + // Do not trigger a change event coming from the TinyMCE undo manager. + // Our global state is already up-to-date. + 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 ); + } ); } setContent( content = '' ) { @@ -808,8 +794,6 @@ export class RichText extends Component { this.setState( ( state ) => ( { formats: merge( {}, state.formats, formats ), } ) ); - - this.editor.setDirty( true ); } render() { @@ -827,7 +811,6 @@ export class RichText extends Component { isSelected = false, formatters, } = this.props; - const { empty } = this.state; const ariaProps = pickAriaProps( this.props ); @@ -835,7 +818,7 @@ export class RichText extends Component { // 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; const classes = classnames( wrapperClassName, 'blocks-rich-text' ); const formatToolbar = ( @@ -889,7 +872,9 @@ export class RichText extends Component { RichText.contextTypes = { onUndo: noop, + onRedo: noop, canUserUseUnfilteredHTML: noop, + onCreateUndoLevel: noop, }; RichText.defaultProps = { diff --git a/blocks/rich-text/provider.js b/blocks/rich-text/provider.js index d8d608f5bb17c..1a68e75e2d564 100644 --- a/blocks/rich-text/provider.js +++ b/blocks/rich-text/provider.js @@ -29,6 +29,8 @@ class RichTextProvider extends Component { RichTextProvider.childContextTypes = { onUndo: noop, + onRedo: noop, + onCreateUndoLevel: noop, }; export default RichTextProvider; diff --git a/blocks/rich-text/test/index.js b/blocks/rich-text/test/index.js index fc0a9af926d2c..192f1a1866b47 100644 --- a/blocks/rich-text/test/index.js +++ b/blocks/rich-text/test/index.js @@ -219,6 +219,7 @@ describe( 'RichText', () => { expect( wrapper.instance().getSettings( settings ) ).toEqual( { setting: 'hi', forced_root_block: false, + custom_undo_redo_levels: 1, } ); } ); diff --git a/editor/components/provider/index.js b/editor/components/provider/index.js index 906092835d2c7..94749075b97fb 100644 --- a/editor/components/provider/index.js +++ b/editor/components/provider/index.js @@ -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'; /** @@ -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 ), ], diff --git a/editor/store/actions.js b/editor/store/actions.js index 1525b8a059dff..78f42b7af98f3 100644 --- a/editor/store/actions.js +++ b/editor/store/actions.js @@ -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. diff --git a/editor/store/reducer.js b/editor/store/reducer.js index 641879e9ce4e0..8ebc7c9b1d2b0 100644 --- a/editor/store/reducer.js +++ b/editor/store/reducer.js @@ -15,6 +15,8 @@ import { findIndex, reject, omitBy, + keys, + isEqual, } from 'lodash'; /** @@ -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. @@ -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. diff --git a/editor/store/test/reducer.js b/editor/store/test/reducer.js index b1370556aca9c..78bc8222efe39 100644 --- a/editor/store/test/reducer.js +++ b/editor/store/test/reducer.js @@ -819,6 +819,74 @@ describe( 'state', () => { expect( state.present.blocksByUid ).toBe( state.present.blocksByUid ); } ); } ); + + describe( 'withHistory', () => { + it( 'should overwrite present history if updating same attributes', () => { + let state; + + state = editor( state, { + type: 'RESET_BLOCKS', + blocks: [ { + uid: 'kumquat', + attributes: {}, + innerBlocks: [], + } ], + } ); + + expect( state.past ).toHaveLength( 1 ); + + state = editor( state, { + type: 'UPDATE_BLOCK_ATTRIBUTES', + uid: 'kumquat', + attributes: { + test: 1, + }, + } ); + + state = editor( state, { + type: 'UPDATE_BLOCK_ATTRIBUTES', + uid: 'kumquat', + attributes: { + test: 2, + }, + } ); + + expect( state.past ).toHaveLength( 2 ); + } ); + + it( 'should not overwrite present history if updating same attributes', () => { + let state; + + state = editor( state, { + type: 'RESET_BLOCKS', + blocks: [ { + uid: 'kumquat', + attributes: {}, + innerBlocks: [], + } ], + } ); + + expect( state.past ).toHaveLength( 1 ); + + state = editor( state, { + type: 'UPDATE_BLOCK_ATTRIBUTES', + uid: 'kumquat', + attributes: { + test: 1, + }, + } ); + + state = editor( state, { + type: 'UPDATE_BLOCK_ATTRIBUTES', + uid: 'kumquat', + attributes: { + other: 1, + }, + } ); + + expect( state.past ).toHaveLength( 3 ); + } ); + } ); } ); describe( 'currentPost()', () => { diff --git a/editor/utils/with-history/index.js b/editor/utils/with-history/index.js index f1af9e1f20573..f036430dbdc8d 100644 --- a/editor/utils/with-history/index.js +++ b/editor/utils/with-history/index.js @@ -1,7 +1,7 @@ /** * External dependencies */ -import { includes } from 'lodash'; +import { includes, first, last, drop, dropRight } from 'lodash'; /** * Reducer enhancer which transforms the result of the original reducer into an @@ -20,38 +20,52 @@ export default function withHistory( reducer, options = {} ) { future: [], }; + let lastAction; + let shouldCreateUndoLevel = false; + + const { + resetTypes = [], + shouldOverwriteState = () => false, + } = options; + return ( state = initialState, action ) => { const { past, present, future } = state; + const previousAction = lastAction; + + lastAction = action; switch ( action.type ) { case 'UNDO': - // Can't undo if no past + // Can't undo if no past. if ( ! past.length ) { - break; + return state; } return { - past: past.slice( 0, past.length - 1 ), - present: past[ past.length - 1 ], + past: dropRight( past ), + present: last( past ), future: [ present, ...future ], }; - case 'REDO': - // Can't redo if no future + // Can't redo if no future. if ( ! future.length ) { - break; + return state; } return { past: [ ...past, present ], - present: future[ 0 ], - future: future.slice( 1 ), + present: first( future ), + future: drop( future ), }; + + case 'CREATE_UNDO_LEVEL': + shouldCreateUndoLevel = true; + return state; } const nextPresent = reducer( present, action ); - if ( includes( options.resetTypes, action.type ) ) { + if ( includes( resetTypes, action.type ) ) { return { past: [], present: nextPresent, @@ -63,8 +77,18 @@ export default function withHistory( reducer, options = {} ) { return state; } + let nextPast = past; + + shouldCreateUndoLevel = ! past.length || shouldCreateUndoLevel; + + if ( shouldCreateUndoLevel || ! shouldOverwriteState( action, previousAction ) ) { + nextPast = [ ...past, present ]; + } + + shouldCreateUndoLevel = false; + return { - past: [ ...past, present ], + past: nextPast, present: nextPresent, future: [], }; diff --git a/editor/utils/with-history/test/index.js b/editor/utils/with-history/test/index.js index d59dcfc39847f..0c89efffe79c3 100644 --- a/editor/utils/with-history/test/index.js +++ b/editor/utils/with-history/test/index.js @@ -31,6 +31,14 @@ describe( 'withHistory', () => { present: 1, future: [], } ); + + state = reducer( state, { type: 'INCREMENT' } ); + + expect( state ).toEqual( { + past: [ 0, 1 ], + present: 2, + future: [], + } ); } ); it( 'should perform undo', () => { @@ -50,72 +58,103 @@ describe( 'withHistory', () => { it( 'should not perform undo on empty past', () => { const reducer = withHistory( counter ); + const state = reducer( undefined, {} ); + + expect( state ).toBe( reducer( state, { type: 'UNDO' } ) ); + } ); + + it( 'should perform redo', () => { + const reducer = withHistory( counter ); let state; state = reducer( undefined, {} ); state = reducer( state, { type: 'INCREMENT' } ); state = reducer( state, { type: 'UNDO' } ); + state = reducer( state, { type: 'REDO' } ); expect( state ).toEqual( { - past: [], - present: 0, - future: [ 1 ], + past: [ 0 ], + present: 1, + future: [], } ); } ); - it( 'should perform redo', () => { + it( 'should not perform redo on empty future', () => { const reducer = withHistory( counter ); + const state = reducer( undefined, {} ); + + expect( state ).toBe( reducer( state, { type: 'REDO' } ) ); + } ); + + it( 'should reset history by options.resetTypes', () => { + const reducer = withHistory( counter, { resetTypes: [ 'RESET_HISTORY' ] } ); let state; state = reducer( undefined, {} ); state = reducer( state, { type: 'INCREMENT' } ); - state = reducer( state, { type: 'UNDO' } ); - state = reducer( state, { type: 'UNDO' } ); + state = reducer( state, { type: 'RESET_HISTORY' } ); expect( state ).toEqual( { past: [], - present: 0, - future: [ 1 ], + present: 1, + future: [], } ); } ); - it( 'should not perform redo on empty future', () => { + it( 'should return same reference if state has not changed', () => { const reducer = withHistory( counter ); + const original = reducer( undefined, {} ); + const state = reducer( original, {} ); + + expect( state ).toBe( original ); + } ); + + it( 'should overwrite present state with option.shouldOverwriteState', () => { + const reducer = withHistory( counter, { + shouldOverwriteState: ( { type } ) => type === 'INCREMENT', + } ); let state; state = reducer( undefined, {} ); state = reducer( state, { type: 'INCREMENT' } ); - state = reducer( state, { type: 'REDO' } ); expect( state ).toEqual( { past: [ 0 ], present: 1, future: [], } ); + + state = reducer( state, { type: 'INCREMENT' } ); + + expect( state ).toEqual( { + past: [ 0 ], + present: 2, + future: [], + } ); } ); - it( 'should reset history by options.resetTypes', () => { - const reducer = withHistory( counter, { resetTypes: [ 'RESET_HISTORY' ] } ); + it( 'should create undo level with option.shouldOverwriteState and CREATE_UNDO_LEVEL', () => { + const reducer = withHistory( counter, { + shouldOverwriteState: ( { type } ) => type === 'INCREMENT', + } ); let state; state = reducer( undefined, {} ); state = reducer( state, { type: 'INCREMENT' } ); - state = reducer( state, { type: 'RESET_HISTORY' } ); - state = reducer( state, { type: 'INCREMENT' } ); - state = reducer( state, { type: 'INCREMENT' } ); + state = reducer( state, { type: 'CREATE_UNDO_LEVEL' } ); expect( state ).toEqual( { - past: [ 1, 2 ], - present: 3, + past: [ 0 ], + present: 1, future: [], } ); - } ); - it( 'should return same reference if state has not changed', () => { - const reducer = withHistory( counter ); - const original = reducer( undefined, {} ); - const state = reducer( original, {} ); + state = reducer( state, { type: 'INCREMENT' } ); - expect( state ).toBe( original ); + expect( state ).toEqual( { + past: [ 0, 1 ], + present: 2, + future: [], + } ); } ); } );