[framework] Add Look Up to selection controls for iOS #130532

LouiseHsu · 2023-07-13T22:58:25Z

This PR adds framework support for the Look Up feature in iOS.

Simulator.Screen.Recording.-.iPhone.14.Pro.Max.-.2023-07-11.at.15.23.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.

hellohuanlin · 2023-07-21T20:56:31Z

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

+      return false;
+    }
+    return !widget.obscureText
+        && !textEditingValue.selection.isCollapsed;


what does collapsed mean?

Collapsed means when the selection is empty - i.e when the selection "bars" don't contain at least one character/whitespace

hellohuanlin · 2023-07-21T20:57:32Z

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


+  /// Look Up current selection.
+  Future<void> lookUpSelection(SelectionChangedCause cause) async {
+    if (widget.obscureText) {


should we assert it's not obscured here?

In production mode, if this somehow gets called when obscureText is true, then I guess we should do nothing and not make the platform channel call? If so then we still need this early return. Maybe an assertion and an early return both?

hellohuanlin · 2023-07-21T20:58:03Z

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

+    if (widget.obscureText) {
+      return;
+    }
+    final String text = textEditingValue.selection.textInside(textEditingValue.text);


do we need to check text not empty?

You shouldn't be able to get here if the text is empty cuz I think that would mean the selection is collapsed - but I will add a check just in case 👀

+1 for a check. I guess someone could just directly call this when the selection is collapsed since it's public.

hellohuanlin · 2023-07-21T20:59:11Z

packages/flutter/test/material/text_field_test.dart

    );

    final RenderEditable renderEditable = findRenderEditable(tester);
+    final bool isTargetPlatformMobile = defaultTargetPlatform == TargetPlatform.iOS;


isTargetPlatformIOS?

hellohuanlin · 2023-07-21T21:00:56Z

packages/flutter_localizations/lib/src/l10n/cupertino_te.arb

  "searchTextFieldPlaceholderLabel": "సెర్చ్ చేయి",
-  "noSpellCheckReplacementsLabel": "No Replacements Found"
+  "noSpellCheckReplacementsLabel": "No Replacements Found",
+  "lookUpButtonLabel": "Look Up"


some of these files seem to be localized properly.

not entirely sure how to handle the translation. @justinmc probably knows

I believe the english is the default and it gets properly localized at a later date
This is what I followed https://github.com/flutter/flutter/tree/master/packages/flutter_localizations#adding-a-new-string-to-localizations

Also I added both material and cupertino translations - is that right or do I only need one of them?

I think you're right but I always mess this up. @thkim1011 can you confirm this localization string was added correctly?

The string looks like it was added properly. One thing I am concerned about is that in the material localizations, "Select all" is cased with a lowercase "a" in "all" so I'm wondering if it should be "Look up" instead with a lowercase "u".

I think it's a little different because Look Up is the name of a feature in iOS, and Select all is more of a... description? What do you think?

Just saw the native iOS behavior is upper case for both "Select All" and "Look Up". Not sure about android tho.

This PR adds framework support for the Look Up feature in iOS. https://github.com/flutter/flutter/assets/36148254/d301df79-4e23-454f-8742-2f8e39c2960c The corresponding merged engine PR can be found [here](flutter/engine#43308). This PR addresses #82907 More details are available in this [design doc.](flutter.dev/go/add-missing-features-to-selection-controls) This is the same PR as #130532, this is an attempt to fix the Google Testing issue

initial pr commit

5a0816a

LouiseHsu added 4 commits July 13, 2023 15:58

Merge branch 'master' into louisehsu/add-lookup-to-selection-controls

52953bd

add missing Cupertino localizations

09aef6d

Merge branch 'master' into louisehsu/add-lookup-to-selection-controls

320989a

Merge branch 'master' into louisehsu/add-lookup-to-selection-controls

94fd690

LouiseHsu requested a review from hellohuanlin July 18, 2023 21:56

LouiseHsu added 5 commits July 18, 2023 14:59

attempt to fix remote tests

a89351d

fix failing test

f7bf698

fix linux-analyze

dc63cfc

Merge branch 'master' into louisehsu/add-lookup-to-selection-controls

632aa23

Merge branch 'master' into louisehsu/add-lookup-to-selection-controls

766e7ab

hellohuanlin reviewed Jul 21, 2023

View reviewed changes

LouiseHsu requested a review from justinmc July 25, 2023 17:54

hellohuanlin approved these changes Jul 25, 2023

View reviewed changes

LouiseHsu force-pushed the louisehsu/add-lookup-to-selection-controls branch from 072404e to 766e7ab Compare July 25, 2023 23:49

LouiseHsu removed the tool Affects the "flutter" command-line tool. See also t: labels. label Jul 25, 2023

LouiseHsu added 4 commits July 26, 2023 16:25

PR feedback

8eaf23c

fix linux analyze

0c3f802

move platform channel check for look up into test from set up

c25c211

fix tests

c2d9cdd

LouiseHsu requested a review from christopherfujino as a code owner July 27, 2023 21:36

LouiseHsu force-pushed the louisehsu/add-lookup-to-selection-controls branch from e0a2548 to c2d9cdd Compare July 27, 2023 21:38

LouiseHsu removed the request for review from christopherfujino July 27, 2023 22:04

restart tests

7b87fe4

github-actions bot added a: desktop Running on desktop c: tech-debt Technical debt, code quality, testing, etc. labels Jul 31, 2023

LouiseHsu force-pushed the louisehsu/add-lookup-to-selection-controls branch from c13d248 to 7b87fe4 Compare July 31, 2023 23:17

LouiseHsu and others added 2 commits July 31, 2023 16:18

Merge branch 'master' into louisehsu/add-lookup-to-selection-controls

7ac29c6

Merge branch 'master' into louisehsu/add-lookup-to-selection-controls

6aeb335

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

justinmc mentioned this pull request Aug 2, 2023

Missing default context menu buttons on each platform #107578

Open

LouiseHsu mentioned this pull request Aug 2, 2023

[framework] Add Look Up to selection controls for iOS #131798

Merged

8 tasks

LouiseHsu closed this Aug 2, 2023

bleroux mentioned this pull request Jan 18, 2024

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

Open

[framework] Add Look Up to selection controls for iOS #130532

[framework] Add Look Up to selection controls for iOS #130532

Uh oh!

Conversation

LouiseHsu commented Jul 13, 2023

Pre-launch Checklist

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants