844371 - Work - Add "Cut" to the text field context menu

Status: RESOLVED FIXED

(Firefox for Metro Graveyard :: General, defect)

Description

[Yuan Wang(:Yuan) – Mobile Firefox Design Lead]

Currently the menu contains: 
- Copy
- Paste
- Search Bing for ".."

I don't think Search on Bing is an useful option in this context. Users' focus on creating/editing, so they don't want to be directed to a search page. Also, on desktop, there is no "Search google" for selected texts in text field.

We should change the menu to: 
- Cut
- Copy
- Paste

The most standard sequence.

If there is no text in the clipboard that can be pasted into text that the user can edit, the default context menu should be:
- Cut
- Copy

Comment 1

[Asa Dotzler [:asa]]

Yuan, presumably you are talking about context menus for selected text *in text areas* and not on regular web page text. If so, then yes I agree.

Comment 2

[Marco Mucci [:MarcoM]]

Thanks Asa.  I will move this Change Story to Iteration #3.  What story can I related it to?

Comment 3

[eyal gruss]

bug 565717, recently added search google for text area selections

Comment 4

[Asa Dotzler [:asa]]

(In reply to eyal gruss (eyaler) from comment #3)
> bug 565717, recently added search google for text area selections

Yeah. We don't need that in Metro. Thanks, though.

Comment 5

[Asa Dotzler [:asa]]

This is actually new work, tied to a new and previously missing story. I've cleaned that up. This work item now blocks the story for text field context menus.

Comment 6

[Rodrigo Silveira [:rsilveira] [:rodms]]

I noticed some strange behavior when copying/pasting using the context menu on text inputs.

1. Copy always copies the entire text content, even when there is text selected. This is very confusing and non-standard.
2. Copy option shows up on text fields even when you don't have anything selected.
3. Double-clicking with the mouse on a word selects it and shows the marker, but the markers are not draggable with the mouse.

In my opinion, 1 is a bug and for 2 we should only show copy/paste when something is selected. For 3 we should not show markers.

The fix I'm working on for this bug addresses 1 & 2. I can open a bug for 3.

Makes sense?

Comment 7

[Rodrigo Silveira [:rsilveira] [:rodms]]

Attachment: Add Cut to the text field context menu

Comment 8

[Matt Brubeck (:mbrubeck)]

Comment on attachment 722604 [details] [diff] [review]
Add Cut to the text field context menu

Review of attachment 722604 [details] [diff] [review]:
-----------------------------------------------------------------

r=mbrubeck with a couple minor changes (below).

::: browser/metro/base/content/ContextCommands.js
@@ +38,5 @@
> +    let target = ContextMenuUI.popupState.target;
> +    if (target.localName === "browser") {
> +      // content
> +      if (ContextMenuUI.popupState.string) {
> +        this.showToast(Strings.browser.GetStringFromName("selectionHelper.textCopied"));

This string isn't appropriate for Cut.  I think we can just leave out the toast.  (We'll probably get rid of it from "copy" too -- it's an Android-ism that doesn't really belong on Metro.  Check with Asa.)

@@ +47,5 @@
> +      }
> +    }
> +
> +    if (target)
> +      target.focus();

If "target" is null/undefined, then the code above will throw an exception.  Instead of "if (target)" here, let's put "if (!target) return;" up near the top of the function.

Comment 9

[Rodrigo Silveira [:rsilveira] [:rodms]]

Asa, do we need the toast for Cut/Copy? It currently says: "Text copied to clipboard"

Comment 10

[Asa Dotzler [:asa]]

No. Please(In reply to Rodrigo Silveira [:rsilveira] from comment #9)
> Asa, do we need the toast for Cut/Copy? It currently says: "Text copied to
> clipboard"

No. Please kill that. Thank you. I think those are all left over from fennec or something.

Comment 11

[Frank Yan (:fryn)]

Comment on attachment 722604 [details] [diff] [review]
Add Cut to the text field context menu

Review of attachment 722604 [details] [diff] [review]:
-----------------------------------------------------------------

You fixed "Cut" and "Copy" in the text-field case, but I think you broke "Copy" for the non-text-field case, i.e. right-click body text -> Copy.
I am okay with this regression, but please fix it in a followup bug as soon as possible.

Comment 12

[Rodrigo Silveira [:rsilveira] [:rodms]]

Attachment: Updated patch

Requesting review again, addressed review comments plus:
- Removed all clipboard related toasts,
- Added cut to chrome editable inputs (address bar),
- Fixed "copy text from body" regression (thanks fryn!)
- Added test for the regression

Comment 13

[Frank Yan (:fryn)]

Comment on attachment 723027 [details] [diff] [review]
Updated patch

Review of attachment 723027 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/metro/base/content/contenthandlers/ContextMenuHandler.js
@@ +179,5 @@
> +      } else {
> +        Util.dumpLn("error: target element does not support nsIDOMNSEditableElement");
> +      }
> +    } else {
> +      let { targetWindow: targetWindow } = this._translateToTopLevelWindow(this._target);

This fails inside iframes.

Why not fall back to the previous behavior of this.clipboard.copyString(ContextMenuUI.popupState.string, this.docRef) in the non-editable case?

Comment 14

[Rodrigo Silveira [:rsilveira] [:rodms]]

Attachment: Update to work on iframe

Updated patch to work on iframe, thanks :fryn for catching this.

Comment 15

[Frank Yan (:fryn)]

Comment on attachment 723668 [details] [diff] [review]
Update to work on iframe

Review of attachment 723668 [details] [diff] [review]:
-----------------------------------------------------------------

\o/

Thanks for working on this. :)

Comment 16

[Rodrigo Silveira [:rsilveira] [:rodms]]

Comment on attachment 723668 [details] [diff] [review]
Update to work on iframe

Can someone check this in please?

Comment 17

[Frank Yan (:fryn)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/75a92967f22c

Comment 18

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/75a92967f22c