Skip to content

Refactor withFocusOutside to hook #27369

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

Merged
talldan merged 32 commits into from
Dec 10, 2020
Merged

Refactor withFocusOutside to hook #27369

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
32 commits
Select commit Hold shift + click to select a range
7a79026
Refactor withFocusOutside to a hook
talldan Nov 30, 2020
5a54bf2
Fix type for ref
talldan Nov 30, 2020
a12e8c0
Try to fix event target
talldan Nov 30, 2020
d240030
Refactor withFocusOutside to use useFocusOutside
talldan Nov 30, 2020
511d3fc
Fix detect-outside which should bind `this`
talldan Nov 30, 2020
0cf2155
Fix type issues
talldan Nov 30, 2020
1789e68
Remove unused ref
talldan Nov 30, 2020
79ebf26
Only cancel blur check on unmount
talldan Nov 30, 2020
ae10891
Remove need to bind callback when using withFocusOutside
talldan Dec 1, 2020
8a5f6b5
cancel blur check when onFocusOutside is no longer defined
talldan Dec 1, 2020
99260ad
Fix issue with HOC handleFocusOutside callback not being triggered du…
talldan Dec 2, 2020
cc4c941
Only return unstable ref when callback arg is not defined
talldan Dec 2, 2020
405e61f
Use an additional param for the unstable ref instead of a return value
talldan Dec 2, 2020
2ab9510
Move use-focus-outside to file alongside other hooks in folder
talldan Dec 3, 2020
440930b
Add tests for use-focus-outside
talldan Dec 3, 2020
e88d7ff
Improve types
talldan Dec 3, 2020
825422d
UseCallback for callbacks returned from hook
talldan Dec 3, 2020
4ed9409
Remove type from typedef
talldan Dec 4, 2020
85dec18
Remove refs from dependency list
talldan Dec 7, 2020
3aa2610
Update dependency list
talldan Dec 7, 2020
4290add
Update dependency list
talldan Dec 7, 2020
ddaf880
code formatting
talldan Dec 7, 2020
14c4786
Use a callback ref to remove the additional `__unstableNodeRef` param…
talldan Dec 8, 2020
3d3d5c3
Fix code review issues
talldan Dec 8, 2020
764e812
Use a ref for onFocusOutside
talldan Dec 9, 2020
33f7f92
Move useFocusOutside to @wordpress/compose and make experimental
talldan Dec 9, 2020
bef83bc
Port react native component
talldan Dec 9, 2020
f21e576
Remove missing export
talldan Dec 9, 2020
2282504
Try something
talldan Dec 9, 2020
96784d8
Revert "Try something"
talldan Dec 9, 2020
c848198
Make native version of hook
talldan Dec 10, 2020
cbd8a1f
Export native version of hook
talldan Dec 10, 2020
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
169 changes: 30 additions & 139 deletions packages/components/src/higher-order/with-focus-outside/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,142 +1,33 @@
/**
* External dependencies
*/
import { includes } from 'lodash';

/**
* WordPress dependencies
*/
import { Component } from '@wordpress/element';
import { createHigherOrderComponent } from '@wordpress/compose';

/**
* Input types which are classified as button types, for use in considering
* whether element is a (focus-normalized) button.
*
* @type {string[]}
*/
const INPUT_BUTTON_TYPES = [ 'button', 'submit' ];

/**
* Returns true if the given element is a button element subject to focus
* normalization, or false otherwise.
*
* @see https://developer.mozilla.org/en-US/docs/Web/HTML/Element/button#Clicking_and_focus
*
* @param {Element} element Element to test.
*
* @return {boolean} Whether element is a button.
*/
function isFocusNormalizedButton( element ) {
switch ( element.nodeName ) {
case 'A':
case 'BUTTON':
return true;

case 'INPUT':
return includes( INPUT_BUTTON_TYPES, element.type );
}

return false;
}

export default createHigherOrderComponent( ( WrappedComponent ) => {
return class extends Component {
constructor() {
super( ...arguments );

this.bindNode = this.bindNode.bind( this );
this.cancelBlurCheck = this.cancelBlurCheck.bind( this );
this.queueBlurCheck = this.queueBlurCheck.bind( this );
this.normalizeButtonFocus = this.normalizeButtonFocus.bind( this );
}

componentWillUnmount() {
this.cancelBlurCheck();
}

bindNode( node ) {
if ( node ) {
this.node = node;
} else {
delete this.node;
this.cancelBlurCheck();
}
}

queueBlurCheck( event ) {
// React does not allow using an event reference asynchronously
// due to recycling behavior, except when explicitly persisted.
event.persist();

// Skip blur check if clicking button. See `normalizeButtonFocus`.
if ( this.preventBlurCheck ) {
return;
}

this.blurCheckTimeout = setTimeout( () => {
// If document is not focused then focus should remain
// inside the wrapped component and therefore we cancel
// this blur event thereby leaving focus in place.
// https://developer.mozilla.org/en-US/docs/Web/API/Document/hasFocus.
if ( ! document.hasFocus() ) {
event.preventDefault();
return;
}
if ( 'function' === typeof this.node.handleFocusOutside ) {
this.node.handleFocusOutside( event );
}
}, 0 );
}

cancelBlurCheck() {
clearTimeout( this.blurCheckTimeout );
}

/**
* Handles a mousedown or mouseup event to respectively assign and
* unassign a flag for preventing blur check on button elements. Some
* browsers, namely Firefox and Safari, do not emit a focus event on
* button elements when clicked, while others do. The logic here
* intends to normalize this as treating click on buttons as focus.
*
* @see https://developer.mozilla.org/en-US/docs/Web/HTML/Element/button#Clicking_and_focus
*
* @param {MouseEvent} event Event for mousedown or mouseup.
*/
normalizeButtonFocus( event ) {
const { type, target } = event;

const isInteractionEnd = includes(
[ 'mouseup', 'touchend' ],
type
);

if ( isInteractionEnd ) {
this.preventBlurCheck = false;
} else if ( isFocusNormalizedButton( target ) ) {
this.preventBlurCheck = true;
}
}

render() {
// Disable reason: See `normalizeButtonFocus` for browser-specific
// focus event normalization.

/* eslint-disable jsx-a11y/no-static-element-interactions */
return (
<div
onFocus={ this.cancelBlurCheck }
onMouseDown={ this.normalizeButtonFocus }
onMouseUp={ this.normalizeButtonFocus }
onTouchStart={ this.normalizeButtonFocus }
onTouchEnd={ this.normalizeButtonFocus }
onBlur={ this.queueBlurCheck }
>
<WrappedComponent ref={ this.bindNode } { ...this.props } />
</div>
);
/* eslint-enable jsx-a11y/no-static-element-interactions */
}
};
}, 'withFocusOutside' );
import { useCallback, useState } from '@wordpress/element';
import {
createHigherOrderComponent,
__experimentalUseFocusOutside as useFocusOutside,
} from '@wordpress/compose';

export default createHigherOrderComponent(
( WrappedComponent ) => ( props ) => {
const [ handleFocusOutside, setHandleFocusOutside ] = useState();
Copy link
Contributor

@youknowriad youknowriad Dec 8, 2020

Choose a reason for hiding this comment

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

Why do we need this state at all, why not just pass the callback to useFocusOutside?

Copy link
Contributor Author

@talldan talldan Dec 9, 2020

Choose a reason for hiding this comment

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

Ok, long story:

Passing the callback works if it's wrapped in a closure in withFocusOutside like in your PR:

useFocusOutside( ( event ) => node.current?.handleFocusOutside( event ) );

However, I went for useState because of this method:

gutenberg/packages/components/src/higher-order/with-focus-outside/index.js

Lines 58 to 65 in 454c708

bindNode( node ) {
if ( node ) {
this.node = node;
} else {
delete this.node;
this.cancelBlurCheck();
}
}

That seems to say when node becomes a falsey value cancelBlurCheck should be triggered.

So the idea of using state is to emulate that, the state gets unset when the node isn't present and cancelBlurCheck is triggered in the other bit of code that you queried here - #27369 (comment)

If I go with the option of using the closure mentioned at the top of this comment, it means there's always an onFocusOutside callback provided to the hook so it won't be reactive to the node being falsey.

TBH, I find the use of cancelBlurCheck in bindNode quite hard to reason about. Is there likely to be a blur check in flight when the node is removed?

Copy link
Contributor

@youknowriad youknowriad Dec 9, 2020

Choose a reason for hiding this comment

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

Did you try to remove it and see whether any test fail?

Copy link
Contributor Author

@talldan talldan Dec 9, 2020

Choose a reason for hiding this comment

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

Just looking now, none of the unit tests fail and I didn't see any issues in manual testing. I can push the change to see if the e2e tests pass?

const bindFocusOutsideHandler = useCallback(
( node ) =>
setHandleFocusOutside( () =>
node?.handleFocusOutside
? node.handleFocusOutside.bind( node )
: undefined
Copy link
Member

@kevin940726 kevin940726 Dec 8, 2020

Choose a reason for hiding this comment

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

If we want to avoid re-rendering, then we should probably return the prev state to bail out update.

),
[]
);

return (
<div { ...useFocusOutside( handleFocusOutside ) }>
<WrappedComponent
ref={ bindFocusOutsideHandler }
{ ...props }
/>
</div>
);
},
'withFocusOutside'
);
156 changes: 30 additions & 126 deletions packages/components/src/higher-order/with-focus-outside/index.native.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,133 +1,37 @@
/**
* External dependencies
*/
import { includes } from 'lodash';
import { View } from 'react-native';

/**
* WordPress dependencies
*/
import { Component } from '@wordpress/element';
import { createHigherOrderComponent } from '@wordpress/compose';

/**
* Input types which are classified as button types, for use in considering
* whether element is a (focus-normalized) button.
*
* @type {string[]}
*/
const INPUT_BUTTON_TYPES = [ 'button', 'submit' ];

/**
* Returns true if the given element is a button element subject to focus
* normalization, or false otherwise.
*
* @see https://developer.mozilla.org/en-US/docs/Web/HTML/Element/button#Clicking_and_focus
*
* @param {Element} element Element to test.
*
* @return {boolean} Whether element is a button.
*/
function isFocusNormalizedButton( element ) {
switch ( element.nodeName ) {
case 'A':
case 'BUTTON':
return true;

case 'INPUT':
return includes( INPUT_BUTTON_TYPES, element.type );
}

return false;
}

export default createHigherOrderComponent( ( WrappedComponent ) => {
return class extends Component {
constructor() {
super( ...arguments );

this.bindNode = this.bindNode.bind( this );
this.cancelBlurCheck = this.cancelBlurCheck.bind( this );
this.queueBlurCheck = this.queueBlurCheck.bind( this );
this.normalizeButtonFocus = this.normalizeButtonFocus.bind( this );
}

componentWillUnmount() {
this.cancelBlurCheck();
}

bindNode( node ) {
if ( node ) {
this.node = node;
} else {
delete this.node;
this.cancelBlurCheck();
}
}

queueBlurCheck( event ) {
// React does not allow using an event reference asynchronously
// due to recycling behavior, except when explicitly persisted.
event.persist();

// Skip blur check if clicking button. See `normalizeButtonFocus`.
if ( this.preventBlurCheck ) {
return;
}

this.blurCheckTimeout = setTimeout( () => {
if ( 'function' === typeof this.node.handleFocusOutside ) {
this.node.handleFocusOutside( event );
}
}, 0 );
}

cancelBlurCheck() {
clearTimeout( this.blurCheckTimeout );
}

/**
* Handles a mousedown or mouseup event to respectively assign and
* unassign a flag for preventing blur check on button elements. Some
* browsers, namely Firefox and Safari, do not emit a focus event on
* button elements when clicked, while others do. The logic here
* intends to normalize this as treating click on buttons as focus.
*
* @see https://developer.mozilla.org/en-US/docs/Web/HTML/Element/button#Clicking_and_focus
*
* @param {MouseEvent} event Event for mousedown or mouseup.
*/
normalizeButtonFocus( event ) {
const { type, target } = event;

const isInteractionEnd = includes(
[ 'mouseup', 'touchend' ],
type
);

if ( isInteractionEnd ) {
this.preventBlurCheck = false;
} else if ( isFocusNormalizedButton( target ) ) {
this.preventBlurCheck = true;
}
}

render() {
// Disable reason: See `normalizeButtonFocus` for browser-specific
// focus event normalization.

return (
<View
onFocus={ this.cancelBlurCheck }
onMouseDown={ this.normalizeButtonFocus }
onMouseUp={ this.normalizeButtonFocus }
onTouchStart={ this.normalizeButtonFocus }
onTouchEnd={ this.normalizeButtonFocus }
onBlur={ this.queueBlurCheck }
>
<WrappedComponent ref={ this.bindNode } { ...this.props } />
</View>
);
}
};
}, 'withFocusOutside' );
import { useCallback, useState } from '@wordpress/element';
import {
createHigherOrderComponent,
__experimentalUseFocusOutside as useFocusOutside,
} from '@wordpress/compose';

export default createHigherOrderComponent(
( WrappedComponent ) => ( props ) => {
const [ handleFocusOutside, setHandleFocusOutside ] = useState();
const bindFocusOutsideHandler = useCallback(
( node ) =>
setHandleFocusOutside( () =>
node?.handleFocusOutside
? node.handleFocusOutside.bind( node )
: undefined
),
[]
);

return (
<View { ...useFocusOutside( handleFocusOutside ) }>
<WrappedComponent
ref={ bindFocusOutsideHandler }
{ ...props }
/>
</View>
);
},
'withFocusOutside'
);
Loading