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

Components: Add onFocusOutside replacement to Popover onClickOutside #14851

Merged
merged 4 commits into from
Aug 2, 2019
Merged

Components: Add onFocusOutside replacement to Popover onClickOutside #14851

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
d0a60d5
Components: Add onFocusOutside alternative to Popover onClickOutside
aduth Apr 5, 2019
2c86e97
Components: Refactor Dropdown to use onFocusOutside
aduth Apr 5, 2019
6231b63
Format Library: Refactor inline link to use onFocusOutside
aduth Apr 5, 2019
738d987
MenuItem: Always use an IconButton component so as to avoid focus loss.
noisysocks Aug 1, 2019
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
1 change: 1 addition & 0 deletions package-lock.json
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

9 changes: 9 additions & 0 deletions packages/components/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,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
Expand Down
1 change: 1 addition & 0 deletions packages/components/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand Down
3 changes: 2 additions & 1 deletion packages/components/src/dropdown-menu/style.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
18 changes: 8 additions & 10 deletions packages/components/src/dropdown/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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();

Expand Down Expand Up @@ -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();
}
}
Expand Down Expand Up @@ -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 }
Expand Down
43 changes: 18 additions & 25 deletions packages/components/src/menu-item/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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';

Expand Down Expand Up @@ -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
Copy link
Member

Choose a reason for hiding this comment

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

It's a bit stretched use of IconButton in the case when there is no icon 😅

However, I can't think of another simple way of handling it where this doesn't trigger re-render of the button. I noticed that this behavior is broken only because we swap IconButton with Button for no reason when only the icon gets removed/added.

Copy link
Member

Choose a reason for hiding this comment

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

Yep, exactly! Switching between the two components causes React to remount the component and lose focus. Looking at IconButton, it seemed to me that icon={} is an optional prop so I felt comfortable doing it this way.

Copy link
Contributor

Choose a reason for hiding this comment

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

One thing we keep discussing here and there is why not just add an icon prop to the Button block and deprecate the IconButton (or make it just a shortcut)

Copy link
Member

Choose a reason for hiding this comment

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

I'm sure @drw158 @cburton4 have some good ideas on how to approach it :)

I'm fine with anything which makes it less confusing. I guess having more flexible Button and IconButton as a backward-compatible alias makes a lot of sense.

Copy link
Contributor

Choose a reason for hiding this comment

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

We have an issue open to discuss combining IconButton and Button in #16541

We are leaning towards keeping them separate, but I'll look at this closer to understand the disadvantage.

Copy link
Contributor

@davewhitley davewhitley Aug 2, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

I'm assuming from the code that this is an issue because Button does not have an icon option. Switching between icon and non-icon buttons for Menu Items is causing technical problems.

Could this be solved by adding an icon option to the Button component, but still keep IconButton a separate component? That way, the unchecked and checked versions of the Menu Item could use just a single component, Button.

Copy link
Contributor

Choose a reason for hiding this comment

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

To be fair, we already do the opposite here. Not pass the "icon" prop to the IconButton (so it behaves like a Button). If it doesn't make sense to merge the two components into one, I feel we shouldn't do anything specific about it.

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>
);
}

Expand Down
8 changes: 4 additions & 4 deletions packages/components/src/menu-item/test/__snapshots__/index.js.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
>
Expand All @@ -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`] = `
Expand All @@ -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)>
`;
6 changes: 4 additions & 2 deletions packages/components/src/popover/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
17 changes: 7 additions & 10 deletions packages/components/src/popover/detect-outside.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,24 +1,21 @@
/**
* External dependencies
* WordPress dependencies
*/
import clickOutside from 'react-click-outside';
Copy link
Member

Choose a reason for hiding this comment

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

Unrelated question to this PR. Can we also refactor Modal component to stop using react-click-outside? :)

Copy link
Member Author

Choose a reason for hiding this comment

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

Unrelated question to this PR. Can we also refactor Modal component to stop using react-click-outside? :)

I would like to, yes.

( #6261 (comment) 😞 )

Copy link
Member

Choose a reason for hiding this comment

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

There is still hope then :)

Copy link
Member

Choose a reason for hiding this comment

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

https://unpkg.com/[email protected]/index.js

Well, it looks like react-click-outside is a simplified version of our own HOC :)

Copy link
Member

Choose a reason for hiding this comment

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

I opened a follow-up to address it – #16878.

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 );
Copy link
Contributor

Choose a reason for hiding this comment

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

This could be a nice hook const useOnFocusOutside( ref, handler )

45 changes: 43 additions & 2 deletions packages/components/src/popover/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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.
Expand All @@ -263,6 +263,8 @@ const Popover = ( {
getAnchorRect,
expandOnMobile,
animate = true,
onClickOutside,
onFocusOutside,
/* eslint-enable no-unused-vars */
...contentProps
} ) => {
Expand Down Expand Up @@ -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',
Expand Down Expand Up @@ -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 } }
Expand Down
34 changes: 19 additions & 15 deletions packages/components/src/popover/test/__snapshots__/index.js.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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>
Expand All @@ -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>
Expand Down
6 changes: 6 additions & 0 deletions packages/format-library/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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)
Expand Down
Loading