Skip to content

Commit

Permalink
Switch TextInput to useLayoutEffect
Browse files Browse the repository at this point in the history
Summary:
This diff fixes an issue in `TextInput` where `TextInputState.currentlyFocusedInputRef` could maintain a ref to a view that no longer exists. This issue was exposed when upgrading React, where cleanups for passive effects are deferred. This change means that `inputRef.current` would no longer reference the host view *to be* destroyed; it would be null because the view was *already destroyed*.

There are two fixes here that would independently fix the bug and fix the issue better together.

First, we convert `useEffect` to `useLayoutEffect`. `useLayoutEffect` is intended to fire synchronously after all host view mutations, and the cleanup function is intended to fire synchronously before the host view is destroyed, similar to the behavior assumed before. This change is now the correct function to use semantically. However, if we made this change without the second then any change in the order the effects fire would surface the same bug.

So second, move the `inputRefValue).blur()` call to the same effect as unregistering. This is because we currently require the `blur` effect to be called to null out `currentlyFocusedInputRef` in addition to calling `unregisterInput`. That makes the semantic ordering of effects in `TextInput` meaningful. Instead, when a TextInput is unregistered we should always `blur` to clear the `currentlyFocusedInputRef`, which will prevent dispatching events to a view that doesn't exist. If we made this change without the first then there's would be a race condition between calling blur on a TextInput and when that TextInput has been unregistered.

Changelog: [Internal]

Reviewed By: JoshuaGross

Differential Revision: D23035358

fbshipit-source-id: ab686b8046d85e2becd8b879b0b4b7e69e672754
  • Loading branch information
rickhanlonii authored and facebook-github-bot committed Aug 14, 2020
1 parent e27d656 commit 208ebed
Showing 1 changed file with 7 additions and 12 deletions.
19 changes: 7 additions & 12 deletions Libraries/Components/TextInput/TextInput.js
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ import type {PressEvent} from '../../Types/CoreEventTypes';
import type {HostComponent} from '../../Renderer/shims/ReactNativeTypes';
import type {TextInputNativeCommands} from './TextInputNativeCommands';

const {useEffect, useRef, useState} = React;
const {useLayoutEffect, useRef, useState} = React;

type ReactRefSetter<T> = {current: null | T, ...} | ((ref: null | T) => mixed);

Expand Down Expand Up @@ -869,7 +869,7 @@ function InternalTextInput(props: Props): React.Node {
// This is necessary in case native updates the text and JS decides
// that the update should be ignored and we should stick with the value
// that we have in JS.
useEffect(() => {
useLayoutEffect(() => {
const nativeUpdate = {};

if (lastNativeText !== props.value && typeof props.value === 'string') {
Expand Down Expand Up @@ -912,27 +912,22 @@ function InternalTextInput(props: Props): React.Node {
viewCommands,
]);

useEffect(() => {
useLayoutEffect(() => {
const inputRefValue = inputRef.current;

if (inputRefValue != null) {
TextInputState.registerInput(inputRefValue);

return () => {
TextInputState.unregisterInput(inputRefValue);

if (TextInputState.currentlyFocusedInput() === inputRefValue) {
nullthrows(inputRefValue).blur();
}
};
}
}, [inputRef]);

useEffect(() => {
// When unmounting we need to blur the input
return () => {
if (isFocused()) {
nullthrows(inputRef.current).blur();
}
};
}, [inputRef]);

function clear(): void {
if (inputRef.current != null) {
viewCommands.setTextAndSelection(
Expand Down

0 comments on commit 208ebed

Please sign in to comment.