Fix the breaking of multi-code-unit characters in obscure mode #123366

justinmc · 2023-03-23T23:46:54Z

This PR changes the character boundary behavior of obscured fields to be based on code points instead of code units.

So it used to be possible to traverse and delete obscured text inside of code points (and breaking a code point like that would cause a crash):

But now moving the cursor and deleting is based on code points:

Native behavior

Native iOS deletes part of the emoji, native Mac deletes the whole emoji. See #122381 (comment). So it's unclear what the desired behavior should actually be.

Resources

Fixes #122381

I thought this might not fix the case where a broken emoji is directly pasted into the field, but it seems to work by trying this: �‍👩‍👦‍👦

CC @LongCatIsLooong

Renzo-Olivares · 2023-03-27T19:16:29Z

It makes sense to me that we should remove the entire emoji when pressing delete since I'm not sure what a user would expect if only a portion of it is deleted in a password field. I do wonder why _CodeUnitBoundary was used vs CharacterBoundary in the case on an obscure field maybe there are some cases where it is needed. Looking at the failing tests it looks like this causes some unexpected changes.

        const String testCluster = '👨‍👩‍👦👨‍👩‍👦👨‍👩‍👦'; // 8 * 3
        const LogicalKeyboardKey trigger = LogicalKeyboardKey.delete;
        testWidgets('cross-cluster obscured text', (WidgetTester tester) async {
          controller.text = testCluster;
          controller.selection = const TextSelection(
            baseOffset: 1,
            extentOffset: 9,
          );

          await tester.pumpWidget(buildEditableText(obscured: true));
          await sendKeyCombination(tester, const SingleActivator(trigger));

          expect(
            controller.text,
            '👨‍👩‍👦👨‍👩‍👦',
          );

          expect(
            controller.selection,
            const TextSelection.collapsed(offset: 1),
          );
        }, variant: TargetPlatformVariant.all(excluding: <TargetPlatform>{ TargetPlatform.iOS }));
      });

Though looking at this test, it looks like we already expect that delete deletes the entire grapheme cluster.

LongCatIsLooong · 2023-03-28T06:26:26Z

This is a bit concerning that the whole emoji gets deleted, other people would easily be able to tell there are zwjs in the password. I think the problem isn't with "complex" characters, the crash is likely caused by surrogate pairs being broken up and the crash happens because it isn't valid encoding. Maybe try turning CodeUnitBoundary into CodePointBoundary (in retrospect it definitely doesn't make sense to introduce CodeUnitBoundary).
Also when obfuscating the password I think each codepoint should be turned into a *, not each code unit ( I think that's what we are doing right now?)

flutter-dashboard · 2023-04-17T06:31:13Z

This pull request executed golden file tests, but it has not been updated in a while (20+ days). Test results from Gold expire after as many days, so this pull request will need to be updated with a fresh commit in order to get results from Gold.

For more guidance, visit Writing a golden file test for package:flutter.

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

goderbauer · 2023-04-25T09:32:32Z

(triage) @justinmc Do you still have plans for this PR?

justinmc · 2023-04-27T17:51:07Z

Taking a look to see if I can turn this PR around, I'll make a decision on it by EOD today.

justinmc · 2023-05-10T23:57:59Z

I forgot about this PR, but I played around with it today and I think I have a solution. Will put this in draft mode and open it for review soon.

LongCatIsLooong · 2023-05-19T21:00:22Z

packages/flutter/lib/src/widgets/editable_text.dart

+    if (position < 0) {
+      return null;
+    }
+    if (position == 0) {


Looks like when position == 0 it's always going to return 0 even when the text is empty?

Good point, I guess we should be returning null when the text is empty? I've pushed a commit to do that for both leading/trailing.

LongCatIsLooong · 2023-05-19T21:27:48Z

packages/flutter/lib/src/widgets/editable_text.dart

+      return position;
+    }
+
+    return _breaksSurrogatePair(position)


If position points to a high surrogate the method should return position?

Yes. If position points to a high surrogate then _breaksSurrogatePair will return false, and position will be returned. This is correct because position is always a code point boundary when it points to a high surrogate.

Let me know if I have anything wrong there.

Ah I see _breaksSurrogatePair also checks for position - 1. It's probably not necessary since the program would have crashed before getting here if the string isn't valid UTF16. That would simplify the logic in this method too.

It seems possible to get here with a malformed string, at least in some cases. I added a malformed string test that seems to pass on linux, let's see if it passes on other platforms. Also, I can paste a malformed string (👨‍👩‍�) into the app on Linux without a crash.

Maybe we should be checking for this kind of thing somewhere.

oh interesting. Do you mind adding a test case for that?

Here's my test ("when inserting a malformed string"): https://github.com/flutter/flutter/pull/123366/files#diff-37dc3ce295078607fe561c83f2ae43fd77eb7b6aead39e5ed57d7fa9cf7525d7R16315

LongCatIsLooong · 2023-05-19T22:31:18Z

packages/flutter/lib/src/painting/text_painter.dart

+  ///   * https://en.wikipedia.org/wiki/UTF-16#Code_points_from_U+010000_to_U+10FFFF
+  ///   * [isLowSurrogate], which checks the same thing for low (second)
+  /// surrogates.
+  static bool isHighSurrogate(int value) {


nit: maybe assert value is in the correct range?

I just tried this on your suggestion, but then realized that there were a million test failures. I think these methods check lots of code units that are not necessarily surrogates.

sorry I meant valid UTF16 code range 0x0 – 0xFFFFF

Ohh agreed, I'll do that!

LongCatIsLooong · 2023-05-19T22:33:58Z

packages/flutter/lib/src/widgets/editable_text.dart

+      return position;
+    }
+
+    return _breaksSurrogatePair(position)


Ah I see _breaksSurrogatePair also checks for position - 1. It's probably not necessary since the program would have crashed before getting here if the string isn't valid UTF16. That would simplify the logic in this method too.

LongCatIsLooong · 2023-05-19T22:35:47Z

packages/flutter/lib/src/widgets/editable_text.dart

+    if (position < 0) {
+      return 0;
+    }
+    if (position == _text.length - 1) {


By defninition, if the last code unit is a low surrogate then getTrailingTextBoundaryAt(_text.length - 1) should return null?

I thought it was right as-is, but let me know if I'm misunderstanding. An example would be the string '👨', a valid surrogate pair, where the last code unit is a low surrogate. If position is in the center of that surrogate pair, then the trailing boundary is at the end of the string, right?

This method should return the next start index of a code point AFTER position. There's nothing after the last code unit so null should be returned?

We talked offline and decided that it is possible to return _text.length here. I've updated the docs to explicitly state this in 754a3b0.

…ngs that aren't surrogates.

LongCatIsLooong · 2023-05-22T20:28:18Z

packages/flutter/test/widgets/editable_text_test.dart

+    );
+
+    await tester.tap(find.byType(EditableText));
+    await tester.enterText(find.byType(EditableText), '👨‍👩‍�');


could you add a comment to explain what code unit this is? I can't really copy that since it's rendered as 0xFFFD and it's copied as such.

Also what happens if there's a sequence of high surrogates or low surrogates?

Good call. In cacb4fc I've replaced these crazy characters with their \u equivalents and left comments about where they come from. I also added two tests, one with a series of high surrogates and one with a series of low surrogates.

flutter/flutter#123366)

flutter/flutter@f86c529...216605b 2023-05-24 engine-flutter-autoroll@skia.org Roll Flutter Engine from aba9dc4640c2 to eebcf36cd38f (3 revisions) (flutter/flutter#127458) 2023-05-24 engine-flutter-autoroll@skia.org Roll Flutter Engine from 76afb431a740 to aba9dc4640c2 (1 revision) (flutter/flutter#127453) 2023-05-24 engine-flutter-autoroll@skia.org Roll Flutter Engine from fc6df9512aa2 to 76afb431a740 (3 revisions) (flutter/flutter#127445) 2023-05-23 49699333+dependabot[bot]@users.noreply.github.com Bump actions/labeler from 6b107e7a7ee5e054e0bcce60de5181d21e2f00fb to 2713f7303c96cb1e69627957ec16eea0fd7f94a4 (flutter/flutter#127438) 2023-05-23 engine-flutter-autoroll@skia.org Roll Flutter Engine from 3535e0d301dd to fc6df9512aa2 (1 revision) (flutter/flutter#127440) 2023-05-23 47866232+chunhtai@users.noreply.github.com Migrates android semanitcs integration to integration test (flutter/flutter#127128) 2023-05-23 engine-flutter-autoroll@skia.org Roll Flutter Engine from 23b04314d5d2 to 3535e0d301dd (3 revisions) (flutter/flutter#127428) 2023-05-23 engine-flutter-autoroll@skia.org Roll Flutter Engine from ccf50f344d5d to 23b04314d5d2 (2 revisions) (flutter/flutter#127421) 2023-05-23 ian@hixie.ch Give channel descriptions in `flutter channel`, use branch instead of upstream for channel name (flutter/flutter#126936) 2023-05-23 72562119+tgucio@users.noreply.github.com Avoid catching errors in TextPainter tests (flutter/flutter#127307) 2023-05-23 christopherfujino@gmail.com [flutter_tools] delete entitlements files after copying to macos build dir (flutter/flutter#127417) 2023-05-23 engine-flutter-autoroll@skia.org Roll Flutter Engine from a6e37bde65fd to ccf50f344d5d (7 revisions) (flutter/flutter#127416) 2023-05-23 113669907+NikolajHarderNota@users.noreply.github.com modal bottom sheet barrier label (flutter/flutter#126431) 2023-05-23 jmccandless@google.com Fix the breaking of multi-code-unit characters in obscure mode (flutter/flutter#123366) 2023-05-23 engine-flutter-autoroll@skia.org Roll Flutter Engine from 168b0bf3f70d to a6e37bde65fd (1 revision) (flutter/flutter#127397) 2023-05-23 vashworth@google.com Revert "Log all output of ios-deploy" (flutter/flutter#127405) 2023-05-23 engine-flutter-autoroll@skia.org Roll Packages from 83959fb to d449a17 (6 revisions) (flutter/flutter#127394) 2023-05-23 vashworth@google.com Log all output of ios-deploy (flutter/flutter#127222) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-packages Please CC bmparr@google.com,rmistry@google.com,stuartmorgan@google.com on the revert to ensure that a human is aware of the problem. To file a bug in Packages: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://bugs.chromium.org/p/skia/issues/entry?template=Autoroller+Bug Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md

@LongCatIsLooong

…er#123366) This PR changes the character boundary behavior of obscured fields to be based on code points instead of code units. So it used to be possible to traverse and delete obscured text inside of code points (and breaking a code point like that would cause a crash): ![output2](https://github.com/flutter/flutter/assets/389558/674c89a4-c47d-4cdc-a402-4cadb5d2f73b) But now moving the cursor and deleting is based on code points: ![output1](https://github.com/flutter/flutter/assets/389558/e46301f7-b5af-48d2-812a-0ad649f1383b) ### Native behavior Native iOS deletes part of the emoji, native Mac deletes the whole emoji. See flutter#122381 (comment). So it's unclear what the desired behavior should actually be. ### Resources Fixes flutter#122381 I thought this might not fix the case where a broken emoji is directly pasted into the field, but it seems to work by trying this: ï¿½â��ð��©â��ð��¦â��ð��¦ CC @LongCatIsLooong

flutter/flutter#123366)

Treat complex characters as a single character in obscure mode

f318984

justinmc requested a review from Renzo-Olivares March 23, 2023 23:46

flutter-dashboard bot added a: text input Entering text in a text field or keyboard related problems framework flutter/packages/flutter repository. See also f: labels. labels Mar 23, 2023

justinmc self-assigned this Mar 23, 2023

Merge branch 'master' into obscure-emojis

abb1f87

justinmc added 2 commits April 27, 2023 12:00

WIP exploring CodePointBoundary

c82cdb6

Finished CodeUnityBoundary, but needs thorough testing

9461294

justinmc marked this pull request as draft May 10, 2023 23:58

Tested and tweaked codeunitboundary

94f105b

justinmc marked this pull request as ready for review May 11, 2023 18:57

justinmc changed the title ~~Treat complex characters as a single character in obscure mode~~ Fix the breaking of multi-code-unit characters in obscure mode May 11, 2023

Update test with new behavior

47027a0

justinmc requested a review from LongCatIsLooong May 11, 2023 20:02

justinmc requested a review from christopherfujino as a code owner May 15, 2023 20:25

justinmc force-pushed the obscure-emojis branch from 7db0a5c to 47027a0 Compare May 16, 2023 18:38

justinmc removed the request for review from christopherfujino May 16, 2023 18:40

justinmc added 2 commits May 18, 2023 11:22

Null cases and logic cleanup

22046ef

Merge remote-tracking branch 'upstream/master' into obscure-emojis

bc9c2fc

LongCatIsLooong reviewed May 19, 2023

View reviewed changes

Handle the case of empty text

a59b331

LongCatIsLooong reviewed May 19, 2023

View reviewed changes

justinmc added 5 commits May 19, 2023 15:49

Assert that value is a surrogate

2ca23bc

Test malformed string

c14ac57

Actually, don't validate surrogates. These methods can be used on thi…

45b7c62

…ngs that aren't surrogates.

Assert valid utf16 in the surrogate methods

6ab9630

Clarify possible return values for boundaries

754a3b0

LongCatIsLooong reviewed May 22, 2023

View reviewed changes

justinmc added 2 commits May 22, 2023 16:43

Be explicit about broken characters

cacb4fc

Merge remote-tracking branch 'upstream/master' into obscure-emojis

cf2f6b3

LongCatIsLooong approved these changes May 23, 2023

View reviewed changes

justinmc added the autosubmit Merge PR when tree becomes green via auto submit App label May 23, 2023

auto-submit bot merged commit 4341ed4 into flutter:master May 23, 2023

justinmc deleted the obscure-emojis branch May 23, 2023 20:22

engine-flutter-autoroll mentioned this pull request May 24, 2023

Roll Flutter from f86c529b2cd2 to 216605b60f67 (18 revisions) flutter/packages#4071

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request May 24, 2023

4341ed4 Fix the breaking of multi-code-unit characters in obscure mode (

4f33d89

flutter/flutter#123366)

This was referenced May 25, 2023

Support minifcation for apps that depend on AGP 8 and integration_test #127628

Merged

i127682 android api 33 test support #127689

Merged

HansMuller mentioned this pull request May 26, 2023

Flutter Framework Team: Notable Commits #121415

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Aug 16, 2023

4341ed4 Fix the breaking of multi-code-unit characters in obscure mode (

c6ba21e

flutter/flutter#123366)

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Aug 17, 2023

4341ed4 Fix the breaking of multi-code-unit characters in obscure mode (

64f1ff4

flutter/flutter#123366)

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Aug 17, 2023

4341ed4 Fix the breaking of multi-code-unit characters in obscure mode (

ce9a19f

flutter/flutter#123366)

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Aug 17, 2023

4341ed4 Fix the breaking of multi-code-unit characters in obscure mode (

346cdbb

flutter/flutter#123366)

Fix the breaking of multi-code-unit characters in obscure mode #123366

Fix the breaking of multi-code-unit characters in obscure mode #123366

Uh oh!

Conversation

justinmc commented Mar 23, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Native behavior

Resources

Uh oh!

Renzo-Olivares commented Mar 27, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

LongCatIsLooong commented Mar 28, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

flutter-dashboard bot commented Apr 17, 2023

Uh oh!

goderbauer commented Apr 25, 2023

Uh oh!

justinmc commented Apr 27, 2023

Uh oh!

justinmc commented May 10, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

LongCatIsLooong May 22, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

justinmc commented Mar 23, 2023 •

edited

Loading

Renzo-Olivares commented Mar 27, 2023 •

edited

Loading

LongCatIsLooong commented Mar 28, 2023 •

edited

Loading

LongCatIsLooong May 22, 2023 •

edited

Loading