Skip to content

Commit

Permalink
Remove autoFocus prop from URLInput and from the inserter search form (
Browse files Browse the repository at this point in the history
  • Loading branch information
youknowriad authored Dec 9, 2020
1 parent bbe7710 commit 0ea7996
Show file tree
Hide file tree
Showing 13 changed files with 41 additions and 111 deletions.
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

0 comments on commit 0ea7996

Please sign in to comment.