iPad keyboard cut/copy/paste buttons #50923
Conversation
| // When scribble is available, the FlutterTextInputView will display the native toolbar unless | ||
| // these text editing actions are disabled. | ||
| if ([self isScribbleAvailable] && sender == NULL) { | ||
| return NO; |
There was a problem hiding this comment.
@fbcouch Any idea if this is still doing anything important?
There was a problem hiding this comment.
It might be worth checking on iPadOS 16 vs 17 just to be sure (I have an iPad that's only on 16, so I can test that one once this is built, if needed), but it may be that other things in the engine or OS have changed to prevent the native toolbar from showing up
There was a problem hiding this comment.
Thanks, I think I have an iPad with 17, I'll try it out tonight and post back.
There was a problem hiding this comment.
I think i looked into this but forgot - in what other cases will sender be nil too? during scribble it seems?
i vaguely remember when you enter memoji sender is nil, but could be wrong.
There was a problem hiding this comment.
I'm not sure, but yeah it looks like during Scribble. Flutter doesn't support selecting a memoji out of the box does it?
I forgot to link my response here (#50923 (comment)), but I did try this out on a physical iOS 17 iPad and it seemed to work fine. I tried all of the Scribble features I could think of, plus normal soft keyboard text editing, including the text selection toolbar.
There was a problem hiding this comment.
i vaguely remember when you enter memoji sender is nil, but could be wrong.
I think i remember it wrongly. memoji has nothing to do with this line.
Flutter doesn't support selecting a memoji out of the box does it?
Yes, but memoji is part of the keyboard that we don't control. So what we did was to make it a no-op when user choose their memoji. This is done with the pasteboard.hasStrings check below.
|
@justinmc Are we making progress on this? |
|
Sorry, meant to try this at home and forgot about it. Making a note to do so tonight. |
|
I've tried this out on a physical device and Scribble works fine. I'll take a look at the failing test, though. |
|
It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact "@test-exemption-reviewer" in the #hackers channel in Chat (don't just cc them here, they won't see it! Use Discord!). If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix? Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. |
| return pasteboard.hasStrings; | ||
| } | ||
| // Forbid pasting images, memojis, or other non-string content. | ||
| return [UIPasteboard generalPasteboard].string != nil; |
There was a problem hiding this comment.
Accessing the pasteboard string here was causing an unnecessary paste notification to show to the user when the keyboard was brought up. Using hasStrings instead fixes it. FYI @hellohuanlin.
| } | ||
| if (action == @selector(paste:)) { | ||
| UIPasteboard* pasteboard = [UIPasteboard generalPasteboard]; | ||
| if (@available(iOS 10, *)) { |
There was a problem hiding this comment.
iirc the minimum is already ios 11?
There was a problem hiding this comment.
According to this page we don't support 11 and under, you're right! I'll remove this.
|
@moffatman may know more about why we had this check in the first place. could you help to take a look? |
|
I don't know why the |
| } | ||
|
|
||
| - (BOOL)canPerformAction:(SEL)action withSender:(id)sender { | ||
| // When scribble is available, the FlutterTextInputView will display the native toolbar unless |
There was a problem hiding this comment.
what is the "native toolbar"? i assume it's not the edit menu?
There was a problem hiding this comment.
From my memory I think the system context menu (AKA text selection toolbar) was showing up? Maybe @fbcouch can confirm.
I did not see that happen just now when I tested Scribble on an iPad running iOS 17.
…144849) flutter/engine@e099277...953927e 2024-03-08 [email protected] Revert test if changes to golden tests (flutter/engine#51291) 2024-03-08 [email protected] iPad keyboard cut/copy/paste buttons (flutter/engine#50923) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-engine-flutter-autoroll Please CC [email protected],[email protected] on the revert to ensure that a human is aware of the problem. To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://issues.skia.org/issues/new?component=1389291&template=1850622 Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
5 participants
The cut/copy/paste buttons above the keyboard on iPad were always greyed out, and this PR enables them to work with the current text input as normal.
Fixes flutter/flutter#102940