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

@@ -15,6 +15,7 @@
 ### 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)
 

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/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)>
 `;