Add right-to-left (RTL) text support - #213
Conversation
kfatehi
changed the title
|
Drafted again due to discovery of some bugs, will be tracking in the BlueBubbles ticket BlueBubblesApp#3049 and then backport the fixes here once I have them |
31b5b8c to
ce8eb8c
Compare
kfatehi
changed the title
Rendering: message bubbles, reply bubbles/previews, conversation tiles, the send-animation bubble, and embedded notification/reaction text render with the correct paragraph direction. Direction via getTextDirection() (UAX#9 first-strong over runes); embedded text uses a first-strong isolate. Input: the compose and subject fields go RTL via TextDirectionBuilder, which rebuilds only when the first-strong direction flips (so caret dragging works). No per-keystroke direction-forcing, no grapheme-repair. Known issue (kept draft): intermittent emoji "??" corruption while composing, not yet reliably reproduced. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
kfatehi
changed the title
Snap a collapsed caret (or selection endpoint) off any UTF-16 surrogate-pair interior in the compose controller (SpellCheckTextEditingController.set value) before committing the value, so a subsequent edit can't split an emoji into lone surrogates -- which the Android text-input channel encodes as '?' (the '??' corruption) and which crash ParagraphBuilder on paint. App-side equivalent of the framework fix in flutter/flutter#188713 (PR flutter/flutter#188719); needs no Flutter upgrade and is scoped to the compose field. Adds test/grapheme_caret_test.dart. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Snap a collapsed caret (or selection endpoint) off any UTF-16 surrogate-pair interior in the compose controller (SpellCheckTextEditingController.set value, both exit paths) before committing the value, so a subsequent edit can't split an emoji into lone surrogates -- which the Android text-input channel encodes as '?' (the '??' corruption) and which crash ParagraphBuilder on paint. App-side equivalent of the framework fix in flutter/flutter#188713 (PR flutter/flutter#188719); needs no Flutter upgrade and is scoped to the compose field. Source-only per this branch's convention; logic unit-tested in the OpenBubbles port (OpenBubbles#213). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The compose-field caret clamp snapped a caret that lands inside an emoji's surrogate pair to the pair start (before the emoji). In RTL a tap aiming for the spot after a trailing emoji lands mid-glyph and got yanked before it, so backspace deleted the adjacent space instead of the emoji and the emoji could not be removed. Snap to the pair end (offset + 1) instead, so the caret lands after the emoji and backspace deletes the whole emoji. It is still a boundary, so the next edit cannot split the pair and the "??" corruption stays fixed. Verified on-device (Android / Gboard, forced-RTL). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FrEin3bnyFrXaxSmL9iLQM
|
Update — un-drafted, ready for review again. The The same fix is upstreamed at flutter/flutter#188719 (now approved by a Flutter maintainer). Because |
|
jsyk the bluebubbles updates are going to be pulled into openbubbles soonish so if it's in their codebase then it'll be coming |
Appreciate the heads-up! There's a branch wrinkle that makes it not quite automatic, though. OB's default branch (rustpush) was forked from BlueBubbles' master, which has been frozen since Nov 2024, and it's since diverged — it's ~728 commits behind BB's development and hasn't pulled from BB in over a year. My BlueBubbles PR (BlueBubblesApp#3049) targets their development, so even once it lands there it won't flow into rustpush on its own. That's actually why this PR exists separately: #213 targets rustpush directly, so it's the path to get RTL into OpenBubbles. It's un-drafted and mergeable now — just needs a maintainer look whenever @TaeHagen has a moment. 🙏 |
Yeah Tae has been working on getting it up to date with BB (although it's been on hold for a little bit) so it will eventually all be synced up |
getTextDirection is called from inside build in the message bubble, the reply bubble, the send animation and the conversation tile, so it re-runs on every Obx/setState rebuild rather than only when the text changes (BlueBubbles BlueBubblesApp#3049 review). Detection early-exits on the first strongly-directional character, so cost tracks how far in that character is, not message length: ordinary text is 8-12 ns and a 60-tile frame doing 120 detections is 0.8 us, about 0.005% of a 16.7 ms frame. But text with no strong character anywhere is scanned to the end -- an all-neutral 200-unit message is 683-874 ns and an emoji-only message 1008-1060 ns, 60-100x worse. Memoize on the text itself. The detection moves unchanged into a private _detectTextDirection and every call site is untouched. No invalidation anywhere: the direction is a pure function of the text, so an entry cannot go stale for its own key. After: all-neutral and emoji-only both drop to ~10 ns flat, ordinary text is unchanged within noise. On a workload of only early-exit strings the memo is neutral, and a miss costs a scan plus an insert, so all-distinct strings are a small net loss -- bounded by the 512 cap. Deliberately not an LRU. Promoting a key on every hit costs a remove plus a re-insert, measured at 34.2 ns/hit against the 13.3 ns scan it replaces, which would make the common case slower than having no cache at all. Insertion-order eviction keeps the hit path to a single lookup (5.9-10.7 ns); a Dart map literal is insertion-ordered, so keys.first is the oldest and eviction is O(1). With a 512 cap and a 120-200 string working set both policies evict the same keys anyway. Keyed on the string value rather than identity, because Message.fullText and MessagePart.fullText allocate a new String on every call. Hashing costs well under 0.1 ns/unit against the scan's ~3.75 ns/unit, so a lookup on a fresh key still beats a scan on a fresh key (42.0 vs 802.1 ns). Adds test/rtl_direction_cache_test.dart (8 tests). The eviction test discriminates: switching the memo to a move-to-end LRU turns it red. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01REhKAmLTibRLmoakxNp6CW
|
Hi, an update on this one rather than a change to the code. The One correction to what I wrote on 29 June: flutter/flutter#188719 did carry an approval, but a later review pushed back that a controller making programmatic changes to the selection is responsible for keeping them off codepoint boundaries. I agreed with that and I've closed it. So the clamp stays where this PR already puts it — in the compose controller. It stops the corruption but not the misplacement, since only the engine knows where the glyph boundaries are. On this eventually arriving through the BlueBubbles sync — one part of it won't. This PR carries three test files, 17 tests, covering the clamp, the direction detection and the memo behind it. They all pass. The BlueBubbles PR can't carry them, because The branch is still up to date with rustpush (4 ahead, 0 behind) and mergeable. Everything underneath it, including the engine side: https://github.com/kfatehi/flutter-rtl-fixes |
2 participants
Adds right-to-left (RTL) text support for Farsi, Arabic, and Hebrew. Closes #174.
What it does
TextDirectionBuilder— which recomputes direction live but rebuilds the field only when the first-strong direction actually flips, not on every keystroke/selection change, so caret dragging works normally.??surrogate-pair corruption that RTL input could trigger (see "Emoji corruption" below).How
getTextDirection()(lib/helpers/ui/text_direction_helpers.dart): UAX#9 "first strong character" detection over runes — chosen overintl'sBidi.startsWithRtl, which misclassifies emoji-leading text as LTR (the leading surrogate falls in its LTR ranges).textDirectionof each messageRichText/Text.richand the conversation tile;getNotificationTextwraps embedded text in a first-strong isolate (U+2068…U+2069).TextFieldis wrapped inTextDirectionBuilder(direction-flip-gated rebuild).SpellCheckTextEditingController.set valuesnaps a collapsed caret (or selection endpoint) off any UTF-16 surrogate-pair interior —snapSelectionOffSurrogatePairs()inlib/helpers/ui/grapheme_caret.dart— before committing the value, so a subsequent edit can never split an emoji. The caret snaps to after the cluster (the trailing edge), so when a tap lands inside a trailing emoji's glyph the caret ends up past it and a backspace deletes the emoji — in RTL, snapping to the leading edge would instead strand the caret before the emoji and delete the adjacent space.Emoji corruption — fixed (this was the draft blocker)
Earlier this PR carried an intermittent
??corruption while composing. With an RTL base direction, tapping on an emoji glyph can land the caret inside the emoji's UTF-16 surrogate pair; the next keystroke then splits the pair into two lone surrogates, which the Android text-input channel encodes as?(giving??) and which also crashParagraphBuilderon paint.This is a pre-existing Flutter framework bug, not something this PR's RTL approach does wrong: Flutter allows a caret to sit inside a surrogate pair and forwards that offset to the platform IME. RTL input is only the trigger — an LTR tap happens to snap the caret to the grapheme boundary, whereas an RTL tap can land it mid-pair; the standalone repro reproduces the defect with plain Latin text and no RTL at all. It's filed with a deterministic, device-free repro and fixed upstream:
This PR fixes it in-app, with no Flutter upgrade required: because
EditableTextforwards the controller's value to the IME, clamping the caret in our own compose controller is equivalent to the framework fix, and it's scoped to the compose field only. Verified on-device — Farsi + emoji + tap-on-the-emoji + typing no longer corrupts.Tests
test/rtl_detection_test.dart—getTextDirectionover Farsi, emoji/punctuation/digit-leading Farsi (first-strong), English, empty/null, and mixed first-strong.test/grapheme_caret_test.dart— the caret clamp: a caret inside a surrogate pair snaps to the pair boundary, boundary carets are untouched, and inserting at the snapped caret leaves the emoji intact.Note on mixed-direction text
Mixed LTR/RTL/emoji runs are laid out by Flutter's standard Unicode Bidi Algorithm — the same reordering iMessage and the platform keyboards apply for a given base direction. Intentional, not a rendering bug.
Mirrored in the BlueBubbles PR: BlueBubblesApp#3049 (kept source-only there, since that base has
flutter_testdisabled; the tests live here).