-
Notifications
You must be signed in to change notification settings - Fork 29.1k
Include Share in iOS SystemContextMenu defaults #173626
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Include Share in iOS SystemContextMenu defaults #173626
Conversation
Fixes flutter#173491. This adds IOSSystemContextMenuItemShare to the default iOS system context menu items when shareEnabled is true, ensuring native 'Share' option appears for text selections.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request correctly adds the 'Share' option to the default iOS system context menu and includes a new test to verify this behavior. The implementation change is straightforward and correct. My review focuses on the new test, where I've suggested improvements for resource management and code clarity by refactoring the test's helper widget to properly handle the lifecycle of its TextEditingController
and FocusNode
. This will prevent resource leaks and make the test easier to understand.
testWidgets('Default iOS SystemContextMenu includes Share for non-empty selection', | ||
(WidgetTester tester) async { | ||
final controller = TextEditingController(text: 'Hello world'); | ||
|
||
// Force iOS + tell Flutter we support the iOS system context menu path. | ||
debugDefaultTargetPlatformOverride = TargetPlatform.iOS; | ||
|
||
await tester.pumpWidget( | ||
MediaQuery( | ||
data: const MediaQueryData(supportsShowingSystemContextMenu: true), | ||
child: const Directionality( | ||
textDirection: TextDirection.ltr, | ||
child: Material( | ||
child: _HostEditable(), // minimal EditableText wrapper | ||
), | ||
), | ||
), | ||
); | ||
|
||
// Setup selection so sharing is meaningful. | ||
final state = tester.state<EditableTextState>(find.byType(EditableText)); | ||
state.widget.controller.text = controller.text; | ||
state.widget.controller.selection = | ||
const TextSelection(baseOffset: 0, extentOffset: 5); // "Hello" | ||
state.widget.focusNode.requestFocus(); | ||
await tester.pump(); | ||
|
||
// Get iOS SYSTEM menu defaults and assert Share is present. | ||
final items = SystemContextMenu.getDefaultItems(state); | ||
expect(items.any((e) => e is IOSSystemContextMenuItemShare), isTrue); | ||
|
||
debugDefaultTargetPlatformOverride = null; | ||
}); | ||
} | ||
|
||
class _HostEditable extends StatelessWidget { | ||
const _HostEditable(); | ||
|
||
@override | ||
Widget build(BuildContext context) { | ||
return EditableText( | ||
controller: TextEditingController(), | ||
focusNode: FocusNode(), | ||
style: const TextStyle(fontSize: 14), | ||
cursorColor: Colors.blue, | ||
backgroundCursorColor: Colors.grey, | ||
// Use the system context menu pipeline on iOS. | ||
contextMenuBuilder: (BuildContext _, EditableTextState s) { | ||
return SystemContextMenu.editableText(editableTextState: s); | ||
}, | ||
); | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This test and the _HostEditable
helper widget have some issues with resource management and could be structured more clearly.
- Resource Leaks: The
TextEditingController
created on line 901 is never disposed. More importantly,_HostEditable
creates a newTextEditingController
andFocusNode
on every build, and these are not disposed, which leads to resource leaks in the test environment. This violates the best practices for writing effective tests mentioned in the style guide.1 - Test Clarity: The test creates a
controller
(line 901) but then modifies a different controller instance inside_HostEditable
viastate.widget.controller
(line 920). This is confusing and not a recommended pattern.
To address this, _HostEditable
should be converted to a StatefulWidget
to properly manage the lifecycle of its TextEditingController
and FocusNode
. This also allows simplifying the test by removing the unnecessary controller and directly setting the text and selection on the state.
Here's a suggested refactoring that fixes the leaks and improves clarity.
testWidgets('Default iOS SystemContextMenu includes Share for non-empty selection',
(WidgetTester tester) async {
// Force iOS + tell Flutter we support the iOS system context menu path.
debugDefaultTargetPlatformOverride = TargetPlatform.iOS;
addTearDown(() => debugDefaultTargetPlatformOverride = null);
await tester.pumpWidget(
const MediaQuery(
data: MediaQueryData(supportsShowingSystemContextMenu: true),
child: Directionality(
textDirection: TextDirection.ltr,
child: Material(
child: _HostEditable(), // minimal EditableText wrapper
),
),
),
);
// Setup selection so sharing is meaningful.
final state = tester.state<EditableTextState>(find.byType(EditableText));
state.updateEditingValue(const TextEditingValue(
text: 'Hello world',
selection: TextSelection(baseOffset: 0, extentOffset: 5), // "Hello"
));
state.widget.focusNode.requestFocus();
await tester.pump();
// Get iOS SYSTEM menu defaults and assert Share is present.
final items = SystemContextMenu.getDefaultItems(state);
expect(items.any((e) => e is IOSSystemContextMenuItemShare), isTrue);
});
}
class _HostEditable extends StatefulWidget {
const _HostEditable();
@override
State<_HostEditable> createState() => _HostEditableState();
}
class _HostEditableState extends State<_HostEditable> {
final TextEditingController _controller = TextEditingController();
final FocusNode _focusNode = FocusNode();
@override
void dispose() {
_controller.dispose();
_focusNode.dispose();
super.dispose();
}
@override
Widget build(BuildContext context) {
return EditableText(
controller: _controller,
focusNode: _focusNode,
style: const TextStyle(fontSize: 14),
cursorColor: Colors.blue,
backgroundCursorColor: Colors.grey,
// Use the system context menu pipeline on iOS.
contextMenuBuilder: (BuildContext _, EditableTextState s) {
return SystemContextMenu.editableText(editableTextState: s);
},
);
}
}
Style Guide References
Footnotes
@hellohuanlin Did we purposely not include Share, or is this ok to add? |
Looking at the existing code, looks like we already have "share" functionality in the engine. So it's just not added from framework side. I don't recall any issue with the share button, so should be fine to add. @muradhossin thanks for adding this. can you post a screenshot or gif in PR description? |
Thanks for the quick response @hellohuanlin! I guess I just forgot this. Let's add it. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some suggestions for the test, but otherwise thank you for noticing this and opening up a PR!
…dget, iOS variant, channel assert)
Demo video has been added.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM with a nit 👍 Thanks for fixing my comments and for adding the demo video.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
This PR fixes #173491 by adding the missing 'Share' option to the default iOS SystemContextMenu when
shareEnabled
is true.Changes:
IOSSystemContextMenuItemShare
togetDefaultItems
insystem_context_menu.dart
.Rationale:
This aligns Flutter's default iOS text selection context menu with native iOS behavior, ensuring users see the expected 'Share' option when selecting text.
Demo:
Video showing Share option in iOS context menu: https://github.com/user-attachments/assets/e04cd1f9-7d92-4147-a09b-719f03d9c625