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

Remove autoFocus prop from URLInput and from the inserter search form #27578

Merged
merged 4 commits into from
Dec 9, 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
5 changes: 0 additions & 5 deletions packages/block-editor/src/components/inserter/menu.js
Original file line number Diff line number Diff line change
Expand Up @@ -116,10 +116,6 @@ function InserterMenu( {
/>
);

// Disable reason (no-autofocus): The inserter menu is a modal display, not one which
// is always visible, and one which already incurs this behavior of autoFocus via
// Popover's focusOnMount.
/* eslint-disable jsx-a11y/no-autofocus */
return (
<div className="block-editor-inserter__menu">
<div className="block-editor-inserter__main-area">
Expand Down Expand Up @@ -173,7 +169,6 @@ function InserterMenu( {
) }
</div>
);
/* eslint-enable jsx-a11y/no-autofocus */
}

export default InserterMenu;
Original file line number Diff line number Diff line change
Expand Up @@ -80,12 +80,6 @@ export default function QuickInserter( {
setInserterIsOpened( true );
};

// Disable reason (no-autofocus): The inserter menu is a modal display, not one which
// is always visible, and one which already incurs this behavior of autoFocus via
// Popover's focusOnMount.
// Disable reason (no-static-element-interactions): Navigational key-presses within
// the menu are prevented from triggering WritingFlow and ObserveTyping interactions.
/* eslint-disable jsx-a11y/no-autofocus, jsx-a11y/no-static-element-interactions */
return (
<div
className={ classnames( 'block-editor-inserter__quick-inserter', {
Expand Down Expand Up @@ -129,5 +123,4 @@ export default function QuickInserter( {
) }
</div>
);
/* eslint-enable jsx-a11y/no-autofocus, jsx-a11y/no-static-element-interactions */
}
6 changes: 0 additions & 6 deletions packages/block-editor/src/components/inserter/search-form.js
Original file line number Diff line number Diff line change
Expand Up @@ -16,10 +16,6 @@ function InserterSearchForm( { className, onChange, value, placeholder } ) {
const instanceId = useInstanceId( InserterSearchForm );
const searchInput = useRef();

// Disable reason (no-autofocus): The inserter menu is a modal display, not one which
// is always visible, and one which already incurs this behavior of autoFocus via
// Popover's focusOnMount.
/* eslint-disable jsx-a11y/no-autofocus */
return (
<div
className={ classnames(
Expand All @@ -39,7 +35,6 @@ function InserterSearchForm( { className, onChange, value, placeholder } ) {
id={ `block-editor-inserter__search-${ instanceId }` }
type="search"
placeholder={ placeholder }
autoFocus
onChange={ ( event ) => onChange( event.target.value ) }
autoComplete="off"
value={ value || '' }
Expand All @@ -59,7 +54,6 @@ function InserterSearchForm( { className, onChange, value, placeholder } ) {
</div>
</div>
);
/* eslint-enable jsx-a11y/no-autofocus */
}

export default InserterSearchForm;
16 changes: 9 additions & 7 deletions packages/block-editor/src/components/link-control/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -120,6 +120,7 @@ function LinkControl( {
withCreateSuggestion = true;
}

const isMounting = useRef( true );
const wrapperNode = useRef();
const [ internalInputValue, setInternalInputValue ] = useState(
( value && value.url ) || ''
Expand All @@ -142,19 +143,20 @@ function LinkControl( {
}, [ forceIsEditingLink ] );

useEffect( () => {
// When `isEditingLink` is set to `false`, a focus loss could occur
if ( isMounting.current ) {
isMounting.current = false;
return;
}
// When `isEditingLink` changes, 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(
wrapperNode.current.ownerDocument.activeElement
);
const hadFocusLoss = ! wrapperNode.current.contains(
wrapperNode.current.ownerDocument.activeElement
);

if ( hadFocusLoss ) {
// Prefer to focus a natural focusable descendent of the wrapper,
Expand Down
56 changes: 5 additions & 51 deletions packages/block-editor/src/components/link-control/test/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ import { default as lodash, first, last, nth, uniqueId } from 'lodash';
/**
* WordPress dependencies
*/
import { useState, useRef } from '@wordpress/element';
import { useState } from '@wordpress/element';
import { UP, DOWN, ENTER } from '@wordpress/keycodes';
/**
* Internal dependencies
Expand Down Expand Up @@ -594,12 +594,13 @@ describe( 'Default search suggestions', () => {
Simulate.click( currentLinkBtn );
} );

const searchInput = getURLInput();
searchInput.focus();

await eventLoopTick();

const searchResultElements = getSearchResults();

const searchInput = getURLInput();

// search input is set to the URL value
expect( searchInput.value ).toEqual( fauxEntitySuggestions[ 0 ].url );

Expand Down Expand Up @@ -1372,6 +1373,7 @@ describe( 'Selecting links', () => {

// Search Input UI
const searchInput = getURLInput();
searchInput.focus();
const form = container.querySelector( 'form' );

// Simulate searching for a term
Expand Down Expand Up @@ -1540,54 +1542,6 @@ describe( 'Selecting links', () => {
expect( mockFetchSearchSuggestions ).toHaveBeenCalledTimes( 1 );
} );
} );

it( 'does not forcefully regain focus if onChange handler had shifted it', () => {
// Regression: Previously, there had been issues where if `onChange`
// would programmatically shift focus, LinkControl would try to force it
// back, based on its internal logic to determine whether it had focus
// when finishing an edit occuring _before_ `onChange` having been run.
//
// See: https://github.com/WordPress/gutenberg/pull/19462

const LinkControlConsumer = () => {
const focusTarget = useRef();

return (
<>
<div tabIndex={ -1 } data-expected ref={ focusTarget } />
<LinkControl
onChange={ () => focusTarget.current.focus() }
/>
</>
);
};

act( () => {
render( <LinkControlConsumer />, container );
} );

// Change value.
const form = container.querySelector( 'form' );
const searchInput = getURLInput();

// Simulate searching for a term
act( () => {
Simulate.change( searchInput, {
target: { value: 'https://example.com' },
} );
} );
act( () => {
Simulate.keyDown( searchInput, { keyCode: ENTER } );
} );
act( () => {
Simulate.submit( form );
} );

const isExpectedFocusTarget = document.activeElement.hasAttribute(
'data-expected'
);
expect( isExpectedFocusTarget ).toBe( true );
} );
} );

describe( 'Addition Settings UI', () => {
Expand Down
6 changes: 0 additions & 6 deletions packages/block-editor/src/components/url-input/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -129,12 +129,6 @@ Renders the URL input field used by the `URLInputButton` component. It can be us

*Optional.* If this property is added, a label will be generated using label property as the content.

### `autoFocus: Boolean`

*Optional.* By default, the input will gain focus when it is rendered, as typically it is displayed conditionally. For example when clicking on `URLInputButton` or editing a block.

If you are not conditionally rendering this component set this property to `false`.

### `className: String`

*Optional.* Adds and optional class to the parent `div` that wraps the URLInput field and popover
Expand Down
4 changes: 0 additions & 4 deletions packages/block-editor/src/components/url-input/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,6 @@ import { withInstanceId, withSafeTimeout, compose } from '@wordpress/compose';
import { withSelect } from '@wordpress/data';
import { isURL } from '@wordpress/url';

/* eslint-disable jsx-a11y/no-autofocus */
class URLInput extends Component {
constructor( props ) {
super( props );
Expand Down Expand Up @@ -393,7 +392,6 @@ class URLInput extends Component {
placeholder = __( 'Paste URL or type to search' ),
__experimentalRenderControl: renderControl,
value = '',
autoFocus = true,
} = this.props;

const {
Expand All @@ -415,7 +413,6 @@ class URLInput extends Component {
const inputProps = {
value,
required: true,
autoFocus,
className: 'block-editor-url-input__input',
type: 'text',
onChange: this.onChange,
Expand Down Expand Up @@ -539,7 +536,6 @@ class URLInput extends Component {
return null;
}
}
/* eslint-enable jsx-a11y/no-autofocus */

/**
* @see https://github.com/WordPress/gutenberg/blob/master/packages/block-editor/src/components/url-input/README.md
Expand Down
2 changes: 1 addition & 1 deletion packages/block-library/src/navigation-link/edit.js
Original file line number Diff line number Diff line change
Expand Up @@ -170,7 +170,7 @@ function NavigationLinkEdit( {

// Show the LinkControl on mount if the URL is empty
// ( When adding a new menu item)
// This can't be done in the useState call because it cconflicts
// This can't be done in the useState call because it conflicts
// with the autofocus behavior of the BlockListBlock component.
useEffect( () => {
if ( ! url ) {
Expand Down
8 changes: 8 additions & 0 deletions packages/e2e-tests/specs/editor/blocks/buttons.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,15 @@ describe( 'Buttons', () => {
// Regression: https://github.com/WordPress/gutenberg/pull/19885
await insertBlock( 'Buttons' );
await pressKeyWithModifier( 'primary', 'k' );
await page.waitForFunction(
() => !! document.activeElement.closest( '.block-editor-url-input' )
);
await page.keyboard.press( 'Escape' );
await page.waitForFunction(
() =>
document.activeElement ===
document.querySelector( '.block-editor-rich-text__editable' )
);
await page.keyboard.type( 'WordPress' );

expect( await getEditedPostContent() ).toMatchSnapshot();
Expand Down
4 changes: 4 additions & 0 deletions packages/edit-post/src/components/layout/popover-wrapper.js
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import {
} from '@wordpress/components';
import { Component } from '@wordpress/element';
import { ESCAPE } from '@wordpress/keycodes';
import { useFocusOnMount } from '@wordpress/compose';

function stopPropagation( event ) {
event.stopPropagation();
Expand Down Expand Up @@ -39,11 +40,14 @@ export default function PopoverWrapper( { onClose, children, className } ) {
}
};

const ref = useFocusOnMount();

// Disable reason: this stops certain events from propagating outside of the component.
// - onMouseDown is disabled as this can cause interactions with other DOM elements
/* eslint-disable jsx-a11y/no-static-element-interactions */
return (
<div
ref={ ref }
className={ className }
onKeyDown={ maybeClose }
onMouseDown={ stopPropagation }
Expand Down
4 changes: 4 additions & 0 deletions packages/edit-site/src/components/editor/popover-wrapper.js
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import {
} from '@wordpress/components';
import { Component } from '@wordpress/element';
import { ESCAPE } from '@wordpress/keycodes';
import { useFocusOnMount } from '@wordpress/compose';

function stopPropagation( event ) {
event.stopPropagation();
Expand Down Expand Up @@ -39,11 +40,14 @@ export default function PopoverWrapper( { onClose, children, className } ) {
}
};

const ref = useFocusOnMount();

// Disable reason: this stops certain events from propagating outside of the component.
// - onMouseDown is disabled as this can cause interactions with other DOM elements
/* eslint-disable jsx-a11y/no-static-element-interactions */
return (
<div
ref={ ref }
className={ className }
onKeyDown={ maybeClose }
onMouseDown={ stopPropagation }
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import {
} from '@wordpress/components';
import { Component } from '@wordpress/element';
import { ESCAPE } from '@wordpress/keycodes';
import { useFocusOnMount } from '@wordpress/compose';

function stopPropagation( event ) {
event.stopPropagation();
Expand Down Expand Up @@ -39,11 +40,14 @@ export default function PopoverWrapper( { onClose, children, className } ) {
}
};

const ref = useFocusOnMount();

// Disable reason: this stops certain events from propagating outside of the component.
// - onMouseDown is disabled as this can cause interactions with other DOM elements
/* eslint-disable jsx-a11y/no-static-element-interactions */
return (
<div
ref={ ref }
className={ className }
onKeyDown={ maybeClose }
onMouseDown={ stopPropagation }
Expand Down
30 changes: 6 additions & 24 deletions packages/format-library/src/link/inline.js
Original file line number Diff line number Diff line change
@@ -1,12 +1,7 @@
/**
* External dependencies
*/
import { uniqueId } from 'lodash';

/**
* WordPress dependencies
*/
import { useMemo, useState } from '@wordpress/element';
import { useState, useRef } from '@wordpress/element';
import { __ } from '@wordpress/i18n';
import { withSpokenMessages, Popover } from '@wordpress/components';
import { prependHTTP } from '@wordpress/url';
Expand Down Expand Up @@ -35,22 +30,6 @@ function InlineLinkUI( {
stopAddingLink,
contentRef,
} ) {
/**
* A unique key is generated when switching between editing and not editing
* a link, based on:
*
* - This component may be rendered _either_ when a link is active _or_
* when adding or editing a link.
* - It's only desirable to shift focus into the Popover when explicitly
* adding or editing a link, not when in the inline boundary of a link.
* - Focus behavior can only be controlled on a Popover at the time it
* mounts, so a new instance of the component must be mounted to
* programmatically enact the focusOnMount behavior.
*
* @type {string}
*/
const mountingKey = useMemo( uniqueId, [ addingLink ] );

/**
* Pending settings to be applied to the next link. When inserting a new
* link, toggle values cannot be applied immediately, because there is not
Expand Down Expand Up @@ -146,11 +125,14 @@ function InlineLinkUI( {

const anchorRef = useAnchorRef( { ref: contentRef, value, settings } );

// The focusOnMount prop shouldn't evolve during render of a Popover
// otherwise it causes a render of the content.
const focusOnMount = useRef( addingLink ? 'firstElement' : false );

return (
<Popover
key={ mountingKey }
anchorRef={ anchorRef }
focusOnMount={ addingLink ? 'firstElement' : false }
focusOnMount={ focusOnMount.current }
onClose={ stopAddingLink }
position="bottom center"
>
Expand Down