fix: don't split a surrogate pair when applying the composing style - #2754
Open
mtallenca wants to merge 1 commit into
Open
fix: don't split a surrogate pair when applying the composing style#2754mtallenca wants to merge 1 commit into
mtallenca wants to merge 1 commit into
Conversation
_TextLineState._splitAndApplyComposingStyle cuts the node text at the composing offsets with substring, without checking that they land on a code-point boundary. The composing range comes from the platform IME and is carried forward onto the new value whenever the document changes (updateRemoteValueIfNeeded only drops it when composingRange.end exceeds the new text length), so a programmatic edit that shifts the text -- e.g. inserting a character ahead of the range -- can leave a boundary inside a UTF-16 surrogate pair. Each part then holds a lone surrogate, and malformed UTF-16 throws 'Invalid argument(s): string is not well-formed UTF-16' from _NativeParagraphBuilder.addText when the line is laid out, taking down the frame (seen in the wild on Android with an emoji in the text). Skip the composing decoration when the range is out of range or would split a pair: the underline is lost for that frame, which beats a span that cannot be rendered.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
_TextLineState._splitAndApplyComposingStylecuts the node's text at the composing offsets with plainsubstring:Nothing checks that those offsets land on a code-point boundary. The composing range comes from the platform IME, and it is carried forward onto the new value every time the document changes —
RawEditorStateTextInputClientMixin.updateRemoteValueIfNeededonly drops it when it no longer fits the text length:So a programmatic edit that shifts the text (in our app, formatting links and inserting a character ahead of the composing range) leaves a range that still "fits" but now points at different characters. When a boundary lands inside a UTF-16 surrogate pair, each part keeps a lone surrogate, and malformed UTF-16 throws in the engine's paragraph builder as soon as the line is laid out — taking down the frame:
We see this in production (Crashlytics, flutter_quill 11.5.1, Android) in an editor whose document is mutated programmatically while the user types, on notes that contain an emoji.
The guards above the split only compare numbers, so they don't help:
isComposingRangeOutOfLinechecks the range against the line bounds andisNodeInComposingRangeagainst the node bounds — a stale range passes both.Fix
Skip the composing decoration when the range is out of range or would split a surrogate pair, instead of building a span that cannot be rendered. The composing underline is lost for that frame; the text still renders and the next IME update restores it.
Test
test/bug_fix_test.dartreproduces it: an editor holdingab😀cdplus a composing range ending at code unit 3 (inside the emoji's surrogate pair) throws the exact stack above before the fix and lays out cleanly after it.Notes
While writing the test I hit a second, unrelated issue:
closeConnectionIfNeededsets_lastKnownRemoteTextEditingValue = null, whose setter notifiescomposingRange, soQuillRawEditorState._onComposingRangeChangedcallssetStatefromdisposeand asserts_lifecycleState != _ElementLifecycle.defunctwhen the editor is torn down with a composing range still set. The test works around it by clearing the composing range first. Happy to send a separate PR for that if you'd like.