From 12101091f6e1a6f3b845f0b33f6643d1aeb8a554 Mon Sep 17 00:00:00 2001 From: Robert Anderson Date: Fri, 10 Aug 2018 15:43:20 +1000 Subject: [PATCH 1/4] Rich Text: Indicate which text will be turned into a link When inserting a link, the text selection disappears when the focus changes into the URLInput text field. This makes it hard to tell which text will be turned into a link. The Classic Editor solves this by inserting a placeholder element, which is the approach that we borrow here. --- .../editor/src/components/rich-text/index.js | 134 ++++++++++++------ .../src/components/rich-text/test/index.js | 83 ++++++----- 2 files changed, 143 insertions(+), 74 deletions(-) diff --git a/packages/editor/src/components/rich-text/index.js b/packages/editor/src/components/rich-text/index.js index 6eeb7a7b7e88e..1cb032bbf5eec 100644 --- a/packages/editor/src/components/rich-text/index.js +++ b/packages/editor/src/components/rich-text/index.js @@ -3,12 +3,13 @@ */ import classnames from 'classnames'; import { - isEqual, + defer, + difference, + find, forEach, - merge, identity, - find, - defer, + isEqual, + merge, noop, } from 'lodash'; import 'element-closest'; @@ -59,15 +60,25 @@ const { Node, getSelection } = window; */ const TINYMCE_ZWSP = '\uFEFF'; -export function getFormatProperties( formatName, parents ) { +export function getFormatValue( formatName, parents ) { switch ( formatName ) { case 'link' : { - const anchor = find( parents, ( node ) => node.nodeName.toLowerCase() === 'a' ); - return !! anchor ? { value: anchor.getAttribute( 'href' ) || '', target: anchor.getAttribute( 'target' ) || '', node: anchor } : {}; + const anchor = find( parents, ( node ) => node.nodeName === 'A' ); + if ( anchor ) { + if ( anchor.hasAttribute( 'data-wp-placeholder' ) ) { + return { isAdding: true }; + } + return { + isActive: true, + value: anchor.getAttribute( 'href' ) || '', + target: anchor.getAttribute( 'target' ) || '', + node: anchor, + }; + } } - default: - return {}; } + + return { isActive: true }; } const DEFAULT_FORMATS = [ 'bold', 'italic', 'strikethrough', 'link', 'code' ]; @@ -385,7 +396,6 @@ export class RichText extends Component { /** * Handles any case where the content of the TinyMCE instance has changed. */ - onChange() { this.savedContent = this.getContent(); this.props.onChange( this.savedContent ); @@ -699,13 +709,12 @@ export class RichText extends Component { return; } + // Remove *non-selected* placeholder links when the selection is changed. + this.removePlaceholderLinks( parents ); + const formatNames = this.props.formattingControls; const formats = this.editor.formatter.matchAll( formatNames ).reduce( ( accFormats, activeFormat ) => { - accFormats[ activeFormat ] = { - isActive: true, - ...getFormatProperties( activeFormat, parents ), - }; - + accFormats[ activeFormat ] = getFormatValue( activeFormat, parents ); return accFormats; }, {} ); @@ -776,6 +785,27 @@ export class RichText extends Component { console.error( 'Formatters passed via `formatters` prop will only be registered once. Formatters can be enabled/disabled via the `formattingControls` prop.' ); } } + + // When the block is unselected, remove placeholder links and hide the formatting toolbar. + if ( ! this.props.isSelected && prevProps.isSelected ) { + this.removePlaceholderLinks(); + this.setState( { formats: {} } ); + } + } + + /** + * Removes any placeholder links from the editor DOM. Placeholder links are + * used when adding a link to indicate which text will become a link. + * + * @param {HTMLElement[]=} linksToKeep If specified, these links will *not* + * be removed. Useful for keeping the + * currently selected link as is. + */ + removePlaceholderLinks( linksToKeep = [] ) { + const placeholderLinks = this.editor.$( 'a[data-wp-placeholder]' ).toArray(); + for ( const placeholderLink of difference( placeholderLinks, linksToKeep ) ) { + this.editor.dom.remove( placeholderLink, /* keepChildren: */ true ); + } } /** @@ -809,37 +839,59 @@ export class RichText extends Component { changeFormats( formats ) { forEach( formats, ( formatValue, format ) => { + const isActive = this.isFormatActive( format ); + if ( format === 'link' ) { - if ( !! formatValue ) { - if ( formatValue.isAdding ) { - return; - } + // Remove the selected link when `formats.link` is set to a falsey value. + if ( ! formatValue ) { + this.editor.execCommand( 'Unlink' ); + return; + } - const { value: href, target } = formatValue; - - if ( ! this.isFormatActive( 'link' ) && this.editor.selection.isCollapsed() ) { - // When no link or text is selected, insert a link with the URL as its text - const anchorHTML = this.editor.dom.createHTML( - 'a', - { href, target }, - this.editor.dom.encode( href ) - ); - this.editor.insertContent( anchorHTML ); - } else { - // Use built-in TinyMCE command turn the selection into a link. This takes - // care of deleting any existing links within the selection - this.editor.execCommand( 'mceInsertLink', false, { href, target } ); + const { isAdding, value: href, target } = formatValue; + const isSelectionCollapsed = this.editor.selection.isCollapsed(); + + // Bail early if the link is still being added. will ask the user + // for a URL and then update `formats.link`. + if ( isAdding ) { + // Create a placeholder so that there's something to indicate which + // text will become a link. Placeholder links are stripped from + // getContent() and removed when the selection changes. + if ( ! isSelectionCollapsed ) { + this.editor.formatter.apply( format, { + href: '#', + 'data-wp-placeholder': true, + 'data-mce-bogus': true, + } ); } - } else { - this.editor.execCommand( 'Unlink' ); + return; } - } else { - const isActive = this.isFormatActive( format ); - if ( isActive && ! formatValue ) { - this.removeFormat( format ); - } else if ( ! isActive && formatValue ) { - this.applyFormat( format ); + + // When no link or text is selected, use the URL as the link's text. + if ( isSelectionCollapsed && ! isActive ) { + this.editor.insertContent( this.editor.dom.createHTML( + 'a', + { href, target }, + this.editor.dom.encode( href ) + ) ); + return; } + + // Use built-in TinyMCE command turn the selection into a link. This takes + // care of deleting any existing links within the current selection. + this.editor.execCommand( 'mceInsertLink', false, { + href, + target, + 'data-wp-placeholder': null, + 'data-mce-bogus': null, + } ); + return; + } + + if ( isActive && ! formatValue ) { + this.removeFormat( format ); + } else if ( ! isActive && formatValue ) { + this.applyFormat( format ); } } ); diff --git a/packages/editor/src/components/rich-text/test/index.js b/packages/editor/src/components/rich-text/test/index.js index b1edab7cc710e..805099fd3446b 100644 --- a/packages/editor/src/components/rich-text/test/index.js +++ b/packages/editor/src/components/rich-text/test/index.js @@ -13,64 +13,81 @@ import deprecated from '@wordpress/deprecated'; */ import { RichText, - getFormatProperties, + getFormatValue, } from '../'; import { diffAriaProps, pickAriaProps } from '../aria'; jest.mock( '@wordpress/deprecated', () => jest.fn() ); -describe( 'getFormatProperties', () => { - const formatName = 'link'; - const node = { - nodeName: 'A', - attributes: { - href: 'https://www.testing.com', - target: '_blank', - }, - }; +describe( 'getFormatValue', () => { + function createMockNode( nodeName, attributes = {} ) { + return { + nodeName, + hasAttribute( name ) { + return !! attributes[ name ]; + }, + getAttribute( name ) { + return attributes[ name ]; + }, + }; + } - test( 'should return an empty object', () => { - expect( getFormatProperties( 'ofSomething' ) ).toEqual( {} ); + test( 'basic formatting', () => { + expect( getFormatValue( 'bold' ) ).toEqual( { + isActive: true, + } ); } ); - test( 'should return an empty object if no anchor element is found', () => { - expect( getFormatProperties( formatName, [ { ...node, nodeName: 'P' } ] ) ).toEqual( {} ); + test( 'link formatting when no anchor is found', () => { + const formatValue = getFormatValue( 'link', [ + createMockNode( 'P' ), + ] ); + expect( formatValue ).toEqual( { + isActive: true, + } ); } ); - test( 'should return a populated object', () => { - const mockNode = { - ...node, - getAttribute: jest.fn().mockImplementation( ( attr ) => mockNode.attributes[ attr ] ), - }; + test( 'link formatting', () => { + const mockNode = createMockNode( 'A', { + href: 'https://www.testing.com', + target: '_blank', + } ); - const parents = [ - mockNode, - ]; + const formatValue = getFormatValue( 'link', [ mockNode ] ); - expect( getFormatProperties( formatName, parents ) ).toEqual( { + expect( formatValue ).toEqual( { + isActive: true, value: 'https://www.testing.com', target: '_blank', node: mockNode, } ); } ); - test( 'should return an object with empty values when no link is found', () => { - const mockNode = { - ...node, - attributes: {}, - getAttribute: jest.fn().mockImplementation( ( attr ) => mockNode.attributes[ attr ] ), - }; + test( 'link formatting when the anchor has no attributes', () => { + const mockNode = createMockNode( 'A' ); - const parents = [ - mockNode, - ]; + const formatValue = getFormatValue( 'link', [ mockNode ] ); - expect( getFormatProperties( formatName, parents ) ).toEqual( { + expect( formatValue ).toEqual( { + isActive: true, value: '', target: '', node: mockNode, } ); } ); + + test( 'link formatting when the link is still being added', () => { + const formatValue = getFormatValue( 'link', [ + createMockNode( 'A', { + href: '#', + 'data-wp-placeholder': 'true', + 'data-mce-bogus': 'true', + } ), + ] ); + expect( formatValue ).toEqual( { + isAdding: true, + } ); + } ); } ); describe( 'RichText', () => { From 7e0be3680256bbf4e8bf2a79e4439d7fbc57fbad Mon Sep 17 00:00:00 2001 From: Robert Anderson Date: Mon, 13 Aug 2018 13:38:29 +1000 Subject: [PATCH 2/4] Add E2E tests for creating, editing and removing links --- .../__snapshots__/managing-links.test.js.snap | 31 ++++ test/e2e/specs/managing-links.test.js | 163 ++++++++++++++++++ 2 files changed, 194 insertions(+) create mode 100644 test/e2e/specs/__snapshots__/managing-links.test.js.snap diff --git a/test/e2e/specs/__snapshots__/managing-links.test.js.snap b/test/e2e/specs/__snapshots__/managing-links.test.js.snap new file mode 100644 index 0000000000000..066c28ab4a5db --- /dev/null +++ b/test/e2e/specs/__snapshots__/managing-links.test.js.snap @@ -0,0 +1,31 @@ +// Jest Snapshot v1, https://goo.gl/fbAQLP + +exports[`Managing links Creating a link by selecting text and clicking the Link UI 1`] = ` +" +

This is Gutenberg

+" +`; + +exports[`Managing links Creating a link by selecting text and using keyboard shortcuts 1`] = ` +" +

This is Gutenberg

+" +`; + +exports[`Managing links Creating a link without any text selected 1`] = ` +" +

This is Gutenberg: https://wordpress.org/gutenberg

+" +`; + +exports[`Managing links Editing a link 1`] = ` +" +

This is Gutenberg

+" +`; + +exports[`Managing links Removing a link 1`] = ` +" +

This is Gutenberg

+" +`; diff --git a/test/e2e/specs/managing-links.test.js b/test/e2e/specs/managing-links.test.js index 0854b5f9e3d78..ead98f31ba2d1 100644 --- a/test/e2e/specs/managing-links.test.js +++ b/test/e2e/specs/managing-links.test.js @@ -2,15 +2,178 @@ * Internal dependencies */ import { + META_KEY, clickBlockAppender, + getEditedPostContent, newPost, + pressWithModifier, } from '../support/utils'; +/** + * The modifier keys needed to invoke a 'select the next word' keyboard shortcut. + * + * @type {string} + */ +const SELECT_WORD_MODIFIER_KEYS = process.platform === 'darwin' ? [ 'Shift', 'Alt' ] : [ 'Shift', 'Control' ]; + describe( 'Managing links', () => { beforeEach( async () => { await newPost(); } ); + it( 'Creating a link by selecting text and clicking the Link UI', async () => { + // Create a block with some text + await clickBlockAppender(); + await page.keyboard.type( 'This is Gutenberg' ); + + // Select some text + await pressWithModifier( SELECT_WORD_MODIFIER_KEYS, 'ArrowLeft' ); + + // Click on the Link button + await page.click( 'button[aria-label="Link"]' ); + + // A placeholder link should have been inserted + expect( await page.$( 'a[data-wp-placeholder]' ) ).not.toBeNull(); + + // Type a URL + await page.keyboard.type( 'https://wordpress.org/gutenberg' ); + + // Click on the Apply button + await page.click( 'button[aria-label="Apply"]' ); + + // There should no longer be a placeholder link + expect( await page.$( 'a[data-wp-placeholder]' ) ).toBeNull(); + + // The link should have been inserted + expect( await getEditedPostContent() ).toMatchSnapshot(); + } ); + + it( 'Creating a link by selecting text and using keyboard shortcuts', async () => { + // Create a block with some text + await clickBlockAppender(); + await page.keyboard.type( 'This is Gutenberg' ); + + // Select some text + await pressWithModifier( SELECT_WORD_MODIFIER_KEYS, 'ArrowLeft' ); + + // Press Cmd+K to insert a link + await pressWithModifier( META_KEY, 'K' ); + + // A placeholder link should have been inserted + expect( await page.$( 'a[data-wp-placeholder]' ) ).not.toBeNull(); + + // Type a URL + await page.keyboard.type( 'https://wordpress.org/gutenberg' ); + + // Press Enter to apply the link + await page.keyboard.press( 'Enter' ); + + // There should no longer be a placeholder link + expect( await page.$( 'a[data-wp-placeholder]' ) ).toBeNull(); + + // The link should have been inserted + expect( await getEditedPostContent() ).toMatchSnapshot(); + } ); + + it( 'Creating a link without any text selected', async () => { + // Create a block with some text + await clickBlockAppender(); + await page.keyboard.type( 'This is Gutenberg: ' ); + + // Press Cmd+K to insert a link + await pressWithModifier( META_KEY, 'K' ); + + // Trigger isTyping = false + await page.mouse.move( 200, 300, { steps: 10 } ); + await page.mouse.move( 250, 350, { steps: 10 } ); + + // A placeholder link should not have been inserted + expect( await page.$( 'a[data-wp-placeholder]' ) ).toBeNull(); + + // Type a URL + await page.keyboard.type( 'https://wordpress.org/gutenberg' ); + + // Press Enter to apply the link + await page.keyboard.press( 'Enter' ); + + // A link with the URL as its text should have been inserted + expect( await getEditedPostContent() ).toMatchSnapshot(); + } ); + + it( 'Creating a link and then cancelling', async () => { + // Create a block with some text + await clickBlockAppender(); + await page.keyboard.type( 'This is Gutenberg' ); + + // Select some text + await pressWithModifier( SELECT_WORD_MODIFIER_KEYS, 'ArrowLeft' ); + + // Click on the Link button + await page.click( 'button[aria-label="Link"]' ); + + // Type a URL + await page.keyboard.type( 'https://wordpress.org/gutenberg' ); + + // Click somewhere else - it doesn't really matter where + await page.click( '.editor-post-title' ); + + // A placeholder link should not have been inserted + expect( await page.$( 'a[data-wp-placeholder]' ) ).toBeNull(); + } ); + + const createAndReselectLink = async () => { + // Create a block with some text + await clickBlockAppender(); + await page.keyboard.type( 'This is Gutenberg' ); + + // Select some text + await pressWithModifier( SELECT_WORD_MODIFIER_KEYS, 'ArrowLeft' ); + + // Click on the Link button + await page.click( 'button[aria-label="Link"]' ); + + // Wait for the URL field to auto-focus + await page.waitForFunction( () => !! document.activeElement.closest( '.editor-url-input' ) ); + + // Type a URL + await page.keyboard.type( 'https://wordpress.org/gutenberg' ); + + // Click on the Apply button + await page.click( 'button[aria-label="Apply"]' ); + + // Click somewhere else - it doesn't really matter where + await page.click( '.editor-post-title' ); + + // Select the link again + await page.click( 'a[href="https://wordpress.org/gutenberg"]' ); + }; + + it( 'Editing a link', async () => { + await createAndReselectLink(); + + // Click on the Edit button + await page.click( 'button[aria-label="Edit"]' ); + + // Change the URL + await page.keyboard.type( '/handbook' ); + + // Click on the Apply button + await page.click( 'button[aria-label="Apply"]' ); + + // The link should have been updated + expect( await getEditedPostContent() ).toMatchSnapshot(); + } ); + + it( 'Removing a link', async () => { + await createAndReselectLink(); + + // Click on the Unlink button + await page.click( 'button[aria-label="Unlink"]' ); + + // The link should have been removed + expect( await getEditedPostContent() ).toMatchSnapshot(); + } ); + const setFixedToolbar = async ( b ) => { await page.click( '.edit-post-more-menu button' ); const button = ( await page.$x( "//button[contains(text(), 'Fix Toolbar to Top')]" ) )[ 0 ]; From 213b8d12e71ffadbb2d878505efd26ea53d54302 Mon Sep 17 00:00:00 2001 From: Robert Anderson Date: Fri, 17 Aug 2018 09:51:03 +1000 Subject: [PATCH 3/4] Use an `if` instead of a `switch` in getFormatValue() A `switch` is overkill when there's only one format value that acts differently. --- .../editor/src/components/rich-text/index.js | 24 +++++++++---------- 1 file changed, 11 insertions(+), 13 deletions(-) diff --git a/packages/editor/src/components/rich-text/index.js b/packages/editor/src/components/rich-text/index.js index 1cb032bbf5eec..db7629457980f 100644 --- a/packages/editor/src/components/rich-text/index.js +++ b/packages/editor/src/components/rich-text/index.js @@ -61,20 +61,18 @@ const { Node, getSelection } = window; const TINYMCE_ZWSP = '\uFEFF'; export function getFormatValue( formatName, parents ) { - switch ( formatName ) { - case 'link' : { - const anchor = find( parents, ( node ) => node.nodeName === 'A' ); - if ( anchor ) { - if ( anchor.hasAttribute( 'data-wp-placeholder' ) ) { - return { isAdding: true }; - } - return { - isActive: true, - value: anchor.getAttribute( 'href' ) || '', - target: anchor.getAttribute( 'target' ) || '', - node: anchor, - }; + if ( formatName === 'link' ) { + const anchor = find( parents, ( node ) => node.nodeName === 'A' ); + if ( anchor ) { + if ( anchor.hasAttribute( 'data-wp-placeholder' ) ) { + return { isAdding: true }; } + return { + isActive: true, + value: anchor.getAttribute( 'href' ) || '', + target: anchor.getAttribute( 'target' ) || '', + node: anchor, + }; } } From 47d0bd8e962ea4b0bce0700a4c92ed957c0452d7 Mon Sep 17 00:00:00 2001 From: Robert Anderson Date: Fri, 17 Aug 2018 10:01:21 +1000 Subject: [PATCH 4/4] Rename Link E2E tests MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Renames managing-links → links and makes the test descriptions read like English sentences. --- ...g-links.test.js.snap => links.test.js.snap} | 10 +++++----- .../{managing-links.test.js => links.test.js} | 18 +++++++++--------- 2 files changed, 14 insertions(+), 14 deletions(-) rename test/e2e/specs/__snapshots__/{managing-links.test.js.snap => links.test.js.snap} (65%) rename test/e2e/specs/{managing-links.test.js => links.test.js} (91%) diff --git a/test/e2e/specs/__snapshots__/managing-links.test.js.snap b/test/e2e/specs/__snapshots__/links.test.js.snap similarity index 65% rename from test/e2e/specs/__snapshots__/managing-links.test.js.snap rename to test/e2e/specs/__snapshots__/links.test.js.snap index 066c28ab4a5db..3bb19928ccb62 100644 --- a/test/e2e/specs/__snapshots__/managing-links.test.js.snap +++ b/test/e2e/specs/__snapshots__/links.test.js.snap @@ -1,30 +1,30 @@ // Jest Snapshot v1, https://goo.gl/fbAQLP -exports[`Managing links Creating a link by selecting text and clicking the Link UI 1`] = ` +exports[`Links can be created by selecting text and clicking Link 1`] = ` "

This is Gutenberg

" `; -exports[`Managing links Creating a link by selecting text and using keyboard shortcuts 1`] = ` +exports[`Links can be created by selecting text and using keyboard shortcuts 1`] = ` "

This is Gutenberg

" `; -exports[`Managing links Creating a link without any text selected 1`] = ` +exports[`Links can be created without any text selected 1`] = ` "

This is Gutenberg: https://wordpress.org/gutenberg

" `; -exports[`Managing links Editing a link 1`] = ` +exports[`Links can be edited 1`] = ` "

This is Gutenberg

" `; -exports[`Managing links Removing a link 1`] = ` +exports[`Links can be removed 1`] = ` "

This is Gutenberg

" diff --git a/test/e2e/specs/managing-links.test.js b/test/e2e/specs/links.test.js similarity index 91% rename from test/e2e/specs/managing-links.test.js rename to test/e2e/specs/links.test.js index ead98f31ba2d1..ece6bda4413a3 100644 --- a/test/e2e/specs/managing-links.test.js +++ b/test/e2e/specs/links.test.js @@ -16,12 +16,12 @@ import { */ const SELECT_WORD_MODIFIER_KEYS = process.platform === 'darwin' ? [ 'Shift', 'Alt' ] : [ 'Shift', 'Control' ]; -describe( 'Managing links', () => { +describe( 'Links', () => { beforeEach( async () => { await newPost(); } ); - it( 'Creating a link by selecting text and clicking the Link UI', async () => { + it( 'can be created by selecting text and clicking Link', async () => { // Create a block with some text await clickBlockAppender(); await page.keyboard.type( 'This is Gutenberg' ); @@ -48,7 +48,7 @@ describe( 'Managing links', () => { expect( await getEditedPostContent() ).toMatchSnapshot(); } ); - it( 'Creating a link by selecting text and using keyboard shortcuts', async () => { + it( 'can be created by selecting text and using keyboard shortcuts', async () => { // Create a block with some text await clickBlockAppender(); await page.keyboard.type( 'This is Gutenberg' ); @@ -75,7 +75,7 @@ describe( 'Managing links', () => { expect( await getEditedPostContent() ).toMatchSnapshot(); } ); - it( 'Creating a link without any text selected', async () => { + it( 'can be created without any text selected', async () => { // Create a block with some text await clickBlockAppender(); await page.keyboard.type( 'This is Gutenberg: ' ); @@ -100,7 +100,7 @@ describe( 'Managing links', () => { expect( await getEditedPostContent() ).toMatchSnapshot(); } ); - it( 'Creating a link and then cancelling', async () => { + it( 'is not created when we click away from the link input', async () => { // Create a block with some text await clickBlockAppender(); await page.keyboard.type( 'This is Gutenberg' ); @@ -148,7 +148,7 @@ describe( 'Managing links', () => { await page.click( 'a[href="https://wordpress.org/gutenberg"]' ); }; - it( 'Editing a link', async () => { + it( 'can be edited', async () => { await createAndReselectLink(); // Click on the Edit button @@ -164,7 +164,7 @@ describe( 'Managing links', () => { expect( await getEditedPostContent() ).toMatchSnapshot(); } ); - it( 'Removing a link', async () => { + it( 'can be removed', async () => { await createAndReselectLink(); // Click on the Unlink button @@ -187,7 +187,7 @@ describe( 'Managing links', () => { } }; - it( 'Pressing Left and Esc in Link Dialog in "Fixed to Toolbar" mode', async () => { + it( 'allows Left to be pressed during creation in "Fixed to Toolbar" mode', async () => { await setFixedToolbar( true ); await clickBlockAppender(); @@ -205,7 +205,7 @@ describe( 'Managing links', () => { expect( modal ).toBeNull(); } ); - it( 'Pressing Left and Esc in Link Dialog in "Docked Toolbar" mode', async () => { + it( 'allows Left to be pressed during creation in "Docked Toolbar" mode', async () => { await setFixedToolbar( false ); await clickBlockAppender();