-
Notifications
You must be signed in to change notification settings - Fork 6k
Conversation
I think it is expected behavior that when custom menus are used the browser will pop-up a permissions dialog. What may be surprising is if this becomes the default behavior on the web. Perhaps what we're struggling with is we want one concept of a context menu and we want it to work in all situations. I wonder if instead it makes sense to formalize the concepts of "platform context menu" and "custom context menu" at the framework level. By default the framework would choose the most appropriate strategy for the platform, but the developer may override based on their app's needs (e.g. Google Docs chooses to ask for permissions and use custom menus). On the web, the default would be the "platform context menu" that does not require permissions or attempt to access the Clipboard API. Other platforms may choose a different default. This is similar to how focus traversal works. On the web, when you reach the last focusable widget on the page, the next TAB will take you out of the page and into browser UI (back button, address bar, etc). But on other platforms there's no "browser UI", the Flutter app is the full app, and the focus traversal must stay within the app (but also customizable by the developer). |
@justinmc, sorry for the very late review. My general approach is to try to minimize requests for user's permission, especially for widgets readily available by the framework. App developers may be unaware that their use of such widget causes a permission dialog to popup on their web app. Based on that, I have a few suggestions:
|
Thanks for your input @mdebbar and @yjbanov! I think we should do the following:
The resulting behavior will be that on web by default, the paste button will always be shown, even when there is nothing to paste on the clipboard. Pressing paste for the first time will show the permissions dialog. Doing so with nothing on the clipboard will just paste nothing (default behavior on the non-Flutter web already). If a user wants to change the behavior by using hasStrings on web, that can do so by writing their own context menu. If this ends up being popular, I can make it easier by adding a parameter as suggested above, but I won't plan to for now. If this plan sounds good, I'd appreciate a review here and on flutter/flutter#132093 please! |
}); | ||
return; | ||
} | ||
_reportGetDataFailure(callback, codec, error); |
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.
The app is only checking whether the clipboard contains strings, it's not doing a paste. This error is going to be confusing in that case. It basically says that the "paste" operation has failed.
I suggest that we fail silently here and just return false
to the hasStrings
call. This covers the case when the user denies clipboard permission, in which case we should tell the app that there are no strings in the clipboard (basically treat the clipboard as if it were empty).
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.
Good call! Fixed and added a test.
By default, Flutter web uses the browser's built-in context menu. <img width="200" src="https://github.com/flutter/flutter/assets/389558/990f99cb-bc38-40f1-9e88-8839bc342da5" /> As of [recently](flutter/engine#38682), it's possible to use a Flutter-rendered context menu like the other platforms. ```dart void main() { runApp(const MyApp()); BrowserContextMenu.disableContextMenu(); } ``` But there is a bug (#129692) that the Paste button is missing and never shows up. <img width="284" alt="Screenshot 2023-08-07 at 2 39 03 PM" src="https://github.com/flutter/flutter/assets/389558/f632be25-28b1-4e2e-98f7-3bb443f077df"> The reason why it's missing is that Flutter first checks if there is any pasteable text on the clipboard before deciding to show the Paste button using the `hasStrings` platform channel method, but that was never implemented for web ([original hasStrings PR](#87678)). So let's just implement hasStrings for web? No, because Chrome shows a permissions prompt when the clipboard is accessed, and there is [no browser clipboard API](https://developer.mozilla.org/en-US/docs/Web/API/Clipboard_API) to avoid it. The prompt will show immediately when the EditableText is built, not just when the Paste button is pressed. <img width="200" src="https://github.com/flutter/flutter/assets/389558/5abdb160-1b13-4f1a-87e1-4653ca19d73e" /> ### This PR's solution Instead, before implementing hasStrings for web, this PR disables the hasStrings check for web. The result is that users will always see a paste button, even in the (unlikely) case that they have nothing pasteable on the clipboard. However, they will not see a permissions dialog until they actually click the Paste button. Subsequent pastes don't show the permission dialog. <details> <summary>Video of final behavior with this PR</summary> https://github.com/flutter/flutter/assets/389558/ed16c925-8111-44a7-99e8-35a09d682748 </details> I think this will be the desired behavior for the vast majority of app developers. Those that want different behavior can use hasStrings themselves, which will be implemented in flutter/engine#43360. ### References Fixes #129692 Engine PR to be merged after this: flutter/engine#43360
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.
…132508) flutter/engine@46c1191...56a0e27 2023-08-14 [email protected] Roll Skia from a690bd1fb8b8 to 1cf6f71c8120 (1 revision) (flutter/engine#44685) 2023-08-14 [email protected] hasStrings for web (flutter/engine#43360) 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],[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://bugs.chromium.org/p/skia/issues/entry?template=Autoroller+Bug Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
Support hasStrings on web, so that developers can check for clipboard content before showing the paste button, for example, if desired.
Awhile back, I added the
Clipboard.hasStrings
platform channel method to all of our platforms, except web, in order to support showing the Paste button only when there was pasteable content on the clipboard. I couldn't just usegetData
because some platforms show a permissions dialog when accessing clipboard data directly. Instead, those platforms offer alternative methods that just check whether there is some text on the clipboard, but don't actual read it.At the time, I did not implement
hasStrings
for web because the web didn't use Flutter's context menu, so there was never a Paste button to show. That changed with #38682, which offers a way to disable the browser's built-in context menu.Now if you disable the browser's context menu on web and use the Flutter-rendered context menu, the paste button will just never show up (flutter/flutter#129692). So I set out in this PR to add
hasStrings
to web and fix that issue.The problem
It turns out that there is no
hasStrings
-like method in the browser's clipboard API, but at least on Chrome, it still shows a permission notification whengetData
is accessed!Solution 1: Do nothing
Close this PR. The paste button will never show on web when the built-in context menu is disabled. But really, that menu is not exactly meant to work out of the box for web. Concerned app developers can implement their own custom context menu for their web app and do whatever behavior they want.
Solution 2: Ignore hasStrings on web
Also close this PR, and in the framework, just ignore the result of
hasStrings
on web. The paste button will always appear. If it's pressed when there is no string available, it will just not paste anything. It's easy for app developers to change this behavior if they want.Solution 3: Merge this PR
All Flutter web apps that disable the browser's context menu and don't implement their own custom context menu will show the above notification the first time it is run. If a user selects "allow", the paste button will appear only when there is something on the clipboard. If they select "block", the paste button will never appear. It's still easy for app developers to change this behavior if they want.
Resources
Fixes flutter/flutter#129692
@yjbanov @mdebbar I'm interested in your thoughts!