fix: don't update text input text directly in setTextAndSelection#46973
fix: don't update text input text directly in setTextAndSelection#46973hannojg wants to merge 2 commits intofacebook:mainfrom
Conversation
|
Description here seems different from code (where the main change is we are flushing event sync). cc @sammy-SC for sync text events. Not sure what our end goal is for them (we want to move more to sync, to avoid the sorts of issues here), but it’s not free. |
|
Yes, so its two changes:
|
| const auto &textInputEventEmitter = static_cast<const TextInputEventEmitter &>(*_eventEmitter); | ||
| // When the textInputDidChange gets called, the text is already updated | ||
| // in the UI. We execute the state update synchronously so that the layout gets calculated immediately. | ||
| textInputEventEmitter.experimental_flushSync([state = _state, data = std::move(data)]() mutable { |
There was a problem hiding this comment.
the API is not ready for a general adoption. We are still iterating on the design and trying to prove its usefulness in production.
There was a problem hiding this comment.
okay, then lets hide it behind a feature flag?
There was a problem hiding this comment.
let me share little more context :)
First of all, the the general approach you are taking here is good. This is one of issues that sync events can fix as React Native will no longer need to do the round trip from the main thread to JS thread and back to main thread, resulting in unpleasant flicker.
As the method name suggests, the sync events are still experimental. We are not sure what the right API is at the moment. We are focusing on making the new architecture the default before fully embracing the new capabilities that is potentially breaking. experimental_flushSync works great in isolated cases but might lead to dropped frames when the JS thread is busy. We currently don't have good tools to help engineers find what the main thread is blocked on.
Once we have a good design for sync events, it will fix some issues with TextInput and significantly simplify the implementation.
Summary: This PR adds an example showcasing a TextInput whose width gets resized dynamically based on the text content width. This example was added for this PR, which tries to address a flickering bug with dynamic sized TextInputs: - #46973 ## Changelog: <!-- Help reviewers and the release process by writing your own changelog entry. Pick one each for the category and type tags: [ANDROID|GENERAL|IOS|INTERNAL] [BREAKING|ADDED|CHANGED|DEPRECATED|REMOVED|FIXED|SECURITY] - Message For more details, see: https://reactnative.dev/contributing/changelogs-in-pull-requests --> [INTERNAL] [ADDED] Added dynamic content width TextInput example Pull Request resolved: #46976 Test Plan: Test in RNTester app: https://github.com/user-attachments/assets/7571b80b-7035-47df-b840-4218ab0802b5 Reviewed By: NickGerleman Differential Revision: D64271454 Pulled By: arushikesarwani94 fbshipit-source-id: 28fe7bd93891ffe5284340d73e9ee947654f1788
|
This PR is stale because it has been open for 180 days with no activity. It will be closed in 7 days unless you comment on it or remove the "Stale" label. |
|
This PR is stale because it has been open for 180 days with no activity. It will be closed in 7 days unless you comment on it or remove the "Stale" label. |
Summary:
This is one possible solution to fix the flickering issue described here:
The main fix is that when we receive the view command call
setTextAndSelectionwe don't want to update the attributedString on the backing TextField immediately, but update the state of the shadow node.This way the layout for the view will be calculated with the updated state, and the updated text + updated layout will be applied at once.
Changelog:
[IOS] [FIXED] - Flicker when changing text in a dynamic sized TextInput
Test Plan:
Run the tester app and make sure that every TextInput example still works as before
I added another PR which adds an explicit example case to the RNTester app:
Screen.Recording.2024-10-11.at.14.41.45.mov
Screen.Recording.2024-10-11.at.15.01.37.mov