From ef9b2baa9afac6eb5f5bf929fb5e52a069fa2728 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Rub=C3=A9n=20Norte?= Date: Mon, 8 Jul 2024 09:42:46 -0700 Subject: [PATCH] Create test to move parts of TextInput state to refs to avoid unnecessary updates in effects (#45321) Summary: Pull Request resolved: https://github.com/facebook/react-native/pull/45321 Changelog: [internal] This creates a variant of the internal hook in `TextInput` that handles the synchronization of the state between native and JS. The new variant moves everything that's not needed for rendering to refs instead of state. One of the reasons for this change is that by not setting state in layout effects, we're not forcing passive effects to be flushed synchronously, which can improve perceived performance (as we can start painting before passive effects are executed). Reviewed By: sammy-SC Differential Revision: D59400624 fbshipit-source-id: 540c20daf49919fbfabd357a1a057ca126ec6b03 --- .../Components/TextInput/TextInput.js | 95 +++- .../TextInput/__tests__/TextInput-test.js | 470 +++++++++--------- .../__snapshots__/TextInput-test.js.snap | 60 ++- .../ReactNativeFeatureFlags.config.js | 5 + .../featureflags/ReactNativeFeatureFlags.js | 8 +- 5 files changed, 407 insertions(+), 231 deletions(-) diff --git a/packages/react-native/Libraries/Components/TextInput/TextInput.js b/packages/react-native/Libraries/Components/TextInput/TextInput.js index f2c8f7acd64..6f7454c1557 100644 --- a/packages/react-native/Libraries/Components/TextInput/TextInput.js +++ b/packages/react-native/Libraries/Components/TextInput/TextInput.js @@ -17,6 +17,7 @@ import type { import type {ViewProps} from '../View/ViewPropTypes'; import type {TextInputType} from './TextInput.flow'; +import * as ReactNativeFeatureFlags from '../../../src/private/featureflags/ReactNativeFeatureFlags'; import usePressability from '../../Pressability/usePressability'; import flattenStyle from '../../StyleSheet/flattenStyle'; import StyleSheet, { @@ -975,7 +976,7 @@ const emptyFunctionThatReturnsTrue = () => true; * in native and in JavaScript. This is necessary due to the asynchronous nature * of text input events. */ -function useTextInputStateSynchronization({ +function useTextInputStateSynchronization_STATE({ props, mostRecentEventCount, selection, @@ -1051,6 +1052,94 @@ function useTextInputStateSynchronization({ return {setLastNativeText, setLastNativeSelection}; } +/** + * This hook handles the synchronization between the state of the text input + * in native and in JavaScript. This is necessary due to the asynchronous nature + * of text input events. + */ +function useTextInputStateSynchronization_REFS({ + props, + mostRecentEventCount, + selection, + inputRef, + text, + viewCommands, +}: { + props: Props, + mostRecentEventCount: number, + selection: ?Selection, + inputRef: React.RefObject>>, + text: string, + viewCommands: ViewCommands, +}): { + setLastNativeText: string => void, + setLastNativeSelection: LastNativeSelection => void, +} { + const lastNativeTextRef = useRef(props.value); + const lastNativeSelectionRef = useRef({ + selection: {start: -1, end: -1}, + mostRecentEventCount: mostRecentEventCount, + }); + + // 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. + useLayoutEffect(() => { + const nativeUpdate: {text?: string, selection?: Selection} = {}; + + const lastNativeSelection = lastNativeSelectionRef.current.selection; + + if ( + lastNativeTextRef.current !== props.value && + typeof props.value === 'string' + ) { + nativeUpdate.text = props.value; + lastNativeTextRef.current = props.value; + } + + if ( + selection && + lastNativeSelection && + (lastNativeSelection.start !== selection.start || + lastNativeSelection.end !== selection.end) + ) { + nativeUpdate.selection = selection; + lastNativeSelectionRef.current = {selection, mostRecentEventCount}; + } + + if (Object.keys(nativeUpdate).length === 0) { + return; + } + + if (inputRef.current != null) { + viewCommands.setTextAndSelection( + inputRef.current, + mostRecentEventCount, + text, + selection?.start ?? -1, + selection?.end ?? -1, + ); + } + }, [ + mostRecentEventCount, + inputRef, + props.value, + props.defaultValue, + selection, + text, + viewCommands, + ]); + + return { + setLastNativeText: lastNativeText => { + lastNativeTextRef.current = lastNativeText; + }, + setLastNativeSelection: lastNativeSelection => { + lastNativeSelectionRef.current = lastNativeSelection; + }, + }; +} + /** * A foundational component for inputting text into the app via a * keyboard. Props provide configurability for several features, such as @@ -1204,6 +1293,10 @@ function InternalTextInput(props: Props): React.Node { : RCTSinglelineTextInputNativeCommands); const [mostRecentEventCount, setMostRecentEventCount] = useState(0); + const useTextInputStateSynchronization = + ReactNativeFeatureFlags.useRefsForTextInputState() + ? useTextInputStateSynchronization_REFS + : useTextInputStateSynchronization_STATE; const {setLastNativeText, setLastNativeSelection} = useTextInputStateSynchronization({ props, diff --git a/packages/react-native/Libraries/Components/TextInput/__tests__/TextInput-test.js b/packages/react-native/Libraries/Components/TextInput/__tests__/TextInput-test.js index 763838352fa..9378b4f9713 100644 --- a/packages/react-native/Libraries/Components/TextInput/__tests__/TextInput-test.js +++ b/packages/react-native/Libraries/Components/TextInput/__tests__/TextInput-test.js @@ -8,6 +8,7 @@ */ const {create} = require('../../../../jest/renderer'); +const ReactNativeFeatureFlags = require('../../../../src/private/featureflags/ReactNativeFeatureFlags'); const ReactNative = require('../../../ReactNative/RendererProxy'); const { enter, @@ -19,160 +20,174 @@ const ReactTestRenderer = require('react-test-renderer'); jest.unmock('../TextInput'); -describe('TextInput tests', () => { - let input; - let inputRef; - let onChangeListener; - let onChangeTextListener; - const initialValue = 'initialValue'; - beforeEach(async () => { - inputRef = React.createRef(null); - onChangeListener = jest.fn(); - onChangeTextListener = jest.fn(); - function TextInputWrapper() { - const [state, setState] = React.useState({text: initialValue}); +[true, false].forEach(useRefsForTextInputState => { + describe(`TextInput tests (useRefsForTextInputState = ${useRefsForTextInputState}`, () => { + let input; + let inputRef; + let onChangeListener; + let onChangeTextListener; + const initialValue = 'initialValue'; + beforeEach(async () => { + jest.resetModules(); + ReactNativeFeatureFlags.override({ + useRefsForTextInputState: () => useRefsForTextInputState, + }); - return ( - { - onChangeTextListener(text); - setState({text}); - }} - onChange={event => { - onChangeListener(event); - }} - /> + inputRef = React.createRef(null); + onChangeListener = jest.fn(); + onChangeTextListener = jest.fn(); + + function TextInputWrapper() { + const [state, setState] = React.useState({text: initialValue}); + + return ( + { + onChangeTextListener(text); + setState({text}); + }} + onChange={event => { + onChangeListener(event); + }} + /> + ); + } + const renderTree = await create(); + input = renderTree.root.findByType(TextInput); + }); + + it('has expected instance functions', () => { + expect(inputRef.current.isFocused).toBeInstanceOf(Function); // Would have prevented S168585 + expect(inputRef.current.clear).toBeInstanceOf(Function); + expect(inputRef.current.focus).toBeInstanceOf(jest.fn().constructor); + expect(inputRef.current.blur).toBeInstanceOf(jest.fn().constructor); + expect(inputRef.current.setNativeProps).toBeInstanceOf( + jest.fn().constructor, ); + expect(inputRef.current.measure).toBeInstanceOf(jest.fn().constructor); + expect(inputRef.current.measureInWindow).toBeInstanceOf( + jest.fn().constructor, + ); + expect(inputRef.current.measureLayout).toBeInstanceOf( + jest.fn().constructor, + ); + }); + it('calls onChange callbacks', () => { + expect(input.props.value).toBe(initialValue); + const message = 'This is a test message'; + ReactTestRenderer.act(() => { + enter(input, message); + }); + expect(input.props.value).toBe(message); + expect(onChangeTextListener).toHaveBeenCalledWith(message); + expect(onChangeListener).toHaveBeenCalledWith({ + nativeEvent: {text: message}, + }); + }); + + async function createTextInput(extraProps) { + const textInputRef = React.createRef(null); + await create( + , + ); + return textInputRef; } - const renderTree = await create(); - input = renderTree.root.findByType(TextInput); - }); - it('has expected instance functions', () => { - expect(inputRef.current.isFocused).toBeInstanceOf(Function); // Would have prevented S168585 - expect(inputRef.current.clear).toBeInstanceOf(Function); - expect(inputRef.current.focus).toBeInstanceOf(jest.fn().constructor); - expect(inputRef.current.blur).toBeInstanceOf(jest.fn().constructor); - expect(inputRef.current.setNativeProps).toBeInstanceOf( - jest.fn().constructor, - ); - expect(inputRef.current.measure).toBeInstanceOf(jest.fn().constructor); - expect(inputRef.current.measureInWindow).toBeInstanceOf( - jest.fn().constructor, - ); - expect(inputRef.current.measureLayout).toBeInstanceOf( - jest.fn().constructor, - ); - }); - it('calls onChange callbacks', () => { - expect(input.props.value).toBe(initialValue); - const message = 'This is a test message'; - ReactTestRenderer.act(() => { - enter(input, message); - }); - expect(input.props.value).toBe(message); - expect(onChangeTextListener).toHaveBeenCalledWith(message); - expect(onChangeListener).toHaveBeenCalledWith({ - nativeEvent: {text: message}, - }); - }); - async function createTextInput(extraProps) { - const textInputRef = React.createRef(null); - await create( - , - ); - return textInputRef; - } + it('focus() should not do anything if the TextInput is not editable', async () => { + const textInputRef = await createTextInput({editable: false}); + textInputRef.current.currentProps = textInputRef.current.props; + expect(textInputRef.current.isFocused()).toBe(false); - it('focus() should not do anything if the TextInput is not editable', async () => { - const textInputRef = await createTextInput({editable: false}); - textInputRef.current.currentProps = textInputRef.current.props; - expect(textInputRef.current.isFocused()).toBe(false); - - TextInput.State.focusTextInput(textInputRef.current); - expect(textInputRef.current.isFocused()).toBe(false); - }); - - it('should have support for being focused and blurred', async () => { - const textInputRef = await createTextInput(); - - expect(textInputRef.current.isFocused()).toBe(false); - ReactNative.findNodeHandle = jest.fn().mockImplementation(ref => { - if (ref == null) { - return null; - } - - if ( - ref === textInputRef.current || - ref === textInputRef.current.getNativeRef() - ) { - return 1; - } - - return 2; + TextInput.State.focusTextInput(textInputRef.current); + expect(textInputRef.current.isFocused()).toBe(false); }); - TextInput.State.focusTextInput(textInputRef.current); - expect(textInputRef.current.isFocused()).toBe(true); - expect(TextInput.State.currentlyFocusedInput()).toBe(textInputRef.current); + it('should have support for being focused and blurred', async () => { + const textInputRef = await createTextInput(); - TextInput.State.blurTextInput(textInputRef.current); - expect(textInputRef.current.isFocused()).toBe(false); - expect(TextInput.State.currentlyFocusedInput()).toBe(null); - }); + expect(textInputRef.current.isFocused()).toBe(false); + ReactNative.findNodeHandle = jest.fn().mockImplementation(ref => { + if (ref == null) { + return null; + } - it('should unfocus when other TextInput is focused', async () => { - const textInputRe1 = React.createRef(null); - const textInputRe2 = React.createRef(null); + if ( + ref === textInputRef.current || + ref === textInputRef.current.getNativeRef() + ) { + return 1; + } - await create( - <> - - - , - ); - ReactNative.findNodeHandle = jest.fn().mockImplementation(ref => { - if ( - ref === textInputRe1.current || - ref === textInputRe1.current.getNativeRef() - ) { - return 1; - } - - if ( - ref === textInputRe2.current || - ref === textInputRe2.current.getNativeRef() - ) { return 2; - } + }); - return 3; + TextInput.State.focusTextInput(textInputRef.current); + expect(textInputRef.current.isFocused()).toBe(true); + expect(TextInput.State.currentlyFocusedInput()).toBe( + textInputRef.current, + ); + + TextInput.State.blurTextInput(textInputRef.current); + expect(textInputRef.current.isFocused()).toBe(false); + expect(TextInput.State.currentlyFocusedInput()).toBe(null); }); - expect(textInputRe1.current.isFocused()).toBe(false); - expect(textInputRe2.current.isFocused()).toBe(false); + it('should unfocus when other TextInput is focused', async () => { + const textInputRe1 = React.createRef(null); + const textInputRe2 = React.createRef(null); - TextInput.State.focusTextInput(textInputRe1.current); + await create( + <> + + + , + ); + ReactNative.findNodeHandle = jest.fn().mockImplementation(ref => { + if ( + ref === textInputRe1.current || + ref === textInputRe1.current.getNativeRef() + ) { + return 1; + } - expect(textInputRe1.current.isFocused()).toBe(true); - expect(textInputRe2.current.isFocused()).toBe(false); - expect(TextInput.State.currentlyFocusedInput()).toBe(textInputRe1.current); + if ( + ref === textInputRe2.current || + ref === textInputRe2.current.getNativeRef() + ) { + return 2; + } - TextInput.State.focusTextInput(textInputRe2.current); + return 3; + }); - expect(textInputRe1.current.isFocused()).toBe(false); - expect(textInputRe2.current.isFocused()).toBe(true); - expect(TextInput.State.currentlyFocusedInput()).toBe(textInputRe2.current); - }); + expect(textInputRe1.current.isFocused()).toBe(false); + expect(textInputRe2.current.isFocused()).toBe(false); - it('should give precedence to `textContentType` when set', async () => { - const instance = await create( - , - ); + TextInput.State.focusTextInput(textInputRe1.current); - expect(instance.toJSON()).toMatchInlineSnapshot(` + expect(textInputRe1.current.isFocused()).toBe(true); + expect(textInputRe2.current.isFocused()).toBe(false); + expect(TextInput.State.currentlyFocusedInput()).toBe( + textInputRe1.current, + ); + + TextInput.State.focusTextInput(textInputRe2.current); + + expect(textInputRe1.current.isFocused()).toBe(false); + expect(textInputRe2.current.isFocused()).toBe(true); + expect(TextInput.State.currentlyFocusedInput()).toBe( + textInputRe2.current, + ); + }); + + it('should give precedence to `textContentType` when set', async () => { + const instance = await create( + , + ); + + expect(instance.toJSON()).toMatchInlineSnapshot(` { underlineColorAndroid="transparent" /> `); + }); + + it('should render as expected', async () => { + await expectRendersMatchingSnapshot( + 'TextInput', + () => , + () => { + jest.dontMock('../TextInput'); + }, + ); + }); }); - it('should render as expected', async () => { - await expectRendersMatchingSnapshot( - 'TextInput', - () => , - () => { - jest.dontMock('../TextInput'); - }, - ); - }); -}); + describe('TextInput', () => { + it('default render', async () => { + const instance = await create(); -describe('TextInput', () => { - it('default render', async () => { - const instance = await create(); - - expect(instance.toJSON()).toMatchInlineSnapshot(` + expect(instance.toJSON()).toMatchInlineSnapshot(` { underlineColorAndroid="transparent" /> `); + }); + + it('has displayName', () => { + expect(TextInput.displayName).toEqual('TextInput'); + }); }); - it('has displayName', () => { - expect(TextInput.displayName).toEqual('TextInput'); - }); -}); + describe('TextInput compat with web', () => { + it('renders core props', async () => { + const props = { + id: 'id', + tabIndex: 0, + testID: 'testID', + }; -describe('TextInput compat with web', () => { - it('renders core props', async () => { - const props = { - id: 'id', - tabIndex: 0, - testID: 'testID', - }; + const instance = await create(); - const instance = await create(); - - expect(instance.toJSON()).toMatchInlineSnapshot(` + expect(instance.toJSON()).toMatchInlineSnapshot(` { underlineColorAndroid="transparent" /> `); - }); + }); - it('renders "aria-*" props', async () => { - const props = { - 'aria-activedescendant': 'activedescendant', - 'aria-atomic': true, - 'aria-autocomplete': 'list', - 'aria-busy': true, - 'aria-checked': true, - 'aria-columncount': 5, - 'aria-columnindex': 3, - 'aria-columnspan': 2, - 'aria-controls': 'controls', - 'aria-current': 'current', - 'aria-describedby': 'describedby', - 'aria-details': 'details', - 'aria-disabled': true, - 'aria-errormessage': 'errormessage', - 'aria-expanded': true, - 'aria-flowto': 'flowto', - 'aria-haspopup': true, - 'aria-hidden': true, - 'aria-invalid': true, - 'aria-keyshortcuts': 'Cmd+S', - 'aria-label': 'label', - 'aria-labelledby': 'labelledby', - 'aria-level': 3, - 'aria-live': 'polite', - 'aria-modal': true, - 'aria-multiline': true, - 'aria-multiselectable': true, - 'aria-orientation': 'portrait', - 'aria-owns': 'owns', - 'aria-placeholder': 'placeholder', - 'aria-posinset': 5, - 'aria-pressed': true, - 'aria-readonly': true, - 'aria-required': true, - role: 'main', - 'aria-roledescription': 'roledescription', - 'aria-rowcount': 5, - 'aria-rowindex': 3, - 'aria-rowspan': 3, - 'aria-selected': true, - 'aria-setsize': 5, - 'aria-sort': 'ascending', - 'aria-valuemax': 5, - 'aria-valuemin': 0, - 'aria-valuenow': 3, - 'aria-valuetext': '3', - }; + it('renders "aria-*" props', async () => { + const props = { + 'aria-activedescendant': 'activedescendant', + 'aria-atomic': true, + 'aria-autocomplete': 'list', + 'aria-busy': true, + 'aria-checked': true, + 'aria-columncount': 5, + 'aria-columnindex': 3, + 'aria-columnspan': 2, + 'aria-controls': 'controls', + 'aria-current': 'current', + 'aria-describedby': 'describedby', + 'aria-details': 'details', + 'aria-disabled': true, + 'aria-errormessage': 'errormessage', + 'aria-expanded': true, + 'aria-flowto': 'flowto', + 'aria-haspopup': true, + 'aria-hidden': true, + 'aria-invalid': true, + 'aria-keyshortcuts': 'Cmd+S', + 'aria-label': 'label', + 'aria-labelledby': 'labelledby', + 'aria-level': 3, + 'aria-live': 'polite', + 'aria-modal': true, + 'aria-multiline': true, + 'aria-multiselectable': true, + 'aria-orientation': 'portrait', + 'aria-owns': 'owns', + 'aria-placeholder': 'placeholder', + 'aria-posinset': 5, + 'aria-pressed': true, + 'aria-readonly': true, + 'aria-required': true, + role: 'main', + 'aria-roledescription': 'roledescription', + 'aria-rowcount': 5, + 'aria-rowindex': 3, + 'aria-rowspan': 3, + 'aria-selected': true, + 'aria-setsize': 5, + 'aria-sort': 'ascending', + 'aria-valuemax': 5, + 'aria-valuemin': 0, + 'aria-valuenow': 3, + 'aria-valuetext': '3', + }; - const instance = await create(); + const instance = await create(); - expect(instance.toJSON()).toMatchInlineSnapshot(` + expect(instance.toJSON()).toMatchInlineSnapshot(` { underlineColorAndroid="transparent" /> `); - }); + }); - it('renders styles', async () => { - const style = { - display: 'flex', - flex: 1, - backgroundColor: 'white', - marginInlineStart: 10, - userSelect: 'none', - verticalAlign: 'middle', - }; + it('renders styles', async () => { + const style = { + display: 'flex', + flex: 1, + backgroundColor: 'white', + marginInlineStart: 10, + userSelect: 'none', + verticalAlign: 'middle', + }; - const instance = await create(); + const instance = await create(); - expect(instance.toJSON()).toMatchInlineSnapshot(` + expect(instance.toJSON()).toMatchInlineSnapshot(` { underlineColorAndroid="transparent" /> `); + }); }); }); diff --git a/packages/react-native/Libraries/Components/TextInput/__tests__/__snapshots__/TextInput-test.js.snap b/packages/react-native/Libraries/Components/TextInput/__tests__/__snapshots__/TextInput-test.js.snap index 598c414a893..6f672ac96c5 100644 --- a/packages/react-native/Libraries/Components/TextInput/__tests__/__snapshots__/TextInput-test.js.snap +++ b/packages/react-native/Libraries/Components/TextInput/__tests__/__snapshots__/TextInput-test.js.snap @@ -1,6 +1,6 @@ // Jest Snapshot v1, https://goo.gl/fbAQLP -exports[`TextInput tests should render as expected: should deep render when mocked (please verify output manually) 1`] = ` +exports[`TextInput tests (useRefsForTextInputState = false should render as expected: should deep render when mocked (please verify output manually) 1`] = ` `; -exports[`TextInput tests should render as expected: should deep render when not mocked (please verify output manually) 1`] = ` +exports[`TextInput tests (useRefsForTextInputState = false should render as expected: should deep render when not mocked (please verify output manually) 1`] = ` + +`; + +exports[`TextInput tests (useRefsForTextInputState = true should render as expected: should deep render when mocked (please verify output manually) 1`] = ` + +`; + +exports[`TextInput tests (useRefsForTextInputState = true should render as expected: should deep render when not mocked (please verify output manually) 1`] = ` > + * @generated SignedSource<<26d20b285e4ef25cd3cc991c9659f4f2>> * @flow strict-local */ @@ -35,6 +35,7 @@ export type ReactNativeFeatureFlagsJsOnly = { shouldUseOptimizedText: Getter, shouldUseRemoveClippedSubviewsAsDefaultOnIOS: Getter, shouldUseSetNativePropsInFabric: Getter, + useRefsForTextInputState: Getter, }; export type ReactNativeFeatureFlagsJsOnlyOverrides = Partial; @@ -115,6 +116,11 @@ export const shouldUseRemoveClippedSubviewsAsDefaultOnIOS: Getter = cre */ export const shouldUseSetNativePropsInFabric: Getter = createJavaScriptFlagGetter('shouldUseSetNativePropsInFabric', true); +/** + * Enable a variant of TextInput that moves some state to refs to avoid unnecessary re-renders + */ +export const useRefsForTextInputState: Getter = createJavaScriptFlagGetter('useRefsForTextInputState', false); + /** * Common flag for testing. Do NOT modify. */