From 238ee2945f13c6fbb9551ed35808c656fcb4925e Mon Sep 17 00:00:00 2001 From: Andrew Duthie Date: Tue, 28 Jan 2020 12:27:03 -0500 Subject: [PATCH 1/4] Block Editor: LinkControl: Prevent focus loss in edit mode toggle --- .../src/components/link-control/index.js | 55 +++++++++++++++++-- .../test/__snapshots__/index.js.snap | 2 +- .../src/components/link-control/test/index.js | 3 + 3 files changed, 55 insertions(+), 5 deletions(-) diff --git a/packages/block-editor/src/components/link-control/index.js b/packages/block-editor/src/components/link-control/index.js index d8e6fa4284fdc8..62d15fb935857b 100644 --- a/packages/block-editor/src/components/link-control/index.js +++ b/packages/block-editor/src/components/link-control/index.js @@ -9,7 +9,7 @@ import { noop, startsWith } from 'lodash'; */ import { Button, ExternalLink, VisuallyHidden } from '@wordpress/components'; import { __, sprintf } from '@wordpress/i18n'; -import { useCallback, useState, Fragment } from '@wordpress/element'; +import { useRef, useCallback, useState, Fragment, useEffect } from '@wordpress/element'; import { safeDecodeURI, filterURLForDisplay, @@ -19,6 +19,7 @@ import { } from '@wordpress/url'; import { useInstanceId } from '@wordpress/compose'; import { useSelect } from '@wordpress/data'; +import { focus } from '@wordpress/dom'; /** * Internal dependencies @@ -89,9 +90,11 @@ function LinkControl( { onChange = noop, showInitialSuggestions, } ) { + const wrapperNode = useRef(); const instanceId = useInstanceId( LinkControl ); const [ inputValue, setInputValue ] = useState( ( value && value.url ) || '' ); const [ isEditingLink, setIsEditingLink ] = useState( ! value || ! value.url ); + const isEndingEditWithFocus = useRef( false ); const { fetchSearchSuggestions } = useSelect( ( select ) => { const { getSettings } = select( 'core/block-editor' ); return { @@ -100,6 +103,33 @@ function LinkControl( { }, [] ); const displayURL = ( value && filterURLForDisplay( safeDecodeURI( value.url ) ) ) || ''; + useEffect( () => { + // When `isEditingLink` is set to `false`, a focus loss could occur + // since the link input may be removed from the DOM. To avoid this, + // reinstate focus to a suitable target if focus has in-fact been lost. + const hadFocusLoss = ( + isEndingEditWithFocus.current && + wrapperNode.current && + ! wrapperNode.current.contains( document.activeElement ) + ); + + if ( hadFocusLoss ) { + // Prefer to focus a natural focusable descendent of the wrapper, + // but settle for the wrapper if there are no other options. Note + // that typically this would be the read-only mode's link element, + // but isn't guaranteed to be, since the input may continue to be + // forced to be shown if the next value is still unassigned. + const nextFocusTarget = ( + focus.focusable.find( wrapperNode.current )[ 0 ] || + wrapperNode.current + ); + + nextFocusTarget.focus(); + } + + isEndingEditWithFocus.current = false; + }, [ isEditingLink ] ); + /** * onChange LinkControlSearchInput event handler * @@ -156,6 +186,19 @@ function LinkControl( { return couldBeURL && ! args.isInitialSuggestions ? results[ 0 ].concat( results[ 1 ] ) : results[ 0 ]; }; + /** + * Cancels editing state and marks that focus may need to be restored after + * the next render, if focus was within the wrapper when editing finished. + */ + function stopEditing() { + isEndingEditWithFocus.current = ( + wrapperNode.current && + wrapperNode.current.contains( document.activeElement ) + ); + + setIsEditingLink( false ); + } + // Effects const getSearchHandler = useCallback( ( val, args ) => { const protocol = getProtocol( val ) || ''; @@ -198,7 +241,7 @@ function LinkControl( { itemProps={ buildSuggestionItemProps( suggestion, index ) } suggestion={ suggestion } onClick={ () => { - setIsEditingLink( false ); + stopEditing(); onChange( { ...value, ...suggestion } ); } } isSelected={ index === selectedSuggestion } @@ -212,13 +255,17 @@ function LinkControl( { }; return ( -
+
{ isEditingLink || ! value ? { - setIsEditingLink( false ); + stopEditing(); onChange( { ...value, ...suggestion } ); } } renderSuggestions={ renderSearchResults } diff --git a/packages/block-editor/src/components/link-control/test/__snapshots__/index.js.snap b/packages/block-editor/src/components/link-control/test/__snapshots__/index.js.snap index 046a4d31fd8610..f634dc91d0985e 100644 --- a/packages/block-editor/src/components/link-control/test/__snapshots__/index.js.snap +++ b/packages/block-editor/src/components/link-control/test/__snapshots__/index.js.snap @@ -1,3 +1,3 @@ // Jest Snapshot v1, https://goo.gl/fbAQLP -exports[`Basic rendering should render 1`] = `"
"`; +exports[`Basic rendering should render 1`] = `"
"`; diff --git a/packages/block-editor/src/components/link-control/test/index.js b/packages/block-editor/src/components/link-control/test/index.js index 6ec5cc3f01e15d..daf5dbbbf32cc2 100644 --- a/packages/block-editor/src/components/link-control/test/index.js +++ b/packages/block-editor/src/components/link-control/test/index.js @@ -598,6 +598,9 @@ describe( 'Selecting links', () => { const currentLinkHTML = currentLink.innerHTML; const currentLinkAnchor = currentLink.querySelector( `[href="${ selectedLink.url }"]` ); + // Make sure focus is retained after submission. + expect( container.contains( document.activeElement ) ).toBe( true ); + expect( currentLinkHTML ).toEqual( expect.stringContaining( selectedLink.title ) ); expect( currentLinkHTML ).toEqual( expect.stringContaining( 'Edit' ) ); expect( currentLinkAnchor ).not.toBeNull(); From 5b72b3a9b38da973cfd4bedd71a3210ddb607d45 Mon Sep 17 00:00:00 2001 From: Andrew Duthie Date: Tue, 28 Jan 2020 12:29:56 -0500 Subject: [PATCH 2/4] Block Library: Remove custom focus loss protection Previously used effect lifecycle to anticipate and respond to focus loss. Now, focus loss is prevented by LinkControl. See: 722a4d6dec --- packages/block-library/src/button/edit.js | 14 +------------- packages/block-library/src/navigation-link/edit.js | 10 ---------- .../e2e-tests/specs/editor/blocks/buttons.test.js | 3 +++ .../specs/editor/blocks/navigation.test.js | 3 +++ 4 files changed, 7 insertions(+), 23 deletions(-) diff --git a/packages/block-library/src/button/edit.js b/packages/block-library/src/button/edit.js index cdddfb0022c0c9..b67c34581c7f0c 100644 --- a/packages/block-library/src/button/edit.js +++ b/packages/block-library/src/button/edit.js @@ -8,11 +8,7 @@ import { escape } from 'lodash'; * WordPress dependencies */ import { __ } from '@wordpress/i18n'; -import { - useCallback, - useEffect, - useState, -} from '@wordpress/element'; +import { useCallback, useState } from '@wordpress/element'; import { compose, } from '@wordpress/compose'; @@ -85,14 +81,6 @@ function BorderPanel( { borderRadius = '', setAttributes } ) { function URLPicker( { isSelected, url, title, setAttributes, opensInNewTab, onToggleOpenInNewTab } ) { const [ isURLPickerOpen, setIsURLPickerOpen ] = useState( false ); - useEffect( - () => { - if ( ! isSelected ) { - setIsURLPickerOpen( false ); - } - }, - [ isSelected ] - ); const openLinkControl = () => { setIsURLPickerOpen( true ); }; diff --git a/packages/block-library/src/navigation-link/edit.js b/packages/block-library/src/navigation-link/edit.js index 1962366767f3a8..0605335859b0c4 100644 --- a/packages/block-library/src/navigation-link/edit.js +++ b/packages/block-library/src/navigation-link/edit.js @@ -68,16 +68,6 @@ function NavigationLinkEdit( { } }, [] ); - /** - * The hook shouldn't be necessary but due to a focus loss happening - * when selecting a suggestion in the link popover, we force close on block unselection. - */ - useEffect( () => { - if ( ! isSelected ) { - setIsLinkOpen( false ); - } - }, [ isSelected ] ); - return ( diff --git a/packages/e2e-tests/specs/editor/blocks/buttons.test.js b/packages/e2e-tests/specs/editor/blocks/buttons.test.js index 6906578e33ea58..bd975d6bdf1697 100644 --- a/packages/e2e-tests/specs/editor/blocks/buttons.test.js +++ b/packages/e2e-tests/specs/editor/blocks/buttons.test.js @@ -36,6 +36,9 @@ describe( 'Buttons', () => { await pressKeyWithModifier( 'primary', 'k' ); await page.keyboard.type( 'https://www.wordpress.org/' ); await page.keyboard.press( 'Enter' ); + // Make sure that the dialog is still opened, and that focus is retained + // within (focusing on the link preview). + await page.waitForSelector( ':focus.block-editor-link-control__search-item-title' ); expect( await getEditedPostContent() ).toMatchSnapshot(); } ); diff --git a/packages/e2e-tests/specs/editor/blocks/navigation.test.js b/packages/e2e-tests/specs/editor/blocks/navigation.test.js index 561b74ba80c421..42c108f0bcaf12 100644 --- a/packages/e2e-tests/specs/editor/blocks/navigation.test.js +++ b/packages/e2e-tests/specs/editor/blocks/navigation.test.js @@ -57,6 +57,9 @@ async function updateActiveNavigationLink( { url, label } ) { await page.keyboard.press( 'ArrowDown' ); // Select the suggestion. await page.keyboard.press( 'Enter' ); + // Make sure that the dialog is still opened, and that focus is retained + // within (focusing on the link preview). + await page.waitForSelector( ':focus.block-editor-link-control__search-item-title' ); } if ( label ) { From 0d6bf7834ccf2ef237651747c89d6c4d024c0235 Mon Sep 17 00:00:00 2001 From: Andrew Duthie Date: Wed, 29 Jan 2020 11:11:02 -0500 Subject: [PATCH 3/4] Block Editor: Rephrase and move forced input rendering comment --- .../block-editor/src/components/link-control/index.js | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/packages/block-editor/src/components/link-control/index.js b/packages/block-editor/src/components/link-control/index.js index 62d15fb935857b..bf6fa1a061aed8 100644 --- a/packages/block-editor/src/components/link-control/index.js +++ b/packages/block-editor/src/components/link-control/index.js @@ -107,6 +107,10 @@ function LinkControl( { // When `isEditingLink` is set to `false`, a focus loss could occur // since the link input may be removed from the DOM. To avoid this, // reinstate focus to a suitable target if focus has in-fact been lost. + // Note that the check is necessary because while typically unsetting + // edit mode would render the read-only mode's link element, it isn't + // guaranteed. The link input may continue to be shown if the next value + // is still unassigned after calling `onChange`. const hadFocusLoss = ( isEndingEditWithFocus.current && wrapperNode.current && @@ -115,10 +119,7 @@ function LinkControl( { if ( hadFocusLoss ) { // Prefer to focus a natural focusable descendent of the wrapper, - // but settle for the wrapper if there are no other options. Note - // that typically this would be the read-only mode's link element, - // but isn't guaranteed to be, since the input may continue to be - // forced to be shown if the next value is still unassigned. + // but settle for the wrapper if there are no other options. const nextFocusTarget = ( focus.focusable.find( wrapperNode.current )[ 0 ] || wrapperNode.current From eb8cac70d55dcb94c04ec6b22e668944e2b5421a Mon Sep 17 00:00:00 2001 From: Andrew Duthie Date: Wed, 29 Jan 2020 13:29:00 -0500 Subject: [PATCH 4/4] Block Editor: Ensure isEndingEditWithFocus always assigned as boolean --- packages/block-editor/src/components/link-control/index.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/block-editor/src/components/link-control/index.js b/packages/block-editor/src/components/link-control/index.js index bf6fa1a061aed8..d56dcc55f7b383 100644 --- a/packages/block-editor/src/components/link-control/index.js +++ b/packages/block-editor/src/components/link-control/index.js @@ -193,7 +193,7 @@ function LinkControl( { */ function stopEditing() { isEndingEditWithFocus.current = ( - wrapperNode.current && + !! wrapperNode.current && wrapperNode.current.contains( document.activeElement ) );