Skip to content

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

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

muradhossin
Copy link

@muradhossin muradhossin commented Aug 12, 2025

This PR fixes #173491 by adding the missing 'Share' option to the default iOS SystemContextMenu when shareEnabled is true.

Changes:

  • Added IOSSystemContextMenuItemShare to getDefaultItems in system_context_menu.dart.
  • Added a widget test to ensure Share is present in the default items for non-empty selections on iOS.

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

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.
@github-actions github-actions bot added the framework flutter/packages/flutter repository. See also f: labels. label Aug 12, 2025
Copy link
Contributor

@gemini-code-assist gemini-code-assist bot left a 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.

Comment on lines 899 to 951
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);
},
);
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

medium

This test and the _HostEditable helper widget have some issues with resource management and could be structured more clearly.

  1. Resource Leaks: The TextEditingController created on line 901 is never disposed. More importantly, _HostEditable creates a new TextEditingController and FocusNode 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
  2. Test Clarity: The test creates a controller (line 901) but then modifies a different controller instance inside _HostEditable via state.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

  1. Tests should follow the guidance in the 'writing effective tests guide', which includes proper management of resources like controllers and focus nodes to avoid leaks. (link)

@justinmc
Copy link
Contributor

@hellohuanlin Did we purposely not include Share, or is this ok to add?

@hellohuanlin
Copy link
Contributor

@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?

@justinmc
Copy link
Contributor

justinmc commented Aug 12, 2025

Thanks for the quick response @hellohuanlin! I guess I just forgot this. Let's add it.

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.

Some suggestions for the test, but otherwise thank you for noticing this and opening up a PR!

@muradhossin
Copy link
Author

Demo video has been added.

  • Removed debugDefaultTargetPlatformOverride (confirmed not present in the file).
  • Removed _HostEditable; inlined the widget tree directly in the test.
  • No extraneous controller/focusNode usage — only a controller is used, and it’s disposed via addTearDown.
  • Dropped unused cursorColor / backgroundCursorColor parameters.
  • Used descriptive parameter names in contextMenuBuilder.
  • Test now listens to ContextMenu.showSystemContextMenu and asserts that a Share item is present.
  • No platform override resets are required.

@muradhossin muradhossin requested a review from justinmc August 20, 2025 18:14
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 a nit 👍 Thanks for fixing my comments and for adding the demo video.

Copy link
Contributor

@ValentinVignal ValentinVignal left a comment

Choose a reason for hiding this comment

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

LGTM

@muradhossin muradhossin requested a review from justinmc August 21, 2025 03:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
framework flutter/packages/flutter repository. See also f: labels.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[iOS] iOS system context menu is missing the "Share" button
4 participants