Components: Add onFocusOutside replacement to Popover onClickOutside (#…

…14851)

* Components: Add onFocusOutside alternative to Popover onClickOutside

* Components: Refactor Dropdown to use onFocusOutside

* Format Library: Refactor inline link to use onFocusOutside

* MenuItem: Always use an IconButton component so as to avoid focus loss.

Switching between `Button` and `IconButton` causes React to remount the
`MenuItem` component. This causes a focus loss as well as E2E failures.

There's no need to use a `Button` for `MenuItems` that are unselected,
since we can simply pass `icon={ undefined }` to the `IconButton`.

packages/components/CHANGELOG.md

@@ -9,6 +9,15 @@
 
 - The `Button` component will no longer assign default styling (`is-default` class) when explicitly assigned as primary (the `isPrimary` prop). This should resolve potential conflicts affecting a combination of `isPrimary`, `isDefault`, and `isLarge` / `isSmall`, where the busy animation would appear with incorrect coloring.
 
+### Deprecations
+
+- The `Popover` component `onClickOutside` prop has been deprecated. Use `onFocusOutside` instead.
+
+### Internal
+
+- The `Dropdown` component has been refactored to focus changes using the `Popover` component's `onFocusOutside` prop.
+- The `MenuItem` component will now always use an `IconButton`. This prevents a focus loss when clicking a menu item.
+
 ## 8.0.0 (2019-06-12)
 
 ### New Feature

packages/components/package.json

@@ -24,6 +24,7 @@
 		"@babel/runtime": "^7.4.4",
 		"@wordpress/a11y": "file:../a11y",
 		"@wordpress/compose": "file:../compose",
+		"@wordpress/deprecated": "file:../deprecated",
 		"@wordpress/dom": "file:../dom",
 		"@wordpress/element": "file:../element",
 		"@wordpress/hooks": "file:../hooks",

packages/components/src/dropdown-menu/style.scss

@@ -99,7 +99,8 @@
 		border-bottom: $border-width solid $light-gray-500;
 	}
 
-	.components-menu-item__button {
+	.components-menu-item__button,
+	.components-menu-item__button.components-icon-button {
 		padding-left: 2rem;
 
 		&.has-icon {

packages/components/src/dropdown/index.js

@@ -14,7 +14,7 @@ class Dropdown extends Component {
 
 		this.toggle = this.toggle.bind( this );
 		this.close = this.close.bind( this );
-		this.closeIfClickOutside = this.closeIfClickOutside.bind( this );
+		this.closeIfFocusOutside = this.closeIfFocusOutside.bind( this );
 
 		this.containerRef = createRef();
 
@@ -46,15 +46,13 @@ class Dropdown extends Component {
 	}
 
 	/**
-	 * Closes the dropdown if a click occurs outside the dropdown wrapper. This
-	 * is intentionally distinct from `onClose` in that a click outside the
-	 * popover may occur in the toggling of the dropdown via its toggle button.
-	 * The correct behavior is to keep the dropdown closed.
-	 *
-	 * @param {MouseEvent} event Click event triggering `onClickOutside`.
+	 * Closes the dropdown if a focus leaves the dropdown wrapper. This is
+	 * intentionally distinct from `onClose` since focus loss from the popover
+	 * is expected to occur when using the Dropdown's toggle button, in which
+	 * case the correct behavior is to keep the dropdown closed.
 	 */
-	closeIfClickOutside( event ) {
-		if ( ! this.containerRef.current.contains( event.target ) ) {
+	closeIfFocusOutside() {
+		if ( ! this.containerRef.current.contains( document.activeElement ) ) {
 			this.close();
 		}
 	}
@@ -87,7 +85,7 @@ class Dropdown extends Component {
 						className={ contentClassName }
 						position={ position }
 						onClose={ this.close }
-						onClickOutside={ this.closeIfClickOutside }
+						onFocusOutside={ this.closeIfFocusOutside }
 						expandOnMobile={ expandOnMobile }
 						headerTitle={ headerTitle }
 						focusOnMount={ focusOnMount }

packages/components/src/menu-item/index.js

@@ -7,12 +7,11 @@ import { isString } from 'lodash';
 /**
  * WordPress dependencies
  */
-import { createElement, cloneElement } from '@wordpress/element';
+import { cloneElement } from '@wordpress/element';
 
 /**
  * Internal dependencies
  */
-import Button from '../button';
 import Shortcut from '../shortcut';
 import IconButton from '../icon-button';
 
@@ -47,32 +46,26 @@ export function MenuItem( {
 		);
 	}
 
-	let tagName = Button;
-
-	if ( icon ) {
-		if ( ! isString( icon ) ) {
-			icon = cloneElement( icon, {
-				className: 'components-menu-items__item-icon',
-				height: 20,
-				width: 20,
-			} );
-		}
-
-		tagName = IconButton;
-		props.icon = icon;
+	if ( icon && ! isString( icon ) ) {
+		icon = cloneElement( icon, {
+			className: 'components-menu-items__item-icon',
+			height: 20,
+			width: 20,
+		} );
 	}
 
-	return createElement(
-		tagName,
-		{
+	return (
+		<IconButton
+			icon={ icon }
 			// Make sure aria-checked matches spec https://www.w3.org/TR/wai-aria-1.1/#aria-checked
-			'aria-checked': ( role === 'menuitemcheckbox' || role === 'menuitemradio' ) ? isSelected : undefined,
-			role,
-			className,
-			...props,
-		},
-		children,
-		<Shortcut className="components-menu-item__shortcut" shortcut={ shortcut } />
+			aria-checked={ ( role === 'menuitemcheckbox' || role === 'menuitemradio' ) ? isSelected : undefined }
+			role={ role }
+			className={ className }
+			{ ...props }
+		>
+			{ children }
+			<Shortcut className="components-menu-item__shortcut" shortcut={ shortcut } />
+		</IconButton>
 	);
 }
 

packages/components/src/menu-item/test/__snapshots__/index.js.snap

@@ -17,7 +17,7 @@ exports[`MenuItem should match snapshot when all props provided 1`] = `
 `;
 
 exports[`MenuItem should match snapshot when info is provided 1`] = `
-<ForwardRef(Button)
+<ForwardRef(IconButton)
   className="components-menu-item__button"
   role="menuitem"
 >
@@ -34,7 +34,7 @@ exports[`MenuItem should match snapshot when info is provided 1`] = `
   <Shortcut
     className="components-menu-item__shortcut"
   />
-</ForwardRef(Button)>
+</ForwardRef(IconButton)>
 `;
 
 exports[`MenuItem should match snapshot when isSelected and role are optionally provided 1`] = `
@@ -53,13 +53,13 @@ exports[`MenuItem should match snapshot when isSelected and role are optionally
 `;
 
 exports[`MenuItem should match snapshot when only label provided 1`] = `
-<ForwardRef(Button)
+<ForwardRef(IconButton)
   className="components-menu-item__button"
   role="menuitem"
 >
   My item
   <Shortcut
     className="components-menu-item__shortcut"
   />
-</ForwardRef(Button)>
+</ForwardRef(IconButton)>
 `;

packages/components/src/popover/README.md

@@ -93,9 +93,11 @@ A callback invoked when the popover should be closed.
 - Type: `Function`
 - Required: No
 
-### onClickOutside
+### onFocusOutside
 
-A callback invoked when the user clicks outside the opened popover, passing the click event. The popover should be closed in response to this interaction. Defaults to `onClose`.
+A callback invoked when the focus leaves the opened popover. This should only be provided in advanced use-cases when a Popover should close under specific circumstances; for example, if the new `document.activeElement` is content of or otherwise controlling Popover visibility.
+
+Defaults to `onClose` when not provided.
 
 - Type: `Function`
 - Required: No

packages/components/src/popover/detect-outside.js

@@ -1,24 +1,21 @@
 /**
- * External dependencies
+ * WordPress dependencies
  */
-import clickOutside from 'react-click-outside';
+import { Component } from '@wordpress/element';
 
 /**
- * WordPress dependencies
+ * Internal dependencies
  */
-import { Component } from '@wordpress/element';
+import withFocusOutside from '../higher-order/with-focus-outside';
 
 class PopoverDetectOutside extends Component {
-	handleClickOutside( event ) {
-		const { onClickOutside } = this.props;
-		if ( onClickOutside ) {
-			onClickOutside( event );
-		}
+	handleFocusOutside( event ) {
+		this.props.onFocusOutside( event );
 	}
 
 	render() {
 		return this.props.children;
 	}
 }
 
-export default clickOutside( PopoverDetectOutside );
+export default withFocusOutside( PopoverDetectOutside );

packages/components/src/popover/index.js

@@ -10,6 +10,7 @@ import { useRef, useState, useEffect } from '@wordpress/element';
 import { focus } from '@wordpress/dom';
 import { ESCAPE } from '@wordpress/keycodes';
 import isShallowEqual from '@wordpress/is-shallow-equal';
+import deprecated from '@wordpress/deprecated';
 
 /**
  * Internal dependencies
@@ -251,7 +252,6 @@ const Popover = ( {
 	onKeyDown,
 	children,
 	className,
-	onClickOutside = onClose,
 	noArrow = false,
 	// Disable reason: We generate the `...contentProps` rest as remainder
 	// of props which aren't explicitly handled by this component.
@@ -263,6 +263,8 @@ const Popover = ( {
 	getAnchorRect,
 	expandOnMobile,
 	animate = true,
+	onClickOutside,
+	onFocusOutside,
 	/* eslint-enable no-unused-vars */
 	...contentProps
 } ) => {
@@ -308,6 +310,45 @@ const Popover = ( {
 		}
 	};
 
+	/**
+	 * Shims an onFocusOutside callback to be compatible with a deprecated
+	 * onClickOutside prop function, if provided.
+	 *
+	 * @param {FocusEvent} event Focus event from onFocusOutside.
+	 */
+	function handleOnFocusOutside( event ) {
+		// Defer to given `onFocusOutside` if specified. Call `onClose` only if
+		// both `onFocusOutside` and `onClickOutside` are unspecified. Doing so
+		// assures backwards-compatibility for prior `onClickOutside` default.
+		if ( onFocusOutside ) {
+			onFocusOutside( event );
+			return;
+		} else if ( ! onClickOutside ) {
+			onClose();
+			return;
+		}
+
+		// Simulate MouseEvent using FocusEvent#relatedTarget as emulated click
+		// target. MouseEvent constructor is unsupported in Internet Explorer.
+		let clickEvent;
+		try {
+			clickEvent = new window.MouseEvent( 'click' );
+		} catch ( error ) {
+			clickEvent = document.createEvent( 'MouseEvent' );
+			clickEvent.initMouseEvent( 'click', true, true, window, 0, 0, 0, 0, 0, false, false, false, false, 0, null );
+		}
+
+		Object.defineProperty( clickEvent, 'target', {
+			get: () => event.relatedTarget,
+		} );
+
+		deprecated( 'Popover onClickOutside prop', {
+			alternative: 'onFocusOutside',
+		} );
+
+		onClickOutside( clickEvent );
+	}
+
 	// Compute the animation position
 	const yAxisMapping = {
 		top: 'bottom',
@@ -339,7 +380,7 @@ const Popover = ( {
 
 	/* eslint-disable jsx-a11y/no-static-element-interactions */
 	let content = (
-		<PopoverDetectOutside onClickOutside={ onClickOutside }>
+		<PopoverDetectOutside onFocusOutside={ handleOnFocusOutside }>
 			<Animate
 				type={ animate && isReadyToAnimate ? 'appear' : null }
 				options={ { origin: animateYAxis + ' ' + animateXAxis } }

packages/components/src/popover/test/__snapshots__/index.js.snap

@@ -6,16 +6,18 @@ exports[`Popover should pass additional props to portaled element 1`] = `
     tabindex="-1"
   >
     <div>
-      <div
-        class="components-popover is-bottom is-center components-animate__appear is-from-top"
-        role="tooltip"
-        style=""
-      >
+      <div>
         <div
-          class="components-popover__content"
-          tabindex="-1"
+          class="components-popover is-bottom is-center components-animate__appear is-from-top"
+          role="tooltip"
+          style=""
         >
-          Hello
+          <div
+            class="components-popover__content"
+            tabindex="-1"
+          >
+            Hello
+          </div>
         </div>
       </div>
     </div>
@@ -29,15 +31,17 @@ exports[`Popover should render content 1`] = `
     tabindex="-1"
   >
     <div>
-      <div
-        class="components-popover is-bottom is-center components-animate__appear is-from-top"
-        style=""
-      >
+      <div>
         <div
-          class="components-popover__content"
-          tabindex="-1"
+          class="components-popover is-bottom is-center components-animate__appear is-from-top"
+          style=""
         >
-          Hello
+          <div
+            class="components-popover__content"
+            tabindex="-1"
+          >
+            Hello
+          </div>
         </div>
       </div>
     </div>

packages/format-library/CHANGELOG.md

@@ -1,3 +1,9 @@
+## Unreleased
+
+### Internal
+
+- The inline link component has been refactored to focus changes using the `Popover` component's `onFocusOutside` prop.
+
 ## 1.2.10 (2019-01-03)
 
 ## 1.2.9 (2018-12-18)