Paste shows only when content on clipboard #54902

justinmc · 2020-04-15T22:07:42Z

Description

On native, the paste button in the text selection menu only shows when there is something to paste on the clipboard. This PR adds the same functionality for Flutter.

There was a long standing TODO in the code about making the canPaste method consider what's on the clipboard. I did not take this approach, because checking the clipboard is asynchronous and canPaste is used synchronously.

Instead, the text selection menus check the clipboard asynchronously when they're rendered and rerender if needed. The clipboard is also checked when the app is resumed. This results in the selection menu always being up to date, even if it's sometimes 1 frame late. The only way I can find to trick it is by clearing the clipboard while the selection menu is still visible using the command line. This shouldn't be possible in a production app, only while running an emulator.

Related Issues

#4355

Tests

Check the appearance of the Paste button when there is and is not content on the clipboard.

goderbauer · 2020-04-15T23:37:16Z

(haven't reviewed the code, but two questions from just reading the description)

doesn't this cause visual glitches if for one frame we render the paste button and then remove it? Could we maybe just delay showing the toolbar until we know whether it's enabled or not?
does this also disable the paste option in TalkBack's local context menu if the clipboard is empty? (Can't remember if this already worked before this change)

LongCatIsLooong · 2020-04-15T23:47:07Z

packages/flutter/lib/src/cupertino/text_selection.dart

+  void _updateClipboardStatus() {
+    Clipboard.getData(Clipboard.kTextPlain).then((ClipboardData value) {
+      final bool isClipboardEmpty = value?.text == null || value.text.isEmpty;
+      if (isClipboardEmpty != _isClipboardEmpty) {


Should we check if the widget is still mounted when the future completes?

Yes, good call.

LongCatIsLooong · 2020-04-15T23:50:14Z

packages/flutter/lib/src/cupertino/text_selection.dart

+
+  @override
+  void didChangeAppLifecycleState(AppLifecycleState state) {
+    if (state == AppLifecycleState.resumed) {


nit: use switch for enums per the style guide.

LongCatIsLooong · 2020-04-15T23:58:54Z

packages/flutter/lib/src/cupertino/text_selection.dart

+  @override Widget build(BuildContext context) {
+    // Asynchronously check if the clipboard has changed, and trigger another
+    // build if so.
+    if (widget.handlePaste != null) {


nit: build should be idempotent.

Yeah I worried about this when I wrote it. I'm going to move it into didChangeDependencies, which is still probably not the best, but I think it's necessary in this case.

Edit: didChangeDependencies, not didUpdateWidget.

Is it possible to do this in initState?

I was worried about the menu getting out of date with the clipboard status, but it seems not to really happen in actual use cases, and it doesn't break any tests. I'll change it to initState.

justinmc · 2020-04-16T23:11:05Z

@goderbauer There don't seem to be any visual glitches, and I think it's because of the fade-in animation. If I don't render the toolbar until I get the content from the clipboard, then many tests fail (over 100) because they're expecting a Paste button 1 frame after tapping. These failures seem fixable by changing pump topumpAndSettle in these cases. Let me know if think it's better to go with that and update the pumps, or if you have another idea.

About TalkBack, I just tried it and it seemed to work. TalkBack highlighted and spoke the visible toolbar buttons. When paste wasn't there, it wasn't usable.

goderbauer · 2020-04-20T18:19:11Z

I would prefer updating the test. Now the tests seem just wrong: They are expecting a Paste button that may actually not become visible to the user.

justinmc · 2020-04-22T17:47:55Z

@goderbauer @LongCatIsLooong Updated to only render when clipboard data is available. I changed many tests to use pumpAndSettle when looking for the selection menu. I agree that this makes more sense!

LongCatIsLooong · 2020-04-22T21:02:35Z

packages/flutter/lib/src/material/text_selection.dart

@@ -66,33 +70,88 @@ class _TextSelectionToolbarState extends State<_TextSelectionToolbar> with Ticke
    );
  }

+  // Asynchronously check if the clipboard has data and update
+  // _isClipboardEmpty if needed.
+  void _updateClipboardStatus() {


nit: should this check if widget.handlePaste is not null first, in case the information isn't needed at all?

+1 (see my comment above)

LongCatIsLooong · 2020-04-22T21:12:11Z

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

  ///
  /// Subclasses can use this to decide if they should expose the paste
  /// functionality to the user.
+  ///
+  /// This does not consider the current contents of the clipboard.


nit: could you elaborate a bit more on this? It confused me a little on the first read-through.

LongCatIsLooong · 2020-04-22T21:29:19Z

packages/flutter/lib/src/cupertino/text_selection.dart

+  void didChangeAppLifecycleState(AppLifecycleState state) {
+    switch (state) {
+      case AppLifecycleState.resumed:
+        _updateClipboardStatus();


Is it possible to control-c when the toolbar is visible?

That's a good point. I wasn't able to achieve this in the simulator, but maybe on desktop it will be possible, or on a mobile device with a physical keyboard attached.

I wish there was a Clipboard listener. I'm not sure how to handle this case if it is possible.

My macbook seems to close the native context menu if I ctrl-C, so maybe we can do that if it comes up on desktop.

LongCatIsLooong

LGTM

LongCatIsLooong · 2020-04-22T21:30:42Z

packages/flutter/lib/src/material/text_selection.dart

  }

  @override
  Widget build(BuildContext context) {
+    // Don't render the menu until the state of the clipboard is known.
+    if (widget.handlePaste != null && _isClipboardEmpty == null) {
+      return const SizedBox(width: 0.0, height: 0.0);


goderbauer · 2020-04-22T22:43:56Z

packages/flutter/lib/src/cupertino/text_selection.dart

+  // Whether the user's system clipboard is empty as of the last check.
+  bool _isClipboardEmpty;
+
+  // Asynchronously check if theh clipboard has data and update


Suggested change

// Asynchronously check if theh clipboard has data and update

// Asynchronously check if the clipboard has data and update

goderbauer · 2020-04-22T22:44:48Z

packages/flutter/lib/src/cupertino/text_selection.dart

+        if (isClipboardEmpty == _isClipboardEmpty) {
+          return;
+        }
+        _isClipboardEmpty = isClipboardEmpty;


move everything except for this line outside of the setState? (Only this line is actually setting state).

I moved everything inside because I was getting weird test failures when the widget would unmount, but I don't see anything like that now. I'll move it outside.

goderbauer · 2020-04-22T22:45:57Z

packages/flutter/lib/src/cupertino/text_selection.dart

+      case AppLifecycleState.resumed:
+        _updateClipboardStatus();
+        break;
+      default:


nit: we try to avoid default cases. That way, every time we add a new value to the enum the analyzer tells us all the places in the code base where we have to consider the new value.

Thanks, that's smart, I didn't know that.

goderbauer · 2020-04-22T22:46:33Z

packages/flutter/lib/src/cupertino/text_selection.dart

+    super.dispose();
+  }
+
+  @override Widget build(BuildContext context) {


nit: stange formatting

goderbauer · 2020-04-22T22:47:50Z

packages/flutter/lib/src/cupertino/text_selection.dart

+    WidgetsBinding.instance.addObserver(this);
+    // Asynchronously check if the clipboard has changed, and trigger another
+    // build if so.
+    _updateClipboardStatus();


should we only do this if widget.handlePaste is set to something? (here and elsewhere where we call it)

(plus we'd need to do it in didUpdateWidget if it changes)

Good call, I'm going to change this to check if widget.handlePaste is set and do this in didUpdateWidget as well. You can actually cause a bug without this change by opening the menu with nothing copied, copying something while the menu is open, and then tapping Select All to make the menu rebuild. At least in the simulator it's possible.

goderbauer · 2020-04-22T22:53:37Z

packages/flutter/lib/src/material/text_selection.dart

@@ -66,33 +70,88 @@ class _TextSelectionToolbarState extends State<_TextSelectionToolbar> with Ticke
    );
  }

+  // Asynchronously check if the clipboard has data and update
+  // _isClipboardEmpty if needed.
+  void _updateClipboardStatus() {


+1 (see my comment above)

goderbauer · 2020-04-22T22:55:54Z

packages/flutter/lib/src/material/text_selection.dart

@@ -49,7 +50,10 @@ class _TextSelectionToolbar extends StatefulWidget {
  _TextSelectionToolbarState createState() => _TextSelectionToolbarState();
 }

-class _TextSelectionToolbarState extends State<_TextSelectionToolbar> with TickerProviderStateMixin {
+class _TextSelectionToolbarState extends State<_TextSelectionToolbar> with TickerProviderStateMixin, WidgetsBindingObserver {


Some of the comments I made on the Cupertino impl also apply here.

justinmc · 2020-04-23T00:06:14Z

Code review changes made, thanks for the comments.

goderbauer · 2020-04-23T16:16:58Z

packages/flutter/lib/src/cupertino/text_selection.dart

+  @override
+  void didChangeAppLifecycleState(AppLifecycleState state) {
+    switch (state) {
+      case AppLifecycleState.resumed:


You will have to list all other cases as no-ops.

goderbauer · 2020-04-23T16:19:52Z

Did you have a chance to check how TalkBack behaves? I think it is showing a paste option in its local context menu. Does the operating system auto-hide that if there is nothing to paste? Or do we need to hide the this pate action from the text field in that case:

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

Line 2007 in 240611b

onPaste: _semanticsOnPaste(controls),

(Same question for pasting with iOS and VoiceOver).

justinmc · 2020-04-23T19:36:55Z

@goderbauer I didn't realize there was a separate context menu for TalkBack! I thought you just meant how TalkBack read out our context menu. It seems that yes, the native TalkBack context menu does hide/show paste based on the content of the clipboard. I think I'll have to add this isClipboardEmpty state to EditableText unfortunately.

goderbauer · 2020-04-23T20:12:54Z

It seems that yes, the native TalkBack context menu does hide/show paste based on the content of the clipboard.

Does it do that based on what's actually in the clipboard, or do we need to tell TalkBack to show/hide the option?

justinmc · 2020-04-23T21:05:48Z

It looks like we need to tell it. If I comment out the onPaste handler in the Semantics widget in EditableText, then it doesn't show the paste option. Otherwise it does, even if nothing is on the clipboard.

justinmc · 2020-04-23T22:27:41Z

@goderbauer I added the same clipboard checking logic to EditableText. Is there a nice way to abstract this code somehow?

goderbauer · 2020-04-27T17:42:32Z

Could we implement a class "ClipBoardStatus" which is a Listenable. The listenable notifies its listeners whenever the clipboard status changes (Textfield and the Toolbar would listen to that). It provides two getters: isPasteStatusPending and canPaste. The first one returns true while we wait for the Clipboard service to give us an answer. The second one returns true when there is data to paste in the clipboard. The class adds itself as a WidgetObserver when the first listener is added and removed its observer when the last listener is removed. This new class can then encapsulate all the logic that's currently duplicated in multiple places. You could even pass this object from the textfield to the Toolbar widget to avoid multiple instances. WDYT?

BTW, all names used in the paragraph above are just placeholders. Feel free to pick better names.

goderbauer · 2020-04-27T17:34:40Z

packages/flutter/lib/src/cupertino/text_selection.dart

+      case AppLifecycleState.detached:
+        break;
+      case AppLifecycleState.inactive:
+        break;
+      case AppLifecycleState.paused:
+        break;


nit: this can be more compact.

Suggested change

case AppLifecycleState.detached:

break;

case AppLifecycleState.inactive:

break;

case AppLifecycleState.paused:

break;

case AppLifecycleState.detached:

case AppLifecycleState.detached:

case AppLifecycleState.paused:

// Nothing to do.

goderbauer · 2020-04-27T17:36:40Z

packages/flutter/lib/src/material/text_selection.dart

+          _updateClipboardStatus();
+        }
+        break;
+      case AppLifecycleState.detached:


skia-gold · 2020-04-30T00:35:25Z

Gold has detected one or more untriaged digests on patchset 29.
Please triage them at https://flutter-gold.skia.org/search?issue=54902.

fluttergithubbot · 2020-04-30T01:09:16Z

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

Changes to golden files are considered breaking changes, so consult Handling Breaking Changes to proceed. While there are exceptions to this rule, if this patch modifies an existing golden file, it is probably not an exception. Only new golden file tests, or downstream changes like those from skia updates are considered non-breaking.

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 #54902 at sha 54a72e9ed0ec4f23ed5c2c15526b85c37f2f6246

goderbauer

LGTM

goderbauer · 2020-05-04T18:12:28Z

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

@@ -1155,11 +1156,17 @@ class EditableTextState extends State<EditableText> with AutomaticKeepAliveClien
  @override
  bool get selectAllEnabled => widget.toolbarOptions.selectAll;

+  // Inform the widget that the value of clipboardStatus has changed.


nit: move this comment also inside the set state to explain why the empty set state is OK here.

Missed that, thanks.

justinmc · 2020-05-07T18:47:19Z

Just had to squash all the commits in order to fix a failure.

This reverts commit 053c388.

…4902)" (flutter#56614)" This reverts commit cd67da2.

Unreverts #54902 with fixes for a failing integration test.

justinmc added a: text input Entering text in a text field or keyboard related problems framework flutter/packages/flutter repository. See also f: labels. labels Apr 15, 2020

justinmc self-assigned this Apr 15, 2020

fluttergithubbot added f: cupertino flutter/packages/flutter/cupertino repository f: material design flutter/packages/flutter/material repository. labels Apr 15, 2020

googlebot added the cla: yes label Apr 15, 2020

justinmc requested review from goderbauer and LongCatIsLooong April 15, 2020 22:31

LongCatIsLooong reviewed Apr 16, 2020

View reviewed changes

LongCatIsLooong reviewed Apr 22, 2020

View reviewed changes

LongCatIsLooong approved these changes Apr 22, 2020

View reviewed changes

LongCatIsLooong reviewed Apr 22, 2020

View reviewed changes

goderbauer reviewed Apr 22, 2020

View reviewed changes

goderbauer reviewed Apr 23, 2020

View reviewed changes

goderbauer reviewed Apr 27, 2020

View reviewed changes

goderbauer approved these changes May 4, 2020

View reviewed changes

justinmc mentioned this pull request May 7, 2020

Paste shows only when content on clipboard (reopened) #56571

Closed

justinmc removed c: API break Backwards-incompatible API changes will affect goldens Changes to golden files labels May 7, 2020

Apply whole pasteable branch to get around weird vesion test failures

5c9bc47

justinmc force-pushed the pasteable branch from 6b6012f to 5c9bc47 Compare May 7, 2020 18:46

Forgot to skip context menu test on web

722ad5a

justinmc added the waiting for tree to go green label May 7, 2020

fluttergithubbot merged commit 053c388 into flutter:master May 7, 2020

justinmc deleted the pasteable branch May 7, 2020 23:29

justinmc added a commit to justinmc/flutter that referenced this pull request May 8, 2020

Revert "Paste shows only when content on clipboard (flutter#54902)"

9a0d374

This reverts commit 053c388.

justinmc mentioned this pull request May 8, 2020

Revert "Paste shows only when content on clipboard (#54902)" #56614

Merged

jmagman pushed a commit that referenced this pull request May 8, 2020

Revert "Paste shows only when content on clipboard (#54902)" (#56614)

cd67da2

This reverts commit 053c388.

justinmc added a commit to justinmc/flutter that referenced this pull request May 8, 2020

Revert "Revert "Paste shows only when content on clipboard (flutter#5…

f657434

…4902)" (flutter#56614)" This reverts commit cd67da2.

justinmc mentioned this pull request May 8, 2020

Bring back paste button hide behavior #56689

Merged

justinmc added a commit that referenced this pull request May 8, 2020

Bring back paste button hide behavior (#56689)

d563498

Unreverts #54902 with fixes for a failing integration test.

This was referenced May 8, 2020

Copy/Paste menu should disable Paste if clipboard is empty #4355

Closed

Bring back paste button hide behavior 2 #56922

Merged

Bring back paste button hide behavior 3 #57139

Merged

justinmc mentioned this pull request May 26, 2020

TextFormField/EditableText widget crashes due to missing clipboard #57952

Closed

This was referenced Jun 23, 2020

Unsolicited iOS14 clipboard permission prompt #60145

Closed

Don't access clipboard passively on iOS #60316

Merged

github-actions bot locked as resolved and limited conversation to collaborators Jul 31, 2021

	// Asynchronously check if theh clipboard has data and update
	// Asynchronously check if the clipboard has data and update

Paste shows only when content on clipboard #54902

Paste shows only when content on clipboard #54902

Uh oh!

Conversation

justinmc commented Apr 15, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Related Issues

Tests

Uh oh!

goderbauer commented Apr 15, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

justinmc Apr 16, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

justinmc commented Apr 16, 2020

Uh oh!

goderbauer commented Apr 20, 2020

Uh oh!

justinmc commented Apr 22, 2020

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!

LongCatIsLooong left a comment

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!

justinmc commented Apr 23, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

goderbauer commented Apr 23, 2020

Uh oh!

justinmc commented Apr 15, 2020 •

edited

Loading

goderbauer commented Apr 15, 2020 •

edited

Loading

justinmc Apr 16, 2020 •

edited

Loading

goderbauer commented Apr 27, 2020 •

edited by HansMuller

Loading