Closed Bug 611741 Opened 9 years ago Closed 9 years ago

Implement copy/paste for text fields in web content

Categories

(Firefox for Android Graveyard :: General, defect)

defect
Not set

Tracking

(fennec-)

VERIFIED FIXED
Tracking Status
fennec - ---

People

(Reporter: mbrubeck, Assigned: blassey)

References

Details

(Keywords: relnote)

Attachments

(1 file, 3 obsolete files)

Use the same context menu items for copy/paste/select that are used for chrome textboxes (bug 585875).
tracking-fennec: --- → ?
Assignee: nobody → mbrubeck
tracking-fennec: ? → 2.0-
Assignee: mbrubeck → wjohnston
Keyboard issues and add-on restart issues should be fixed first
Assignee: wjohnston → nobody
Flags: in-testsuite?
Flags: in-litmus?
Flags: in-litmus? → in-litmus?(ayanshah62)
Attached patch WIP patch (obsolete) — Splinter Review
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.
Keywords: relnote
Duplicate of this bug: 623646
Attached patch WIP (obsolete) — Splinter Review
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.
Attachment #490765 - Attachment is obsolete: true
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
(In reply to comment #5)
> What is the status of this? 

Not blocking Fennec, so no one is working on it (officially)
It still not available on HTC Desire.
Mozilla/5.0 (Android; Linux armv7l; rv:2.0b12pre)
Gecko/20110217 Firefox/4.0b12pre Fennec/4.0b5
Attached patch patch (obsolete) — Splinter Review
Assignee: nobody → blassey.bugs
Attachment #503698 - Attachment is obsolete: true
Attachment #515003 - Flags: review?(mark.finkle)
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?
Attachment #515003 - Flags: review?(mark.finkle) → review-
Blocks: 636666
Attached patch patchSplinter Review
Attachment #515003 - Attachment is obsolete: true
also... on android this enables the input method selector menu item for content
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
Attachment #515010 - Flags: review+
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.
filed bug 636666 for that, will fix that next
Attachment #515010 - Flags: approval2.0?
Comment on attachment 515010 [details] [diff] [review]
patch

approved with the "paste in content" caveat
Attachment #515010 - Flags: approval2.0? → approval2.0+
pushed https://hg.mozilla.org/mobile-browser/rev/311f0315b998
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
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?
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
Blocks: 638479
Verified fix on Mozilla/5.0 (Android; Linux armv71; rv:2.0b13pre) Gecko/20110303 Firefox/4.0b13pre Fennec/4.0b6pre
Status: RESOLVED → VERIFIED
Depends on: 639969
Depends on: 641217
https://litmus.mozilla.org/show_test.cgi?id=15212
Flags: in-litmus?(ayanshah62) → in-litmus+
You need to log in before you can comment on or make changes to this bug.