From 935c7cb947604be32a2104b18a7a8aa1d99c8cbb Mon Sep 17 00:00:00 2001 From: iseulde Date: Tue, 13 Nov 2018 17:03:47 +0100 Subject: [PATCH 1/4] Own undo history --- .../editor/src/components/rich-text/index.js | 96 +++++++------------ .../rich-text/remove-browser-shortcuts.js | 45 +++++++++ .../src/components/rich-text/tinymce.js | 14 ++- .../e2e/specs/__snapshots__/undo.test.js.snap | 36 +++++++ test/e2e/specs/undo.test.js | 44 ++++++++- 5 files changed, 169 insertions(+), 66 deletions(-) create mode 100644 packages/editor/src/components/rich-text/remove-browser-shortcuts.js diff --git a/packages/editor/src/components/rich-text/index.js b/packages/editor/src/components/rich-text/index.js index fbf64b2c4c526..d42e753ab2943 100644 --- a/packages/editor/src/components/rich-text/index.js +++ b/packages/editor/src/components/rich-text/index.js @@ -3,7 +3,6 @@ */ import classnames from 'classnames'; import { - defer, find, isNil, isEqual, @@ -21,7 +20,7 @@ import { getScrollContainer, } from '@wordpress/dom'; import { createBlobURL } from '@wordpress/blob'; -import { BACKSPACE, DELETE, ENTER, rawShortcut } from '@wordpress/keycodes'; +import { BACKSPACE, DELETE, ENTER, LEFT, RIGHT, UP, DOWN } from '@wordpress/keycodes'; import { withDispatch, withSelect } from '@wordpress/data'; import { pasteHandler, children, getBlockTransforms, findTransform } from '@wordpress/blocks'; import { withInstanceId, withSafeTimeout, compose } from '@wordpress/compose'; @@ -61,6 +60,7 @@ import { pickAriaProps } from './aria'; import { getPatterns } from './patterns'; import { withBlockEditContext } from '../block-edit/context'; import { ListEdit } from './list-edit'; +import { RemoveBrowserShortcuts } from './remove-browser-shortcuts'; /** * Browser dependencies @@ -91,7 +91,6 @@ export class RichText extends Component { this.onSplit = this.props.unstableOnSplit; } - this.onInit = this.onInit.bind( this ); this.onSetup = this.onSetup.bind( this ); this.onFocus = this.onFocus.bind( this ); this.onChange = this.onChange.bind( this ); @@ -99,7 +98,6 @@ export class RichText extends Component { this.onDeleteKeyDown = this.onDeleteKeyDown.bind( this ); this.onKeyDown = this.onKeyDown.bind( this ); this.onKeyUp = this.onKeyUp.bind( this ); - this.onPropagateUndo = this.onPropagateUndo.bind( this ); this.onPaste = this.onPaste.bind( this ); this.onCreateUndoLevel = this.onCreateUndoLevel.bind( this ); this.setFocusedElement = this.setFocusedElement.bind( this ); @@ -129,14 +127,7 @@ export class RichText extends Component { this.state = {}; this.usedDeprecatedChildrenSource = Array.isArray( value ); - } - - componentDidMount() { - document.addEventListener( 'selectionchange', this.onSelectionChange ); - } - - componentWillUnmount() { - document.removeEventListener( 'selectionchange', this.onSelectionChange ); + this.lastHistoryValue = value; } setRef( node ) { @@ -157,12 +148,7 @@ export class RichText extends Component { */ onSetup( editor ) { this.editor = editor; - - editor.on( 'init', this.onInit ); editor.on( 'nodechange', this.onNodeChange ); - editor.on( 'BeforeExecCommand', this.onPropagateUndo ); - // The change event in TinyMCE fires every time an undo level is added. - editor.on( 'change', this.onCreateUndoLevel ); } setFocusedElement() { @@ -171,35 +157,6 @@ export class RichText extends Component { } } - onInit() { - this.editor.shortcuts.add( rawShortcut.primary( 'z' ), '', 'Undo' ); - this.editor.shortcuts.add( rawShortcut.primaryShift( 'z' ), '', 'Redo' ); - - // Remove TinyMCE Core shortcut for consistency with global editor - // shortcuts. Also clashes with Mac browsers. - this.editor.shortcuts.remove( 'meta+y', '', 'Redo' ); - } - - /** - * Handles an undo event from TinyMCE. - * - * @param {UndoEvent} event The undo event as triggered by TinyMCE. - */ - onPropagateUndo( event ) { - const { onUndo, onRedo } = this.props; - const { command } = event; - - if ( command === 'Undo' && onUndo ) { - defer( onUndo ); - event.preventDefault(); - } - - if ( command === 'Redo' && onRedo ) { - defer( onRedo ); - event.preventDefault(); - } - } - /** * Get the current record (value and selection) from props and state. * @@ -411,7 +368,9 @@ export class RichText extends Component { const record = this.createRecord(); const transformed = this.patterns.reduce( ( accumlator, transform ) => transform( accumlator ), record ); - this.onChange( transformed ); + this.onChange( transformed, { + withoutHistory: true, + } ); } onCompositionEnd() { @@ -451,7 +410,7 @@ export class RichText extends Component { * @param {boolean} _withoutApply If true, the record won't be applied to * the live DOM. */ - onChange( record, _withoutApply ) { + onChange( record, { withoutHistory, _withoutApply } = {} ) { if ( ! _withoutApply ) { this.applyRecord( record ); } @@ -461,28 +420,20 @@ export class RichText extends Component { this.savedContent = this.valueToFormat( record ); this.props.onChange( this.savedContent ); this.setState( { start, end } ); - } - onCreateUndoLevel( event ) { - // TinyMCE fires a `change` event when the first letter in an instance - // is typed. This should not create a history record in Gutenberg. - // https://github.com/tinymce/tinymce/blob/4.7.11/src/core/main/ts/api/UndoManager.ts#L116-L125 - // In other cases TinyMCE won't fire a `change` with at least a previous - // record present, so this is a reliable check. - // https://github.com/tinymce/tinymce/blob/4.7.11/src/core/main/ts/api/UndoManager.ts#L272-L275 - if ( event && event.lastLevel === null ) { - return; + if ( ! withoutHistory ) { + this.onCreateUndoLevel(); } + } - // Always ensure the content is up-to-date. This is needed because e.g. - // making something bold will trigger a TinyMCE change event but no - // input event. Avoid dispatching an action if the original event is - // blur because the content will already be up-to-date. - if ( ! event || ! event.originalEvent || event.originalEvent.type !== 'blur' ) { - this.onChange( this.createRecord(), true ); + onCreateUndoLevel() { + // If the content is the same, no level needs to be created. + if ( this.lastHistoryValue === this.savedContent ) { + return; } this.props.onCreateUndoLevel(); + this.lastHistoryValue = this.savedContent; } /** @@ -629,6 +580,10 @@ export class RichText extends Component { this.splitContent(); } } + + if ( [ LEFT, RIGHT, UP, DOWN ].indexOf( keyCode ) >= 0 ) { + this.onCreateUndoLevel(); + } } /** @@ -808,6 +763,18 @@ export class RichText extends Component { const record = this.formatToValue( value ); this.applyRecord( record ); } + + if ( isSelected && ! prevProps.isSelected ) { + document.addEventListener( 'selectionchange', this.onSelectionChange ); + window.addEventListener( 'mousedown', this.onCreateUndoLevel ); + window.addEventListener( 'touchstart', this.onCreateUndoLevel ); + } + + if ( ! isSelected && prevProps.isSelected ) { + document.removeEventListener( 'selectionchange', this.onSelectionChange ); + window.removeEventListener( 'mousedown', this.onCreateUndoLevel ); + window.removeEventListener( 'touchstart', this.onCreateUndoLevel ); + } } formatToValue( value ) { @@ -963,6 +930,7 @@ export class RichText extends Component { ) } + { isSelected && } ); } diff --git a/packages/editor/src/components/rich-text/remove-browser-shortcuts.js b/packages/editor/src/components/rich-text/remove-browser-shortcuts.js new file mode 100644 index 0000000000000..165566a5bbbd1 --- /dev/null +++ b/packages/editor/src/components/rich-text/remove-browser-shortcuts.js @@ -0,0 +1,45 @@ +/** + * External dependencies + */ +import { fromPairs } from 'lodash'; + +/** + * WordPress dependencies + */ +import { rawShortcut } from '@wordpress/keycodes'; +import { KeyboardShortcuts } from '@wordpress/components'; + +/** + * Set of keyboard shortcuts handled internally by RichText. + * + * @type {Array} + */ +const HANDLED_SHORTCUTS = [ + rawShortcut.primary( 'z' ), + rawShortcut.primaryShift( 'z' ), + rawShortcut.primary( 'y' ), +]; + +/** + * An instance of a KeyboardShortcuts element pre-bound for the handled + * shortcuts. Since shortcuts never change, the element can be considered + * static, and can be skipped in reconciliation. + * + * @type {WPElement} + */ +const SHORTCUTS_ELEMENT = ( + { + return [ shortcut, ( event ) => event.preventDefault() ]; + } ) ) } + /> +); + +/** + * Component which registered keyboard event handlers to prevent default + * behaviors for key combinations otherwise handled internally by RichText. + * + * @return {WPElement} WordPress element. + */ +export const RemoveBrowserShortcuts = () => SHORTCUTS_ELEMENT; diff --git a/packages/editor/src/components/rich-text/tinymce.js b/packages/editor/src/components/rich-text/tinymce.js index 1888e3c2bc91e..91dc9612c9ae1 100644 --- a/packages/editor/src/components/rich-text/tinymce.js +++ b/packages/editor/src/components/rich-text/tinymce.js @@ -221,7 +221,18 @@ export default class TinyMCE extends Component { } ); editor.on( 'init', () => { - // See https://github.com/tinymce/tinymce/blob/master/src/core/main/ts/keyboard/FormatShortcuts.ts + // History is handled internally by RichText. + // + // See: https://github.com/tinymce/tinymce/blob/master/src/core/main/ts/api/UndoManager.ts + [ 'z', 'y' ].forEach( ( character ) => { + editor.shortcuts.remove( `meta+${ character }` ); + } ); + editor.shortcuts.remove( 'meta+shift+z' ); + + // Reset TinyMCE's default formatting shortcuts, since + // RichText supports only registered formats. + // + // See: https://github.com/tinymce/tinymce/blob/master/src/core/main/ts/keyboard/FormatShortcuts.ts [ 'b', 'i', 'u' ].forEach( ( character ) => { editor.shortcuts.remove( `meta+${ character }` ); } ); @@ -229,6 +240,7 @@ export default class TinyMCE extends Component { editor.shortcuts.remove( `access+${ number }` ); } ); + // Restore the original `setHTML` once initialized. editor.dom.setHTML = setHTML; } ); diff --git a/test/e2e/specs/__snapshots__/undo.test.js.snap b/test/e2e/specs/__snapshots__/undo.test.js.snap index 812ab8e25993a..1871326750e11 100644 --- a/test/e2e/specs/__snapshots__/undo.test.js.snap +++ b/test/e2e/specs/__snapshots__/undo.test.js.snap @@ -13,3 +13,39 @@ exports[`undo Should undo to expected level intervals 1`] = `

test

" `; + +exports[`undo should undo typing after arrow navigation 1`] = ` +" +

before keyboar after keyboardd

+" +`; + +exports[`undo should undo typing after arrow navigation 2`] = ` +" +

before keyboard

+" +`; + +exports[`undo should undo typing after mouse move 1`] = ` +" +

before move after move

+" +`; + +exports[`undo should undo typing after mouse move 2`] = ` +" +

before move

+" +`; + +exports[`undo should undo typing after non input change 1`] = ` +" +

before keyboard after keyboard

+" +`; + +exports[`undo should undo typing after non input change 2`] = ` +" +

before keyboard 

+" +`; diff --git a/test/e2e/specs/undo.test.js b/test/e2e/specs/undo.test.js index cf671065a4375..82c71382d8398 100644 --- a/test/e2e/specs/undo.test.js +++ b/test/e2e/specs/undo.test.js @@ -10,10 +10,52 @@ import { } from '../support/utils'; describe( 'undo', () => { - beforeAll( async () => { + beforeEach( async () => { await newPost(); } ); + it( 'should undo typing after mouse move', async () => { + await clickBlockAppender(); + + await page.keyboard.type( 'before move' ); + await page.mouse.down(); + await page.keyboard.type( ' after move' ); + + expect( await getEditedPostContent() ).toMatchSnapshot(); + + await pressWithModifier( META_KEY, 'z' ); + + expect( await getEditedPostContent() ).toMatchSnapshot(); + } ); + + it( 'should undo typing after non input change', async () => { + await clickBlockAppender(); + + await page.keyboard.type( 'before keyboard ' ); + await pressWithModifier( META_KEY, 'b' ); + await page.keyboard.type( 'after keyboard' ); + + expect( await getEditedPostContent() ).toMatchSnapshot(); + + await pressWithModifier( META_KEY, 'z' ); + + expect( await getEditedPostContent() ).toMatchSnapshot(); + } ); + + it( 'should undo typing after arrow navigation', async () => { + await clickBlockAppender(); + + await page.keyboard.type( 'before keyboard' ); + await page.keyboard.press( 'ArrowLeft' ); + await page.keyboard.type( ' after keyboard' ); + + expect( await getEditedPostContent() ).toMatchSnapshot(); + + await pressWithModifier( META_KEY, 'z' ); + + expect( await getEditedPostContent() ).toMatchSnapshot(); + } ); + it( 'Should undo to expected level intervals', async () => { await clickBlockAppender(); From 30205fccbc8efe18d03c14ba37dfe0e351ad82ad Mon Sep 17 00:00:00 2001 From: iseulde Date: Tue, 13 Nov 2018 18:05:12 +0100 Subject: [PATCH 2/4] Adjust e2e test --- test/e2e/specs/__snapshots__/undo.test.js.snap | 2 +- test/e2e/specs/undo.test.js | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/test/e2e/specs/__snapshots__/undo.test.js.snap b/test/e2e/specs/__snapshots__/undo.test.js.snap index 1871326750e11..6fddf05734a0d 100644 --- a/test/e2e/specs/__snapshots__/undo.test.js.snap +++ b/test/e2e/specs/__snapshots__/undo.test.js.snap @@ -16,7 +16,7 @@ exports[`undo Should undo to expected level intervals 1`] = ` exports[`undo should undo typing after arrow navigation 1`] = ` " -

before keyboar after keyboardd

+

before keyboard after keyboard

" `; diff --git a/test/e2e/specs/undo.test.js b/test/e2e/specs/undo.test.js index 82c71382d8398..7b63f838bf292 100644 --- a/test/e2e/specs/undo.test.js +++ b/test/e2e/specs/undo.test.js @@ -46,7 +46,7 @@ describe( 'undo', () => { await clickBlockAppender(); await page.keyboard.type( 'before keyboard' ); - await page.keyboard.press( 'ArrowLeft' ); + await page.keyboard.press( 'ArrowRight' ); await page.keyboard.type( ' after keyboard' ); expect( await getEditedPostContent() ).toMatchSnapshot(); From e6e9151ddeb15cfd845dfbc0ba42464b2b6305ab Mon Sep 17 00:00:00 2001 From: iseulde Date: Tue, 13 Nov 2018 18:41:32 +0100 Subject: [PATCH 3/4] Add undo level after input timeout --- .../editor/src/components/rich-text/index.js | 30 ++++++++----------- .../e2e/specs/__snapshots__/undo.test.js.snap | 20 +++---------- test/e2e/specs/undo.test.js | 22 +++----------- 3 files changed, 21 insertions(+), 51 deletions(-) diff --git a/packages/editor/src/components/rich-text/index.js b/packages/editor/src/components/rich-text/index.js index d42e753ab2943..59ca2f6bf7e15 100644 --- a/packages/editor/src/components/rich-text/index.js +++ b/packages/editor/src/components/rich-text/index.js @@ -20,7 +20,7 @@ import { getScrollContainer, } from '@wordpress/dom'; import { createBlobURL } from '@wordpress/blob'; -import { BACKSPACE, DELETE, ENTER, LEFT, RIGHT, UP, DOWN } from '@wordpress/keycodes'; +import { BACKSPACE, DELETE, ENTER } from '@wordpress/keycodes'; import { withDispatch, withSelect } from '@wordpress/data'; import { pasteHandler, children, getBlockTransforms, findTransform } from '@wordpress/blocks'; import { withInstanceId, withSafeTimeout, compose } from '@wordpress/compose'; @@ -130,6 +130,14 @@ export class RichText extends Component { this.lastHistoryValue = value; } + componentDidMount() { + document.addEventListener( 'selectionchange', this.onSelectionChange ); + } + + componentWillUnmount() { + document.removeEventListener( 'selectionchange', this.onSelectionChange ); + } + setRef( node ) { this.editableRef = node; } @@ -371,6 +379,10 @@ export class RichText extends Component { this.onChange( transformed, { withoutHistory: true, } ); + + // Create an undo level when input stops for over a second. + this.props.clearTimeout( this.onInput.timeout ); + this.onInput.timeout = this.props.setTimeout( this.onCreateUndoLevel, 1000 ); } onCompositionEnd() { @@ -580,10 +592,6 @@ export class RichText extends Component { this.splitContent(); } } - - if ( [ LEFT, RIGHT, UP, DOWN ].indexOf( keyCode ) >= 0 ) { - this.onCreateUndoLevel(); - } } /** @@ -763,18 +771,6 @@ export class RichText extends Component { const record = this.formatToValue( value ); this.applyRecord( record ); } - - if ( isSelected && ! prevProps.isSelected ) { - document.addEventListener( 'selectionchange', this.onSelectionChange ); - window.addEventListener( 'mousedown', this.onCreateUndoLevel ); - window.addEventListener( 'touchstart', this.onCreateUndoLevel ); - } - - if ( ! isSelected && prevProps.isSelected ) { - document.removeEventListener( 'selectionchange', this.onSelectionChange ); - window.removeEventListener( 'mousedown', this.onCreateUndoLevel ); - window.removeEventListener( 'touchstart', this.onCreateUndoLevel ); - } } formatToValue( value ) { diff --git a/test/e2e/specs/__snapshots__/undo.test.js.snap b/test/e2e/specs/__snapshots__/undo.test.js.snap index 6fddf05734a0d..1ae6c8e4458b7 100644 --- a/test/e2e/specs/__snapshots__/undo.test.js.snap +++ b/test/e2e/specs/__snapshots__/undo.test.js.snap @@ -14,27 +14,15 @@ exports[`undo Should undo to expected level intervals 1`] = ` " `; -exports[`undo should undo typing after arrow navigation 1`] = ` +exports[`undo should undo typing after a pause 1`] = ` " -

before keyboard after keyboard

+

before pause after pause

" `; -exports[`undo should undo typing after arrow navigation 2`] = ` +exports[`undo should undo typing after a pause 2`] = ` " -

before keyboard

-" -`; - -exports[`undo should undo typing after mouse move 1`] = ` -" -

before move after move

-" -`; - -exports[`undo should undo typing after mouse move 2`] = ` -" -

before move

+

before pause

" `; diff --git a/test/e2e/specs/undo.test.js b/test/e2e/specs/undo.test.js index 7b63f838bf292..1ef7b9b32b88c 100644 --- a/test/e2e/specs/undo.test.js +++ b/test/e2e/specs/undo.test.js @@ -14,12 +14,12 @@ describe( 'undo', () => { await newPost(); } ); - it( 'should undo typing after mouse move', async () => { + it( 'should undo typing after a pause', async () => { await clickBlockAppender(); - await page.keyboard.type( 'before move' ); - await page.mouse.down(); - await page.keyboard.type( ' after move' ); + await page.keyboard.type( 'before pause' ); + await new Promise( ( resolve ) => setTimeout( resolve, 1000 ) ); + await page.keyboard.type( ' after pause' ); expect( await getEditedPostContent() ).toMatchSnapshot(); @@ -42,20 +42,6 @@ describe( 'undo', () => { expect( await getEditedPostContent() ).toMatchSnapshot(); } ); - it( 'should undo typing after arrow navigation', async () => { - await clickBlockAppender(); - - await page.keyboard.type( 'before keyboard' ); - await page.keyboard.press( 'ArrowRight' ); - await page.keyboard.type( ' after keyboard' ); - - expect( await getEditedPostContent() ).toMatchSnapshot(); - - await pressWithModifier( META_KEY, 'z' ); - - expect( await getEditedPostContent() ).toMatchSnapshot(); - } ); - it( 'Should undo to expected level intervals', async () => { await clickBlockAppender(); From 9dba9e51de78d0ffb36ada5bdcdb970b304abefb Mon Sep 17 00:00:00 2001 From: iseulde Date: Thu, 15 Nov 2018 18:40:10 +0100 Subject: [PATCH 4/4] Remove _withoutApply --- packages/editor/src/components/rich-text/index.js | 15 +++++++-------- 1 file changed, 7 insertions(+), 8 deletions(-) diff --git a/packages/editor/src/components/rich-text/index.js b/packages/editor/src/components/rich-text/index.js index 59ca2f6bf7e15..3bda199650d23 100644 --- a/packages/editor/src/components/rich-text/index.js +++ b/packages/editor/src/components/rich-text/index.js @@ -418,14 +418,13 @@ export class RichText extends Component { * Sync the value to global state. The node tree and selection will also be * updated if differences are found. * - * @param {Object} record The record to sync and apply. - * @param {boolean} _withoutApply If true, the record won't be applied to - * the live DOM. + * @param {Object} record The record to sync and apply. + * @param {Object} $2 Named options. + * @param {boolean} $2.withoutHistory If true, no undo level will be + * created. */ - onChange( record, { withoutHistory, _withoutApply } = {} ) { - if ( ! _withoutApply ) { - this.applyRecord( record ); - } + onChange( record, { withoutHistory } = {} ) { + this.applyRecord( record ); const { start, end } = record; @@ -604,7 +603,7 @@ export class RichText extends Component { // The input event does not fire when the whole field is selected and // BACKSPACE is pressed. if ( keyCode === BACKSPACE ) { - this.onChange( this.createRecord(), true ); + this.onChange( this.createRecord() ); } // `scrollToRect` is called on `nodechange`, whereas calling it on