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

Status: RESOLVED FIXED

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

Description

[Mathieu Pellerin]

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.

Comment 1

[Mathieu Pellerin]

Attachment: simple contenteditable=true div test case

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)

Comment 2

[Aaron Train [:aaronmt]]

Additional note here, text-selection on any value wont also allow for the copying operation through tap or context-menu.

Comment 4

[Aaron Train [:aaronmt]]

I dont think this is dupe; <textarea>'s work fine.

Comment 5

[Mathieu Pellerin]

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.

Comment 6

[Mathieu Pellerin]

(I meant problem is still present with contenteditable=true div while <textarea> works fine)

Comment 7

[Mathieu Pellerin]

Pasting into contenteditable enabled div working fine on iOS, should this bug be flagged with a parity keyword?

Comment 8

[Aaron Train [:aaronmt]]

I take it, stock Android works? In that case we can whiteboard add [parity-stock]. What about Chrome ([parity-chrome])?

Comment 9

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

Sriram - Can you look into why we don't get a context menu?

Comment 11

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

filter on [mass-p5]

Comment 12

[Mike Taylor [:miketaylr]]

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

Comment 13

[Mark Capella [:capella]]

I wouldn't think a second bug is required. We could ask that the solution be provided here in separate small patches though.

Comment 14

[Mike Taylor [:miketaylr]]

Thanks. Will split the patches.

Comment 15

[Mike Taylor [:miketaylr]]

Attachment: 783846-1.patch

Comment 16

[Mike Taylor [:miketaylr]]

Attachment: 783846-2.patch

Comment 17

[Mike Taylor [:miketaylr]]

Attachment: 783846-3.patch

Comment 18

[Mike Taylor [:miketaylr]]

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 19

[Mark Capella [:capella]]

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

Comment 20

[Mark Capella [:capella]]

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

also to wesj

Comment 21

[Mark Capella [:capella]]

Comment on attachment 8519067 [details] [diff] [review]
783846-3.patch

Also

Comment 22

[Mike Taylor [:miketaylr]]

Cool, no worries.

Comment 23

[Wesley Johnston (:wesj)]

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

Comment 24

[Mike Taylor [:miketaylr]]

(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 25

[Wesley Johnston (:wesj)]

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.

Comment 26

[Mike Taylor [:miketaylr]]

(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.

Comment 27

[Mike Taylor [:miketaylr]]

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.

Comment 28

[Mike Taylor [:miketaylr]]

Attachment: 783846-2-1.patch

Comment 29

[Mike Taylor [:miketaylr]]

Attachment: 783846-2-2.patch

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).

Comment 30

[Mike Taylor [:miketaylr]]

Attachment: 783846-2-2.patch

Crap. Forgot to convert to hg patches. >_<

Comment 31

[Mike Taylor [:miketaylr]]

Attachment: 783846-2-1.patch

Comment 32

[Mike Taylor [:miketaylr]]

Thanks Wes.

Comment 33

[Carsten Book [:Tomcat]]

could we get a try link here? Thanks!

Comment 34

[Mike Taylor [:miketaylr]]

I don't have rights to push to try servers. Wes could you help please?

Comment 35

[:Margaret Leibovic]

(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/

Comment 36

[Mike Taylor [:miketaylr]]

Oh, cool. Thanks! :)

Comment 37

[:Margaret Leibovic]

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=ed30a16a9005

Comment 38

[Mark Capella [:capella]]

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.

Comment 39

[Mike Taylor [:miketaylr]]

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.

Comment 40

[Mark Capella [:capella]]

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

Comment 41

[Mike Taylor [:miketaylr]]

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.

Comment 42

[Mike Taylor [:miketaylr]]

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

Comment 43

[Mike Taylor [:miketaylr]]

Attachment: 783846-3-1.patch

Try servers are green, uploading rebased patches for checkin.

Comment 44

[Mike Taylor [:miketaylr]]

Attachment: 783846-3-2.patch

Comment 45

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/fx-team/rev/1a29df76bdc8
https://hg.mozilla.org/integration/fx-team/rev/c909641ec171

Comment 46

[Ryan VanderMeulen [:RyanVM]]

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

Comment 47

[Mike Taylor [:miketaylr]]

Sorry about that--still learning the ropes of try syntax. Didn't realize I had excluded some robocop tests. Will investigate.

Comment 48

[Mike Taylor [:miketaylr]]

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. :)

Comment 49

[Mike Taylor [:miketaylr]]

> 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

Comment 50

[Mike Taylor [:miketaylr]]

Attachment: 783846-Part-1.patch

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).

Comment 51

[Mike Taylor [:miketaylr]]

Attachment: 783846-Part-2.patch

Comment 52

[Mike Taylor [:miketaylr]]

Attachment: 783846-Part-3.patch

Carrying forward r+ on this one.

Comment 53

[Wesley Johnston (:wesj)]

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 :))

Comment 54

[Mike Taylor [:miketaylr]]

Thanks Wes!

Comment 55

[Mark Capella [:capella]]

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/

Comment 56

[Mike Taylor [:miketaylr]]

Sweet, thanks! Let's try checkin-needed again.

Comment 57

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/fx-team/rev/7a58fbd76acd
https://hg.mozilla.org/integration/fx-team/rev/1d9b1d65a77c
https://hg.mozilla.org/integration/fx-team/rev/c37984bc95db

Comment 58

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if 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

Comment 59

[Liz Henry (:lizzard) (relman/hg->git project)]

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?

Comment 60

[Mike Taylor [:miketaylr]]

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.

Comment 61

[Mike Taylor [:miketaylr]]

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.

Comment 62

[Liz Henry (:lizzard) (relman/hg->git project)]

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.