From 7d834d6df488fc6b0a13fdf253897c617743a7ba Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ella=20Van=C2=A0Dorpe?= Date: Thu, 1 Nov 2018 10:21:51 +0100 Subject: [PATCH] RichText: fix buggy enter/delete behaviour (#11287) * Fix buggy enter/delete behaviour in RichText * Fix setting selection on merge when value stays the same * Re-add horizontal navigation comment --- .../editor/src/components/rich-text/index.js | 185 ++++++------------ .../src/components/rich-text/tinymce.js | 85 +++++++- test/e2e/specs/block-deletion.test.js | 4 - test/e2e/specs/splitting-merging.test.js | 6 - test/e2e/support/utils.js | 31 --- 5 files changed, 140 insertions(+), 171 deletions(-) diff --git a/packages/editor/src/components/rich-text/index.js b/packages/editor/src/components/rich-text/index.js index 2548240a6671f9..685ea899e55c47 100644 --- a/packages/editor/src/components/rich-text/index.js +++ b/packages/editor/src/components/rich-text/index.js @@ -6,7 +6,6 @@ import { defer, find, isNil, - noop, isEqual, omit, } from 'lodash'; @@ -22,7 +21,7 @@ import { getScrollContainer, } from '@wordpress/dom'; import { createBlobURL } from '@wordpress/blob'; -import { BACKSPACE, DELETE, ENTER, LEFT, RIGHT, rawShortcut } from '@wordpress/keycodes'; +import { BACKSPACE, DELETE, ENTER, rawShortcut } from '@wordpress/keycodes'; import { withDispatch, withSelect } from '@wordpress/data'; import { rawHandler, children, getBlockTransforms, findTransform } from '@wordpress/blocks'; import { withInstanceId, withSafeTimeout, compose } from '@wordpress/compose'; @@ -41,8 +40,6 @@ import { unstableToDom, getSelectionStart, getSelectionEnd, - charAt, - LINE_SEPARATOR, remove, isCollapsed, } from '@wordpress/rich-text'; @@ -55,7 +52,7 @@ import Autocomplete from '../autocomplete'; import BlockFormatControls from '../block-format-controls'; import FormatEdit from './format-edit'; import FormatToolbar from './format-toolbar'; -import TinyMCE from './tinymce'; +import TinyMCE, { TINYMCE_ZWSP } from './tinymce'; import { pickAriaProps } from './aria'; import { getPatterns } from './patterns'; import { withBlockEditContext } from '../block-edit/context'; @@ -64,17 +61,7 @@ import { withBlockEditContext } from '../block-edit/context'; * Browser dependencies */ -const { Node, getSelection } = window; - -/** - * Zero-width space character used by TinyMCE as a caret landing point for - * inline boundary nodes. - * - * @see tinymce/src/core/main/ts/text/Zwsp.ts - * - * @type {string} - */ -const TINYMCE_ZWSP = '\uFEFF'; +const { getSelection } = window; export class RichText extends Component { constructor( { value, onReplace, multiline } ) { @@ -95,7 +82,6 @@ export class RichText extends Component { this.onChange = this.onChange.bind( this ); this.onNodeChange = this.onNodeChange.bind( this ); this.onDeleteKeyDown = this.onDeleteKeyDown.bind( this ); - this.onHorizontalNavigationKeyDown = this.onHorizontalNavigationKeyDown.bind( this ); this.onKeyDown = this.onKeyDown.bind( this ); this.onKeyUp = this.onKeyUp.bind( this ); this.onPropagateUndo = this.onPropagateUndo.bind( this ); @@ -184,10 +170,7 @@ export class RichText extends Component { editor.on( 'init', this.onInit ); editor.on( 'nodechange', this.onNodeChange ); - editor.on( 'keydown', this.onKeyDown ); - editor.on( 'keyup', this.onKeyUp ); editor.on( 'BeforeExecCommand', this.onPropagateUndo ); - editor.on( 'focus', this.onFocus ); // The change event in TinyMCE fires every time an undo level is added. editor.on( 'change', this.onCreateUndoLevel ); @@ -245,7 +228,7 @@ export class RichText extends Component { } createRecord() { - const range = window.getSelection().getRangeAt( 0 ); + const range = getSelection().getRangeAt( 0 ); return create( { element: this.editableRef, @@ -278,11 +261,11 @@ export class RichText extends Component { } /** - * Handles a paste event from TinyMCE. + * Handles a paste event. * * Saves the pasted data as plain text in `pastedPlainText`. * - * @param {PasteEvent} event The paste event as triggered by TinyMCE. + * @param {PasteEvent} event The paste event. */ onPaste( event ) { const clipboardData = event.clipboardData; @@ -509,7 +492,9 @@ export class RichText extends Component { * * @link https://en.wikipedia.org/wiki/Caret_navigation * - * @param {tinymce.EditorEvent} event Keydown event. + * @param {KeyboardEvent} event Keydown event. + * + * @return {?boolean} True if the event was handled. */ onDeleteKeyDown( event ) { const { onMerge, onRemove } = this.props; @@ -521,7 +506,7 @@ export class RichText extends Component { const isReverse = keyCode === BACKSPACE; // Only process delete if the key press occurs at uncollapsed edge. - if ( ! getSelection().isCollapsed ) { + if ( ! isCollapsed( this.createRecord() ) ) { return; } @@ -549,118 +534,46 @@ export class RichText extends Component { onRemove( ! isReverse ); } - event.preventDefault(); - - // Calling onMerge() or onRemove() will destroy the editor, so it's - // important that we stop other handlers (e.g. ones registered by - // TinyMCE) from also handling this event. - event.stopImmediatePropagation(); + return true; } /** - * Handles a horizontal navigation key down event to handle the case where - * TinyMCE attempts to preventDefault when on the outside edge of an inline - * boundary when arrowing _away_ from the boundary, not within it. Replaces - * the TinyMCE event `preventDefault` behavior with a noop, such that those - * relying on `defaultPrevented` are not misinformed about the arrow event. - * - * If TinyMCE#4476 is resolved, this handling may be removed. - * - * @see https://github.com/tinymce/tinymce/issues/4476 + * Handles a keydown event. * - * @param {tinymce.EditorEvent} event Keydown event. - */ - onHorizontalNavigationKeyDown( event ) { - const { focusNode } = getSelection(); - const { nodeType, nodeValue } = focusNode; - - if ( nodeType !== Node.TEXT_NODE ) { - return; - } - - if ( nodeValue.length !== 1 || nodeValue[ 0 ] !== TINYMCE_ZWSP ) { - return; - } - - const { keyCode } = event; - - // Consider to be moving away from inline boundary based on: - // - // 1. Within a text fragment consisting only of ZWSP. - // 2. If in reverse, there is no previous sibling. If forward, there is - // no next sibling (i.e. end of node). - const isReverse = keyCode === LEFT; - const edgeSibling = isReverse ? 'previousSibling' : 'nextSibling'; - if ( ! focusNode[ edgeSibling ] ) { - // Note: This is not reassigning on the native event, rather the - // "fixed" TinyMCE copy, which proxies its preventDefault to the - // native event. By reassigning here, we're effectively preventing - // the proxied call on the native event, but not otherwise mutating - // the original event object. - event.preventDefault = noop; - } - } - - /** - * Handles a keydown event from TinyMCE. - * - * @param {KeydownEvent} event The keydown event as triggered by TinyMCE. + * @param {KeyboardEvent} event The keydown event. */ onKeyDown( event ) { const { keyCode } = event; - const isHorizontalNavigation = keyCode === LEFT || keyCode === RIGHT; - if ( isHorizontalNavigation ) { - this.onHorizontalNavigationKeyDown( event ); - } - if ( keyCode === DELETE || keyCode === BACKSPACE ) { - if ( this.multilineTag ) { - const value = this.createRecord(); - const start = getSelectionStart( value ); - const end = getSelectionEnd( value ); - - let newValue; - - if ( keyCode === BACKSPACE ) { - if ( charAt( value, start - 1 ) === LINE_SEPARATOR ) { - newValue = remove( - value, - // Only remove the line if the selection is - // collapsed. - isCollapsed( value ) ? start - 1 : start, - end - ); - } - } else if ( charAt( value, end ) === LINE_SEPARATOR ) { - newValue = remove( - value, - start, - // Only remove the line if the selection is collapsed. - isCollapsed( value ) ? end + 1 : end, - ); - } - - if ( newValue ) { - this.onChange( newValue ); + event.preventDefault(); - event.preventDefault(); - // It's important that we stop other handlers (e.g. ones - // registered by TinyMCE) from also handling this event. - event.stopImmediatePropagation(); - } + if ( this.onDeleteKeyDown( event ) ) { + return; } - this.onDeleteKeyDown( event ); - } - - // If we click shift+Enter on inline RichTexts, we avoid creating two contenteditables - // We also split the content and call the onSplit prop if provided. - if ( keyCode === ENTER ) { + const value = this.createRecord(); + const start = getSelectionStart( value ); + const end = getSelectionEnd( value ); + + if ( keyCode === BACKSPACE ) { + this.onChange( remove( + value, + // Only remove the line if the selection is + // collapsed. + isCollapsed( value ) ? start - 1 : start, + end + ) ); + } else { + this.onChange( remove( + value, + start, + // Only remove the line if the selection is collapsed. + isCollapsed( value ) ? end + 1 : end, + ) ); + } + } else if ( keyCode === ENTER ) { event.preventDefault(); - // It's important that we stop other handlers (e.g. ones registered - // by TinyMCE) from also handling this event. - event.stopImmediatePropagation(); const record = this.createRecord(); @@ -707,9 +620,10 @@ export class RichText extends Component { } /** - * Handles TinyMCE key up event. + * Handles a keyup event. * - * @param {number} keyCode The key code that has been pressed on the keyboard. + * @param {number} $1.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 @@ -826,7 +740,7 @@ export class RichText extends Component { } componentDidUpdate( prevProps ) { - const { tagName, value } = this.props; + const { tagName, value, isSelected } = this.props; if ( tagName === prevProps.tagName && @@ -845,7 +759,7 @@ export class RichText extends Component { const record = this.formatToValue( value ); - if ( this.isActive() ) { + if ( isSelected ) { const prevRecord = this.formatToValue( prevProps.value ); const length = getTextContent( prevRecord ).length; record.start = length; @@ -855,6 +769,18 @@ export class RichText extends Component { this.applyRecord( record ); this.savedContent = value; } + + // If blocks are merged, but the content remains the same, e.g. merging + // an empty paragraph into another, then also set the selection to the + // end. + if ( isSelected && ! prevProps.isSelected && ! this.isActive() ) { + const record = this.formatToValue( value ); + const prevRecord = this.formatToValue( prevProps.value ); + const length = getTextContent( prevRecord ).length; + record.start = length; + record.end = length; + this.applyRecord( record ); + } } formatToValue( value ) { @@ -971,6 +897,9 @@ export class RichText extends Component { key={ key } onPaste={ this.onPaste } onInput={ this.onInput } + onKeyDown={ this.onKeyDown } + onKeyUp={ this.onKeyUp } + onFocus={ this.onFocus } multilineTag={ this.multilineTag } multilineWrapperTags={ this.multilineWrapperTags } setRef={ this.setRef } diff --git a/packages/editor/src/components/rich-text/tinymce.js b/packages/editor/src/components/rich-text/tinymce.js index c589dde327075d..d2a300a8d9cdb2 100644 --- a/packages/editor/src/components/rich-text/tinymce.js +++ b/packages/editor/src/components/rich-text/tinymce.js @@ -2,14 +2,14 @@ * External dependencies */ import tinymce from 'tinymce'; -import { isEqual } from 'lodash'; +import { isEqual, noop } from 'lodash'; import classnames from 'classnames'; /** * WordPress dependencies */ import { Component, createElement } from '@wordpress/element'; -import { BACKSPACE, DELETE } from '@wordpress/keycodes'; +import { BACKSPACE, DELETE, ENTER, LEFT, RIGHT } from '@wordpress/keycodes'; import { toHTMLString } from '@wordpress/rich-text'; import { children } from '@wordpress/blocks'; @@ -18,6 +18,23 @@ import { children } from '@wordpress/blocks'; */ import { diffAriaProps, pickAriaProps } from './aria'; +/** + * Browser dependencies + */ + +const { getSelection } = window; +const { TEXT_NODE } = window.Node; + +/** + * Zero-width space character used by TinyMCE as a caret landing point for + * inline boundary nodes. + * + * @see tinymce/src/core/main/ts/text/Zwsp.ts + * + * @type {string} + */ +export const TINYMCE_ZWSP = '\uFEFF'; + /** * Determines whether we need a fix to provide `input` events for contenteditable. * @@ -102,9 +119,14 @@ export default class TinyMCE extends Component { super(); this.bindEditorNode = this.bindEditorNode.bind( this ); this.onFocus = this.onFocus.bind( this ); + this.onKeyDown = this.onKeyDown.bind( this ); } onFocus() { + if ( this.props.onFocus ) { + this.props.onFocus(); + } + this.initialize(); } @@ -198,6 +220,8 @@ export default class TinyMCE extends Component { editor.dom.setHTML = setHTML; } ); + + editor.on( 'keydown', this.onKeyDown, true ); }, } ); } @@ -223,6 +247,59 @@ export default class TinyMCE extends Component { } } + onKeyDown( event ) { + const { keyCode } = event; + + // Disables TinyMCE behaviour. + if ( keyCode === ENTER || keyCode === BACKSPACE || keyCode === DELETE ) { + event.preventDefault(); + // For some reason this is needed to also prevent the insertion of + // line breaks. + return false; + } + + // Handles a horizontal navigation key down event to handle the case + // where TinyMCE attempts to preventDefault when on the outside edge of + // an inline boundary when arrowing _away_ from the boundary, not within + // it. Replaces the TinyMCE event `preventDefault` behavior with a noop, + // such that those relying on `defaultPrevented` are not misinformed + // about the arrow event. + // + // If TinyMCE#4476 is resolved, this handling may be removed. + // + // @see https://github.com/tinymce/tinymce/issues/4476 + if ( keyCode !== LEFT && keyCode !== RIGHT ) { + return; + } + + const { focusNode } = getSelection(); + const { nodeType, nodeValue } = focusNode; + + if ( nodeType !== TEXT_NODE ) { + return; + } + + if ( nodeValue.length !== 1 || nodeValue[ 0 ] !== TINYMCE_ZWSP ) { + return; + } + + // Consider to be moving away from inline boundary based on: + // + // 1. Within a text fragment consisting only of ZWSP. + // 2. If in reverse, there is no previous sibling. If forward, there is + // no next sibling (i.e. end of node). + const isReverse = event.keyCode === LEFT; + const edgeSibling = isReverse ? 'previousSibling' : 'nextSibling'; + if ( ! focusNode[ edgeSibling ] ) { + // Note: This is not reassigning on the native event, rather the + // "fixed" TinyMCE copy, which proxies its preventDefault to the + // native event. By reassigning here, we're effectively preventing + // the proxied call on the native event, but not otherwise mutating + // the original event object. + event.preventDefault = noop; + } + } + render() { const ariaProps = pickAriaProps( this.props ); const { @@ -235,6 +312,8 @@ export default class TinyMCE extends Component { onInput, multilineTag, multilineWrapperTags, + onKeyDown, + onKeyUp, } = this.props; /* @@ -284,6 +363,8 @@ export default class TinyMCE extends Component { onPaste, onInput, onFocus: this.onFocus, + onKeyDown, + onKeyUp, } ); } } diff --git a/test/e2e/specs/block-deletion.test.js b/test/e2e/specs/block-deletion.test.js index fb6dd2b40abd5b..7da0695f1ab436 100644 --- a/test/e2e/specs/block-deletion.test.js +++ b/test/e2e/specs/block-deletion.test.js @@ -7,7 +7,6 @@ import { newPost, pressWithModifier, ACCESS_MODIFIER_KEYS, - waitForRichTextInitialization, } from '../support/utils'; const addThreeParagraphsToNewPost = async () => { @@ -17,10 +16,8 @@ const addThreeParagraphsToNewPost = async () => { await clickBlockAppender(); await page.keyboard.type( 'First paragraph' ); await page.keyboard.press( 'Enter' ); - await waitForRichTextInitialization(); await page.keyboard.type( 'Second paragraph' ); await page.keyboard.press( 'Enter' ); - await waitForRichTextInitialization(); }; const clickOnBlockSettingsMenuItem = async ( buttonLabel ) => { @@ -99,7 +96,6 @@ describe( 'block deletion -', () => { // Add a third paragraph for this test. await page.keyboard.type( 'Third paragraph' ); await page.keyboard.press( 'Enter' ); - await waitForRichTextInitialization(); // Press the up arrow once to select the third and fourth blocks. await pressWithModifier( 'Shift', 'ArrowUp' ); diff --git a/test/e2e/specs/splitting-merging.test.js b/test/e2e/specs/splitting-merging.test.js index fca2a8820a0135..d059fbf42ea63d 100644 --- a/test/e2e/specs/splitting-merging.test.js +++ b/test/e2e/specs/splitting-merging.test.js @@ -8,7 +8,6 @@ import { pressTimes, pressWithModifier, META_KEY, - waitForRichTextInitialization, } from '../support/utils'; describe( 'splitting and merging blocks', () => { @@ -133,9 +132,7 @@ describe( 'splitting and merging blocks', () => { await pressWithModifier( META_KEY, 'b' ); await page.keyboard.press( 'ArrowRight' ); await page.keyboard.press( 'Enter' ); - await waitForRichTextInitialization(); await page.keyboard.press( 'Enter' ); - await waitForRichTextInitialization(); await page.keyboard.press( 'Backspace' ); @@ -175,11 +172,8 @@ describe( 'splitting and merging blocks', () => { await insertBlock( 'Paragraph' ); await page.keyboard.type( 'First' ); await page.keyboard.press( 'Enter' ); - await waitForRichTextInitialization(); await page.keyboard.press( 'Enter' ); - await waitForRichTextInitialization(); await page.keyboard.press( 'Enter' ); - await waitForRichTextInitialization(); await page.keyboard.type( 'Second' ); await page.keyboard.press( 'ArrowUp' ); await page.keyboard.press( 'ArrowUp' ); diff --git a/test/e2e/support/utils.js b/test/e2e/support/utils.js index 7275da4d1d2fcf..ee7380de6cf198 100644 --- a/test/e2e/support/utils.js +++ b/test/e2e/support/utils.js @@ -86,34 +86,6 @@ async function login() { ] ); } -/** - * Returns a promise which resolves once it's determined that the active DOM - * element is not within a RichText field, or the RichText field's TinyMCE has - * completed initialization. This is an unfortunate workaround to address an - * issue where TinyMCE takes its time to become ready for user input. - * - * TODO: This is a code smell, indicating that "too fast" resulting in breakage - * could be equally problematic for a fast human. It should be explored whether - * all event bindings we assign to TinyMCE to handle could be handled through - * the DOM directly instead. - * - * @return {Promise} Promise resolving once RichText is initialized, or is - * determined to not be a container of the active element. - */ -export async function waitForRichTextInitialization() { - const isInRichText = await page.evaluate( () => { - return !! document.activeElement.closest( '.editor-rich-text__tinymce' ); - } ); - - if ( ! isInRichText ) { - return; - } - - return page.waitForFunction( () => { - return !! document.activeElement.closest( '.mce-content-body' ); - } ); -} - export async function visitAdmin( adminPath, query ) { await goToWPPath( join( 'wp-admin', adminPath ), query ); @@ -238,7 +210,6 @@ export async function ensureSidebarOpened() { */ export async function clickBlockAppender() { await page.click( '.editor-default-block-appender__content' ); - await waitForRichTextInitialization(); } /** @@ -268,7 +239,6 @@ export async function insertBlock( searchTerm, panelName = null ) { await panelButton.click(); } await page.click( `button[aria-label="${ searchTerm }"]` ); - await waitForRichTextInitialization(); } export async function convertBlock( name ) { @@ -276,7 +246,6 @@ export async function convertBlock( name ) { await page.mouse.move( 250, 350, { steps: 10 } ); await page.click( '.editor-block-switcher__toggle' ); await page.click( `.editor-block-types-list__item[aria-label="${ name }"]` ); - await waitForRichTextInitialization(); } /**