Closed Bug 783846 Opened 8 years ago Closed 5 years ago

firefox android won't allow user to paste clipboard content into a contenteditable=true element (e.g. <div>)

Categories

(Firefox for Android :: Text Selection, defect, P5)

ARM
Android
defect

Tracking

()

RESOLVED FIXED
Firefox 39
Tracking Status
firefox15 --- affected
firefox16 --- affected
firefox17 --- affected
firefox39 --- fixed
relnote-firefox --- 39+
fennec + ---

People

(Reporter: nirvn.asia, Assigned: miketaylr)

References

()

Details

Attachments

(4 files, 9 obsolete files)

386 bytes, text/html
Details
2.96 KB, patch
wesj
: review+
Details | Diff | Splinter Review
5.59 KB, patch
wesj
: review+
capella
: feedback+
Details | Diff | Splinter Review
1.44 KB, patch
miketaylr
: review+
Details | Diff | Splinter Review
The summary is pretty self explanatory. Firefox for Android doesn't offer any way to paste clipboard content into a contenteditable=true element. That's a pretty substantial implementation issue.
To reproduce issue:
1. load attached test case
2. hold touch within the <div> element
3. witness the absence of the "paste" feature

(you can verify that the <div> element has the contenteditable=true tag by clicking on the <div> and write stuff in it through keyboard)
Attachment #653132 - Attachment mime type: text/plain → text/html
Status: UNCONFIRMED → NEW
tracking-fennec: --- → ?
Ever confirmed: true
OS: Linux → Android
Hardware: x86 → ARM
Additional note here, text-selection on any value wont also allow for the copying operation through tap or context-menu.
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 780906
I dont think this is dupe; <textarea>'s work fine.
Status: RESOLVED → REOPENED
Resolution: DUPLICATE → ---
This is not a duplicate. I tried using a nightly build with bug 780906 applied and the problem is still present.

As Aaron mentioned, copying text to clipboard within contenteditable=true div also fails.
(I meant problem is still present with contenteditable=true div while <textarea> works fine)
Pasting into contenteditable enabled div working fine on iOS, should this bug be flagged with a parity keyword?
I take it, stock Android works? In that case we can whiteboard add [parity-stock]. What about Chrome ([parity-chrome])?
Status: REOPENED → NEW
Sriram - Can you look into why we don't get a context menu?
Assignee: nobody → sriram
tracking-fennec: ? → 17+
tracking-fennec: 17+ → +
Duplicate of this bug: 951716
filter on [mass-p5]
Priority: -- → P5
Assignee: sriram.mozilla → nobody
Mentor: markcapella
I spent some time looking at this last night,

In http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/SelectionHandler.js#629, the paste actionbar action is never added because isElementEditableText[1] doesn't consider contenteditable elements as editable (or designMode documents).

Fixing that and we get paste UI, but you have to add some execCommand code for non-nsIDOMNSEditableElement elements in the action (handler? method?), i.e., contenteditable stuff to get it to actually paste. 

I noticed cut doesn't support contenteditable either, it assumes a value property on aElement[2], so it would need some execCommand support as well.

Mark, would you suggest I open another bug for cut or is it OK to lump both fixes in this bug?

[1] http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/SelectionHandler.js#796
[2] http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/SelectionHandler.js#578
Assignee: nobody → miket
I wouldn't think a second bug is required. We could ask that the solution be provided here in separate small patches though.
Flags: needinfo?(markcapella)
Component: General → Text Selection
Thanks. Will split the patches.
Attached patch 783846-1.patch (obsolete) — Splinter Review
Attached patch 783846-2.patch (obsolete) — Splinter Review
Attached patch 783846-3.patch (obsolete) — Splinter Review
Here's the simple test page I was using, https://miketaylr.com/bzla/951716.html.

Range selection needs some work in contenteditable, but these patches fix CUT and PASTE (and the 3rd one fixes a billion null pointer errors when moving a range inside of contentEditable.
Comment on attachment 8519065 [details] [diff] [review]
783846-1.patch

Looks good in theory :-) I didn't dig in and will be mostly away for the next month or so ... kicking over to wesj for r? as he's a proper module peer
Attachment #8519065 - Flags: review?(wjohnston)
Attachment #8519065 - Flags: review?(markcapella)
Attachment #8519065 - Flags: feedback+
Comment on attachment 8519066 [details] [diff] [review]
783846-2.patch

also to wesj
Attachment #8519066 - Flags: review?(wjohnston)
Attachment #8519066 - Flags: review?(markcapella)
Attachment #8519066 - Flags: feedback+
Comment on attachment 8519067 [details] [diff] [review]
783846-3.patch

Also
Attachment #8519067 - Flags: review?(wjohnston)
Attachment #8519067 - Flags: review?(markcapella)
Attachment #8519067 - Flags: feedback+
Cool, no worries.
Comment on attachment 8519065 [details] [diff] [review]
783846-1.patch

Review of attachment 8519065 [details] [diff] [review]:
-----------------------------------------------------------------

These are nice fixes! It shouldn't be hard to write some simple tests for them. Would you mind? We already do some basic tests for textSelection code here:

http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/tests/roboextender/testSelectionHandler.html?force=1#29

The framework is a little ad-hoc there, but you'd basically add a function to that list of promises. Even a simple function would be nice. i.e.:

let clipboard = Cc["@mozilla.org/widget/clipboardhelper;1"].getService(Ci.nsIClipboardHelper);
clipboard.copyString("Test text", document);
let elements = document.querySelectorAll("thingsToTest");
var promises = [];
for (var i = 0; i < elements.length; i++) {
   SelectionHandler.PASTE.action(elements[i]);
   if (elements[i].hasAttribute("passing")) {
     promises.push(is(elements[i].value, "Test text", "Pasted correctly"));
     promises.push(ok(SelectionHandler.isElementEditableText(elements[i]), "Element is editable");
   } else {
     promises.push(isnot(elements[i].value, "Test text", "Paste failed correctly"));
     promises.push(ok(!SelectionHandler.isElementEditableText(elements[i]), "Element is not editable");
   }
}

return Promise.all(promises);

designMode might be a bit more work...

::: mobile/android/chrome/content/SelectionHandler.js
@@ +795,5 @@
>  
>    isElementEditableText: function (aElement) {
>      return (((aElement instanceof HTMLInputElement && aElement.mozIsTextField(false)) ||
> +            (aElement instanceof HTMLTextAreaElement)) && !aElement.readOnly) ||
> +            aElement.contentEditable == "true" || aElement.ownerDocument.designMode == "on";

:( Ouch
Attachment #8519065 - Flags: review?(wjohnston) → review+
(In reply to Wesley Johnston (:wesj) from comment #23)

> It shouldn't be hard to write some simple tests for them. Would you mind?

Oh yeah, cool. Will put something together.

> >    isElementEditableText: function (aElement) {
> >      return (((aElement instanceof HTMLInputElement && aElement.mozIsTextField(false)) ||
> > +            (aElement instanceof HTMLTextAreaElement)) && !aElement.readOnly) ||
> > +            aElement.contentEditable == "true" || aElement.ownerDocument.designMode == "on";
> 
> :( Ouch

Heh, yeah.
Comment on attachment 8519066 [details] [diff] [review]
783846-2.patch

Review of attachment 8519066 [details] [diff] [review]:
-----------------------------------------------------------------

::: mobile/android/chrome/content/SelectionHandler.js
@@ +580,5 @@
> +        if (aElement instanceof Ci.nsIDOMNSEditableElement) {
> +          aElement.value = aElement.value.substring(0, start) + aElement.value.substring(end);
> +        } else {
> +          // Handle contentEditable and designMode cutting.
> +          aElement.textContent = aElement.textContent.substring(0, start) + aElement.textContent.substring(end);

I have a feeling this breaks if the selection covers multiple nodes. I have a feeling our old code doesn't handle that very well either (although since it was limited to textboxes, maybe that wasn't an issue).

I think we should probably try to get the editor and call cut() on it here anyway. Actually, I see editor has a canCut(). Maybe we should use that below... I feel a bit bad we didn't use it more originally here.
Attachment #8519066 - Flags: review?(wjohnston) → review-
Attachment #8519067 - Flags: review?(wjohnston) → review+
(In reply to Wesley Johnston (:wesj) from comment #25)
> I think we should probably try to get the editor and call cut() on it here
> anyway. Actually, I see editor has a canCut(). Maybe we should use that
> below... I feel a bit bad we didn't use it more originally here.

Now that I read through http://mxr.mozilla.org/mozilla-central/source/editor/nsIEditor.idl, we can simplify isElementEditableText with editor.isDocumentEditable and editor.isSelectionEditable as well.
Small status update (and wow, a month goes by really quickly): I spent some time on tests yesterday but am struggling with contentEditable failing (though working manually) the paste tests based on Comment 23. 

Will keep poking at it this weekend.
Attached patch 783846-2-1.patch (obsolete) — Splinter Review
Attachment #8519065 - Attachment is obsolete: true
Attachment #8519066 - Attachment is obsolete: true
Attachment #8519067 - Attachment is obsolete: true
Attached patch 783846-2-2.patch (obsolete) — Splinter Review
So I'm punting on getting "cut" to work without using textContent string slicing--nsIEditor just doesn't want to cooperate with me. I'll open another bug to come back to that one.

So here's the two patches related to paste, including tests. Also punting on designMode because I couldn't get any of my tests to pass (and we probably don't need this for mobile compat--at least I don't know of any open bugs).
See Also: → 1112276
Attached patch 783846-2-2.patch (obsolete) — Splinter Review
Crap. Forgot to convert to hg patches. >_<
Attachment #8537395 - Attachment is obsolete: true
Attached patch 783846-2-1.patch (obsolete) — Splinter Review
Attachment #8537401 - Flags: review?(wjohnston) → review+
Attachment #8537402 - Flags: review?(wjohnston) → review+
Thanks Wes.
Keywords: checkin-needed
could we get a try link here? Thanks!
Keywords: checkin-needed
I don't have rights to push to try servers. Wes could you help please?
(In reply to Mike Taylor [:miketaylr] from comment #34)
> I don't have rights to push to try servers. Wes could you help please?

You should request L1 commit access! I'll vouch for you.

https://www.mozilla.org/en-US/about/governance/policies/commit/
Oh, cool. Thanks! :)
Checking in w/Mike ... or you still pursuing this?

We've had some changes in that test recently, you may find this patch bit-rotted.
Holy smokes time goes by fast. Yeah, I'll investigate what is up with the try run that Margaret pasted (rebasing first) in the next few days. Thanks for the ping.
Oh sure, I realized I'm still listed as "mentor" :)

I can't believe you don't have l1 access, seems like you've done your share of bugs !!! If you need try push again or anything, I'm happy to help.

Heh - if you'd rather, I can drop off as mentor and quit nagging you :-D
Ah thanks. I actually got level 1 taken care of just around the new year. ^_^ Still learning the ropes of try syntax and figuring out all that related stuff.
OK, rebased patches (and fixed conflict) and pushed again to try to see what (if anything) changed: https://treeherder.mozilla.org/#/jobs?repo=try&revision=d7efe22a18fc
Attached patch 783846-3-1.patch (obsolete) — Splinter Review
Try servers are green, uploading rebased patches for checkin.
Attachment #8537389 - Attachment is obsolete: true
Attachment #8537402 - Attachment is obsolete: true
Attached patch 783846-3-2.patch (obsolete) — Splinter Review
Attachment #8537401 - Attachment is obsolete: true
Keywords: checkin-needed
Backed out for testSelectionHandler robocop failures. Not sure why you specified only a subset of them in your Try push instead of running all of them?
https://hg.mozilla.org/integration/fx-team/rev/a22f43770da8

https://treeherder.mozilla.org/logviewer.html#?job_id=1885897&repo=fx-team
Sorry about that--still learning the ropes of try syntax. Didn't realize I had excluded some robocop tests. Will investigate.
Update: unsure why the tests are failing now when they weren't before. Will investigate a bit more next week. Also, relieving Mark of mentor duty. :)
Mentor: markcapella
> Error in selection test TypeError: this._contentWindow is null

This was likely due to passing in <input type=button> to the PASTE action. SelectionHandler._contentWindow is never set for type=button inputs due to the early return in SelectionHandler.startSelection.

So I've updated my tests to account for this and cleaned up some bit-rot. Also prevent the CUT action from handling contentEditable elements until someone tackles Bug 1112276.

Let's try again: https://treeherder.mozilla.org/#/jobs?repo=try&revision=f647f4ce4a59
Not much has changed since the first r+'d patch, except for disallowing CUT for contentEditable and using the isContentEditable property (which I just learned about, hey).
Attachment #8558923 - Attachment is obsolete: true
Carrying forward r+ on this one.
Attachment #8558924 - Attachment is obsolete: true
Whiteboard: [fixed-in-fx-team]
Attachment #8576469 - Flags: review?(wjohnston) → review+
Comment on attachment 8576473 [details] [diff] [review]
783846-Part-2.patch

Review of attachment 8576473 [details] [diff] [review]:
-----------------------------------------------------------------

These look good. Mark's been doing a lot of work here, so mostly want to make sure he's aware of these changes (but feel free to review them if you have time quickly :))
Attachment #8576473 - Flags: review?(wjohnston)
Attachment #8576473 - Flags: review+
Attachment #8576473 - Flags: feedback?(markcapella)
Thanks Wes!
Comment on attachment 8576473 [details] [diff] [review]
783846-Part-2.patch

Review of attachment 8576473 [details] [diff] [review]:
-----------------------------------------------------------------

Yep :) I'm good with wes approval ... was watching and no impact to my plans so \o/
Attachment #8576473 - Flags: feedback?(markcapella) → feedback+
Sweet, thanks! Let's try checkin-needed again.
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/7a58fbd76acd
https://hg.mozilla.org/mozilla-central/rev/1d9b1d65a77c
https://hg.mozilla.org/mozilla-central/rev/c37984bc95db
Status: NEW → RESOLVED
Closed: 8 years ago5 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 39
Release Note Request (optional, but appreciated)
[Why is this notable]: It's nice that this will work now. 
[Suggested wording]: New: Paste Android clipboard content into web content
[Links (documentation, blog post, etc)]:  Mike, want to write a post about this? Or is there one already?
Flags: needinfo?(miket)
I was planning on writing something up about this on my own blog this afternoon--will paste the link in here when it's ready.
So um, paste in contenteditable isn't very exciting so I spiced it up a little bit here: https://miketaylr.com/posts/2015/03/contenteditable-paste.html

Maybe using "Paste Android clipboard content into editable web content" would be better? That way it doesn't sound like we just added paste for input elements, etc.
Your blog posts bring all the android pasteable content to the... ummm.... editable yard!

Relnoted for 39 with your suggested wording and link from comment 39.
You need to log in before you can comment on or make changes to this bug.