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.

Sign up for GitHub

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

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

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
238ee29
Block Editor: LinkControl: Prevent focus loss in edit mode toggle
aduth Jan 28, 2020
5b72b3a
Block Library: Remove custom focus loss protection
aduth Jan 28, 2020
0d6bf78
Block Editor: Rephrase and move forced input rendering comment
aduth Jan 29, 2020
eb8cac7
Block Editor: Ensure isEndingEditWithFocus always assigned as boolean
aduth Jan 29, 2020
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
55 changes: 51 additions & 4 deletions packages/block-editor/src/components/link-control/index.js
View file
Open in desktop
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,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 )
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. 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
*
Expand Down Expand Up @@ -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 )
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess as of #19831 I could simplify this using optional chaining:

Suggested change
wrapperNode.current &&
wrapperNode.current.contains( document.activeElement )
wrapperNode.current?.contains( document.activeElement )

What's interesting to me is looking at the other, negated instance of this condition, I sense there's going to be a very common pitfall associated with optional chaining:

gutenberg/packages/block-editor/src/components/link-control/index.js

Lines 112 to 113 in 5b72b3a

wrapperNode.current &&
! wrapperNode.current.contains( document.activeElement )

If someone treats optional chaining as a way to get to the value by any means necessary, it might be easy to overlook that negating would produce true if the optional chaining aborts early:

const wrapperNode = { current: undefined };
! wrapperNode.current?.contains( document.activeElement );
// true

😯 It's definitely not the expected result.

It seems like in the same category of issues we restrict with this ESLint rule:

https://eslint.org/docs/rules/no-unsafe-negation

I could imagine some improvement to that rule which forbids negation of optional chains.

cc @gziolo @sainthkh (more an FYI, as it was interesting to me)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another interesting pitfall: If you're expecting a boolean out of a value or function result at the end of an optional chain, you might be surprised to find that a non-boolean can be assigned if the chain aborts early, since it will evaluate to the value at the point at which it aborted.

const wrapperNode = { current: undefined };
const hasFocus = wrapperNode.current?.contains( document.activeElement );
console.log( typeof hasFocus === 'boolean' );
// `false`

This is similar to what can happen when using &&, where if the left-hand is falsy, the expression doesn't evaluate to an explicit false, but rather to the left-hand value.

You may have run into this one before 😄 https://codepen.io/aduth/pen/dWwzrL

It's not often a problem as long as the developer proceeds to use the value in a way which considers it as truthy or falsy, but it could be important to consider for satisfying function contracts, where we'd want to be considerate to adhere to the expected types.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very interesting case, thanks for sharing. To make it more interesting, let me share a bit different example which in the different context might also produce an unexpected result:

const wrapperNode = { current: 0 };
wrapperNode.current && ! wrapperNode.current.contains( document.activeElement );
// 0

It's always a challenge to make such checks work as intended. The only answer is to learn JavaScript deeply or cover code with tests 😅

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I just now observe that the same && applies to the current proposed implementation, at least if the expectation is that it would always assign a boolean, that's not currently happening (it can assign undefined).

);
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 +241,7 @@ function LinkControl( {
itemProps={ buildSuggestionItemProps( suggestion, index ) }
suggestion={ suggestion }
onClick={ () => {
setIsEditingLink( false );
stopEditing();
onChange( { ...value, ...suggestion } );
} }
isSelected={ index === selectedSuggestion }
Expand All @@ -212,13 +255,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
2 changes: 1 addition & 1 deletion packages/block-editor/src/components/link-control/test/__snapshots__/index.js.snap
View file
Open in desktop
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>"`;
3 changes: 3 additions & 0 deletions packages/block-editor/src/components/link-control/test/index.js
View file
Open in desktop
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
View file
Open in desktop
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
View file
Open in desktop
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
View file
Open in desktop
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
View file
Open in desktop
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