Last Comment Bug 844371 - Work - Add "Cut" to the text field context menu
: Work - Add "Cut" to the text field context menu
Status: RESOLVED FIXED
feature=work
:
Product: Firefox for Metro
Classification: Client Software
Component: General (show other bugs)
: Trunk
: x86 Windows 8.1
: -- normal (vote)
: Firefox 22
Assigned To: Rodrigo Silveira [:rsilveira] [:rodms]
: juan becerra [:juanb]
Mentors:
Depends on:
Blocks: 844658
  Show dependency treegraph
 
Reported: 2013-02-22 17:29 PST by Yuan Wang(:Yuan) – Mobile Firefox Design Lead
Modified: 2014-07-24 11:06 PDT (History)
6 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
Add Cut to the text field context menu (15.35 KB, patch)
2013-03-07 18:15 PST, Rodrigo Silveira [:rsilveira] [:rodms]
mbrubeck: review+
fryn: review+
Details | Diff | Review
Updated patch (23.12 KB, patch)
2013-03-08 18:00 PST, Rodrigo Silveira [:rsilveira] [:rodms]
mbrubeck: review+
fryn: review-
Details | Diff | Review
Update to work on iframe (24.18 KB, patch)
2013-03-11 15:40 PDT, Rodrigo Silveira [:rsilveira] [:rodms]
fryn: review+
fryn: checkin+
Details | Diff | Review

Description Yuan Wang(:Yuan) – Mobile Firefox Design Lead 2013-02-22 17:29:47 PST
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] 2013-02-23 16:54:13 PST
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] 2013-02-23 16:57:58 PST
Thanks Asa.  I will move this Change Story to Iteration #3.  What story can I related it to?
Comment 3 eyal gruss (eyaler) 2013-02-24 07:49:42 PST
bug 565717, recently added search google for text area selections
Comment 4 Asa Dotzler [:asa] 2013-02-24 13:56:21 PST
(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] 2013-02-24 14:00:40 PST
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] 2013-03-07 14:32:19 PST
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] 2013-03-07 18:15:23 PST
Created attachment 722604 [details] [diff] [review]
Add Cut to the text field context menu
Comment 8 Matt Brubeck (:mbrubeck) 2013-03-07 21:11:05 PST
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] 2013-03-08 10:51:42 PST
Asa, do we need the toast for Cut/Copy? It currently says: "Text copied to clipboard"
Comment 10 Asa Dotzler [:asa] 2013-03-08 12:56:34 PST
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) 2013-03-08 14:13:38 PST
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] 2013-03-08 18:00:18 PST
Created attachment 723027 [details] [diff] [review]
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) 2013-03-11 13:29:45 PDT
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] 2013-03-11 15:40:28 PDT
Created attachment 723668 [details] [diff] [review]
Update to work on iframe

Updated patch to work on iframe, thanks :fryn for catching this.
Comment 15 Frank Yan (:fryn) 2013-03-12 08:20:24 PDT
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] 2013-03-12 13:41:39 PDT
Comment on attachment 723668 [details] [diff] [review]
Update to work on iframe

Can someone check this in please?
Comment 18 Ed Morley [:emorley] 2013-03-13 05:37:43 PDT
https://hg.mozilla.org/mozilla-central/rev/75a92967f22c

Note You need to log in before you can comment on or make changes to this bug.