-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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.
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
Refactor withFocusOutside to hook #27369
Conversation
Size Change: -77 B (0%) Total Size: 1.21 MB
ℹ️ View Unchanged
|
if ( isInteractionEnd ) { | ||
preventBlurCheck.current = false; | ||
} else if ( | ||
isFocusNormalizedButton( /** @type {HTMLElement} */ ( target ) ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be currentTarget
?
The
currentTarget
read-only property of theEvent
interface identifies the current target for the event, as the event traverses the DOM. It always refers to the element to which the event handler has been attached, as opposed toEvent.target
, which identifies the element on which the event occurred and which may be its descendant.
Take this with a grain of salt, I'm not especially familiar with how this is intended to be used.
The default type arguments for SyntheticEvent
suggest this, where currentTarget
defaults to type Element
(which extends Node
). Synthetic event (with default type arguments) here resolves to something like:
interface SyntheticEvent {
currentTarget: EventTarget & Element;
target: EventTarget;
// …
}
Note that SyntheticEvent
doesn't give us access to pass parameters to target
, which suggests that we can't reason much about it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I always have to look at this code closely to re-understand it!
I don't think it should be currentTarget
, as mentioned that would be the element the handler is bound to (usually some wrapping div
), but what this function is trying to determine is whether a child input, link or button had some interaction.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice, that's really helpful 👍
I took a look at why these types might be like this and found this MDN on EventTarget
:
Element
,Document
, andWindow
are the most common event targets, but other objects can be event targets, too. For exampleXMLHttpRequest
,AudioNode
,AudioContext
, and others.
It seems like the safest thing to do may be to make isFocusNormalizedButton
accept an EventTarget
.
Diff to accept `EventTarget`
diff --git a/packages/components/src/utils/hooks/use-focus-outside/index.js b/packages/components/src/utils/hooks/use-focus-outside/index.js
index b04efbf10d..690acd0a15 100644
--- a/packages/components/src/utils/hooks/use-focus-outside/index.js
+++ b/packages/components/src/utils/hooks/use-focus-outside/index.js
@@ -16,18 +16,22 @@ import { useEffect, useRef } from '@wordpress/element';
*/
const INPUT_BUTTON_TYPES = [ 'button', 'submit' ];
+/* eslint-disable jsdoc/valid-types */
/**
* 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 {HTMLElement} element Element to test.
+ * @param {EventTarget} eventTarget Element to test.
*
- * @return {boolean} Whether element is a button.
+ * @return {EventTarget is HTMLElement} Whether element is a button.
*/
-function isFocusNormalizedButton( element ) {
- switch ( element.nodeName ) {
+function isFocusNormalizedButton( eventTarget ) {
+ if ( ! ( eventTarget instanceof window.HTMLElement ) ) {
+ return false;
+ }
+ switch ( eventTarget.nodeName ) {
case 'A':
case 'BUTTON':
return true;
@@ -35,12 +39,13 @@ function isFocusNormalizedButton( element ) {
case 'INPUT':
return includes(
INPUT_BUTTON_TYPES,
- /** @type {HTMLInputElement} */ ( element ).type
+ /** @type {HTMLInputElement} */ ( eventTarget ).type
);
}
return false;
}
+/* eslint-enable jsdoc/valid-types */
/**
* @typedef {import('react').SyntheticEvent} SyntheticEvent
@@ -102,9 +107,7 @@ export default function useFocusOutside( onFocusOutside, __unstableNodeRef ) {
if ( isInteractionEnd ) {
preventBlurCheck.current = false;
- } else if (
- isFocusNormalizedButton( /** @type {HTMLElement} */ ( target ) )
- ) {
+ } else if ( isFocusNormalizedButton( target ) ) {
preventBlurCheck.current = true;
}
};
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome, thanks for the suggestion, that does feel better!
Does something like the following also work for the return value of isFocusNormalizedButton
?
/**
* @typedef {HTMLButtonElement | HTMLLinkElement | HTMLInputElement} FocusNormalizedButton
*
* // ...
*
* @return {eventTarget is FocusNormalizedButton} Whether element is a button.
*/
Was wondering if that'd be more accurate (since the return could also be a link or a button)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work 😎 That's more accurate, but it may not be very different because we'll only be able to use the common parts of that type union. For example, you can access href
on something of type HTMLLinkElement
, but not HTMLLinkElement | HTMLButtonElement
because href
doesn't exist on HTMLButtonElement
.
👍 This bit is to go either way and should be safer.
26dfffe
to
c6717b4
Compare
201074a
to
adca380
Compare
/** | ||
* @typedef {Object} FocusOutsideReactElement | ||
* @property {EventCallback} handleFocusOutside callback for a focus outside event. | ||
*/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've typed this instance of a react class component as an {Object}
with the method a @property
, which works, but I'm not sure if there's a better option like declaring it as an interface. I couldn't figure out how to do that with JSDoc!
Couldn't see any suggestions online, so
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We want something that satisfies an interface where we have a handleFocusOutside
function, which is what you've described here. It doesn't look like we need to care about whether it's a Component or something else as long as it satisfies our interface.
FYI: you can omit {Object}
in this case where we have @property
in the typedef.
/** | ||
* @typedef {import('react').SyntheticEvent} SyntheticEvent | ||
*/ | ||
|
||
/** | ||
* @callback EventCallback | ||
* @param {SyntheticEvent} event input related event. | ||
*/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This highlights one of the limitations of JSDoc compared to TypeScript. Default type arguments cannot be expressed, and we have to provide type parameters on the imported types. If we want to rely on default type arguments, we either cannot do a single import like /** @typedef {import('react').SyntheticEvent} SyntheticEvent */
or we have to do imports for each of the default types. I'll try to clarify
// 👇 SyntheticEvent accepts type parameters, but now we can't pass them. This is "fixed" as `SyntheticEvent<Element, Event>`.
/**
* @typedef {import('react').SyntheticEvent} SyntheticEvent
*/
// Now, although later we can reason about different event types we can't pass them into synthetic event 😞
/**
* @callback EventCallback
* @param {SyntheticEvent} event input related event.
*/
// If we want to wrap an imported type but keep (some of) the type parameters, we have to import here:
/**
* @template {Event} E
* @callback EventCallback
* @param {SyntheticEvent<Element, E>} event input related event.
* @return void
*/
// Now, for e.g. `handleFocusOutside` we can be very specific that it's a _focus_ event handler
/**
* @typedef FocusOutsideReactElement
* @property {EventCallback<FocusEvent>} handleFocusOutside callback for a focus outside event.
*/
This is a tradeoff, if we want an EventCallback
for Event
, we need to type it as EventCallback<Event>
or include another typedef without a type parameter.
We don't need to consider any of these tradeoffs in TypeScript syntax, it's better:
import type { SyntheticEvent } from 'react'
type SyntheticEventHandler<E extends Event = Event> = (e: SyntheticEvent<Element, E>) => void;
interface HandleFocusOutside {
handleFocusOutside: SyntheticEventHandler<FocusEvent>;
handleAnyEvent: SyntheticEventHandler; // No type params, handles `Event` (for demonstration)
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I noticed this when trying to alias MutableRefObject
to reduce the line length of some of the documentation. That doesn't have a default so it can't be aliased at all.
/** | ||
* @typedef {Object} FocusOutsideReactElement | ||
* @property {EventCallback} handleFocusOutside callback for a focus outside event. | ||
*/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We want something that satisfies an interface where we have a handleFocusOutside
function, which is what you've described here. It doesn't look like we need to care about whether it's a Component or something else as long as it satisfies our interface.
FYI: you can omit {Object}
in this case where we have @property
in the typedef.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Haven't tested it yet, just some nitpicking and code smell.
onFocus: cancelBlurCheck, | ||
onMouseDown: normalizeButtonFocus, | ||
onMouseUp: normalizeButtonFocus, | ||
onTouchStart: normalizeButtonFocus, | ||
onTouchEnd: normalizeButtonFocus, | ||
onBlur: queueBlurCheck, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A potential issue would be if the element receiving these props also has some of them defined, then these will override them. We can expect the developer to group the event handlers by themselves, or we can do it for them here, I'm okay with both approaches.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I don't like making the implementer handle so many callbacks, but I don't think there's a good alternative. Using DOM event handlers and a ref would be possible if the hook didn't have to handle virtual event bubbling, but it does.
What are the options for improving this? I've seen the pattern where the hook takes and returns the props from a component, which is slightly easier to use:
const propsWithFocusOutside = useFocusOutside( props, onFocusOutside );
return (
<div { ...propsWithFocusOutside }>
Is that what you mean when you say we can do it for them?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wait, nvm. I was thinking component design 😅 .
I think this is fine and I don't think we should pass props
to useFocusOutside
. However, I think we can try to refactor this to a component instead.
<FocusOutside as="div">
// ...
</FocusOutside>
And we can then handle the composition/grouping of the event handlers for them.
<FocusOutside as="div" onMouseDown={() => console.log("mousedown")}>
// ...
</FocusOutside>
function FocusOutside(props) {
// ...
return {
// ...
onMouseDown: groupEvents(normalizeButtonFocus, props.onMouseDown)
};
}
Maybe we can still try to keep this hook, but build the component with this hook. Either way I feel like the default export should be the component but not the hook.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hah 😄
Yeah, it's an interesting idea making it a component. I definitely need to think about future usage here. My original goal was to remove the wrapping div
introduced in the combobox component in #27367.
But within Popovers or Modals this will likely be used with some other HOCs/components:
withFocusReturn
(another HOC that renders adiv
)withConstrainedTabbing
(yet again another HOC that renders adiv
)IsolatedEventContainer
(a wrapper that renders adiv
)
That's why these components end up with so many div
wrappers when they render, and it'd be nice to reduce that as much as possible with a consistent approach (refactor to hooks/components).
Does the as
pattern scale well to multiple components that render the same element? I think that's where hooks might be better suited to the problem and are more explicit
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh this is interesting, thx for the additional info!
I think in theory we can still do it with as
prop:
<FocusOutside as={FocusReturn}>
// ...
</FocusOutside>
But this can easily become tedious if we want to combine from multiple sources. Hooks with a useCompositeProps
hook could solve this.
const htmlProps = useCompositeProps(
useFocusOutside(),
useFocusReturn(),
useConstrainedTabbing(),
// ...
);
<div {...htmlProps></div>
But this could also be a little bit overly complicated.
Maybe just export mergeEventHandlers
to the user so that they can combine them by themselves?
Might be worth studying how reakit solves this though.
packages/components/src/higher-order/with-focus-outside/index.js
Outdated
Show resolved
Hide resolved
packages/components/src/higher-order/with-focus-outside/index.js
Outdated
Show resolved
Hide resolved
node, | ||
node.handleFocusOutside | ||
) | ||
: undefined |
There was a problem hiding this comment.
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.
packages/components/src/higher-order/with-focus-outside/index.js
Outdated
Show resolved
Hide resolved
|
||
export default createHigherOrderComponent( | ||
( WrappedComponent ) => ( props ) => { | ||
const [ handleFocusOutside, setHandleFocusOutside ] = useState(); |
There was a problem hiding this comment.
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
?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?
I'd have preferred to find a way to implement the hook by returning a ref but it seems there's no way to attach React event handlers that way and we need that nested popovers bubbling. |
Let's make this experimental (until we try the ref approach) in @wordpress/compose and ship it. |
d1d1f9c
to
d9f0e01
Compare
Some failing e2e tests, the reusable block ones and the React Native tests. Might need to double-check I ported the native HOC ok in d9f0e01. But I've re-run them to confirm it's a legit failure. |
…e to unbound ref at render time
Co-authored-by: Kai Hao <[email protected]>
This reverts commit a4a4988.
b7bbe3b
to
cbd8a1f
Compare
Merging this and will follow up with any of the discussed points. |
// this blur event thereby leaving focus in place. | ||
// https://developer.mozilla.org/en-US/docs/Web/API/Document/hasFocus. | ||
if ( ! document.hasFocus() ) { | ||
event.preventDefault(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think you can prevent default an event after a timeout?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point! I did some digging and it looks like Andrew made a similar comment on the PR that introduced this, focus events aren't cancelable so preventDefault
would have no effect - #17051 (comment)
I'll remove this line in a separate PR.
Description
Refactors
withFocusOutside
to a react hookuseFocusOutside
. The hook is now used withinwithFocusOutside
to provide back compat.The hook has the following API:
How has this been tested?
Existing tests for
withFocusOutside
still pass.Add matching tests for
useFocusOutside
.Manual testing.
Types of changes
Non-breaking refactor.
Checklist: