[framework] Add Search Web to selection controls for iOS #131898

LouiseHsu · 2023-08-03T22:45:01Z

This PR adds framework support for the Search Web feature in iOS.

Simulator.Screen.Recording.-.iPhone.14.Pro.Max.-.2023-08-03.at.15.39.56.mp4

The corresponding merged engine PR can be found here.
This PR addresses #82907
More details are available in this design doc

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] and the [C++, Objective-C, Java style guides].
I listed at least one issue that this PR fixes in the description above.
I added new tests to check the change I am making or feature I am adding, or Hixie said the PR is test-exempt. See [testing the engine] for instructions on writing and running engine tests.
I updated/added relevant documentation (doc comments with ///).
I signed the [CLA].
All existing and new tests are passing.

justinmc

I'm so excited for this again, each of these buttons we add is such an improvement to the iOS text editing experience.

Could you add a test that presses the search button and checks that it calls the right platform channel method (and maybe also that the toolbar is dismissed)? I think you did something similar in the Look Up PR.

    String? lastLookUp;
    setUp(() {
      TestDefaultBinaryMessengerBinding.instance.defaultBinaryMessenger
        .setMockMethodCallHandler(SystemChannels.platform, (MethodCall methodCall) async {
          if (methodCall.method == 'LookUp.invoke') {
            expect(methodCall.arguments, isA<String>());
            lastLookUp = methodCall.arguments as String;
          }
        });
    });
    tearDown(() {
      TestDefaultBinaryMessengerBinding.instance.defaultBinaryMessenger
          .setMockMethodCallHandler(SystemChannels.platform, null);
    });

justinmc · 2023-08-04T16:12:31Z

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

+  /// Launch a web search on the current selection, as in the "Search Web" edit menu button on iOS.
+  /// Currently this is only implemented for iOS.
+  /// Throws an error if the selection is empty or collapsed.


Nit: Separate these paragraphs with an empty line of ///.

Also, it mentions that it throws an error. Is it textInside that actually throws the error? Just making sure since I don't see a throw.

And maybe also mention that it does nothing if obscureText is true?

justinmc · 2023-08-04T16:13:12Z

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

+    assert(!widget.obscureText);
+
+    final String text = textEditingValue.selection.textInside(textEditingValue.text);
+    if (widget.obscureText || text.isEmpty) {


Nit: Maybe do the widget.obscureText case before doing textInside to avoid the unnecessary operation. Not a big deal though, readability is more important.

justinmc · 2023-08-04T16:14:29Z

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

+    if (widget.obscureText || text.isEmpty) {
+      return;
+    }
+    await SystemChannels.platform.invokeMethod(


Nit: I think you could also remove the async from this method and instead return the result of this call instead? Just calling out that as an option to consider, it's up to you if that's any better or not.

That makes sense, but Im using async for both this function and the corresponding Look Up equivalent because Im following the pattern that pasteText uses and I wanted to be consistent - is there a benefit to not using async in this case?

No benefit that I'm aware of, I would keep it as-is in that case 👍

justinmc · 2023-08-04T16:17:56Z

packages/flutter/test/cupertino/adaptive_text_selection_toolbar_test.dart

+  Finder findOverflowNextButton() => find.byWidgetPredicate((Widget widget) =>
+  widget is CustomPaint &&
+      '${widget.painter?.runtimeType}' == '_RightCupertinoChevronPainter',
+  );


Nit: The multiple inline function => things are a little bit confusing, maybe turn one or both into regular () {} functions?

Or at least indent lines 34 and 35 I think.

justinmc · 2023-08-04T16:18:51Z

packages/flutter/test/cupertino/text_field_test.dart


-      // Selected text shows 3 toolbar buttons.
-      expect(find.byType(CupertinoButton), isContextMenuProvidedByPlatform ? findsNothing : findsNWidgets(4));
+      // Selected text shows 5 toolbar buttons.


Whoops that 3 wasn't right before 😊

justinmc · 2023-08-04T16:20:07Z

packages/flutter/test/material/adaptive_text_selection_toolbar_test.dart

+  Finder findOverflowNextButton() => find.byWidgetPredicate((Widget widget) =>
+  widget is CustomPaint &&
+      '${widget.painter?.runtimeType}' == '_RightCupertinoChevronPainter',
+  );


Same comment here about =>.

hellohuanlin

looks clean

…tter/flutter#131898)

flutter/flutter@436df69...f4c25bb 2023-08-09 [email protected] Revert "Handle breaking changes in leak_tracker." (flutter/flutter#132223) 2023-08-09 [email protected] Roll Flutter Engine from 8ec575168d44 to 3d3fb4faed02 (3 revisions) (flutter/flutter#132193) 2023-08-09 [email protected] Use pattern matching to avoid strange type annotations (flutter/flutter#131964) 2023-08-09 [email protected] Roll Flutter Engine from 6839b88283b6 to 8ec575168d44 (1 revision) (flutter/flutter#132182) 2023-08-09 [email protected] Roll Flutter Engine from 29b117aa4192 to 6839b88283b6 (1 revision) (flutter/flutter#132181) 2023-08-09 [email protected] Roll Flutter Engine from 82292b8390cb to 29b117aa4192 (5 revisions) (flutter/flutter#132179) 2023-08-09 [email protected] Add more info to `OverlayState.insert` error messages (flutter/flutter#129363) 2023-08-08 [email protected] Add `textCapitalization` property for `SearchBar` and `SearchAnchor` (flutter/flutter#131459) 2023-08-08 [email protected] Roll Flutter Engine from 934ebb005d02 to 82292b8390cb (3 revisions) (flutter/flutter#132166) 2023-08-08 49699333+dependabot[bot]@users.noreply.github.com Bump github/codeql-action from 2.21.2 to 2.21.3 (flutter/flutter#132165) 2023-08-08 [email protected] Revert of #120385 (flutter/flutter#132167) 2023-08-08 [email protected] Add `PopupMenuButton.iconColor`, `PopupMenuTheme.iconSize` and fix button icon using unexpected color propert (flutter/flutter#132054) 2023-08-08 [email protected] More PageStorage clarity in the documentation (flutter/flutter#131954) 2023-08-08 [email protected] Document that missed_frame_build_budget_count is misleading (flutter/flutter#132137) 2023-08-08 [email protected] Locate the template directory using a TemplatePathProvider. (flutter/flutter#132156) 2023-08-08 [email protected] Roll Flutter Engine from dd03fae51d38 to 934ebb005d02 (4 revisions) (flutter/flutter#132159) 2023-08-08 [email protected] Roll Flutter Engine from 22bd35a19352 to dd03fae51d38 (3 revisions) (flutter/flutter#132149) 2023-08-08 [email protected] Fix `TabBarTheme.indicatorColor` not applied in Material 2 (flutter/flutter#132123) 2023-08-08 [email protected] Paginated Data Table : Fixed Row number at footer in last page (flutter/flutter#130389) 2023-08-08 [email protected] [web] Use benchmark callback from `dart:ui_web` (flutter/flutter#132087) 2023-08-08 [email protected] Roll Flutter Engine from 99fdac88f3c6 to 22bd35a19352 (3 revisions) (flutter/flutter#132141) 2023-08-08 [email protected] [framework] Add Search Web to selection controls for iOS (flutter/flutter#131898) 2023-08-08 [email protected] Remove Iterator from _History (flutter/flutter#132101) 2023-08-08 [email protected] Update dartdoc driver to match current behaviour (flutter/flutter#132078) 2023-08-08 [email protected] More documentation about warm-up frames (flutter/flutter#132085) 2023-08-08 [email protected] Roll Flutter Engine from 146c4c9487fc to 99fdac88f3c6 (3 revisions) (flutter/flutter#132135) 2023-08-08 [email protected] Roll Packages from d7ee75a to ac41376 (8 revisions) (flutter/flutter#132133) 2023-08-08 [email protected] Handle breaking changes in leak_tracker. (flutter/flutter#131998) 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 [email protected],[email protected],[email protected] 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

…tter/flutter#131898)

initial commit for adding search web to selection controls on IOS

e646914

LouiseHsu added 2 commits August 3, 2023 15:46

Merge branch 'master' into add-search-web-to-selection-controls

5d72ac2

fix failing test and remove unused function

dcfa6a3

LouiseHsu marked this pull request as ready for review August 3, 2023 23:29

LouiseHsu requested review from hellohuanlin and justinmc August 3, 2023 23:29

justinmc reviewed Aug 4, 2023

View reviewed changes

Formatting, add test to check Search Web only shows up on IOS

d021d3a

justinmc mentioned this pull request Aug 8, 2023

Missing default context menu buttons on each platform #107578

Open

hellohuanlin approved these changes Aug 8, 2023

View reviewed changes

LouiseHsu added the autosubmit Merge PR when tree becomes green via auto submit App label Aug 8, 2023

auto-submit bot merged commit d9cb50e into flutter:master Aug 8, 2023

engine-flutter-autoroll mentioned this pull request Aug 9, 2023

Roll Flutter from 436df69a4684 to d78b3ac0af0e (27 revisions) flutter/packages#4665

Closed

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

d9cb50e [framework] Add Search Web to selection controls for iOS (flu…

4eef661

…tter/flutter#131898)

engine-flutter-autoroll mentioned this pull request Aug 9, 2023

Roll Flutter from 436df69a4684 to f4c25bbb351c (28 revisions) flutter/packages#4666

Merged

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

d9cb50e [framework] Add Search Web to selection controls for iOS (flu…

8ecb0b9

…tter/flutter#131898)

HansMuller mentioned this pull request Aug 11, 2023

Flutter Framework Team: Notable Commits #121415

Closed

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

d9cb50e [framework] Add Search Web to selection controls for iOS (flu…

a598d3b

…tter/flutter#131898)

bleroux mentioned this pull request Jan 18, 2024

[iOS] Add default buttons to SelectionArea context menu #141775

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[framework] Add Search Web to selection controls for iOS #131898

[framework] Add Search Web to selection controls for iOS #131898

Uh oh!

LouiseHsu commented Aug 3, 2023 •

edited

Loading

Uh oh!

justinmc left a comment

Uh oh!

justinmc Aug 4, 2023

Uh oh!

justinmc Aug 4, 2023

Uh oh!

justinmc Aug 4, 2023

Uh oh!

LouiseHsu Aug 7, 2023

Uh oh!

justinmc Aug 8, 2023

Uh oh!

justinmc Aug 4, 2023

Uh oh!

justinmc Aug 4, 2023

Uh oh!

justinmc Aug 4, 2023

Uh oh!

hellohuanlin left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

[framework] Add Search Web to selection controls for iOS #131898

[framework] Add Search Web to selection controls for iOS #131898

Uh oh!

Conversation

LouiseHsu commented Aug 3, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pre-launch Checklist

Uh oh!

justinmc left a comment

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!

hellohuanlin left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

LouiseHsu commented Aug 3, 2023 •

edited

Loading