<files-triage> This is related to  bug 762694 .

Alexey - this is P1 for M69, PTAL :)

weifangsun@, which pop-up needs to be removed?

Checked with omrilio@ on expected behavior - We should show the fuller menu (more left in the screenshot).

General guidance is the right menu is the default, but for apps which have the fuller menu, we can override.

The fuller menu is FilesApp specific and we are in control for that.
weifangsun@, can you find anybody (a component?) who is responsible for the left menu? (Any pointers in the code would be helpful). Thanks!

weifangsun@, also FYI: the "Fuller" menu isn't fuller. The left menu doesn't check dynamically if Paste operation is available. Paste appears in the right menu as well (if your clipboard is not empty and compatible). Therefore, I would leave a standard menu - it can be supported at OS-wide level. 

loyso@ - did you need help with the code? :) I believe the context menu you're looking for is "text-context-menu" in main.html.

Although, I do agree with your comment of using the OS-wide context menu, from a UI perspective it can appear inconsistent. So I think we should stick to using our own menu.

If it doesn't dynamically check if the Paste operation is available, we should fix that :)

I'm aware about the FilesApp's text-context-menu. Interested in the other menu which we have for every <input> HTML element in touch mode. Can't find any traces.

There is a confusion again: My proposal was to get rid of non-standard FilesApp menu in touch mode and stick with OS-wide menu.

Ah, not sure where that menu is :) But there should be a web flag to disable the context menu for a text area, e.g. document.addEventListener('contextmenu', event => event.preventDefault());

I understand your proposal :) But I think it can appear inconsistent from a UI perspective, since the rest of the Files App uses the other type of context menu.

With that said... Maybe touch users expect the same menu across text boxes. Is it more important that we have a consistent context menu across apps, or inside the app itself? Weifang, your thoughts? :)

The right menu is not web context menu, AFAIU. According to my attempts, at least.

Hmm. Adding UI>Input>Touch who might know what the touch context menu is.

o/ It's ui/views/touchui/touch_selection_menu_runner_views.cc :)

(I had a chat with Alexey) Note there are "useful" things added, or being added, to context menus for text editing.

Examples (all platforms):
 - chrome://flags/#enable-emoji-context-menu
 - The "Change writing direction" option (standard on Mac, and proposed for textfields everywhere)
 - Speak selection aloud

I think those apply equally to editing a filename. On mac there is also "services" integration and dictionary lookup, and in some contexts there is also spell check (Those are perhaps less relevant for files.)

But basically I agree with Alexey that apps shouldn't attempt to re-implement this context menu to provide their own cut/copy/paste. As it is, it seems worse for supporting i18n and a11y, without giving any value-add, and could miss out on other cool stuff.

Thank you so much, tapted@!
Yes, if I return false in TouchSelectionControllerClientAura::IsCommandIdEnabled then the right menu also disappears.
There is no way to opt-out.

FilesApp also has two context menus in touch mode when creating/renaming folder/file...
Probably, there is more cases. 

It's funny that you have "Zip Selection" in your context menu if you are renaming file/folder and you have input text selection...

The Files app specific context menu which would be relevant for this use case is the one that is associated with the rename/selection of a file name (screenshot attached) as opposed to the other more extensive context menus that Files app supports. Not dynamically checking Paste seems to be a bug. :)

On whether to use the Files app context menu or a more general touch context menu - guidance was to use the app-specific context menu if it exists. +omrilio@ - Can you confirm this is the approach we are taking for tablets? I think both options are functional, so my priority would be to maintain UX consistency across the system on what behavior we use.

Deleted: Screenshot 2018-05-30 at 11.55.01 AM.png 2.6 MB

	Screenshot 2018-05-30 at 11.55.01 AM.png 2.6 MB View Download

It may be worth treating the text-edit context menu separately from other context menus in the files app.

However, IMO, It feels a bit clunky to use a WebContents context menu for a system app regardless. I filed Issue 848105 . There are many shortcomings when compared to a "real" context menu, and we have the power to do whatever we want with the one provided by the renderer -- most of the plumbing is already in place.

I can't repro this on M67 Eve, or on my local ToT with --force-tablet-mode=touch_view. Am I missing something? Can we do a bisect to see what introduced this context menu?

Deleted: right-click.png 97.5 KB

	right-click.png 97.5 KB View Download

Re comment #25: the goal is to use the best menu at any given moment. Our assumption is that if an app has built its own menu, it is likely better than our simple clipboard menu. So in this case, the Files app context menu should take over the clipboard one.

This is a general problem which I wish we had a global solution for that apps could simply disable the clipboard menu when they have their own menu (e.g. of another use-case is for Google Docs).

> Our assumption is that if an app has built its own menu, it is likely better than our simple clipboard menu
What if app menu is 10 years old?

> This is a general problem
A solution: https://bugs.chromium.org/p/chromium/issues/detail?id=848105 proposes combined menus, AFAIU.

Overall, in Tablet Mode we have two context menus:
1) TouchSelectionController menu which supports touch text selection
2) FilesApp menu which doesn't support touch text selection.
I don't understand logic why menu 2) is preferred.

For some reason the initial screenshot from #c0 doesn't show blue start/end touch handles for touch text selection (?).

It's worth to clarify that those blue touch handles are a part of TouchSelectionController menu (the right one, OS menu). The handles participate in touch selection UX process. If we remove system context menu, we also remove those blue handles.

mohsen@chromium.org, please correct me if I'm wrong.

+weifang/omrilio, after reproing the bug I believe we should be disabling the File Manager context menu in this textbox, and keeping the native one. Let me explain why:

The menu shown in the screenshot is not actually a context menu. It's a 'text selection' menu that appears when you have any text selected in tablet mode. It also displays little blue handlebars next to the selected text that allow you to drag the current selection to be larger or smaller. If we disable this, we lose the handles, which also means we lose the ability for touch users to control selecting text (you can try this yourself in the omnibox by long-pressing to select text).

Also, the file manager context menu shouldn't appear for text boxes. It's designed to appear for files in the file manager list, and we should keep it for that. Try right-clicking on any text box in the browser - none of these options appear when in a File Manager text box (which includes extensions), and they should. If we want to hide these, we can keep our custom context menu for text boxes in non-touch mode, since the user has a mouse/keyboard so we don't need it to allow them to select text. In touch mode it's important that we have support for dragging a selection (with the blue handles), so we should keep it. This is consistent with the way the other md pages work, eg bookmark manager.

loyso: The way we can hide the File Manager context menu for touch devices is like this (this also matches the way md_bookmarks implements this):

1. When any touch event occurs, set a global variable e.g. '_lastInteractionWasTouchEvent' to true. This is NOT the same as being 'in touch mode' (e.g. a tablet device), since you can have a regular device with a touchscreen and we still need to hide the context menu in this mode.
2. Just before displaying a context menu for a text box, check if this variable is true. If it is, don't display the context menu.
3. When any mouse or keyboard event occurs, set this variable back to true.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/4325366d69502145fb6ea7cf4137976f18c76e56

commit 4325366d69502145fb6ea7cf4137976f18c76e56
Author: Alexey Baskakov <loyso@chromium.org>
Date: Thu Jun 21 00:51:41 2018

ChromeBrowserMainExtraPartsAsh: Move tablet_mode_client_.reset() to dtor

ChromeBrowserMainParts::PostMainMessageLoopRun() calls
ChromeBrowserMainExtraPartsAsh::PostMainMessageLoopRun() to destroy all
Ash objects before
BrowserProcessImpl::StartTearDown().

Extensions perform shutdown in BrowserProcessImpl::StartTearDown().

This change allows extensions to remove observers from TabletModeClient.

Bug:  812056 
Change-Id: I0c96a9cee72c01ab0e4122ee811eb0535e80e7d2
Reviewed-on: https://chromium-review.googlesource.com/1107031
Commit-Queue: Alexey Baskakov <loyso@chromium.org>
Reviewed-by: James Cook <jamescook@chromium.org>
Reviewed-by: Scott Violet <sky@chromium.org>
Cr-Commit-Position: refs/heads/master@{#569113}
[modify] https://crrev.com/4325366d69502145fb6ea7cf4137976f18c76e56/chrome/browser/ui/ash/chrome_browser_main_extra_parts_ash.cc

Interesting use case:
(1) Put device in clamshell mode.
(2) Save a file from the browser
(3) The "Save as" modal window opens with "Filename" input element owning the focus (the bold underline)
(4) Make a text selection and show a context menu
(5) Put device in tablet mode while "Filename" has a focus (!)
(6) User shouldn't observe double menus

Re #c34:
> 3. When any mouse or keyboard event occurs, set this variable back to true.
It won't work. When the user touches the screen both touch and click events will occur.
I'll check how md_bookmarks on all desktops works.

Hi all - Thanks for all the investigation on this one. Checked in with omrilio@ and we agree that there's a solid argument here for keeping the native context menu.

In general, we know we have work to do on the Files context menu so will make sure we track this one in our backlog for PE :)

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/b017844723c4de4d2036b8c78cd2f3bc7b0efb34

commit b017844723c4de4d2036b8c78cd2f3bc7b0efb34
Author: Alexey Baskakov <loyso@chromium.org>
Date: Tue Jun 26 08:07:21 2018

cros FilesApp: Disable context menus for input elements on touch events.

Use touchstart as a criterion to stop contextmenu propagation.

Bug:  812056 
Cq-Include-Trybots: luci.chromium.try:closure_compilation
Change-Id: I2d6272b08c0f1ad8229fe340825607e8ef6df007
Reviewed-on: https://chromium-review.googlesource.com/1109668
Commit-Queue: Alexey Baskakov <loyso@chromium.org>
Reviewed-by: Noel Gordon <noel@chromium.org>
Reviewed-by: calamity <calamity@chromium.org>
Cr-Commit-Position: refs/heads/master@{#570347}
[modify] https://crrev.com/b017844723c4de4d2036b8c78cd2f3bc7b0efb34/chrome/browser/chromeos/file_manager/file_manager_browsertest.cc
[modify] https://crrev.com/b017844723c4de4d2036b8c78cd2f3bc7b0efb34/ui/file_manager/file_manager/background/js/test_util_base.js
[modify] https://crrev.com/b017844723c4de4d2036b8c78cd2f3bc7b0efb34/ui/file_manager/file_manager/foreground/js/file_manager.js
[modify] https://crrev.com/b017844723c4de4d2036b8c78cd2f3bc7b0efb34/ui/file_manager/integration_tests/file_manager/context_menu.js