Closed
Bug 844371
Opened 11 years ago
Closed 11 years ago
Work - Add "Cut" to the text field context menu
Categories
(Firefox for Metro Graveyard :: General, defect)
Tracking
(Not tracked)
RESOLVED
FIXED
Firefox 22
People
(Reporter: ywang, Assigned: rsilveira)
References
Details
(Whiteboard: feature=work )
Attachments
(1 file, 2 obsolete files)
24.18 KB,
patch
|
fryn
:
review+
fryn
:
checkin+
|
Details | Diff | Splinter Review |
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
Reporter | ||
Updated•11 years ago
|
Whiteboard: [metro-mvp?]
Updated•11 years ago
|
Blocks: metrov1triage
Flags: needinfo?(asa)
Comment 1•11 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)
Comment 2•11 years ago
|
||
Thanks Asa. I will move this Change Story to Iteration #3. What story can I related it to?
Updated•11 years ago
|
Updated•11 years ago
|
Priority: -- → P1
Updated•11 years ago
|
Whiteboard: [metro-mvp?] → feature=change c=tbd u=tbd p=0
Comment 3•11 years ago
|
||
bug 565717, recently added search google for text area selections
Comment 4•11 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•11 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•11 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: metrov1it3
Updated•11 years ago
|
QA Contact: jbecerra
Updated•11 years ago
|
Assignee: nobody → fyan
Status: NEW → ASSIGNED
Updated•11 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•11 years ago
|
Assignee: fyan → rsilveira
Assignee | ||
Comment 6•11 years ago
|
||
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?
Assignee | ||
Comment 7•11 years ago
|
||
Attachment #722604 -
Flags: review?(mbrubeck)
Attachment #722604 -
Flags: review?(fyan)
Comment 8•11 years ago
|
||
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+
Assignee | ||
Comment 9•11 years ago
|
||
Asa, do we need the toast for Cut/Copy? It currently says: "Text copied to clipboard"
Flags: needinfo?(asa)
Comment 10•11 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•11 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+
Assignee | ||
Comment 12•11 years ago
|
||
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)
Updated•11 years ago
|
Attachment #723027 -
Flags: review?(mbrubeck) → review+
Comment 13•11 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-
Assignee | ||
Comment 14•11 years ago
|
||
Updated patch to work on iframe, thanks :fryn for catching this.
Attachment #723027 -
Attachment is obsolete: true
Attachment #723668 -
Flags: review?(fyan)
Comment 15•11 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+
Assignee | ||
Comment 16•11 years ago
|
||
Comment on attachment 723668 [details] [diff] [review] Update to work on iframe Can someone check this in please?
Attachment #723668 -
Flags: checkin?(fyan)
Updated•11 years ago
|
Attachment #723668 -
Flags: checkin?(fyan) → checkin+
Comment 17•11 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/75a92967f22c
Target Milestone: --- → Firefox 22
Comment 18•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/75a92967f22c
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Updated•10 years ago
|
OS: Windows 8 Metro → Windows 8.1
You need to log in
before you can comment on or make changes to this bug.
Description
•