Skip to content

Conversation

@LouiseHsu
Copy link
Contributor

@LouiseHsu LouiseHsu commented Aug 15, 2023

In native iOS, users are able to select text and initiate a share menu, which provides several standard services, such as copy, sharing to social media, direct ability to send to various contacts through messaging apps, etc.

Simulator.Screen.Recording.-.iPhone.14.Pro.Max.-.2023-08-09.at.18.47.02.mp4

This PR is the framework portion of the changes that will allow Share to be implemented.
The corresponding merged engine PR is here
This PR addresses #107578
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.

@github-actions github-actions bot added a: text input Entering text in a text field or keyboard related problems framework flutter/packages/flutter repository. See also f: labels. f: material design flutter/packages/flutter/material repository. f: cupertino flutter/packages/flutter/cupertino repository labels Aug 15, 2023
@github-actions github-actions bot added the a: internationalization Supporting other languages or locales. (aka i18n) label Aug 16, 2023
@LouiseHsu LouiseHsu changed the title initial PR [Framework] Add Share to selection controls Aug 16, 2023
@LouiseHsu LouiseHsu marked this pull request as ready for review August 16, 2023 19:58
Copy link
Contributor

@hellohuanlin hellohuanlin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm after addressing those comments

String get searchWebButtonLabel => 'Search Web';

@override
String get shareButtonLabel => 'Share...';
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can't remember - does native ios has the ...?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep!
image

skip: isContextMenuProvidedByPlatform, // [intended] only applies to platforms where we supply the context menu.
);

testWidgets('Share shows up on iOS only (CupertinoTextField)', (WidgetTester tester) async {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

does share show up in material for ios?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no (?), looks like cupertino to me

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Regardless of whether TextField or CupertinoTextField is being used, the default context menu will reflect the current platform. So if you use TextField on an iPhone, you'll still get the iOS context menu.

So this test and the following test look correct as written to me. The only thing I would change is to move the TextField test below to material/text_field_test.dart.

Copy link
Contributor

@justinmc justinmc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM with nits 👍

}

/// Launch the share interface for the current selection,
/// as in the "Share" edit menu button on iOS.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: Some weird indentation here after the ///.

/// Launch the share interface for the current selection,
/// as in the "Share" edit menu button on iOS.
///
/// Currently this is only implemented for iOS.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: Add an empty line of /// after this line.

skip: isContextMenuProvidedByPlatform, // [intended] only applies to platforms where we supply the context menu.
);

testWidgets('Share shows up on iOS only (CupertinoTextField)', (WidgetTester tester) async {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Regardless of whether TextField or CupertinoTextField is being used, the default context menu will reflect the current platform. So if you use TextField on an iPhone, you'll still get the iOS context menu.

So this test and the following test look correct as written to me. The only thing I would change is to move the TextField test below to material/text_field_test.dart.

skip: isContextMenuProvidedByPlatform, // [intended] only applies to platforms where we supply the context menu.
);

testWidgets('Share shows up on iOS only (TextField)', (WidgetTester tester) async {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As mentioned above, this test should be in material/text_field_test.dart.

await gesture.up();
await tester.pump();
expect(find.byType(CupertinoButton), findsNWidgets(3));
expect(find.byType(CupertinoButton), findsNWidgets(4));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: Seeing you change these numbers all over the place in these PRs makes me think we should have a better way to test this... expect(contextMenuButtonsShown(ContextMenuButtons.all)); or something like that.

That's probably beyond the scope of this PR, though. Also it might not be worth it if we don't add many more buttons. Just throwing out a thought.

@LouiseHsu LouiseHsu added the autosubmit Merge PR when tree becomes green via auto submit App label Aug 17, 2023
@auto-submit auto-submit bot removed the autosubmit Merge PR when tree becomes green via auto submit App label Aug 17, 2023
@auto-submit
Copy link
Contributor

auto-submit bot commented Aug 17, 2023

auto label is removed for flutter/flutter/132599, due to - The status or check suite Linux analyze has failed. Please fix the issues identified (or deflake) before re-applying this label.

@LouiseHsu LouiseHsu added the autosubmit Merge PR when tree becomes green via auto submit App label Aug 17, 2023
@auto-submit auto-submit bot merged commit 3f34b48 into flutter:master Aug 17, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Aug 18, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Aug 18, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Aug 18, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Aug 18, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Nov 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

a: internationalization Supporting other languages or locales. (aka i18n) a: text input Entering text in a text field or keyboard related problems autosubmit Merge PR when tree becomes green via auto submit App f: cupertino flutter/packages/flutter/cupertino repository f: material design flutter/packages/flutter/material repository. framework flutter/packages/flutter repository. See also f: labels.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants