-
Notifications
You must be signed in to change notification settings - Fork 29.8k
Reland "Make FilteringTextInputFormatter's filtering Selection/Composing Region agnostic" #89327 #90211
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.
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
Reland "Make FilteringTextInputFormatter's filtering Selection/Composing Region agnostic" #89327 #90211
Conversation
justinmc
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for putting all of the new changes in a single commit, that was much easier to re-review. Can you explain why this was reverted originally?
| final Iterable<Match> matches = filterPattern.allMatches(newValue.text); | ||
| Match? previousMatch; | ||
| for (final Match match in matches) { | ||
| assert(match.end >= match.start); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In what case can match.start == match.end? I guess that came up and caused this to throw an error.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
final RegExp filter = RegExp('[A-Za-z0-9.@-]*');
print(filter.allMatches('ab&&ca@bcabc').map((m)=> '${m.start} - ${m.end}'));prints (0 - 2, 2 - 2, 3 - 3, 4 - 12, 12 - 12) so mismatched characters are represented as an empty match it seems
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah got it, thanks.
| value, | ||
| (TextEditingValue newValue, TextInputFormatter formatter) => formatter.formatEditUpdate(_value, newValue), | ||
| ) ?? value; | ||
| try { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So if the formatter throws an error, then rather than halting execution here and ending up in a broken state, updateEditingValue will continue without formatting the value? When the formatter throws an error, is that because the app developer has created a poorly written input formatter?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I think here it's all or nothing. Assigning the formatted value to value is the last step.
is that because the app developer has created a poorly written input formatter
input formatters are user callbacks so yeah that's typically the reason why the whole thing throws. fold is pure so I don't think it throws errors (unless there's a stack overflow or something).
| expect(error.toString(), contains(errorText)); | ||
| }); | ||
|
|
||
| testWidgets('input formatters can thorw errors', (WidgetTester tester) async { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typo on "thorw" here.
|
|
||
| final dynamic error = tester.takeException(); | ||
| expect(error, isFlutterError); | ||
| expect(error.toString(), contains(errorText)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And the text should be updated to include "text" now in spite of the error, right? Would it be valuable to expect that too?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah good idea.
justinmc
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 👍
| final Iterable<Match> matches = filterPattern.allMatches(newValue.text); | ||
| Match? previousMatch; | ||
| for (final Match match in matches) { | ||
| assert(match.end >= match.start); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah got it, thanks.
…ing Region agnostic" flutter#89327 (flutter#90211)
Fixes an issue with CJK IMEs wherein a text input state update may be sent to the framework that misleads the framework into assuming that IME composing has ended. As an example, when inputting Korean text, characters are built up keystroke by keystroke until the point that either: * the user presses space/enter to terminate composing and commit the character, or; * the user presses a key such that the character currently being composed cannot be modified further, and the IME determines that the user has begun composing the next character. The following is an example sequence of events for the latter case: 1. User presses ㅂ. Begin compose event followed by change event received with ㅂ. Embedder sends state update to framework. 2. User presses ㅏ. im_preedit_changed_cb with 바. Embedder sends state update to framework. 3. User presses ㄴ. im_preedit_changed_cb with 반. Embedder sends state update to framework. 4. User presses ㅏ. At this point, the current character being composed (반) cannot be modified in a meaningful way, and the IME determines that the user is typing 바 followed by 나. im_commit_cb received with 바, immediately followed by im_preedit_changed event with 나. In step 4, we previously sent two events to the framework, one immediately after the other: * im_commit_cb triggers the text input model to commit the current composing region to the string under edit. This causes the composing region to collapse to an empty range. * im_preedit_change_cb triggers the text input model to insert the new composing character and set the composing region to that character. Conceptually, this is an atomic operation. The fourth keystroke causes the 반 character to be broken into two (바 and ㄴ) and the latter to be modified to 나. From the user's point of view, as well as from the IME's point of view, the user has NOT stopped composing, and the composing region has simply moved on to the next character. Flutter has no concept of whether the user is composing or not other that whether a non-empty composing region exists. As such, sending a state update after the commit event misleads the framework into believing that composing has ended. This triggers a serious bug: Text fields with input formatters applied do not perform input formatting updates while composing is active; instead they wait until composing has ended to apply any formatting. The previous behaviour would thus trigger input formatters to be applied each time the user input caused a new character to be input. This has the add-on negative effect that once formatting has been applied, it sends an update back to the embedder so that the native OS text input state can be updated. However, since the commit event is immediately followed by a preedit change, the state has changed in the meantime, and the embedder is left processing an update (the intermediate state sent after the commit) which is now out of date (i.e. missing the new state from the change event). The source of this bug is as follows: * Commit event for a character/compose region is sent from the engine. The engine TextInputModel still models its `composing` field as true. An update is sent to the framework with the committed text and an empty composing range such as (1, 1). Note that the engine previously only sends a range of (-1, -1) when composing has ended, NOT just when it has an empty composing region. * Framework receives commit event and updates the text to match. The framework does not model the system composing state; instead its understanding of whether the user is composing or not is entirely predicated on whether the composing region is empty or not. If it is, it triggers input formatters, which in this case have no effect on the text/selection. However, the framework consistently models empty compose regions as (-1, -1) and resets the text editing value as such. Because the framework triggered a change to the TextEditingValue, it dutifully sends the update back to the engine. * In the meantime, in parallel with the above step, the engine starts processing the change event immediately following the commit, and updates the text and composing region with the next character. This change is promptly stomped on by the incoming framework update. To avoid this, we have the engine consistently send empty compose regions as (-1, -1) to the framework. After the input formatter is applied on commit, the compose region is still (-1, -1) and there are therefore no diffs, and the framework will not send an update back to the engine and stomp on any new state on the engine side. Longer-term, we really should add some form of versioning information to the text edit protocol so as to detect and resolve conflicts rather than relying entirely on not creating races in the first place. This bug was revealed by flutter/flutter#90211 which applies an input formatter to single-line text fields in order to suppress newlines. Issue: flutter/flutter#97174
Additional changes: 36cefa6
Pre-launch Checklist
///).If you need help, consider asking for advice on the #hackers-new channel on Discord.