From 357351693a1d008eebfb77b88e5c9f87bda5faea Mon Sep 17 00:00:00 2001 From: iseulde Date: Tue, 13 Feb 2018 11:24:02 +0100 Subject: [PATCH 01/16] Overwrite undo + editable signal --- blocks/rich-text/index.js | 103 +++++++++----------- blocks/rich-text/provider.js | 1 + editor/components/provider/index.js | 4 +- editor/store/actions.js | 4 + editor/store/reducer.js | 25 ++++- editor/store/selectors.js | 3 +- editor/store/test/reducer.js | 66 ++++++++++++- editor/utils/with-history/index.js | 74 +++++++++++--- editor/utils/with-history/test/index.js | 122 ++++++++++++++---------- 9 files changed, 274 insertions(+), 128 deletions(-) diff --git a/blocks/rich-text/index.js b/blocks/rich-text/index.js index e68cec125bb2bf..237d0aef6031a4 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,24 @@ 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 || ( value.length === 1 && ! value[ 0 ] ); } /** @@ -180,16 +181,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 ] ); @@ -242,42 +242,18 @@ 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 ) { + onPropagateUndo( event ) { const { onUndo } = this.context; - if ( onUndo && event.command === 'Undo' && ! this.editor.undoManager.hasUndo() ) { - defer( onUndo ); + const { command } = event; - // 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 ( onUndo && ( command === 'Undo' || command === 'Redo' ) ) { + defer( onUndo ); + event.preventDefault(); } } @@ -412,14 +388,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() ) { + + onChange() { + this.isEmpty = this.editor.dom.isEmpty( this.editor.getBody() ); + this.savedContent = this.isEmpty ? [] : this.getContent(); + this.props.onChange( this.savedContent ); + } + + onAddUndo( { lastLevel } ) { + if ( ! lastLevel ) { 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(); } /** @@ -547,7 +534,7 @@ export class RichText extends Component { return; } - this.onChange( false ); + this.onCreateUndoLevel(); const forward = event.keyCode === DELETE; @@ -586,6 +573,7 @@ export class RichText extends Component { } event.preventDefault(); + this.onCreateUndoLevel(); const childNodes = Array.from( rootNode.childNodes ); const index = dom.nodeIndex( selectedNode ); @@ -598,6 +586,7 @@ export class RichText extends Component { this.props.onSplit( beforeElement, afterElement ); } else { event.preventDefault(); + this.onCreateUndoLevel(); if ( event.shiftKey || ! this.props.onSplit ) { this.editor.execCommand( 'InsertLineBreak', false, event ); @@ -614,8 +603,9 @@ export class RichText extends Component { * @param {number} keyCode The key code that has been pressed on the keyboard. */ onKeyUp( { keyCode } ) { + // See https://bugs.chromium.org/p/chromium/issues/detail?id=725890 if ( keyCode === BACKSPACE ) { - this.onSelectionChange(); + this.onChange(); } } @@ -725,17 +715,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 ) ); } @@ -756,7 +735,11 @@ export class RichText extends Component { this.props.value !== prevProps.value && this.props.value !== this.savedContent ) { - this.updateContent(); + const bookmark = this.editor.selection.getBookmark( 2, true ); + + this.savedContent = this.props.value; + this.setContent( this.savedContent ); + this.editor.selection.moveToBookmark( bookmark ); } } @@ -827,7 +810,6 @@ export class RichText extends Component { isSelected = false, formatters, } = this.props; - const { empty } = this.state; const ariaProps = pickAriaProps( this.props ); @@ -835,7 +817,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 = ( @@ -890,6 +872,7 @@ export class RichText extends Component { RichText.contextTypes = { onUndo: noop, canUserUseUnfilteredHTML: noop, + onCreateUndoLevel: noop, }; RichText.defaultProps = { diff --git a/blocks/rich-text/provider.js b/blocks/rich-text/provider.js index d8d608f5bb17ca..ede5951e8614a0 100644 --- a/blocks/rich-text/provider.js +++ b/blocks/rich-text/provider.js @@ -29,6 +29,7 @@ class RichTextProvider extends Component { RichTextProvider.childContextTypes = { onUndo: noop, + onCreateUndoLevel: noop, }; export default RichTextProvider; diff --git a/editor/components/provider/index.js b/editor/components/provider/index.js index 906092835d2c74..a0f556b3194281 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, createUndoLevel, initializeMetaBoxState } from '../../store/actions'; import store from '../../store'; /** @@ -105,10 +105,12 @@ class EditorProvider extends Component { // RichText provider: // // - context.onUndo + // - context.onCreateUndoLevel [ RichTextProvider, bindActionCreators( { onUndo: undo, + onCreateUndoLevel: createUndoLevel, }, this.store.dispatch ), ], diff --git a/editor/store/actions.js b/editor/store/actions.js index 1525b8a059dff2..bdf04a4ac52428 100644 --- a/editor/store/actions.js +++ b/editor/store/actions.js @@ -302,6 +302,10 @@ export function undo() { return { type: 'UNDO' }; } +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 641879e9ce4e0d..e7c921a171f0d6 100644 --- a/editor/store/reducer.js +++ b/editor/store/reducer.js @@ -15,6 +15,9 @@ import { findIndex, reject, omitBy, + includes, + keys, + isEqual, } from 'lodash'; /** @@ -115,7 +118,27 @@ 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( action, previousAction ) { + if ( ! includes( [ 'UPDATE_BLOCK_ATTRIBUTES', 'EDIT_POST', 'RESET_POST' ], action.type ) ) { + return false; + } + + 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 true; + }, + } ), // Track whether changes exist, resetting at each post save. Relies on // editor initialization firing post reset as an effect. diff --git a/editor/store/selectors.js b/editor/store/selectors.js index 33b57d93410242..04d2902fda1743 100644 --- a/editor/store/selectors.js +++ b/editor/store/selectors.js @@ -97,7 +97,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; } /** diff --git a/editor/store/test/reducer.js b/editor/store/test/reducer.js index b1370556aca9c7..9d23c0b6b871e2 100644 --- a/editor/store/test/reducer.js +++ b/editor/store/test/reducer.js @@ -72,7 +72,7 @@ describe( 'state', () => { it( 'should return history (empty edits, blocksByUid, blockOrder), dirty flag by default', () => { const state = editor( undefined, {} ); - expect( state.past ).toEqual( [] ); + expect( state.past ).toEqual( [ state.present ] ); expect( state.future ).toEqual( [] ); expect( state.present.edits ).toEqual( {} ); expect( state.present.blocksByUid ).toEqual( {} ); @@ -819,6 +819,70 @@ 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: [], + } ], + } ); + + 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: [], + } ], + } ); + + 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 f1af9e1f20573b..76aff66bc587b9 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 @@ -14,46 +14,82 @@ import { includes } from 'lodash'; * @return {Function} Enhanced reducer. */ export default function withHistory( reducer, options = {} ) { + const initialPresent = reducer( undefined, {} ); + const initialState = { - past: [], - present: reducer( undefined, {} ), + // Past already contains record of present since changes are buffered in present. + past: [ initialPresent ], + present: initialPresent, future: [], }; + let lastAction; + return ( state = initialState, action ) => { const { past, present, future } = state; + const previousAction = lastAction; + const { + resetTypes = [], + shouldOverwriteState = () => false, + } = options; + + lastAction = action; switch ( action.type ) { case 'UNDO': - // Can't undo if no past - if ( ! past.length ) { - break; + // If there are changes in buffer, push buffer to the future. + if ( last( past ) !== present ) { + return { + past, + present: last( past ), + future: [ present, ...future ], + }; + } + + // Can't undo if no past. + // If the present "buffer" is the same as the last record, + // There is no further past. + if ( past.length < 2 ) { + return state; } + const newPast = dropRight( past ); + return { - past: past.slice( 0, past.length - 1 ), - present: past[ past.length - 1 ], + past: newPast, + present: last( newPast ), 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, first( future ) ], + present: first( future ), + future: drop( future ), + }; + + case 'CREATE_UNDO_LEVEL': + // Already has this level. + if ( last( past ) === present ) { + return state; } return { past: [ ...past, present ], - present: future[ 0 ], - future: future.slice( 1 ), + present, + future: [], }; } const nextPresent = reducer( present, action ); - if ( includes( options.resetTypes, action.type ) ) { + if ( includes( resetTypes, action.type ) ) { return { - past: [], + past: [ nextPresent ], present: nextPresent, future: [], }; @@ -63,6 +99,14 @@ export default function withHistory( reducer, options = {} ) { return state; } + if ( shouldOverwriteState( action, previousAction ) ) { + return { + past, + present: nextPresent, + future, + }; + } + return { past: [ ...past, present ], present: nextPresent, diff --git a/editor/utils/with-history/test/index.js b/editor/utils/with-history/test/index.js index d59dcfc39847f2..4e84092af718e4 100644 --- a/editor/utils/with-history/test/index.js +++ b/editor/utils/with-history/test/index.js @@ -1,121 +1,145 @@ +/** + * External dependencies + */ +import { last } from 'lodash'; + /** * Internal dependencies */ import withHistory from '../'; describe( 'withHistory', () => { - const counter = ( state = 0, { type } ) => ( - type === 'INCREMENT' ? state + 1 : state - ); + const counter = ( state = { count: 0 }, { type } ) => { + if ( type === 'INCREMENT' ) { + return { count: state.count + 1 }; + } + + if ( type === 'RESET' ) { + return { count: 0 }; + } + + return state; + }; + + const reducer = withHistory( counter, { + shouldOverwriteState: ( { type } ) => type === 'INCREMENT', + resetTypes: [ 'RESET_HISTORY' ], + } ); it( 'should return a new reducer', () => { - const reducer = withHistory( counter ); + const state = reducer( undefined, {} ); - expect( typeof reducer ).toBe( 'function' ); - expect( reducer( undefined, {} ) ).toEqual( { - past: [], - present: 0, + expect( state ).toEqual( { + past: [ { count: 0 } ], + present: { count: 0 }, future: [], } ); - } ); - it( 'should track history', () => { - const reducer = withHistory( counter ); + expect( last( state.past ) ).toBe( state.present ); + } ); + it( 'should track changes in present', () => { let state; state = reducer( undefined, {} ); state = reducer( state, { type: 'INCREMENT' } ); expect( state ).toEqual( { - past: [ 0 ], - present: 1, + past: [ { count: 0 } ], + present: { count: 1 }, future: [], } ); } ); - it( 'should perform undo', () => { - const reducer = withHistory( counter ); - + it( 'should create undo level if buffer is available', () => { let state; state = reducer( undefined, {} ); state = reducer( state, { type: 'INCREMENT' } ); - state = reducer( state, { type: 'UNDO' } ); + state = reducer( state, { type: 'CREATE_UNDO_LEVEL' } ); expect( state ).toEqual( { - past: [], - present: 0, - future: [ 1 ], + past: [ { count: 0 }, { count: 1 } ], + present: { count: 1 }, + future: [], } ); - } ); - it( 'should not perform undo on empty past', () => { - const reducer = withHistory( counter ); + expect( state ).toBe( reducer( state, { type: 'CREATE_UNDO_LEVEL' } ) ); + } ); + it( 'should perform undo of buffer', () => { let state; state = reducer( undefined, {} ); state = reducer( state, { type: 'INCREMENT' } ); state = reducer( state, { type: 'UNDO' } ); expect( state ).toEqual( { - past: [], - present: 0, - future: [ 1 ], + past: [ { count: 0 } ], + present: { count: 0 }, + future: [ { count: 1 } ], } ); - } ); - it( 'should perform redo', () => { - const reducer = withHistory( counter ); + expect( last( state.past ) ).toBe( state.present ); + expect( state ).toBe( reducer( state, { type: 'UNDO' } ) ); + } ); + it( 'should perform undo of last level', () => { let state; state = reducer( undefined, {} ); state = reducer( state, { type: 'INCREMENT' } ); - state = reducer( state, { type: 'UNDO' } ); + state = reducer( state, { type: 'CREATE_UNDO_LEVEL' } ); state = reducer( state, { type: 'UNDO' } ); expect( state ).toEqual( { - past: [], - present: 0, - future: [ 1 ], + past: [ { count: 0 } ], + present: { count: 0 }, + future: [ { count: 1 } ], } ); - } ); - it( 'should not perform redo on empty future', () => { - const reducer = withHistory( counter ); + expect( last( state.past ) ).toBe( state.present ); + expect( state ).toBe( reducer( state, { type: 'UNDO' } ) ); + } ); + it( 'should perform redo', () => { let state; state = reducer( undefined, {} ); state = reducer( state, { type: 'INCREMENT' } ); + state = reducer( state, { type: 'CREATE_UNDO_LEVEL' } ); + state = reducer( state, { type: 'UNDO' } ); state = reducer( state, { type: 'REDO' } ); expect( state ).toEqual( { - past: [ 0 ], - present: 1, + past: [ { count: 0 }, { count: 1 } ], + present: { count: 1 }, future: [], } ); + + expect( last( state.past ) ).toBe( state.present ); + 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: 'CREATE_UNDO_LEVEL' } ); state = reducer( state, { type: 'RESET_HISTORY' } ); - state = reducer( state, { type: 'INCREMENT' } ); - state = reducer( state, { type: 'INCREMENT' } ); expect( state ).toEqual( { - past: [ 1, 2 ], - present: 3, + past: [ { count: 1 } ], + present: { count: 1 }, future: [], } ); } ); - it( 'should return same reference if state has not changed', () => { - const reducer = withHistory( counter ); - const original = reducer( undefined, {} ); - const state = reducer( original, {} ); + it( 'should create history record for non buffer types', () => { + let state; + state = reducer( undefined, {} ); + state = reducer( state, { type: 'INCREMENT' } ); + state = reducer( state, { type: 'RESET' } ); - expect( state ).toBe( original ); + expect( state ).toEqual( { + past: [ { count: 0 }, { count: 1 } ], + present: { count: 0 }, + future: [], + } ); } ); } ); From be5268eb29884bd05c2def8a7ce7273c4bde40d3 Mon Sep 17 00:00:00 2001 From: iseulde Date: Tue, 13 Feb 2018 11:58:12 +0100 Subject: [PATCH 02/16] Prevent TinyMCE from accumulating history --- blocks/rich-text/index.js | 3 +++ 1 file changed, 3 insertions(+) diff --git a/blocks/rich-text/index.js b/blocks/rich-text/index.js index 237d0aef6031a4..37b31cb98056f9 100644 --- a/blocks/rich-text/index.js +++ b/blocks/rich-text/index.js @@ -162,6 +162,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, } ); } From aad0c3c1ac888f30b5ea9de6cbad68787b6c4c35 Mon Sep 17 00:00:00 2001 From: iseulde Date: Tue, 13 Feb 2018 11:59:28 +0100 Subject: [PATCH 03/16] Setting content is no longer needed on split if state is in sync --- blocks/rich-text/index.js | 3 --- 1 file changed, 3 deletions(-) diff --git a/blocks/rich-text/index.js b/blocks/rich-text/index.js index 37b31cb98056f9..2f4b32ce05956b 100644 --- a/blocks/rich-text/index.js +++ b/blocks/rich-text/index.js @@ -585,7 +585,6 @@ 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(); @@ -644,10 +643,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 ); } } From 3ae5775110ed29ca2854a8a02a759d7bc8a3e04f Mon Sep 17 00:00:00 2001 From: iseulde Date: Tue, 13 Feb 2018 19:34:16 +0100 Subject: [PATCH 04/16] When setting editor content, do not create an undo level --- blocks/rich-text/index.js | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/blocks/rich-text/index.js b/blocks/rich-text/index.js index 2f4b32ce05956b..ea2486c830904c 100644 --- a/blocks/rich-text/index.js +++ b/blocks/rich-text/index.js @@ -735,11 +735,14 @@ export class RichText extends Component { this.props.value !== prevProps.value && this.props.value !== this.savedContent ) { - const bookmark = this.editor.selection.getBookmark( 2, true ); + // 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 ); + this.savedContent = this.props.value; + this.setContent( this.savedContent ); + this.editor.selection.moveToBookmark( bookmark ); + } ); } } From 5b7ceaba1ecf71ae73dc8a89968a10adc61ae524 Mon Sep 17 00:00:00 2001 From: iseulde Date: Wed, 14 Feb 2018 17:22:44 +0100 Subject: [PATCH 05/16] Add context.onRedo for RichText --- blocks/rich-text/index.js | 11 ++++++++--- blocks/rich-text/provider.js | 1 + editor/components/provider/index.js | 4 +++- editor/store/actions.js | 6 ++++++ 4 files changed, 18 insertions(+), 4 deletions(-) diff --git a/blocks/rich-text/index.js b/blocks/rich-text/index.js index ea2486c830904c..32204dcb2ab3a0 100644 --- a/blocks/rich-text/index.js +++ b/blocks/rich-text/index.js @@ -143,7 +143,6 @@ export class RichText extends Component { this.state = { formats: {}, - empty: ! value || ! value.length, selectedNodeId: 0, }; @@ -251,13 +250,18 @@ export class RichText extends Component { * @param {UndoEvent} event The undo event as triggered by TinyMCE. */ onPropagateUndo( event ) { - const { onUndo } = this.context; + const { onUndo, onRedo } = this.context; const { command } = event; - if ( onUndo && ( command === 'Undo' || command === 'Redo' ) ) { + if ( command === 'Undo' && onUndo ) { defer( onUndo ); event.preventDefault(); } + + if ( command === 'Redo' && onRedo ) { + defer( onRedo ); + event.preventDefault(); + } } /** @@ -874,6 +878,7 @@ export class RichText extends Component { RichText.contextTypes = { onUndo: noop, + onRedo: noop, canUserUseUnfilteredHTML: noop, onCreateUndoLevel: noop, }; diff --git a/blocks/rich-text/provider.js b/blocks/rich-text/provider.js index ede5951e8614a0..1a68e75e2d564c 100644 --- a/blocks/rich-text/provider.js +++ b/blocks/rich-text/provider.js @@ -29,6 +29,7 @@ class RichTextProvider extends Component { RichTextProvider.childContextTypes = { onUndo: noop, + onRedo: noop, onCreateUndoLevel: noop, }; diff --git a/editor/components/provider/index.js b/editor/components/provider/index.js index a0f556b3194281..94749075b97fb7 100644 --- a/editor/components/provider/index.js +++ b/editor/components/provider/index.js @@ -19,7 +19,7 @@ import { /** * Internal Dependencies */ -import { setupEditor, undo, createUndoLevel, initializeMetaBoxState } from '../../store/actions'; +import { setupEditor, undo, redo, createUndoLevel, initializeMetaBoxState } from '../../store/actions'; import store from '../../store'; /** @@ -105,11 +105,13 @@ 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 bdf04a4ac52428..78f42b7af98f34 100644 --- a/editor/store/actions.js +++ b/editor/store/actions.js @@ -302,6 +302,12 @@ 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' }; } From 36b0d557d504a163a9a2265c6a0a532b8531b21f Mon Sep 17 00:00:00 2001 From: iseulde Date: Wed, 14 Feb 2018 17:27:00 +0100 Subject: [PATCH 06/16] Fix failing test --- blocks/rich-text/test/index.js | 1 + 1 file changed, 1 insertion(+) diff --git a/blocks/rich-text/test/index.js b/blocks/rich-text/test/index.js index fc0a9af926d2cd..192f1a1866b47d 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, } ); } ); From a052e3a11805cf81b88c4742a7b333fc988101bd Mon Sep 17 00:00:00 2001 From: iseulde Date: Thu, 15 Feb 2018 12:42:48 +0100 Subject: [PATCH 07/16] Simplify history reducer --- blocks/rich-text/index.js | 2 +- editor/store/test/reducer.js | 6 +-- editor/utils/with-history/index.js | 70 +++++++++---------------- editor/utils/with-history/test/index.js | 32 +++++------ 4 files changed, 43 insertions(+), 67 deletions(-) diff --git a/blocks/rich-text/index.js b/blocks/rich-text/index.js index 32204dcb2ab3a0..b95a8537bd9735 100644 --- a/blocks/rich-text/index.js +++ b/blocks/rich-text/index.js @@ -146,7 +146,7 @@ export class RichText extends Component { selectedNodeId: 0, }; - this.isEmpty = ! value || ! value.length || ( value.length === 1 && ! value[ 0 ] ); + this.isEmpty = ! value || ! value.length; } /** diff --git a/editor/store/test/reducer.js b/editor/store/test/reducer.js index 9d23c0b6b871e2..45f717c3b4b91e 100644 --- a/editor/store/test/reducer.js +++ b/editor/store/test/reducer.js @@ -72,7 +72,7 @@ describe( 'state', () => { it( 'should return history (empty edits, blocksByUid, blockOrder), dirty flag by default', () => { const state = editor( undefined, {} ); - expect( state.past ).toEqual( [ state.present ] ); + expect( state.past ).toEqual( [] ); expect( state.future ).toEqual( [] ); expect( state.present.edits ).toEqual( {} ); expect( state.present.blocksByUid ).toEqual( {} ); @@ -849,7 +849,7 @@ describe( 'state', () => { }, } ); - expect( state.past ).toHaveLength( 2 ); + expect( state.past ).toHaveLength( 1 ); } ); it( 'should not overwrite present history if updating same attributes', () => { @@ -880,7 +880,7 @@ describe( 'state', () => { }, } ); - expect( state.past ).toHaveLength( 3 ); + expect( state.past ).toHaveLength( 2 ); } ); } ); } ); diff --git a/editor/utils/with-history/index.js b/editor/utils/with-history/index.js index 76aff66bc587b9..975003b421ba59 100644 --- a/editor/utils/with-history/index.js +++ b/editor/utils/with-history/index.js @@ -14,50 +14,36 @@ import { includes, first, last, drop, dropRight } from 'lodash'; * @return {Function} Enhanced reducer. */ export default function withHistory( reducer, options = {} ) { - const initialPresent = reducer( undefined, {} ); - const initialState = { - // Past already contains record of present since changes are buffered in present. - past: [ initialPresent ], - present: initialPresent, + past: [], + present: reducer( undefined, {} ), future: [], }; - let lastAction; + let lastAction = null; + let shouldCreateUndoLevel = false; + + const { + resetTypes = [], + shouldOverwriteState = () => false, + } = options; return ( state = initialState, action ) => { const { past, present, future } = state; const previousAction = lastAction; - const { - resetTypes = [], - shouldOverwriteState = () => false, - } = options; lastAction = action; switch ( action.type ) { case 'UNDO': - // If there are changes in buffer, push buffer to the future. - if ( last( past ) !== present ) { - return { - past, - present: last( past ), - future: [ present, ...future ], - }; - } - // Can't undo if no past. - // If the present "buffer" is the same as the last record, - // There is no further past. - if ( past.length < 2 ) { + if ( ! past.length ) { return state; } - const newPast = dropRight( past ); - return { - past: newPast, - present: last( newPast ), + past: dropRight( past ), + present: last( past ), future: [ present, ...future ], }; case 'REDO': @@ -67,29 +53,21 @@ export default function withHistory( reducer, options = {} ) { } return { - past: [ ...past, first( future ) ], + past: [ ...past, present ], present: first( future ), future: drop( future ), }; case 'CREATE_UNDO_LEVEL': - // Already has this level. - if ( last( past ) === present ) { - return state; - } - - return { - past: [ ...past, present ], - present, - future: [], - }; + shouldCreateUndoLevel = true; + return state; } const nextPresent = reducer( present, action ); if ( includes( resetTypes, action.type ) ) { return { - past: [ nextPresent ], + past: [], present: nextPresent, future: [], }; @@ -99,16 +77,18 @@ export default function withHistory( reducer, options = {} ) { return state; } - if ( shouldOverwriteState( action, previousAction ) ) { - return { - past, - present: nextPresent, - future, - }; + let nextPast = [ ...past, present ]; + + shouldCreateUndoLevel = ! past.length || shouldCreateUndoLevel; + + if ( ! shouldCreateUndoLevel && shouldOverwriteState( action, previousAction ) ) { + nextPast = past; } + 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 4e84092af718e4..7e7831ed4705df 100644 --- a/editor/utils/with-history/test/index.js +++ b/editor/utils/with-history/test/index.js @@ -1,8 +1,3 @@ -/** - * External dependencies - */ -import { last } from 'lodash'; - /** * Internal dependencies */ @@ -30,12 +25,10 @@ describe( 'withHistory', () => { const state = reducer( undefined, {} ); expect( state ).toEqual( { - past: [ { count: 0 } ], + past: [], present: { count: 0 }, future: [], } ); - - expect( last( state.past ) ).toBe( state.present ); } ); it( 'should track changes in present', () => { @@ -50,19 +43,25 @@ describe( 'withHistory', () => { } ); } ); - it( 'should create undo level if buffer is available', () => { + it( 'should create undo level', () => { let state; state = reducer( undefined, {} ); state = reducer( state, { type: 'INCREMENT' } ); state = reducer( state, { type: 'CREATE_UNDO_LEVEL' } ); expect( state ).toEqual( { - past: [ { count: 0 }, { count: 1 } ], + past: [ { count: 0 } ], present: { count: 1 }, future: [], } ); - expect( state ).toBe( reducer( state, { type: 'CREATE_UNDO_LEVEL' } ) ); + state = reducer( state, { type: 'INCREMENT' } ); + + expect( state ).toEqual( { + past: [ { count: 0 }, { count: 1 } ], + present: { count: 2 }, + future: [], + } ); } ); it( 'should perform undo of buffer', () => { @@ -72,12 +71,11 @@ describe( 'withHistory', () => { state = reducer( state, { type: 'UNDO' } ); expect( state ).toEqual( { - past: [ { count: 0 } ], + past: [], present: { count: 0 }, future: [ { count: 1 } ], } ); - expect( last( state.past ) ).toBe( state.present ); expect( state ).toBe( reducer( state, { type: 'UNDO' } ) ); } ); @@ -89,12 +87,11 @@ describe( 'withHistory', () => { state = reducer( state, { type: 'UNDO' } ); expect( state ).toEqual( { - past: [ { count: 0 } ], + past: [], present: { count: 0 }, future: [ { count: 1 } ], } ); - expect( last( state.past ) ).toBe( state.present ); expect( state ).toBe( reducer( state, { type: 'UNDO' } ) ); } ); @@ -107,12 +104,11 @@ describe( 'withHistory', () => { state = reducer( state, { type: 'REDO' } ); expect( state ).toEqual( { - past: [ { count: 0 }, { count: 1 } ], + past: [ { count: 0 } ], present: { count: 1 }, future: [], } ); - expect( last( state.past ) ).toBe( state.present ); expect( state ).toBe( reducer( state, { type: 'REDO' } ) ); } ); @@ -124,7 +120,7 @@ describe( 'withHistory', () => { state = reducer( state, { type: 'RESET_HISTORY' } ); expect( state ).toEqual( { - past: [ { count: 1 } ], + past: [], present: { count: 1 }, future: [], } ); From 7e0c36068f616218f1dea5a50323058dc5a1e516 Mon Sep 17 00:00:00 2001 From: iseulde Date: Thu, 15 Feb 2018 22:03:35 +0100 Subject: [PATCH 08/16] Rewrite shouldOverwriteState --- editor/store/reducer.js | 45 +++++++++++++++++++++--------------- editor/store/test/reducer.js | 8 +++++-- 2 files changed, 32 insertions(+), 21 deletions(-) diff --git a/editor/store/reducer.js b/editor/store/reducer.js index e7c921a171f0d6..8ebc7c9b1d2b02 100644 --- a/editor/store/reducer.js +++ b/editor/store/reducer.js @@ -15,7 +15,6 @@ import { findIndex, reject, omitBy, - includes, keys, isEqual, } from 'lodash'; @@ -98,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. @@ -120,24 +144,7 @@ export const editor = flow( [ // Track undo history, starting at editor initialization. partialRight( withHistory, { resetTypes: [ 'SETUP_NEW_POST', 'SETUP_EDITOR' ], - shouldOverwriteState( action, previousAction ) { - if ( ! includes( [ 'UPDATE_BLOCK_ATTRIBUTES', 'EDIT_POST', 'RESET_POST' ], action.type ) ) { - return false; - } - - 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 true; - }, + shouldOverwriteState, } ), // Track whether changes exist, resetting at each post save. Relies on diff --git a/editor/store/test/reducer.js b/editor/store/test/reducer.js index 45f717c3b4b91e..78bc8222efe396 100644 --- a/editor/store/test/reducer.js +++ b/editor/store/test/reducer.js @@ -833,6 +833,8 @@ describe( 'state', () => { } ], } ); + expect( state.past ).toHaveLength( 1 ); + state = editor( state, { type: 'UPDATE_BLOCK_ATTRIBUTES', uid: 'kumquat', @@ -849,7 +851,7 @@ describe( 'state', () => { }, } ); - expect( state.past ).toHaveLength( 1 ); + expect( state.past ).toHaveLength( 2 ); } ); it( 'should not overwrite present history if updating same attributes', () => { @@ -864,6 +866,8 @@ describe( 'state', () => { } ], } ); + expect( state.past ).toHaveLength( 1 ); + state = editor( state, { type: 'UPDATE_BLOCK_ATTRIBUTES', uid: 'kumquat', @@ -880,7 +884,7 @@ describe( 'state', () => { }, } ); - expect( state.past ).toHaveLength( 2 ); + expect( state.past ).toHaveLength( 3 ); } ); } ); } ); From f11607ceea484943616e320a6a092e5ed940d1ac Mon Sep 17 00:00:00 2001 From: iseulde Date: Thu, 15 Feb 2018 22:28:42 +0100 Subject: [PATCH 09/16] Rewrite tests --- editor/utils/with-history/index.js | 2 +- editor/utils/with-history/test/index.js | 88 +++++++++++++++---------- 2 files changed, 53 insertions(+), 37 deletions(-) diff --git a/editor/utils/with-history/index.js b/editor/utils/with-history/index.js index 975003b421ba59..04dfef423cee25 100644 --- a/editor/utils/with-history/index.js +++ b/editor/utils/with-history/index.js @@ -20,7 +20,7 @@ export default function withHistory( reducer, options = {} ) { future: [], }; - let lastAction = null; + let lastAction; let shouldCreateUndoLevel = false; const { diff --git a/editor/utils/with-history/test/index.js b/editor/utils/with-history/test/index.js index 7e7831ed4705df..5a11179a4f5758 100644 --- a/editor/utils/with-history/test/index.js +++ b/editor/utils/with-history/test/index.js @@ -5,23 +5,11 @@ import withHistory from '../'; describe( 'withHistory', () => { const counter = ( state = { count: 0 }, { type } ) => { - if ( type === 'INCREMENT' ) { - return { count: state.count + 1 }; - } - - if ( type === 'RESET' ) { - return { count: 0 }; - } - - return state; + return type === 'INCREMENT' ? { count: state.count + 1 } : state; }; - const reducer = withHistory( counter, { - shouldOverwriteState: ( { type } ) => type === 'INCREMENT', - resetTypes: [ 'RESET_HISTORY' ], - } ); - it( 'should return a new reducer', () => { + const reducer = withHistory( counter ); const state = reducer( undefined, {} ); expect( state ).toEqual( { @@ -32,6 +20,8 @@ describe( 'withHistory', () => { } ); it( 'should track changes in present', () => { + const reducer = withHistory( counter ); + let state; state = reducer( undefined, {} ); state = reducer( state, { type: 'INCREMENT' } ); @@ -44,10 +34,12 @@ describe( 'withHistory', () => { } ); it( 'should create undo level', () => { + const reducer = withHistory( counter ); + let state; state = reducer( undefined, {} ); state = reducer( state, { type: 'INCREMENT' } ); - state = reducer( state, { type: 'CREATE_UNDO_LEVEL' } ); + // state = reducer( state, { type: 'CREATE_UNDO_LEVEL' } ); expect( state ).toEqual( { past: [ { count: 0 } ], @@ -64,26 +56,12 @@ describe( 'withHistory', () => { } ); } ); - it( 'should perform undo of buffer', () => { - let state; - state = reducer( undefined, {} ); - state = reducer( state, { type: 'INCREMENT' } ); - state = reducer( state, { type: 'UNDO' } ); - - expect( state ).toEqual( { - past: [], - present: { count: 0 }, - future: [ { count: 1 } ], - } ); - - expect( state ).toBe( reducer( state, { type: 'UNDO' } ) ); - } ); + it( 'should perform undo', () => { + const reducer = withHistory( counter ); - it( 'should perform undo of last level', () => { let state; state = reducer( undefined, {} ); state = reducer( state, { type: 'INCREMENT' } ); - state = reducer( state, { type: 'CREATE_UNDO_LEVEL' } ); state = reducer( state, { type: 'UNDO' } ); expect( state ).toEqual( { @@ -96,10 +74,11 @@ describe( 'withHistory', () => { } ); it( 'should perform redo', () => { + const reducer = withHistory( counter ); + let state; state = reducer( undefined, {} ); state = reducer( state, { type: 'INCREMENT' } ); - state = reducer( state, { type: 'CREATE_UNDO_LEVEL' } ); state = reducer( state, { type: 'UNDO' } ); state = reducer( state, { type: 'REDO' } ); @@ -113,10 +92,11 @@ describe( 'withHistory', () => { } ); 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: 'CREATE_UNDO_LEVEL' } ); state = reducer( state, { type: 'RESET_HISTORY' } ); expect( state ).toEqual( { @@ -126,15 +106,51 @@ describe( 'withHistory', () => { } ); } ); - it( 'should create history record for non buffer types', () => { + 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: 'RESET' } ); + + expect( state ).toEqual( { + past: [ { count: 0 } ], + present: { count: 1 }, + future: [], + } ); + + state = reducer( state, { type: 'INCREMENT' } ); + + expect( state ).toEqual( { + past: [ { count: 0 } ], + present: { count: 2 }, + future: [], + } ); + } ); + + 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: 'CREATE_UNDO_LEVEL' } ); + + expect( state ).toEqual( { + past: [ { count: 0 } ], + present: { count: 1 }, + future: [], + } ); + + state = reducer( state, { type: 'INCREMENT' } ); expect( state ).toEqual( { past: [ { count: 0 }, { count: 1 } ], - present: { count: 0 }, + present: { count: 2 }, future: [], } ); } ); From d1da833598dd7554126ae49f142b66fd6227b531 Mon Sep 17 00:00:00 2001 From: iseulde Date: Thu, 15 Feb 2018 22:35:24 +0100 Subject: [PATCH 10/16] Adjust hasEditorUndo --- editor/store/selectors.js | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/editor/store/selectors.js b/editor/store/selectors.js index 04d2902fda1743..33b57d93410242 100644 --- a/editor/store/selectors.js +++ b/editor/store/selectors.js @@ -97,8 +97,7 @@ export function isSavingMetaBoxes( state ) { * @return {boolean} Whether undo history exists. */ export function hasEditorUndo( state ) { - const { past, present } = state.editor; - return past.length > 1 || last( past ) !== present; + return state.editor.past.length > 0; } /** From 2217c1d8e6da888fd7c527d69a3c4885dc7bf463 Mon Sep 17 00:00:00 2001 From: iseulde Date: Thu, 15 Feb 2018 22:56:38 +0100 Subject: [PATCH 11/16] Minimise diff --- editor/utils/with-history/test/index.js | 81 +++++++++++++------------ 1 file changed, 42 insertions(+), 39 deletions(-) diff --git a/editor/utils/with-history/test/index.js b/editor/utils/with-history/test/index.js index 5a11179a4f5758..0c89efffe79c35 100644 --- a/editor/utils/with-history/test/index.js +++ b/editor/utils/with-history/test/index.js @@ -4,54 +4,39 @@ import withHistory from '../'; describe( 'withHistory', () => { - const counter = ( state = { count: 0 }, { type } ) => { - return type === 'INCREMENT' ? { count: state.count + 1 } : state; - }; + const counter = ( state = 0, { type } ) => ( + type === 'INCREMENT' ? state + 1 : state + ); it( 'should return a new reducer', () => { const reducer = withHistory( counter ); - const state = reducer( undefined, {} ); - expect( state ).toEqual( { + expect( typeof reducer ).toBe( 'function' ); + expect( reducer( undefined, {} ) ).toEqual( { past: [], - present: { count: 0 }, - future: [], - } ); - } ); - - it( 'should track changes in present', () => { - const reducer = withHistory( counter ); - - let state; - state = reducer( undefined, {} ); - state = reducer( state, { type: 'INCREMENT' } ); - - expect( state ).toEqual( { - past: [ { count: 0 } ], - present: { count: 1 }, + present: 0, future: [], } ); } ); - it( 'should create undo level', () => { + it( 'should track history', () => { const reducer = withHistory( counter ); let state; state = reducer( undefined, {} ); state = reducer( state, { type: 'INCREMENT' } ); - // state = reducer( state, { type: 'CREATE_UNDO_LEVEL' } ); expect( state ).toEqual( { - past: [ { count: 0 } ], - present: { count: 1 }, + past: [ 0 ], + present: 1, future: [], } ); state = reducer( state, { type: 'INCREMENT' } ); expect( state ).toEqual( { - past: [ { count: 0 }, { count: 1 } ], - present: { count: 2 }, + past: [ 0, 1 ], + present: 2, future: [], } ); } ); @@ -66,9 +51,14 @@ describe( 'withHistory', () => { expect( state ).toEqual( { past: [], - present: { count: 0 }, - future: [ { count: 1 } ], + present: 0, + future: [ 1 ], } ); + } ); + + it( 'should not perform undo on empty past', () => { + const reducer = withHistory( counter ); + const state = reducer( undefined, {} ); expect( state ).toBe( reducer( state, { type: 'UNDO' } ) ); } ); @@ -83,10 +73,15 @@ describe( 'withHistory', () => { state = reducer( state, { type: 'REDO' } ); expect( state ).toEqual( { - past: [ { count: 0 } ], - present: { count: 1 }, + past: [ 0 ], + present: 1, future: [], } ); + } ); + + it( 'should not perform redo on empty future', () => { + const reducer = withHistory( counter ); + const state = reducer( undefined, {} ); expect( state ).toBe( reducer( state, { type: 'REDO' } ) ); } ); @@ -101,11 +96,19 @@ describe( 'withHistory', () => { expect( state ).toEqual( { past: [], - present: { count: 1 }, + 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, {} ); + + expect( state ).toBe( original ); + } ); + it( 'should overwrite present state with option.shouldOverwriteState', () => { const reducer = withHistory( counter, { shouldOverwriteState: ( { type } ) => type === 'INCREMENT', @@ -116,16 +119,16 @@ describe( 'withHistory', () => { state = reducer( state, { type: 'INCREMENT' } ); expect( state ).toEqual( { - past: [ { count: 0 } ], - present: { count: 1 }, + past: [ 0 ], + present: 1, future: [], } ); state = reducer( state, { type: 'INCREMENT' } ); expect( state ).toEqual( { - past: [ { count: 0 } ], - present: { count: 2 }, + past: [ 0 ], + present: 2, future: [], } ); } ); @@ -141,16 +144,16 @@ describe( 'withHistory', () => { state = reducer( state, { type: 'CREATE_UNDO_LEVEL' } ); expect( state ).toEqual( { - past: [ { count: 0 } ], - present: { count: 1 }, + past: [ 0 ], + present: 1, future: [], } ); state = reducer( state, { type: 'INCREMENT' } ); expect( state ).toEqual( { - past: [ { count: 0 }, { count: 1 } ], - present: { count: 2 }, + past: [ 0, 1 ], + present: 2, future: [], } ); } ); From 9cd9c180290f4b9281ee795f391b4a04586309fe Mon Sep 17 00:00:00 2001 From: iseulde Date: Thu, 15 Feb 2018 23:07:11 +0100 Subject: [PATCH 12/16] Leave comment for backspace onChange --- blocks/rich-text/index.js | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/blocks/rich-text/index.js b/blocks/rich-text/index.js index b95a8537bd9735..58b8f375860308 100644 --- a/blocks/rich-text/index.js +++ b/blocks/rich-text/index.js @@ -609,7 +609,8 @@ export class RichText extends Component { * @param {number} keyCode The key code that has been pressed on the keyboard. */ onKeyUp( { keyCode } ) { - // See https://bugs.chromium.org/p/chromium/issues/detail?id=725890 + // The input event does not fire when the whole field is selected and + // BACKSPACE is pressed. if ( keyCode === BACKSPACE ) { this.onChange(); } From bbe1dff88b0df8e329dc638b97ffc1e32d8d8bc0 Mon Sep 17 00:00:00 2001 From: iseulde Date: Fri, 16 Feb 2018 11:16:37 +0100 Subject: [PATCH 13/16] Use change event instead of addundo which fires during init --- blocks/rich-text/index.js | 12 ++---------- 1 file changed, 2 insertions(+), 10 deletions(-) diff --git a/blocks/rich-text/index.js b/blocks/rich-text/index.js index 58b8f375860308..52756d06f9adfc 100644 --- a/blocks/rich-text/index.js +++ b/blocks/rich-text/index.js @@ -138,7 +138,6 @@ export class RichText extends Component { 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 = { @@ -191,7 +190,8 @@ export class RichText extends Component { editor.on( 'PastePreProcess', this.onPastePreProcess, true /* Add before core handlers */ ); editor.on( 'paste', this.onPaste, true /* Add before core handlers */ ); editor.on( 'input', this.onChange ); - editor.on( 'addundo', this.onAddUndo ); + // The change event in TinyMCE fires every time an undo level is added. + editor.on( 'change', this.onCreateUndoLevel ); patterns.apply( this, [ editor ] ); @@ -402,14 +402,6 @@ export class RichText extends Component { this.props.onChange( this.savedContent ); } - onAddUndo( { lastLevel } ) { - if ( ! lastLevel ) { - return; - } - - this.onCreateUndoLevel(); - } - onCreateUndoLevel() { // Always ensure the content is up-to-date. this.onChange(); From 2c7d283ed3e6fb0a013edb1e3d9077e75d54c1fb Mon Sep 17 00:00:00 2001 From: iseulde Date: Fri, 16 Feb 2018 11:20:49 +0100 Subject: [PATCH 14/16] Remove dirty references --- blocks/rich-text/index.js | 4 ---- 1 file changed, 4 deletions(-) diff --git a/blocks/rich-text/index.js b/blocks/rich-text/index.js index 52756d06f9adfc..cb08f620872c97 100644 --- a/blocks/rich-text/index.js +++ b/blocks/rich-text/index.js @@ -392,8 +392,6 @@ 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() { @@ -791,8 +789,6 @@ export class RichText extends Component { this.setState( ( state ) => ( { formats: merge( {}, state.formats, formats ), } ) ); - - this.editor.setDirty( true ); } render() { From 6d4cd204231605511e2e517f85ba688548499bb9 Mon Sep 17 00:00:00 2001 From: iseulde Date: Fri, 16 Feb 2018 11:26:01 +0100 Subject: [PATCH 15/16] Restore updateContent --- blocks/rich-text/index.js | 21 +++++++++++++-------- 1 file changed, 13 insertions(+), 8 deletions(-) diff --git a/blocks/rich-text/index.js b/blocks/rich-text/index.js index cb08f620872c97..8a94d86794e8b9 100644 --- a/blocks/rich-text/index.js +++ b/blocks/rich-text/index.js @@ -710,6 +710,18 @@ export class RichText extends Component { this.setState( { formats, focusPosition, selectedNodeId: this.state.selectedNodeId + 1 } ); } + updateContent() { + // 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 = '' ) { this.editor.setContent( renderToString( content ) ); } @@ -730,14 +742,7 @@ export class RichText extends Component { this.props.value !== prevProps.value && this.props.value !== this.savedContent ) { - // 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 ); - } ); + this.updateContent(); } } From 9750e4bff2219d2a9ed2a862d2e0c9fd0951bb65 Mon Sep 17 00:00:00 2001 From: iseulde Date: Fri, 16 Feb 2018 11:29:20 +0100 Subject: [PATCH 16/16] Avoid wastefully calculating nextPast --- editor/utils/with-history/index.js | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/editor/utils/with-history/index.js b/editor/utils/with-history/index.js index 04dfef423cee25..f036430dbdc8d3 100644 --- a/editor/utils/with-history/index.js +++ b/editor/utils/with-history/index.js @@ -77,12 +77,12 @@ export default function withHistory( reducer, options = {} ) { return state; } - let nextPast = [ ...past, present ]; + let nextPast = past; shouldCreateUndoLevel = ! past.length || shouldCreateUndoLevel; - if ( ! shouldCreateUndoLevel && shouldOverwriteState( action, previousAction ) ) { - nextPast = past; + if ( shouldCreateUndoLevel || ! shouldOverwriteState( action, previousAction ) ) { + nextPast = [ ...past, present ]; } shouldCreateUndoLevel = false;