611741 - Implement copy/paste for text fields in web content

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Matt Brubeck (:mbrubeck)]

Use the same context menu items for copy/paste/select that are used for chrome textboxes (bug 585875).

Comment 1

[Mark Finkle (:mfinkle) (use needinfo?)]

Keyboard issues and add-on restart issues should be fixed first

Comment 2

[Wesley Johnston (:wesj)]

Attachment: WIP patch

I had started work on this last week, but have other things to work on right now. Thought I might as well put the work up, and delete it from my queue.

Unfortunately, on Android we can not access the system clipboard from the content proces. So we have to pass messages back and forth in order to know if we can paste, and to do the paste.

To get around that, this uses a hack, stealing a spot in the "modifiers", and within the content process pretending this is a context menu event where the ctrl-key is pressed.

Comment 4

[Wesley Johnston (:wesj)]

Attachment: WIP

Updated this to tip for fun. Removed some hackishness, and added the ability to copy href's from links. Doesn't quite work in all the text areas it should work in.

Comment 5

[Thomas Arend [:tarend]]

What is the status of this? 
As a stop-gap, could we potentially add copy & paste to https://addons.mozilla.org/en-US/mobile/addon/touching-is-good/ ? In the "mouse" mode, the add-on already allows for text selection. We could copy selected text automatically or with text select -> tap and hold on selected text -> context menu shows "copy" option

Comment 6

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to comment #5)
> What is the status of this? 

Not blocking Fennec, so no one is working on it (officially)

Comment 7

[Ernest Chiang [:ernestchiang] (MozTW.org)]

It still not available on HTC Desire.
Mozilla/5.0 (Android; Linux armv7l; rv:2.0b12pre)
Gecko/20110217 Firefox/4.0b12pre Fennec/4.0b5

Comment 8

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: patch

Comment 9

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 515003 [details] [diff] [review]
patch


>diff --git a/chrome/content/ContextCommands.js b/chrome/content/ContextCommands.js

>   paste: function cc_paste() {

>+    let content = ContextHelper.popupState.content;
>+    ContextHelper.popupState.pasteData = Ci.nsIClipboard.kGlobalClipboard;
>+    if (content) {

Can't we just assume that if target is null, then it's content? I don't like to carry too much extra stuff

>   selectAll: function cc_selectAll() {

>+    let content = ContextHelper.popupState.content;
>+    if (content) {

Same

>diff --git a/chrome/content/content.js b/chrome/content/content.js

>+      case "Browser:SelectAll":
>+        {
>+	  let wrappedTarget = elementFromPoint(x, y);
>+	  if (!wrappedTarget)
>+	    break;
>+	  let target = wrappedTarget.QueryInterface(Ci.nsIDOMNSEditableElement);
>+	  if (!target)
>+	    break;
>+	  target.editor.selectAll();
>+	  target.focus();
>+        }
>+	break;
>+
>+      case "Browser:Paste":
>+        {
>+	  let wrappedTarget = elementFromPoint(x, y);
>+	  if (!wrappedTarget)
>+	    break;
>+	  let target = wrappedTarget.QueryInterface(Ci.nsIDOMNSEditableElement);
>+	  if (!target)
>+	    break;
>+	  target.editor.paste(Ci.nsIClipboard.kGlobalClipboard);
>+	  target.focus();
>+	}
>+	break;

Use a single Browser:ContextCommand and include the command in the JSON: { command: "paste" ...
That way you can reuse the first 6 lines of each handler in a single handler

>         let event = content.document.createEvent("PopupEvents");
>         event.initEvent("contextmenu", true, true);
>+	event.x = x;
>+	event.y = y;

Wrong indent

>-      mediaURL: ""
>+      mediaURL: "",
>+      content: true,
>+      x: aEvent.x,
>+      y: aEvent.y

Not a huge fan of needing the x/y - but I don't know if we have "handles" working yet. A handle would allow you to "reference" an element, send reference to chrome and back to content. Looks like handles are only in the nsIJetpackService

Maybe the x / y could come in handy.

>+        } else if (elem instanceof Ci.nsIDOMHTMLInputElement && 
>+		   elem.type === "text") {

No wrapping needed

>+	  let selectionStart = elem.selectionStart;
>+          let selectionEnd = elem.selectionEnd;

Wrong indent

>+          if (hasData && (!elem.readOnly))

Remove extra parens

>+            state.types.push("paste");
>+	  break;
>+	}
>       }

Wrong indent

>+ContextHandler.registerType("input-text", function(aState, aElement) {
>+    return true;
>+});

Can't we check the element here to see if it is a text input?

r- for cleanup

How does this patch compare to Wes's WIP patch? Same level of features?

Comment 10

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: patch

Comment 11

[Brad Lassey [:blassey] (use needinfo?)]

also... on android this enables the input method selector menu item for content

Comment 12

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 515010 [details] [diff] [review]
patch

>diff --git a/chrome/content/ContextCommands.js b/chrome/content/ContextCommands.js

>   paste: function cc_paste() {

>+      let json = {x: x, y: y, command: "paste" };

add a leading space:
        let json = { x: x, y: y, command: "select-all" };

>   selectAll: function cc_selectAll() {

>+      let json = {x: x, y: y, command: "select-all" };

add a leading space:
        let json = { x: x, y: y, command: "select-all" };

>diff --git a/chrome/content/content.js b/chrome/content/content.js

>     switch (aMessage.name) {
>+      case "Browser:ContextCommand":
>+        {

put the { on the same line as the case:

case "Browser:ContextCommand": {

>+	  target.focus();
>+	}
>+	break;

swap these lines:

          break;
        }

Also, there are TABs in here!

r+ with nit fixes and TABs removed

Also, if we can't get child process Paste working, I think we should back this out for Fennec 4

Comment 13

[Wesley Johnston (:wesj)]

This doesn't support paste on device... right? While our clipboard service is available, I don't think the system clipboard can be accessed from the child process.

Comment 14

[Brad Lassey [:blassey] (use needinfo?)]

filed bug 636666 for that, will fix that next

Comment 15

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 515010 [details] [diff] [review]
patch

approved with the "paste in content" caveat

Comment 16

[Brad Lassey [:blassey] (use needinfo?)]

pushed https://hg.mozilla.org/mobile-browser/rev/311f0315b998

Comment 17

[Ernest Chiang [:ernestchiang] (MozTW.org)]

I installed 2/28's Fennec nightly to test this function. On my HTC Desire in Chinese (zh-TW), when I long-press on a text field, it shows "Select Input Method" now, but not copy/paste context menu. Is it normal?

Comment 18

[Brad Lassey [:blassey] (use needinfo?)]

you won't get paste as an option until bug 636666 lands. For "copy all" to show up you'll need text to be entered in the box

Comment 19

[Tony Chung [:tchung]]

Verified fix on Mozilla/5.0 (Android; Linux armv71; rv:2.0b13pre) Gecko/20110303 Firefox/4.0b13pre Fennec/4.0b6pre

Comment 20

[Ioana Chiorean]

https://litmus.mozilla.org/show_test.cgi?id=15212