Skip to content

fix: don't update text input text directly in setTextAndSelection #46973

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.

Sign up for GitHub

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

Open
hannojg wants to merge 2 commits into
base: main
Choose a base branch
from
Open

fix: don't update text input text directly in setTextAndSelection #46973

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
0aa017d
fix: don't update text input text directly in setTextAndSelection
hannojg Oct 11, 2024
dd5b017
cleanup lines
hannojg Oct 11, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 14 additions & 4 deletions .../react-native/React/Fabric/Mounting/ComponentViews/TextInput/RCTTextInputComponentView.mm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -482,8 +482,7 @@ - (void)setTextAndSelection:(NSInteger)eventCount
if (value && ![value isEqualToString:_backedTextInputView.attributedText.string]) {
NSAttributedString *attributedString =
[[NSAttributedString alloc] initWithString:value attributes:_backedTextInputView.defaultTextAttributes];
[self _setAttributedString:attributedString];
[self _updateState];
[self _updateStateWithString:attributedString];
}

UITextPosition *startPosition = [_backedTextInputView positionFromPosition:_backedTextInputView.beginningOfDocument
Expand All @@ -493,6 +492,7 @@ - (void)setTextAndSelection:(NSInteger)eventCount

if (startPosition && endPosition) {
UITextRange *range = [_backedTextInputView textRangeFromPosition:startPosition toPosition:endPosition];
// _updateStateWithString executes any state updates sync, so its safe to update the selection as the attributedString is already updated!
[_backedTextInputView setSelectedTextRange:range notifyDelegate:NO];
}
_comingFromJS = NO;
Expand Down Expand Up @@ -614,17 +614,27 @@ - (void)handleInputAccessoryDoneButton
}

- (void)_updateState
{
NSAttributedString *attributedString = _backedTextInputView.attributedText;
[self _updateStateWithString:attributedString];
}

- (void)_updateStateWithString:(NSAttributedString*)attributedString
{
if (!_state) {
return;
}
NSAttributedString *attributedString = _backedTextInputView.attributedText;
auto data = _state->getData();
_lastStringStateWasUpdatedWith = attributedString;
data.attributedStringBox = RCTAttributedStringBoxFromNSAttributedString(attributedString);
_mostRecentEventCount += _comingFromJS ? 0 : 1;
data.mostRecentEventCount = _mostRecentEventCount;
_state->updateState(std::move(data));
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 {
Copy link
Contributor

@sammy-SC sammy-SC Oct 16, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the API is not ready for a general adoption. We are still iterating on the design and trying to prove its usefulness in production.

Copy link
Contributor Author

@hannojg hannojg Oct 17, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

okay, then lets hide it behind a feature flag?

Copy link
Contributor

@sammy-SC sammy-SC Oct 18, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

state->updateState(std::move(data));
});
}

- (AttributedString::Range)_selectionRange
Expand Down