Repository navigation
Conversation
github-actions
Bot
added
a: text input
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request updates TextInputModel::DeleteSurrounding to handle surrogate pairs and prevent out-of-bounds indexing when deleting text, and adds corresponding unit tests. The review feedback suggests preventing surrogate pairs from being split at the end of the editable range by checking the remaining space before stepping forward, and recommends adding a test case to verify deletion behavior at the cursor when a surrogate pair straddles the end of the composing range.
7e0ff6b to
c7589ac
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces the helper functions NextCharacterBoundary and PreviousCharacterBoundary to safely navigate UTF-16 surrogate pairs and boundaries, and refactors DeleteSurrounding to use them. It also adds several unit tests covering edge cases with surrogate pairs and composing ranges. The review feedback suggests refining both helper functions to explicitly verify that leading and trailing surrogates form valid pairs within the specified limits, preventing unpaired surrogates from being incorrectly handled.
c7589ac to
016c37b
Compare
Fixes surrogate pair stepping in `DeleteSurrounding`, `Delete`, and `Backspace`. This is not my first rodeo: flutter-team-archive/engine#3590. Walking forward over the characters to delete was measuring each one against `text_.at(start)` rather than `text_.at(end)`, so after the first step we kept re-reading the character we started from. That over-deletes when the first character is a surrogate pair, and splits the pair when a later one is, leaving an unpaired surrogate in `text_` that `GetText` then can't convert, resulting in a range error. This also limits both ends of the deletion to the editable range. We need to protect against a surrogate pair that straddles the boundary. We now stop the walk at the boundary rather than step past it. `start` is clamped on entry because the selection is not required to sit inside the composing range either. `SetComposingRange` accepts a cursor offset that could place the caret beyond the range's end. Without this, `start` could exceed `end` and underflow `deleted_length`, which erases the rest of the buffer and wraps the composing range, or index out of bounds and blow up. Extracted helpers rather than copy-pasting even more than we already had. Spotted which working on: flutter#190704
016c37b to
500fc64
Compare
cbracken
changed the title
LongCatIsLooong
left a comment
LongCatIsLooong
left a comment
There was a problem hiding this comment.
I think we can assume the framework (including the app developer) and the system (including the IME) never break surrogate pairs (not even temporarily). If they do then we should assert.
| return (code_point & 0xFFFFFC00) == 0xDC00; | ||
| } | ||
|
|
||
| // Returns the position one character after |pos|, or |limit| if a whole |
There was a problem hiding this comment.
terminology nit, character -> codepoint
| // stepped over alone rather than swallowing whatever follows it. | ||
| size_t NextCharacterBoundary(const std::u16string& text, | ||
| size_t pos, | ||
| size_t limit) { |
There was a problem hiding this comment.
limit does not seem necessary. We should never break a surrogate pair given the string is not malformed.
There was a problem hiding this comment.
limit is about the bounds of the editable range, not just surrogate pairs. Take a look at DeleteEndComposing and related reversed and backspace tests as an example of where we're using this in place of the previous position != editable_range().start() etc. This is reducing copy-paste (and potentially forgetting/being incosistent) about those checks.
Note that in those tests, the string is all well-formed but they would fail if we removed limit.
| return pos; | ||
| } | ||
| size_t step = IsTrailingSurrogate(text.at(pos - 1)) && pos >= 2 && | ||
| IsLeadingSurrogate(text.at(pos - 2)) |
There was a problem hiding this comment.
If pos - 1 is a trailing surrogate then pos - 2 if exists, must be a leading surrogate otherwise the string is just malformed. We should not check that and should probably assert (however it would be better if the framework asserts that).
There was a problem hiding this comment.
(Same with the previous helper function).
There was a problem hiding this comment.
I think this comes back to the more general question of whether it's safe to assume that the string is well-formed. We don't assert that anywhere in FlutterTextInputPlugin (and I suspect not Windows or Linux either). Asserting that in the framework is insufficient since the API boundary is the engine boundary, not the framework, I talk a little more about this below and I know that's a bit of a (policy-wise) controversial position.
But even if we claim we don't support alternatives built on dart:ui, from a technical point of view, its far, far safer and more future-proof to treat everything outside the engine/embedder as a black box, and not rely on internal implementation details behind a half dozen layers of API boundaries.
| EXPECT_STREQ(model->GetText().c_str(), "😄🙃🧐"); | ||
| } | ||
|
|
||
| TEST(TextInputModel, DeleteSplitPairAtComposingEnd) { |
There was a problem hiding this comment.
I don't think this is a valid use case. It should be safe to assume composing ranges can only contain valid unicode strings.
There was a problem hiding this comment.
This test's string is valid unicode. "a😄bcd" is well-formed. This is defending against the boundary being in the middle of a composing range.
|
|
||
| auto end = start; | ||
| for (int i = 0; i < count && end != max_pos; i++) { | ||
| end += IsLeadingSurrogate(text_.at(start)) ? 2 : 1; |
There was a problem hiding this comment.
I vaguely remember this is already fixed by a different PR? Maybe it wasn't merged?
There was a problem hiding this comment.
I did a look through the commit history for lands/reverts and don't see anything, but maybe it was a PR that was closed unmerged? I didn't check other embedders though (well other than my own one from a decade ago in iOS 😂)
I don't think this is correct.
I do strongly agree we should be using more asserts, because there's huge room for (and lots of history of) programming errors on our part. But I don't think that's sufficient. There's a whole class of issues with the data coming from IMEs (definitely) and over the channel (out of paranoia) that are runtime issues we need to validate at runtime. Right now we do zero validation at all on the incoming bytes at the embedder entrypoint here (lines 938-942 if the link doesn't work). We also never check anywhere in our code that range endpoints don't fall in the middle of a surrogate pair and we have had a long history of being wrong about that, admittedly typically due to our own bugs. This got pretty long but I think a reasonable TL;DR would be something like "yes we should assert more, but that's not enough. We need runtime checks". I could easily be convinced we should push those runtime checks up into the platform-specific embedders right at the input boundaries from the IME and channel, but the whole point of TextInputModel was that I was tired of rewriting this code over and over. (In hindsight, I'm still not sure if it was the right choice) |
|
Yeah in theory it is possible for IMEs to send us malformed strings (which fortunately is unlikely. A selection / composing region that fails to account for surrogate pairs tho is a bit more likely). But when that happens I believe even if the text input plugin doesn't crash immediately (because of failing to encode the update into a So while attempting to handle lone surrogates gracefully in places like
Does that mean the macOS IME protocol is racy by design (that can be true but I'm hoping that's not the case)? Or it is just our text input plugins have a bug somewhere (like failing to inform the system that the text's been changed by a different party so the previous setMarkedText command must be discarded)?
(Looks like the sentence is unfinished here). Ah I remember this one. I didn't review it and the patch itself doesn't really explain why, so I don't fully understand what the root cause was for this bug. I suspect that was a (now fixed) bug in the
Yes there are a lot of different ways the app developer can change the input text (I think you probably meant input formatters), but the text input plugin communication protocol always communicates
Fair point but in practice the text input plugin always sends the updated
Yeah that would be ideal. I think it already crashes when the framework sends the embedder a malformed string on macOS and iOS. |
Fixes surrogate pair stepping in
DeleteSurrounding,Delete, andBackspace. Ironically, I landed a very similar change for iOS almost a decade ago: flutter-team-archive/engine#3590.Walking forward over the characters to delete was measuring each one against
text_.at(start)rather thantext_.at(end), so after the first step we kept re-reading the character we started from. That over-deletes when the first character is a surrogate pair, and splits the pair when a later one is, leaving an unpaired surrogate intext_thatGetTextthen can't convert, resulting in a range error.This also limits both ends of the deletion to the editable range. We need to protect against a surrogate pair that straddles the boundary.
We now stop the walk at the boundary rather than step past it.
startis clamped on entry because the selection is not required to sit inside the composing range either.SetComposingRangeaccepts a cursor offset that could place the caret beyond the range's end. Without this,startcould exceedendand underflowdeleted_length, which erases the rest of the buffer and wraps the composing range, or index out of bounds and blow up.Extracted helpers rather than copy-pasting even more than we already had.
Spotted which working on: #190704
Pre-launch Checklist
///).If you need help, consider asking for advice on the #hackers-new channel on Discord.
If this change needs to override an active code freeze, provide a comment explaining why. The code freeze workflow can be overridden by code reviewers. See pinned issues for any active code freezes with guidance.
Note: The Flutter team is currently trialing the use of Gemini Code Assist for GitHub. Comments from the
gemini-code-assistbot should not be taken as authoritative feedback from the Flutter team. If you find its comments useful you can update your code accordingly, but if you are unsure or disagree with the feedback, please feel free to wait for a Flutter team member's review for guidance on which automated comments should be addressed.