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

RESOLVED FIXED in Firefox 22

Status

Firefox for Metro
General
RESOLVED FIXED
4 years ago
3 years ago

People

(Reporter: Yuan, Assigned: rsilveira)

Tracking

Trunk
Firefox 22
x86
Windows 8.1

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: feature=work )

Attachments

(1 attachment, 2 obsolete attachments)

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
Whiteboard: [metro-mvp?]

Updated

4 years ago
Blocks: 841214
Flags: needinfo?(asa)

Comment 1

4 years ago
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.
Flags: needinfo?(asa)
Thanks Asa.  I will move this Change Story to Iteration #3.  What story can I related it to?

Updated

4 years ago
Blocks: 839392
No longer blocks: 841214

Updated

4 years ago
Priority: -- → P1

Updated

4 years ago
Whiteboard: [metro-mvp?] → feature=change c=tbd u=tbd p=0

Comment 3

4 years ago
bug 565717, recently added search google for text area selections

Comment 4

4 years ago
(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.

Updated

4 years ago
Blocks: 844658

Updated

4 years ago
Priority: P1 → --
Summary: Change - Add "Cut" and remove "Search Bing" on text clipboard context menu → Work - Add "Cut" and remove "Search Bing" on text clipboard context menu
Whiteboard: feature=change c=tbd u=tbd p=0 → feature=work

Comment 5

4 years ago
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.
No longer blocks: 839392

Updated

4 years ago
QA Contact: jbecerra

Updated

4 years ago
Assignee: nobody → fyan
Status: NEW → ASSIGNED

Updated

4 years ago
Summary: Work - Add "Cut" and remove "Search Bing" on text clipboard context menu → Work - Add "Cut" to the text field context menu

Updated

4 years ago
Assignee: fyan → rsilveira
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?
Created attachment 722604 [details] [diff] [review]
Add Cut to the text field context menu
Attachment #722604 - Flags: review?(mbrubeck)
Attachment #722604 - Flags: review?(fyan)
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.
Attachment #722604 - Flags: review?(mbrubeck) → review+
Asa, do we need the toast for Cut/Copy? It currently says: "Text copied to clipboard"
Flags: needinfo?(asa)

Comment 10

4 years ago
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.
Flags: needinfo?(asa)

Comment 11

4 years ago
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.
Attachment #722604 - Flags: review?(fyan) → review+
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
Attachment #722604 - Attachment is obsolete: true
Attachment #723027 - Flags: review?(mbrubeck)
Attachment #723027 - Flags: review?(fyan)
Attachment #723027 - Flags: review?(mbrubeck) → review+

Comment 13

4 years ago
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?
Attachment #723027 - Flags: review?(fyan) → review-
Created attachment 723668 [details] [diff] [review]
Update to work on iframe

Updated patch to work on iframe, thanks :fryn for catching this.
Attachment #723027 - Attachment is obsolete: true
Attachment #723668 - Flags: review?(fyan)

Comment 15

4 years ago
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. :)
Attachment #723668 - Flags: review?(fyan) → review+
Comment on attachment 723668 [details] [diff] [review]
Update to work on iframe

Can someone check this in please?
Attachment #723668 - Flags: checkin?(fyan)

Updated

4 years ago
Attachment #723668 - Flags: checkin?(fyan) → checkin+

Comment 17

4 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/75a92967f22c
Target Milestone: --- → Firefox 22
https://hg.mozilla.org/mozilla-central/rev/75a92967f22c
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
OS: Windows 8 Metro → Windows 8.1
You need to log in before you can comment on or make changes to this bug.