[react-interactions] Refine custom active element blur logic #17354
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
trueadm
requested review from
threepointone,
sebmarkbage,
necolas and
gaearon
|
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. Latest deployment of this branch, based on commit eb5ba18:
|
trueadm
force-pushed
the
add-another-event
branch
from
2b81cbc to
8ac453f
Compare
Details of bundled changes.Comparing: b53ea6c...eb5ba18 react-dom
react-interactions
Size changes (stable) |
Details of bundled changes.Comparing: b53ea6c...eb5ba18 react-dom
react-interactions
ReactDOM: size: 0.0%, gzip: 0.0% Size changes (experimental) |
trueadm
force-pushed
the
add-another-event
branch
from
8ac453f to
e7ec5e1
Compare
tatermelon
reviewed
Nov 13, 2019
remove debugger Add beforeRemoveInstance fix typi
trueadm
force-pushed
the
add-another-event
branch
from
e7ec5e1 to
26a362a
Compare
necolas
reviewed
Nov 13, 2019
There was a problem hiding this comment.
I think the names need to be changed, as every blur event occurs on the activeElement. I suggest something like onFocusedElementDetached and onBeforeFocusedElementDetached.
An alternative approach might be not to use dedicated events and instead overload the blur events with additional meta-information, but this was not deemed a desirable approach internally because other event responders that listen to blur also need to know about this additional meta-information.
I don't understand this reasoning. Which event responders listening to blur would this impact? How does a new event avoid the issues?
|
@necolas Thanks. I've addressed all your feedback :) |
trueadm
force-pushed
the
add-another-event
branch
from
88b33d1 to
911ea82
Compare
necolas
reviewed
Nov 13, 2019
necolas
approved these changes
Nov 13, 2019
Fix typo remove dead code prettier
trueadm
force-pushed
the
add-another-event
branch
from
cae9f5c to
eb5ba18
Compare
necolas
reviewed
Nov 13, 2019
| @@ -104,6 +104,12 @@ export type ChildSet = void; // Unused | |||
| export type TimeoutHandle = TimeoutID; | |||
| export type NoTimeout = -1; | |||
|
|
|||
| type SelectionInformation = {| | |||
| blurredActiveElement: null | HTMLElement, | |||
There was a problem hiding this comment.
Suggested change
| blurredActiveElement: null | HTMLElement, | |
| detachedActiveElement: null | HTMLElement, |
|
|
||
| type FocusWithinProps = { | ||
| disabled?: boolean, | ||
| onFocusWithin?: (e: FocusEvent) => void, | ||
| onBlurWithin?: (e: FocusEvent) => void, | ||
| onFocusWithinChange?: boolean => void, | ||
| onFocusWithinVisibleChange?: boolean => void, | ||
| onDetachedVisibleNode?: (e: FocusEvent) => void, | ||
| onBeforeFocusedElementDetached?: (e: FocusEvent) => void, |
There was a problem hiding this comment.
onBeforeBlurWithin, only called when the element is going to be removed from the document
|
|
||
| type FocusWithinProps = { | ||
| disabled?: boolean, | ||
| onFocusWithin?: (e: FocusEvent) => void, | ||
| onBlurWithin?: (e: FocusEvent) => void, | ||
| onFocusWithinChange?: boolean => void, | ||
| onFocusWithinVisibleChange?: boolean => void, | ||
| onDetachedVisibleNode?: (e: FocusEvent) => void, | ||
| onBeforeFocusedElementDetached?: (e: FocusEvent) => void, | ||
| onFocusedElementDetached?: (e: FocusEvent) => void, |
There was a problem hiding this comment.
This wouldn't be needed, onBlurWithin payload can include the elementAttached data
Comment on lines
+65
to
+66
| | 'focusedelementdetached' | ||
| | 'beforefocusedelementdetached'; |
There was a problem hiding this comment.
Replaced with beforeblurwithin
Comment on lines
+487
to
+491
| function dispatchBeforeActiveElementBlur(element: HTMLElement): void { | ||
| const targtInstance = getClosestInstanceFromNode(element); | ||
| ((selectionInformation: any): SelectionInformation).blurredActiveElement = element; | ||
| dispatchFlareDetachedBlurEvent(false, targtInstance, element); | ||
| } |
There was a problem hiding this comment.
Suggested change
| function dispatchBeforeActiveElementBlur(element: HTMLElement): void { | |
| const targtInstance = getClosestInstanceFromNode(element); | |
| ((selectionInformation: any): SelectionInformation).blurredActiveElement = element; | |
| dispatchFlareDetachedBlurEvent(false, targtInstance, element); | |
| } | |
| function dispatchBeforeDetachedBlur(target: HTMLElement): void { | |
| const targtInstance = getClosestInstanceFromNode(element); | |
| selectionInformation.detachedActiveElement = target; | |
| dispatchEventForResponderEventSystem( | |
| 'beforeblur', | |
| targetInstance, | |
| ({ | |
| target, | |
| timeStamp: Date.now(), | |
| }: any), | |
| target, | |
| RESPONDER_EVENT_SYSTEM | IS_PASSIVE, | |
| ); |
Comment on lines
+215
to
+219
| const blurredActiveElement = (selectionInformation: any) | ||
| .blurredActiveElement; | ||
| if (blurredActiveElement !== null) { | ||
| dispatchActiveElementBlur(blurredActiveElement); | ||
| } |
There was a problem hiding this comment.
Suggested change
| const blurredActiveElement = (selectionInformation: any) | |
| .blurredActiveElement; | |
| if (blurredActiveElement !== null) { | |
| dispatchActiveElementBlur(blurredActiveElement); | |
| } | |
| const activeElementDetached = (selectionInformation: any).activeElementDetached; | |
| if (activeElementDetached !== null) { | |
| dispatchDetachedBlur(activeElementDetached); | |
| } |
Comment on lines
+493
to
+497
| function dispatchActiveElementBlur( | ||
| node: Instance | TextInstance | SuspenseInstance, | ||
| ): void { | ||
| dispatchFlareDetachedBlurEvent(true, null, ((node: any): HTMLElement)); | ||
| } |
There was a problem hiding this comment.
Suggested change
| function dispatchActiveElementBlur( | |
| node: Instance | TextInstance | SuspenseInstance, | |
| ): void { | |
| dispatchFlareDetachedBlurEvent(true, null, ((node: any): HTMLElement)); | |
| } | |
| function dispatchDetachedBlur(target) { | |
| const targtInstance = getClosestInstanceFromNode(target); | |
| dispatchEventForResponderEventSystem( | |
| 'blur', | |
| targetInstance, | |
| ({ | |
| isTargetAttached: false, | |
| target, | |
| timeStamp: Date.now(), | |
| }: any), | |
| target, | |
| RESPONDER_EVENT_SYSTEM | IS_PASSIVE, | |
| ); |
| ((child: any): Document | Element), | ||
| RESPONDER_EVENT_SYSTEM | IS_PASSIVE, | ||
| ); | ||
| dispatchBeforeActiveElementBlur(((instance: any): HTMLElement)); |
There was a problem hiding this comment.
Suggested change
| dispatchBeforeActiveElementBlur(((instance: any): HTMLElement)); | |
| dispatchBeforeDetachedBlur(((instance: any): HTMLElement)); |
| @@ -100,6 +100,8 @@ export function hasSelectionCapabilities(elem) { | |||
| export function getSelectionInformation() { | |||
| const focusedElem = getActiveElementDeep(); | |||
| return { | |||
| // Used by Flare | |||
| blurredActiveElement: null, | |||
There was a problem hiding this comment.
Suggested change
| blurredActiveElement: null, | |
| activeElementDetached: null, |
NMinhNguyen
referenced
this pull request
in enzymejs/react-shallow-renderer
Jan 29, 2020
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is a follow up to #17291. Specifically, these PR tackles three things:
onDetachedVisibleNode->onBeforeFocusedElementDetachedonFocusedElementDetached.beforeRemoveInstanceto correctly unify this logic (removing it from theremoveChildandremoveChildFromContainermethods)Why do we need two events? What is the for?
These callbacks are to help with our experimental internal accessibility work at Facebook. There are various ways to tackle this problem. I've been working closely with @tatermelon (Tatiana from the accessibility team) to guide this implementation internally and apply it.
The problem these tackle are when active elements (elements that are focused on the document) are unmounted. This is a big problem with accessibility and it's been a big problem that folks have tried to tackle for a while with hacks. The realism is that there's no real good way to do this in user-land today, which is evident by how many websites simply don't handle this use-case.
This problem is made more evident with the introduction of Suspense fallbacks, where content if focused for a spinner, but is then removed to show the actual real content after X time. We need some way to know that this has occurred, so we can re-align with the document and imperatively move focus to the right node.
Two events are essential here:
onBeforeFocusedElementDetachedfires before the active focused element is removed from the DOM. This is the perfect place to work out the "before" state, find what nodes are in view and do some calculations needed for the actual active blur event.onFocusedElementDetachedfires after all the changes have been committed and it's time to use the logic we stored in the before event, to work out what node we should imperatively focus next.