Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Block Editor: LinkControl: Prevent focus loss in edit mode toggle #19931

Merged
merged 4 commits into from
Jan 30, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
56 changes: 52 additions & 4 deletions packages/block-editor/src/components/link-control/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -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
Expand Down Expand Up @@ -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 {
Expand All @@ -100,6 +103,34 @@ 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.
// 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 &&
! wrapperNode.current.contains( document.activeElement )
aduth marked this conversation as resolved.
Show resolved Hide resolved
);

if ( hadFocusLoss ) {
// Prefer to focus a natural focusable descendent of the wrapper,
// but settle for the wrapper if there are no other options.
const nextFocusTarget = (
focus.focusable.find( wrapperNode.current )[ 0 ] ||
wrapperNode.current
);

nextFocusTarget.focus();
}

isEndingEditWithFocus.current = false;
}, [ isEditingLink ] );

/**
* onChange LinkControlSearchInput event handler
*
Expand Down Expand Up @@ -156,6 +187,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 )
);
aduth marked this conversation as resolved.
Show resolved Hide resolved

setIsEditingLink( false );
}

// Effects
const getSearchHandler = useCallback( ( val, args ) => {
const protocol = getProtocol( val ) || '';
Expand Down Expand Up @@ -198,7 +242,7 @@ function LinkControl( {
itemProps={ buildSuggestionItemProps( suggestion, index ) }
suggestion={ suggestion }
onClick={ () => {
setIsEditingLink( false );
stopEditing();
onChange( { ...value, ...suggestion } );
} }
isSelected={ index === selectedSuggestion }
Expand All @@ -212,13 +256,17 @@ function LinkControl( {
};

return (
<div className="block-editor-link-control">
<div
tabIndex={ -1 }
ref={ wrapperNode }
className="block-editor-link-control"
>
{ isEditingLink || ! value ?
<LinkControlSearchInput
value={ inputValue }
onChange={ onInputChange }
onSelect={ ( suggestion ) => {
setIsEditingLink( false );
stopEditing();
onChange( { ...value, ...suggestion } );
} }
renderSuggestions={ renderSearchResults }
Expand Down
Original file line number Diff line number Diff line change
@@ -1,3 +1,3 @@
// Jest Snapshot v1, https://goo.gl/fbAQLP

exports[`Basic rendering should render 1`] = `"<div class=\\"block-editor-link-control\\"><form><div class=\\"components-base-control block-editor-url-input block-editor-link-control__search-input\\"><div class=\\"components-base-control__field\\"><input type=\\"text\\" aria-label=\\"URL\\" required=\\"\\" placeholder=\\"Search or type url\\" role=\\"combobox\\" aria-expanded=\\"false\\" aria-autocomplete=\\"list\\" aria-owns=\\"block-editor-url-input-suggestions-0\\" value=\\"\\"></div></div><button type=\\"reset\\" disabled=\\"\\" class=\\"components-button block-editor-link-control__search-reset has-icon\\" aria-label=\\"Reset\\"><svg aria-hidden=\\"true\\" role=\\"img\\" focusable=\\"false\\" xmlns=\\"http://www.w3.org/2000/svg\\" width=\\"20\\" height=\\"20\\" viewBox=\\"0 0 20 20\\" class=\\"dashicon dashicons-no-alt\\"><path d=\\"M14.95 6.46L11.41 10l3.54 3.54-1.41 1.41L10 11.42l-3.53 3.53-1.42-1.42L8.58 10 5.05 6.47l1.42-1.42L10 8.58l3.54-3.53z\\"></path></svg></button></form></div>"`;
exports[`Basic rendering should render 1`] = `"<div tabindex=\\"-1\\" class=\\"block-editor-link-control\\"><form><div class=\\"components-base-control block-editor-url-input block-editor-link-control__search-input\\"><div class=\\"components-base-control__field\\"><input type=\\"text\\" aria-label=\\"URL\\" required=\\"\\" placeholder=\\"Search or type url\\" role=\\"combobox\\" aria-expanded=\\"false\\" aria-autocomplete=\\"list\\" aria-owns=\\"block-editor-url-input-suggestions-0\\" value=\\"\\"></div></div><button type=\\"reset\\" disabled=\\"\\" class=\\"components-button block-editor-link-control__search-reset has-icon\\" aria-label=\\"Reset\\"><svg aria-hidden=\\"true\\" role=\\"img\\" focusable=\\"false\\" xmlns=\\"http://www.w3.org/2000/svg\\" width=\\"20\\" height=\\"20\\" viewBox=\\"0 0 20 20\\" class=\\"dashicon dashicons-no-alt\\"><path d=\\"M14.95 6.46L11.41 10l3.54 3.54-1.41 1.41L10 11.42l-3.53 3.53-1.42-1.42L8.58 10 5.05 6.47l1.42-1.42L10 8.58l3.54-3.53z\\"></path></svg></button></form></div>"`;
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand Down
14 changes: 1 addition & 13 deletions packages/block-library/src/button/edit.js
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand Down Expand Up @@ -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 );
};
Expand Down
10 changes: 0 additions & 10 deletions packages/block-library/src/navigation-link/edit.js
Original file line number Diff line number Diff line change
Expand Up @@ -68,16 +68,6 @@ function NavigationLinkEdit( {
}
}, [] );

/**
aduth marked this conversation as resolved.
Show resolved Hide resolved
* 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 (
<Fragment>
<BlockControls>
Expand Down
3 changes: 3 additions & 0 deletions packages/e2e-tests/specs/editor/blocks/buttons.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -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();
} );
Expand Down
3 changes: 3 additions & 0 deletions packages/e2e-tests/specs/editor/blocks/navigation.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -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 ) {
Expand Down