Re-land "Support Scribble Handwriting" (#96615) by fbcouch · Pull Request #97437 · flutter/flutter

fbcouch · 2022-01-28T16:12:16Z

Original PR #75472

Migration Guide: https://github.com/flutter/website/blob/main/src/release/breaking-changes/scribble-text-input-client.md

It looks like the migration to SkParagraph very slightly changed the results of renderEditable.getBoxesForSelection so that placeholders are now included, but have an empty list. Here's the change to accommodate the difference:

diff --git a/packages/flutter/lib/src/widgets/editable_text.dart b/packages/flutter/lib/src/widgets/editable_text.dart
index 3064b9cee7..f91e9e61de 100644
--- a/packages/flutter/lib/src/widgets/editable_text.dart
+++ b/packages/flutter/lib/src/widgets/editable_text.dart
@@ -2728,7 +2728,7 @@ class EditableTextState extends State<EditableText> with AutomaticKeepAliveClien
     if (WidgetsBinding.instance!.window.physicalSize.shortestSide < _kIPadWidth)
       return;
 
-    final String text = renderEditable.text?.toPlainText(includeSemanticsLabels: false, includePlaceholders: false) ?? '';
+    final String text = renderEditable.text?.toPlainText(includeSemanticsLabels: false) ?? '';
     final List<Rect> firstSelectionBoxes = renderEditable.getBoxesForSelection(const TextSelection(baseOffset: 0, extentOffset: 1));
     final Rect? firstRect = firstSelectionBoxes.isNotEmpty ? firstSelectionBoxes.first : null;
     final ScrollDirection scrollDirection = _scrollController.position.userScrollDirection;
@@ -2752,8 +2752,12 @@ class EditableTextState extends State<EditableText> with AutomaticKeepAliveClien
             return null;
 
           final int offset = _cachedText.characters.getRange(0, i).string.length;
+          final List<Rect> boxes = renderEditable.getBoxesForSelection(TextSelection(baseOffset: offset, extentOffset: offset + _cachedText.characters.characterAt(i).string.length));
+          if (boxes.isEmpty)
+            return null;
+
           final SelectionRect selectionRect = SelectionRect(
-            bounds: renderEditable.getBoxesForSelection(TextSelection(baseOffset: offset, extentOffset: offset + _cachedText.characters.characterAt(i).string.length)).first,
+            bounds: boxes.first,
             position: offset,
           );
           if (renderEditable.paintBounds.bottom < selectionRect.bounds.top) {

Pre-launch Checklist

I read the Contributor Guide and followed the process outlined there for submitting PRs.
I read the Tree Hygiene wiki page, which explains my responsibilities.
I read and followed the Flutter Style Guide, including Features we expect every widget to implement.
I signed the CLA.
I listed at least one issue that this PR fixes in the description above.
I updated/added relevant documentation (doc comments with ///).
I added new tests to check the change I am making, or this PR is test-exempt.
All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

justinmc

LGTM 👍

Thanks for the diff of the change to fix the tests.

fbcouch · 2022-01-31T21:10:06Z

@LongCatIsLooong @justinmc Think we can try landing this again?

…ter#97437)

justinmc · 2022-02-01T19:27:09Z

Looks like the build succeeded for this 👍 . Let's continue to keep an eye out and make sure that nothing is broken internally by these changes.

…ter#97437)

flar · 2022-02-02T18:49:41Z

Our benchmarks that monitor web deployment sizes saw a very small increase due to this feature, but I imagine that is to be expected and within reason for this feature?

https://flutter-flutter-perf.skia.org/e/?begin=1643716522&end=1643740360&keys=X2ae1480879add9e7079b55d2f713c856&num_commits=50&request_type=1&xbaroffset=27379

@justinmc @LongCatIsLooong

justinmc · 2022-02-02T20:50:01Z

I was not expecting a size increase from this PR (or the engine PR), but maybe I've forgotten something that @LongCatIsLooong or @fbcouch would know? Each PR is only +~1,200 lines.

Looking at the code, I see that this PR is using StringCharacters from the characters package for the first time (line), would that do it? Elsewhere we import the entire characters package, but I don't think we've referenced StringCharacters before, maybe it was being tree-shaken out before this?

flar · 2022-02-02T23:43:10Z

The increase is around 12kb uncompressed or 4kb compressed which is around .4% in each case.

justinmc · 2022-02-03T00:12:18Z

Did you mean kb? I wouldn't expect this to increase the size of any APK by 12mb even if it is the StringCharacters thing.

flar · 2022-02-03T00:27:52Z

Sorry, I think you're right. I'll edit my comment.

LongCatIsLooong · 2022-02-03T02:12:52Z

I was not expecting a size increase from this PR (or the engine PR), but maybe I've forgotten something that @LongCatIsLooong or @fbcouch would know? Each PR is only +~1,200 lines.

Looking at the code, I see that this PR is using StringCharacters from the characters package for the first time (line), would that do it? Elsewhere we import the entire characters package, but I don't think we've referenced StringCharacters before, maybe it was being tree-shaken out before this?

Yeah looks like the characters package itself defined a relatively large const string here: https://github.com/dart-lang/characters/blob/master/lib/src/grapheme_clusters/table.dart. If this becomes part of the binary then I wouldn't be too surprised that there's a 4kb increase. We'll probably need to use that library in more places in the future so it's just a matter of time for the size increase to happen.

flar · 2022-02-03T03:09:40Z

The regression has been triaged. Thanks all!

justinmc · 2022-02-03T17:26:38Z

Ah got it, thanks!

…ter#97437)

Reland Scribble, take 2

4abcb44

fbcouch force-pushed the reland-scribble-2 branch from 82801ec to 4abcb44 Compare January 28, 2022 16:30

justinmc approved these changes Jan 28, 2022

View reviewed changes

justinmc requested a review from LongCatIsLooong January 28, 2022 17:14

LongCatIsLooong approved these changes Jan 31, 2022

View reviewed changes

LongCatIsLooong added the waiting for tree to go green label Feb 1, 2022

fluttergithubbot merged commit af89a25 into flutter:master Feb 1, 2022

engine-flutter-autoroll mentioned this pull request Feb 1, 2022

Roll Flutter from e5cf7d1833a6 to af89a25e5b7b (1 revision) flutter/packages#619

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Feb 1, 2022

af89a25 Re-land "Support Scribble Handwriting" (#96615) (flutter/flut…

ff3ed7c

…ter#97437)

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Feb 1, 2022

af89a25 Re-land "Support Scribble Handwriting" (#96615) (flutter/flut…

4350f44

…ter#97437)

engine-flutter-autoroll mentioned this pull request Feb 1, 2022

Roll Flutter from e5cf7d1833a6 to 25aa7f86acd1 (2 revisions) flutter/packages#620

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Feb 1, 2022

af89a25 Re-land "Support Scribble Handwriting" (#96615) (flutter/flut…

046dc6b

…ter#97437)

engine-flutter-autoroll mentioned this pull request Feb 1, 2022

Roll Flutter from e5cf7d1833a6 to 24bf85e17fc1 (3 revisions) flutter/packages#621

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Feb 1, 2022

af89a25 Re-land "Support Scribble Handwriting" (#96615) (flutter/flut…

0dbb7f2

…ter#97437)

engine-flutter-autoroll mentioned this pull request Feb 1, 2022

Roll Flutter from e5cf7d1833a6 to d010a85476db (4 revisions) flutter/packages#622

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Feb 1, 2022

af89a25 Re-land "Support Scribble Handwriting" (#96615) (flutter/flut…

2328d24

…ter#97437)

engine-flutter-autoroll mentioned this pull request Feb 1, 2022

Roll Flutter from e5cf7d1833a6 to 98d625c243c5 (5 revisions) flutter/packages#623

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Feb 1, 2022

af89a25 Re-land "Support Scribble Handwriting" (#96615) (flutter/flut…

63a42d1

…ter#97437)

engine-flutter-autoroll mentioned this pull request Feb 1, 2022

Roll Flutter from e5cf7d1833a6 to 39c5d2fa0a98 (6 revisions) flutter/packages#624

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/plugins that referenced this pull request Feb 4, 2022

af89a25 Re-land "Support Scribble Handwriting" (#96615) (flutter/flut…

c6d556d

…ter#97437)

clocksmith pushed a commit to clocksmith/flutter that referenced this pull request Mar 8, 2022

Re-land "Support Scribble Handwriting" (flutter#96615) (flutter#97437)

fbd355d

mattrussell-sonocent mentioned this pull request Apr 26, 2022

Scribble (handwriting) input not working on iPad #102561

Closed

justinmc mentioned this pull request Sep 16, 2022

TextField to support samsung spen handwritting input #111003

Open

justinmc mentioned this pull request Jul 21, 2023

Can't delete and separate words by Scribble with Apple Pencil written on TextFormField when setting maxLines 1 #121868

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Re-land "Support Scribble Handwriting" (#96615)#97437

Re-land "Support Scribble Handwriting" (#96615)#97437
fluttergithubbot merged 1 commit intoflutter:masterfrom
twinsunllc:reland-scribble-2

fbcouch commented Jan 28, 2022

Uh oh!

justinmc left a comment

Uh oh!

fbcouch commented Jan 31, 2022

Uh oh!

justinmc commented Feb 1, 2022

Uh oh!

flar commented Feb 2, 2022 •

edited

Loading

Uh oh!

justinmc commented Feb 2, 2022

Uh oh!

flar commented Feb 2, 2022 •

edited

Loading

Uh oh!

justinmc commented Feb 3, 2022

Uh oh!

flar commented Feb 3, 2022

Uh oh!

LongCatIsLooong commented Feb 3, 2022 •

edited

Loading

Uh oh!

flar commented Feb 3, 2022

Uh oh!

justinmc commented Feb 3, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Conversation

fbcouch commented Jan 28, 2022

Pre-launch Checklist

Uh oh!

justinmc left a comment

Choose a reason for hiding this comment

Uh oh!

fbcouch commented Jan 31, 2022

Uh oh!

justinmc commented Feb 1, 2022

Uh oh!

flar commented Feb 2, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

justinmc commented Feb 2, 2022

Uh oh!

flar commented Feb 2, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

justinmc commented Feb 3, 2022

Uh oh!

flar commented Feb 3, 2022

Uh oh!

LongCatIsLooong commented Feb 3, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

flar commented Feb 3, 2022

Uh oh!

justinmc commented Feb 3, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

flar commented Feb 2, 2022 •

edited

Loading

flar commented Feb 2, 2022 •

edited

Loading

LongCatIsLooong commented Feb 3, 2022 •

edited

Loading