Add 'Share' button to the selection toolbar on Android #139479

bleroux · 2023-12-04T14:46:03Z

Description

This PR adds the 'Share' button to the text selection toolbar on Android.

Related Issue

Fixes #138728

Tests

Refactor a lot of existing tests in order to:

make them more readable (avoid duplication by introducing helper functions, specify explictly check which buttons are expected).
make them more accurate (check that expected buttons are visible instead of just checking the number of buttons).

For instance, previous tests contained sections such as:

      // Collapsed toolbar shows 3 buttons.
      expect(
        find.byType(CupertinoButton),
        isContextMenuProvidedByPlatform ? findsNothing : isTargetPlatformIOS ? findsNWidgets(6) : findsNWidgets(3)
      );

Where the comment is obsolete, the two cases (6 widgets and 3 widgets) are not explicit (which buttons are expected?), and not accurate (will pass if the number of buttons is right but the buttons are the wrong ones).

flutter-dashboard · 2023-12-04T15:31:14Z

Golden file changes have been found for this pull request. Click here to view and triage (e.g. because this is an intentional change).

If you are still iterating on this change and are not ready to resolve the images on the Flutter Gold dashboard, consider marking this PR as a draft pull request above. You will still be able to view image results on the dashboard, commenting will be silenced, and the check will not try to resolve itself until marked ready for review.

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.

Changes reported for pull request #139479 at sha 74b42ed

bleroux · 2023-12-04T16:06:48Z

FYI, the golden change for material.text_field_opacity_test.0 is expected because it is a golden showing the text selection toolbar on Android and this PR adds the Share option to this menu.

flutter-dashboard · 2023-12-05T15:45:15Z

Golden file changes are available for triage from new commit, Click here to view.

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.

Changes reported for pull request #139479 at sha 3e9b284

Renzo-Olivares · 2023-12-05T18:24:09Z

packages/flutter/test/cupertino/text_field_test.dart

@@ -671,6 +664,8 @@ void main() {
            selectionColor: selectionColor,
            cursorColor: cursorColor,
            child: Column(
+              // TODO(bleroux): investigate why the test fail without setting the mainAxisAlignment.


That is interesting, was this test working with no changes before this PR? I don't see why this PR would cause that to change.

Yes, I can confirm that this test is working on master without this change.
With this PR, I just found that another solution to make this test green is to add a call to pumpAndSettle() on line 692, so there is probably an animation going on.
Do you think it is better to keep this TODO or to replace it with the call to pumpAndSettle? There are dozens of pumpAndSettle in this test file, so I think it makes sense to use one here, the remaining question is why was it not necesseray before? (it might be related to the toolbar content being longer than before).

Adding a pumpAndSettle sounds reasonable. I tried the pr out locally and found that if I change the logic in bool get shareEnabled to the code below the test passes. I'm not at all sure why this works. I think we should we keep the switch statement and add a pumpAndSettle since we might support other platforms.

if (defaultTargetPlatform != TargetPlatform.iOS || defaultTargetPlatform != TargetPlatform.android) { return false; } return !widget.obscureText && !textEditingValue.selection.isCollapsed && textEditingValue.selection.textInside(textEditingValue.text).trim() != '';

Renzo-Olivares · 2023-12-05T18:27:37Z

packages/flutter/test/material/text_field_test.dart

+    } else {
+      expect(find.text('Share'), findsOneWidget);
+      await tester.tap(find.text('Share'));
+      expect(lastShare, 'Test');


nit: consider moving expect(lastShare, 'Test'); out of this if statement since it is common to both branches.

Renzo-Olivares · 2023-12-05T18:38:01Z

packages/flutter/test/widgets/selectable_text_test.dart

@@ -144,6 +143,41 @@ void main() {
    return renderEditable;
  }

+  // Check that the Cupertino text selection toolbar is the expected one on iOS and macOS.
+  // TODO(bleroux): try to merge this into text_selection_toolbar_utils.dart


Is this planned for this PR, or in the future?, same for below.

I plan to work on this later because it is not a blocker for this PR and it requires some careful thinking because:

it should be implemented correctly for all platforms.

It might make the helper functions in the 'utils' file less readable (and I wanted to keep them as readable as possible for this review and for the next PR I'm working on, the one related to [Android] Respect custom system-wide text selection toolbar buttons #139361).

Sounds good to me!

Renzo-Olivares · 2023-12-05T18:39:52Z

Thank you for the contribution @bleroux! These improvements to tests are awesome! I just had a few small comments, but this look great!

flutter-dashboard · 2023-12-06T11:19:16Z

Golden file changes are available for triage from new commit, Click here to view.

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.

Changes reported for pull request #139479 at sha 075f9dc

Renzo-Olivares

LGTM after the pumpAndSettle is added. Thanks for the contribution @bleroux

flutter-dashboard · 2023-12-08T13:45:16Z

Golden file changes are available for triage from new commit, Click here to view.

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.

Changes reported for pull request #139479 at sha a29f89a

…tter/flutter#139479)

…9738) ## Description This PR adds custom system-wide text selection toolbar buttons on Android. ~~This is a WIP until #139479 is merged (potential conflicts).~~ ## Related Issue Fixes #139361 ## Tests Adds 5 tests.

…tter#139738) ## Description This PR adds custom system-wide text selection toolbar buttons on Android. ~~This is a WIP until flutter#139479 is merged (potential conflicts).~~ ## Related Issue Fixes flutter#139361 ## Tests Adds 5 tests.

## Description This PR adds the 'Share' button to the text selection toolbar on Android. ## Related Issue Fixes flutter#138728 ## Tests Refactor a lot of existing tests in order to: - make them more readable (avoid duplication by introducing helper functions, specify explictly check which buttons are expected). - make them more accurate (check that expected buttons are visible instead of just checking the number of buttons). For instance, previous tests contained sections such as: ```dart // Collapsed toolbar shows 3 buttons. expect( find.byType(CupertinoButton), isContextMenuProvidedByPlatform ? findsNothing : isTargetPlatformIOS ? findsNWidgets(6) : findsNWidgets(3) ); ``` Where the comment is obsolete, the two cases (6 widgets and 3 widgets) are not explicit (which buttons are expected?), and not accurate (will pass if the number of buttons is right but the buttons are the wrong ones).

…tter#139738) ## Description This PR adds custom system-wide text selection toolbar buttons on Android. ~~This is a WIP until flutter#139479 is merged (potential conflicts).~~ ## Related Issue Fixes flutter#139361 ## Tests Adds 5 tests.

## Description This PR adds the share button to text selection toolbar buttons on Android ~~and iOS~~ for `SelectableRegion` (and therefore `SelectionArea`). #139479 adds this button for `EditableText` (which is used by `TextField` and `SelectableText` but not by `SelectionArea`). **Edit**: supporting this on iOS will need more work (see #141447 (comment) and #141775). ## Related Issue Follow up for #138728 ## Tests Adds 1 test.

…tter/flutter#139479)

flutter-dashboard bot added the will affect goldens Changes to golden files label Dec 4, 2023

bleroux requested a review from Renzo-Olivares December 4, 2023 16:09

bleroux force-pushed the add_android_share_option_to_selection_toolbar branch from 74b42ed to 3e9b284 Compare December 5, 2023 15:16

Renzo-Olivares reviewed Dec 5, 2023

View reviewed changes

bleroux requested a review from Renzo-Olivares December 6, 2023 15:26

This was referenced Dec 7, 2023

[Android] Add 'Share' button to selection controls #138728

Closed

Add custom system-wide text selection toolbar buttons on Android #139738

Merged

Renzo-Olivares approved these changes Dec 7, 2023

View reviewed changes

bleroux and others added 3 commits December 8, 2023 14:11

Add 'Share' button to the selection toolbar on Android

7055a49

Remove duplicate call

47192de

Call pumpAndSettle to fix one Cupertino test

a29f89a

bleroux force-pushed the add_android_share_option_to_selection_toolbar branch from 075f9dc to a29f89a Compare December 8, 2023 13:15

Empty commit to check golden test

67fa35b

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

auto-submit bot merged commit e070417 into flutter:master Dec 8, 2023
67 checks passed

bleroux deleted the add_android_share_option_to_selection_toolbar branch December 8, 2023 15:35

engine-flutter-autoroll mentioned this pull request Dec 8, 2023

Roll Flutter from f35d049e0d8e to e070417afeb6 (86 revisions) flutter/packages#5617

Closed

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

e070417af Add 'Share' button to the selection toolbar on Android (flu…

4d97655

…tter/flutter#139479)

engine-flutter-autoroll mentioned this pull request Dec 8, 2023

Roll Flutter from f35d049e0d8e to 6142e7cbdbc1 (87 revisions) flutter/packages#5618

Closed

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

e070417af Add 'Share' button to the selection toolbar on Android (flu…

a188e46

…tter/flutter#139479)

engine-flutter-autoroll mentioned this pull request Dec 8, 2023

Roll Flutter from f35d049e0d8e to 0c0b77f34274 (88 revisions) flutter/packages#5620

Closed

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

e070417af Add 'Share' button to the selection toolbar on Android (flu…

c369565

…tter/flutter#139479)

engine-flutter-autoroll mentioned this pull request Dec 8, 2023

Roll Flutter from f35d049e0d8e to 82bc9ca3273b (89 revisions) flutter/packages#5621

Closed

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

e070417af Add 'Share' button to the selection toolbar on Android (flu…

84b2867

…tter/flutter#139479)

engine-flutter-autoroll mentioned this pull request Dec 8, 2023

Roll Flutter from f35d049e0d8e to 3df9d4423348 (90 revisions) flutter/packages#5622

Closed

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

e070417af Add 'Share' button to the selection toolbar on Android (flu…

a25f231

…tter/flutter#139479)

engine-flutter-autoroll mentioned this pull request Dec 8, 2023

Roll Flutter from f35d049e0d8e to a5acec1b8d30 (91 revisions) flutter/packages#5624

Closed

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

e070417af Add 'Share' button to the selection toolbar on Android (flu…

d8b0f12

…tter/flutter#139479)

engine-flutter-autoroll mentioned this pull request Dec 8, 2023

Roll Flutter from f35d049e0d8e to c642f4e012f4 (92 revisions) flutter/packages#5626

Closed

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

e070417af Add 'Share' button to the selection toolbar on Android (flu…

1a8cf5b

…tter/flutter#139479)

bleroux mentioned this pull request Jan 12, 2024

Add Share button to the SelectableRegion toolbar on Android #141447

Merged

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

e070417af Add 'Share' button to the selection toolbar on Android (flu…

a8dc7cc

…tter/flutter#139479)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add 'Share' button to the selection toolbar on Android #139479

Add 'Share' button to the selection toolbar on Android #139479

bleroux commented Dec 4, 2023 •

edited

flutter-dashboard bot commented Dec 4, 2023

bleroux commented Dec 4, 2023

flutter-dashboard bot commented Dec 5, 2023

Renzo-Olivares Dec 5, 2023

bleroux Dec 6, 2023

Renzo-Olivares Dec 7, 2023

Renzo-Olivares Dec 5, 2023

Renzo-Olivares Dec 5, 2023

bleroux Dec 6, 2023 •

edited

Renzo-Olivares Dec 7, 2023

Renzo-Olivares commented Dec 5, 2023

flutter-dashboard bot commented Dec 6, 2023

Renzo-Olivares left a comment

flutter-dashboard bot commented Dec 8, 2023

Add 'Share' button to the selection toolbar on Android #139479

Add 'Share' button to the selection toolbar on Android #139479

Conversation

bleroux commented Dec 4, 2023 • edited

Description

Related Issue

Tests

flutter-dashboard bot commented Dec 4, 2023

bleroux commented Dec 4, 2023

flutter-dashboard bot commented Dec 5, 2023

Renzo-Olivares Dec 5, 2023

Choose a reason for hiding this comment

bleroux Dec 6, 2023

Choose a reason for hiding this comment

Renzo-Olivares Dec 7, 2023

Choose a reason for hiding this comment

Renzo-Olivares Dec 5, 2023

Choose a reason for hiding this comment

Renzo-Olivares Dec 5, 2023

Choose a reason for hiding this comment

bleroux Dec 6, 2023 • edited

Choose a reason for hiding this comment

Renzo-Olivares Dec 7, 2023

Choose a reason for hiding this comment

Renzo-Olivares commented Dec 5, 2023

flutter-dashboard bot commented Dec 6, 2023

Renzo-Olivares left a comment

Choose a reason for hiding this comment

flutter-dashboard bot commented Dec 8, 2023

bleroux commented Dec 4, 2023 •

edited

bleroux Dec 6, 2023 •

edited