Skip to content

Commit

Permalink
RichText: Fix empty value delete behaviors (#8735)
Browse files Browse the repository at this point in the history
* Testing: Wait for RichText initialization

* RichText: Fix empty value delete behaviors

* RichText: Handle remove only on backspace

* docs: Very minor comment readability tweak
  • Loading branch information
aduth authored Aug 9, 2018
1 parent f5617ac commit 45e3391
Show file tree
Hide file tree
Showing 4 changed files with 164 additions and 27 deletions.
88 changes: 62 additions & 26 deletions packages/editor/src/components/rich-text/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -81,6 +81,7 @@ export class RichText extends Component {
this.onFocus = this.onFocus.bind( this );
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 );
Expand Down Expand Up @@ -381,6 +382,64 @@ export class RichText extends Component {
this.context.onCreateUndoLevel();
}

/**
* Handles a delete keyDown event to handle merge or removal for collapsed
* selection where caret is at directional edge: forward for a delete key,
* reverse for a backspace key.
*
* @link https://en.wikipedia.org/wiki/Caret_navigation
*
* @param {tinymce.EditorEvent<KeyboardEvent>} event Keydown event.
*/
onDeleteKeyDown( event ) {
const { onMerge, onRemove } = this.props;
if ( ! onMerge && ! onRemove ) {
return;
}

const { keyCode } = event;
const isReverse = keyCode === BACKSPACE;
const { isCollapsed } = getSelection();

// Only process delete if the key press occurs at uncollapsed edge.
if ( ! isCollapsed ) {
return;
}

// It is important to consider emptiness because an empty container
// will include a bogus TinyMCE BR node _after_ the caret, so in a
// forward deletion the isHorizontalEdge function will incorrectly
// interpret the presence of the bogus node as not being at the edge.
const isEmpty = this.isEmpty();
const isEdge = (
isEmpty ||
isHorizontalEdge( this.editor.getBody(), isReverse )
);

if ( ! isEdge ) {
return;
}

if ( onMerge ) {
onMerge( ! isReverse );
}

// Only handle remove on Backspace. This serves dual-purpose of being
// an intentional user interaction distinguishing between Backspace and
// Delete to remove the empty field, but also to avoid merge & remove
// causing destruction of two fields (merge, then removed merged).
if ( onRemove && isEmpty && isReverse ) {
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();
}

/**
* Handles a horizontal navigation key down event to handle the case where
* TinyMCE attempts to preventDefault when on the outside edge of an inline
Expand Down Expand Up @@ -435,32 +494,9 @@ export class RichText extends Component {
const rootNode = this.editor.getBody();
const { keyCode } = event;

if (
getSelection().isCollapsed && (
( keyCode === BACKSPACE && isHorizontalEdge( rootNode, true ) ) ||
( keyCode === DELETE && isHorizontalEdge( rootNode, false ) )
)
) {
if ( ! this.props.onMerge && ! this.props.onRemove ) {
return;
}

const forward = keyCode === DELETE;

if ( this.props.onMerge ) {
this.props.onMerge( forward );
}

if ( this.props.onRemove && this.isEmpty() ) {
this.props.onRemove( forward );
}

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();
const isDelete = keyCode === DELETE || keyCode === BACKSPACE;
if ( isDelete ) {
this.onDeleteKeyDown( event );
}

const isHorizontalNavigation = keyCode === LEFT || keyCode === RIGHT;
Expand Down
22 changes: 22 additions & 0 deletions test/e2e/specs/__snapshots__/splitting-merging.test.js.snap
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,12 @@ exports[`splitting and merging blocks should delete an empty first line 1`] = `
<!-- /wp:paragraph -->"
`;

exports[`splitting and merging blocks should forward delete from an empty paragraph 1`] = `
"<!-- wp:paragraph -->
<p>Second</p>
<!-- /wp:paragraph -->"
`;

exports[`splitting and merging blocks should gracefully handle if placing caret in empty container 1`] = `
"<!-- wp:paragraph -->
<p><strong>Foo</strong></p>
Expand All @@ -32,6 +38,22 @@ exports[`splitting and merging blocks should not merge paragraphs if the selecti
<!-- /wp:paragraph -->"
`;

exports[`splitting and merging blocks should remove at most one paragraph in forward direction 1`] = `
"<!-- wp:paragraph -->
<p>First</p>
<!-- /wp:paragraph -->
<!-- wp:paragraph -->
<p></p>
<!-- /wp:paragraph -->
<!-- wp:paragraph -->
<p>Second</p>
<!-- /wp:paragraph -->"
`;

exports[`splitting and merging blocks should remove empty paragraph block on backspace 1`] = `""`;

exports[`splitting and merging blocks should split and merge paragraph blocks using Enter and Backspace 1`] = `
"<!-- wp:paragraph -->
<p>First</p>
Expand Down
51 changes: 50 additions & 1 deletion test/e2e/specs/splitting-merging.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -86,16 +86,22 @@ describe( 'splitting and merging blocks', () => {
} );

it( 'should not merge paragraphs if the selection is not collapsed', async () => {
// Regression Test: When all of a paragraph is selected, pressing
// backspace should delete the contents, not merge to previous.
//
// See: https://github.com/WordPress/gutenberg/issues/8268
await insertBlock( 'Paragraph' );
await page.keyboard.type( 'Foo' );
await insertBlock( 'Paragraph' );
await page.keyboard.type( 'Bar' );

// Select text.
await page.keyboard.down( 'Shift' );
await pressTimes( 'ArrowLeft', 3 );
await page.keyboard.up( 'Shift' );
await page.keyboard.press( 'Backspace' );

// Delete selection.
await page.keyboard.press( 'Backspace' );
expect( await getEditedPostContent() ).toMatchSnapshot();
} );

Expand All @@ -121,4 +127,47 @@ describe( 'splitting and merging blocks', () => {

expect( await getEditedPostContent() ).toMatchSnapshot();
} );

it( 'should forward delete from an empty paragraph', async () => {
// Regression test: Bogus nodes in a TinyMCE container can interfere
// with isHorizontalEdge detection, preventing forward deletion.
//
// See: https://github.com/WordPress/gutenberg/issues/8731
await insertBlock( 'Paragraph' );
await page.keyboard.press( 'Enter' );
await page.keyboard.type( 'Second' );
await page.keyboard.press( 'ArrowUp' );
await page.keyboard.press( 'Delete' );

expect( await getEditedPostContent() ).toMatchSnapshot();
} );

it( 'should remove empty paragraph block on backspace', async () => {
// Regression Test: In a sole empty paragraph, pressing backspace
// should remove the block.
//
// See: https://github.com/WordPress/gutenberg/pull/8306
await insertBlock( 'Paragraph' );
await page.keyboard.press( 'Backspace' );

expect( await getEditedPostContent() ).toMatchSnapshot();
} );

it( 'should remove at most one paragraph in forward direction', async () => {
// Regression Test: A forward delete on empty RichText previously would
// destroy two paragraphs on the dual-action of merge & remove.
//
// See: https://github.com/WordPress/gutenberg/pull/8735
await insertBlock( 'Paragraph' );
await page.keyboard.type( 'First' );
await page.keyboard.press( 'Enter' );
await page.keyboard.press( 'Enter' );
await page.keyboard.press( 'Enter' );
await page.keyboard.type( 'Second' );
await page.keyboard.press( 'ArrowUp' );
await page.keyboard.press( 'ArrowUp' );
await page.keyboard.press( 'Delete' );

expect( await getEditedPostContent() ).toMatchSnapshot();
} );
} );
30 changes: 30 additions & 0 deletions test/e2e/support/utils.js
Original file line number Diff line number Diff line change
Expand Up @@ -77,6 +77,34 @@ 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.
*/
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 );

Expand Down Expand Up @@ -170,6 +198,7 @@ export async function ensureSidebarOpened() {
*/
export async function clickBlockAppender() {
await expect( page ).toClick( '.editor-default-block-appender__content' );
await waitForRichTextInitialization();
}

/**
Expand Down Expand Up @@ -199,6 +228,7 @@ export async function insertBlock( searchTerm, panelName = null ) {
await panelButton.click();
}
await page.click( `button[aria-label="${ searchTerm }"]` );
await waitForRichTextInitialization();
}

/**
Expand Down

0 comments on commit 45e3391

Please sign in to comment.