Last Comment Bug 407983 - Add support clipboardData object for the onpaste, oncopy, oncut events
: Add support clipboardData object for the onpaste, oncopy, oncut events
Status: RESOLVED FIXED
[evang-wanted][devtools-needed]
: dev-doc-complete
Product: Core
Classification: Components
Component: DOM: Core & HTML (show other bugs)
: unspecified
: All All
: -- enhancement with 54 votes (vote)
: mozilla22
Assigned To: Neil Deakin (away until Oct 3)
:
Mentors:
: 689590 (view as bug list)
Depends on: 280959 501154 850198 861512 CVE-2013-6672 920405
Blocks: 810636 844941
  Show dependency treegraph
 
Reported: 2007-12-11 15:07 PST by spam
Modified: 2016-04-02 17:35 PDT (History)
64 users (show)
dsicore: wanted‑next+
dsicore: blocking1.9-
enndeakin: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
22+


Attachments
Testcase for the clipboardData object in IE and WebKit (747 bytes, text/html)
2007-12-12 04:13 PST, spam
no flags Details
work in progress (56.28 KB, patch)
2009-07-02 17:20 PDT, Neil Deakin (away until Oct 3)
no flags Details | Diff | Splinter Review
patch part 1 - add clipboardData to cut/copy/paste events (66.21 KB, patch)
2011-06-24 07:04 PDT, Neil Deakin (away until Oct 3)
no flags Details | Diff | Splinter Review
Patch part 2 - global clipboardData property (48.14 KB, patch)
2011-06-24 07:09 PDT, Neil Deakin (away until Oct 3)
no flags Details | Diff | Splinter Review
Patch part 3 - clipboard changed checks (19.27 KB, patch)
2011-06-24 07:13 PDT, Neil Deakin (away until Oct 3)
no flags Details | Diff | Splinter Review
patch part 1 - add clipboardData to cut/copy/paste events (65.48 KB, patch)
2011-11-16 10:26 PST, Fabien Cazenave [:kaze]
no flags Details | Diff | Splinter Review
patch part 2 - global clipboardData property (50.45 KB, patch)
2011-11-16 10:27 PST, Fabien Cazenave [:kaze]
no flags Details | Diff | Splinter Review
patch part 3 - clipboard changed checks (20.15 KB, patch)
2011-11-16 10:28 PST, Fabien Cazenave [:kaze]
no flags Details | Diff | Splinter Review
Part 1 - add clipboardData to cut/copy/paste events (66.34 KB, patch)
2012-06-15 11:40 PDT, Neil Deakin (away until Oct 3)
ehsan: feedback+
Details | Diff | Splinter Review
Part 2 - global clipboardData property (50.03 KB, patch)
2012-06-15 11:41 PDT, Neil Deakin (away until Oct 3)
ehsan: feedback-
Details | Diff | Splinter Review
Part 3 - clipboard changed checks (23.40 KB, patch)
2012-06-15 11:42 PDT, Neil Deakin (away until Oct 3)
ehsan: feedback+
Details | Diff | Splinter Review
Latest patch (68.71 KB, patch)
2012-10-22 12:01 PDT, Neil Deakin (away until Oct 3)
ehsan: review+
Details | Diff | Splinter Review
Rebased patch and address some review comments (57.97 KB, patch)
2013-02-07 04:22 PST, Julian Viereck
no flags Details | Diff | Splinter Review
Updated testcase that uses onCut and onCopy as well. (1.73 KB, text/html)
2013-02-07 04:25 PST, Julian Viereck
no flags Details
Updated testcase that uses onCut and onCopy as well. [bugfix] (1.73 KB, text/html)
2013-02-07 05:14 PST, Julian Viereck
no flags Details
Rebase v2 with already_AddRefed (59.67 KB, text/plain)
2013-02-07 05:18 PST, Julian Viereck
no flags Details
Rebase v3 (59.62 KB, patch)
2013-02-09 08:50 PST, Julian Viereck
no flags Details | Diff | Splinter Review
Disable failing test on Android (59.62 KB, patch)
2013-02-11 09:03 PST, Julian Viereck
bugs: superreview-
Details | Diff | Splinter Review
Right patch to disable failing test on Android (1.58 KB, text/plain)
2013-02-14 11:53 PST, Julian Viereck
no flags Details
WIP 4 - fix nits (62.95 KB, patch)
2013-02-23 03:01 PST, Julian Viereck
no flags Details | Diff | Splinter Review
Add support for clipboardData in cut/copy/paste events (64.30 KB, patch)
2013-02-27 06:33 PST, Neil Deakin (away until Oct 3)
bugs: superreview+
Details | Diff | Splinter Review
Add clipboard constructor (14.55 KB, patch)
2013-02-27 06:36 PST, Neil Deakin (away until Oct 3)
bugs: review-
Details | Diff | Splinter Review
Part 1 - Add support for clipboardData in cut/copy/paste events (65.07 KB, patch)
2013-03-01 14:20 PST, Neil Deakin (away until Oct 3)
bugs: superreview-
Details | Diff | Splinter Review
Part 1 - Add support for clipboardData in cut/copy/paste events (65.87 KB, patch)
2013-03-06 12:14 PST, Neil Deakin (away until Oct 3)
bugs: superreview+
Details | Diff | Splinter Review
Part 2 - add clipboard constructor (15.37 KB, patch)
2013-03-06 12:19 PST, Neil Deakin (away until Oct 3)
bugs: review+
Details | Diff | Splinter Review

Description spam 2007-12-11 15:07:15 PST
User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9b2pre) Gecko/2007121108 Minefield/3.0b2pre
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9b2pre) Gecko/2007121108 Minefield/3.0b2pre

There is currently no way of retrieving the clipboard data in JavaScript, WebKit and IE both have methods for this but Gecko currently lacks this functionality.

WebKit uses a method of attaching the an clipboardData object to the onpaste event. This method is better since it only gives JS access to the clipboard contents when the user performs an paste action.

This is how WebKit does it, contents can be retrieved from the onpaste event object:
eventObj.clipboardData.getData('text/plain');

While in IE you can use, this enables to to retrieve the contents at any time:
window.clipboardData.getData('Text');

IE and WebKit have only the ability to return plain text contents ideal would be to have the ability to get the rich HTML contents to be used in Midas/designMode implementations.

Reproducible: Always
Comment 1 Martijn Wargers [:mwargers] (not working for Mozilla) 2007-12-11 15:17:46 PST
Could you perhaps attach example code of how this is working in IE and Safari?
Also, giving the urls that reference this behavior in IE and Safari would be nice.
Comment 2 spam 2007-12-12 04:13:11 PST
Created attachment 292749 [details]
Testcase for the clipboardData object in IE and WebKit

This attachment displays how the plaintext contents can be extracted in IE and WebKit using the clipboardData object.
Comment 4 Olli Pettay [:smaug] (TPAC) 2008-02-26 05:55:21 PST
Has anyone studied the possible security problems clipboardData may expose?
Do IE or Safari somehow limit the use of .clipboardData? (I do hope so)
The documentation isn't very clear.
Comment 5 spam 2008-02-26 06:01:09 PST
IE will popup an confirm dialog before you can extract anything from the clipboard at least IE7. Safari will only provide the clipboardData object on a paste event so it will only be available if the user actually pastes some contents into the browser window so both methods are pretty secure.
Comment 6 Dan POPA 2008-02-26 06:18:54 PST
I agree this is secure.


In IE6 you can change the security to: disable clipboard access, enable or prompt. By default in IE6 is enabled. If prompt, a nice prompt asks the user permission for the clipboard operation. In IE7 default is prompt.

In Safari: access to clipboardData.getData(flavor) is given only on the paste event inside the onpaste event handlers. So only an user initiated event can exposes clipboard contents to the underlying application.



If a malicious script somehow would get to listen to the onpaste event to steal/alter the clipboard then it would certainly be able to do all sorts of stuff, much worse than this. It would not even need to get the clipboard, it would redirect the form submission itself.

If a script can listen to the onpaste event then it certainly can still your form data.

Comment 7 Jeff Walden [:Waldo] (remove +bmo to email) 2008-02-27 22:45:49 PST
Odds are I can't clear the flag, but given that onpaste and friends are being backed out, this doesn't block.
Comment 8 Jeff Walden [:Waldo] (remove +bmo to email) 2008-02-27 22:51:50 PST
Blah, I suck -- it's only the onbefore* versions that are being removed.  Restoring blocking request...
Comment 9 Damon Sicore (:damons) 2008-03-04 14:27:08 PST
New support this late?  -'ing.

And, can you explain why you are requesting blocking again?
Comment 10 Jeff Walden [:Waldo] (remove +bmo to email) 2008-03-04 14:31:51 PST
Dan, can you answer that?

(Note I was just restoring a flag I accidentally removed, not seriously requesting blocking here!)
Comment 11 Dan POPA 2008-03-04 14:54:31 PST
(In reply to comment #10)
> Dan, can you answer that?
> (Note I was just restoring a flag I accidentally removed, not seriously
> requesting blocking here!)

The clipboardData may be used toghether with the contentEditable attribute.

Application logic may require that the onpaste and ondrop events to be stopped and the clipboard content be retrieved so it can be cleaned/transformed/whatever and then inserted into the document.

It would be nice to have this in FF3 timeframe so apps can implement cross-browser custom paste handlers.

Comment 12 Damon Sicore (:damons) 2008-03-05 17:22:46 PST
This is something we probably wouldn't even take for 1.9 at this point in the release cycle (after confirming with jst).  We should have the discussion of whether or not this blocks the next release (Moz 2).  Marking wanted-next for now.
Comment 13 Danny Bloemendaal 2009-02-04 04:21:45 PST
What is the status for this issue?
Imho it's kinda silly that you implement a paste event but don't give a handle to what is actually being pasted.

(also, why is this more a security issue that the keydown/press/up event? if you want to do bad things that that's the event I would spy on. Not paste probably).
Comment 14 Danny Bloemendaal 2009-02-04 04:23:36 PST
Actually.. I don't need access to the clipboard at all. Just as long as the event passes me the string of what it wants to paste (for my to change (read: remove Word markup ****)).
Comment 15 Dan POPA 2009-02-04 04:45:55 PST
That would work for me too: tell me what its about to be pasted and let me change it, only for the paste event, thank you.

This may be text content when pasting into form controls and a HTML string when pasting into contenteditable elements.
Comment 16 Dion Almaer 2009-02-22 18:56:48 PST
I am excited to see this support added as it would mean that we could enable a full copy/cut/paste experience for Bespin (bespin.mozilla.com).

We currently have it working great in WebKit, but are stuck with poor support in Firefox (the buffer is not the system clipboard, but rather simply within the app itself).
Comment 17 spam 2009-04-07 08:01:30 PDT
As you have a dataTransfer object on the drop event now in the latest nightly it should be a easy fix to add this to the paste event as well. According to the HTML 5 spec a paste operation should fire a drop event. However I would prefer to have this dataTransfer object to the specific paste event as suggested in this feature request.

http://www.whatwg.org/specs/web-apps/current-work/#copy-and-paste
Comment 18 Neil Deakin (away until Oct 3) 2009-04-21 19:10:05 PDT
The basics of this are reasonably simple to implement. Just add a pointer to the dataTransfer in the cut/copy/paste events, then hook up the mappings to the transferables. We already support the events.

We should not implement any clipboard operations through drag events. The 'spec' is absurd in this regard.
Comment 19 Eli Grey (:sephr) 2009-04-21 19:17:12 PDT
(In reply to comment #18)
> We should not implement any clipboard operations through drag events. The
> 'spec' is absurd in this regard.

In the same regard to how paste is metaphorically just an instant drag from the system clipboard, copy would be the inverse. In my opinion, it doesn't make sense to only support paste/drop but not copy/drag.
Comment 20 spam 2009-04-22 06:10:46 PDT
Does anyone know if there is a master bug for the new drag/drop feature added in FF 3.5? Since this is somewhat related I want to attach this bug as a dependency if there is one.

I understand that the drop event is basically the same as a paste event but that doesn't stop the possibility to have the dataTransfer object added both to the paste and drop event. And firing the drop event on a paste operation would make it compatible with the HTML 5 spec. Then one could choose to use the paste event or the drop event if you want to distinct the two.
Comment 21 Alan Siu-Lung Tam 2009-06-30 22:30:23 PDT
Just installed Firefox 3.5 today. Simply cannot imagine why clipboardData is still left out. The "need discussion" resolution mentioned in Comment 12 is already 15 months old. It has not given concrete reasons why it can only be implemented in Gecko 2.0 timeframe the earliest.

Comment 18 also mentioned that it is fairly easy to implement. The only concern is perhaps that "we should not implement any clipboard operations through drag event". However, as Comment 20 stated, we only need to make sure the dataTransfer object is added to both events, without really need to merge the code for handling the events.

So, what are we still waiting for?
Comment 22 Olli Pettay [:smaug] (TPAC) 2009-07-01 02:04:53 PDT
(In reply to comment #21)
> So, what are we still waiting for?

Someone doing the work. (Bug 501154 looks promising.)
Comment 23 Neil Deakin (away until Oct 3) 2009-07-02 17:20:19 PDT
Created attachment 386637 [details] [diff] [review]
work in progress

This patch implements event.clipboardData and cuts/copies any data set into it onto the clipboard, as well as fills in the clipboard data on paste.

It doesn't implement window.clipboardData, nor the beforeXXX events.
Comment 24 Neil Deakin (away until Oct 3) 2009-07-03 06:12:34 PDT
Builds are at https://build.mozilla.org/tryserver-builds/neil@mozilla.com-clipboard4/
Comment 25 Alan Siu-Lung Tam 2009-08-12 14:34:57 PDT
The build seems lost. Anywhere to try it out now?
Comment 26 Jesse Ruderman 2009-08-17 22:59:03 PDT
Please add tests to ensure web pages can only use getData/setData *during* the appropriate event.  For example, they shouldn't be able to save off an event or clipboardData object from a copy event, and then later call setData successfully.
Comment 27 Tobias 2010-02-13 03:04:40 PST
Hi there, I am new here so this may be the wrong place for this request:

I really dont understand why browser dont allow me to allow it to paste images into an rich-text-editor or whatever enabled UI element in the browser. 
Think about how much easier blogging would be if you just could paste the screenshot you just made or the imagefile you just copied from your desktop to the RTE and go on editing it (with the awesome html5- or flash image editors).
I know there is some security issue with that. But we already have solutions for this, dont we? I dont think every site should be allowed to access the imanges in my clipboard. It would be enough for me to whitelist the sites like firefox already does with plugin-installation-sites or with java-applets that request for HDD access or something.
In my opinion it cannot be that it is easier for me to make a screencast of my system with screenr.com that to add an image to my blog-article.

What do you thinks?
Comment 28 Stéphane Roucheray 2010-05-02 14:29:28 PDT
This must be implemented in order for Bespin to work properly as a text editor.

Where are we for now ?
Comment 29 Alan Siu-Lung Tam 2010-06-03 10:40:20 PDT
Now that bug 501154 is fixed, I guess we only need to wait for Neil Deakin to update his patch.
Comment 30 Tobias 2010-06-03 12:35:40 PDT
@AlanSiu-LungTam: Fixed in which way? Will I be able to paste images from my system into the browser for further use? (See https://bugzilla.mozilla.org/show_bug.cgi?id=407983#c27) -- Or should I open a different ticket for that?
Thanks
Comment 31 Alan Siu-Lung Tam 2010-06-03 14:40:36 PDT
Tobias,

Once this bug is fixed, hopefully your requested feature can be implementable by javascript that captures the whole image data via clipboardData in the onpaste event. However, I am not 100% it will work flawlessly, since webkit based browsers currently allow reading texts from clipboard but not images.
Comment 32 Tobias 2010-06-07 02:12:24 PDT
Thanks Alan!
I created a new request 570460 that describes in detail what I am hoping to have some day: https://bugzilla.mozilla.org/show_bug.cgi?id=570460

Maybe some of you here can have a look at the ticket – it is my first one so I hope you will put it right if I did anything wrong. Thanks!
Comment 33 Neil Deakin (away until Oct 3) 2011-06-24 07:04:29 PDT
Created attachment 541671 [details] [diff] [review]
patch part 1 - add clipboardData to cut/copy/paste events
Comment 34 Neil Deakin (away until Oct 3) 2011-06-24 07:09:19 PDT
Created attachment 541672 [details] [diff] [review]
Patch part 2 - global clipboardData property

This patch adds a window.clipboardData property as IE does for clipboard access outside a clipboard event. Probably should be defined on nsIDOMChromeWindow, but I wrote this a while ago and didn't want to change it now.

In any case, security checks are made for non-chrome to ensure that access is only granted during clipboard events. Chrome callers have access at any time.

There's also a window.selectionClipboardData but I'm not sure how useful that really is to customize.
Comment 35 Neil Deakin (away until Oct 3) 2011-06-24 07:13:06 PDT
Created attachment 541673 [details] [diff] [review]
Patch part 3 - clipboard changed checks

This patch adds an optimization such that the global clipboard data is only re-retrieved if the clipboard has changed.

I think this only works on gtk when data on the clipboard comes from us, not from other applications, so an additional check will need to be added.
Comment 36 Olli Pettay [:smaug] (TPAC) 2011-06-26 13:17:46 PDT
How close is the implementation to http://dev.w3.org/2006/webapi/clipops/ ?
(I haven't reviewed the draft spec )
Comment 37 Neil Deakin (away until Oct 3) 2011-06-27 06:32:47 PDT
This spec seems a bit vague in places so its hard to tell, but it looks to be the same. Some differences:

- 4.1 bullet 3 about pasting images and html isn't done, but that doesn't seem specific to clipboard usage and should just apply to data transfers in general. My patch populates the data transfer using the same code as with drag and drop.
- the clipboard event interface uses a type/data, whereas I use a DataTransfer, which is more consistent with the drag/drop events
- this spec seems to imply that access to the system clipboard is live. My implementation doesn't have a live version used for event.clipboardData during an event. The clipboard is only modified after the cut/copy event has finished. This spec is inconsistent though, in some places in says a live clipboard is used, but in another (4.1 bullet 5) it says that the clipboard is only modified if the default action is prevented, which is only known after the event finishes.
- this spec seems to want clipboard events fired on keydown, whereas we use keypress. Not clear why this spec is specifying this, since the manner in which keyboard shortcuts are handled seems browser/platform specific
- I don't attempt to process, filter or perform any special security checks on the data, beyond what the data transfer and existing clipboard code already does.
Comment 38 :Ehsan Akhgari 2011-10-19 08:23:23 PDT
*** Bug 689590 has been marked as a duplicate of this bug. ***
Comment 39 Fabien Cazenave [:kaze] 2011-10-20 10:32:24 PDT
FWIW, I’ve started rebasing Neil’s patches: https://tbpl.mozilla.org/?tree=Try&rev=5e527f465905

Seems to work here, now looking more closely at the spec.
Comment 40 Fabien Cazenave [:kaze] 2011-11-16 10:26:20 PST
Created attachment 574933 [details] [diff] [review]
patch part 1 - add clipboardData to cut/copy/paste events
Comment 41 Fabien Cazenave [:kaze] 2011-11-16 10:27:24 PST
Created attachment 574934 [details] [diff] [review]
patch part 2 - global clipboardData property
Comment 42 Fabien Cazenave [:kaze] 2011-11-16 10:28:16 PST
Created attachment 574935 [details] [diff] [review]
patch part 3 - clipboard changed checks
Comment 43 :Ehsan Akhgari 2012-01-12 12:58:00 PST
Fabien, what is the status of these patches?
Comment 44 Rob Campbell [:rc] (:robcee) 2012-03-06 10:15:25 PST
pinging for a status update on this.

The fix for this bug would make our Source Editor integration with Orion much better. Also, this would allow the Orion team to remove some fairly ugly hacks from their editor implementation for Firefox. If it would be good for them, it would be good for the rest of the web.
Comment 45 Dmitry Alexeenko [MSFT] 2012-06-12 16:46:10 PDT
Guys, is there any update on this issue by chance?
Comment 46 :Ehsan Akhgari 2012-06-12 17:02:47 PDT
Neil, do you think you'll have a chance to work on this in the near future?
Comment 47 Neil Deakin (away until Oct 3) 2012-06-12 17:12:33 PDT
I can provide updated patches, if someone would like to volunteer to provide feedback on them. Comment 37 probably still applies, but I can review the updated spec again, (or someone else can)
Comment 48 :Ehsan Akhgari 2012-06-12 17:22:36 PDT
I can try to provide some feedback for you.  :-)
Comment 49 Neil Deakin (away until Oct 3) 2012-06-15 11:40:30 PDT
Created attachment 633602 [details] [diff] [review]
Part 1 - add clipboardData to cut/copy/paste events
Comment 50 Neil Deakin (away until Oct 3) 2012-06-15 11:41:37 PDT
Created attachment 633603 [details] [diff] [review]
Part 2 - global clipboardData property

See comment 34 for info about this part.
Comment 51 Neil Deakin (away until Oct 3) 2012-06-15 11:42:25 PDT
Created attachment 633604 [details] [diff] [review]
Part 3 - clipboard changed checks

This is really only implemented on Windows and Mac.
Comment 52 Dmitry Alexeenko [MSFT] 2012-06-21 17:54:42 PDT
Just wondering if you guys have some approx. timeline in mind for this fix?

Also - will clipboardData contain DataTransferItemList, so clipboardData users can look through it to see what kind of content is in clipboardData?
Comment 53 Neil Deakin (away until Oct 3) 2012-06-22 05:17:23 PDT
(In reply to Dmitry Alexeenko [MSFT] from comment #52)
> Also - will clipboardData contain DataTransferItemList, so clipboardData
> users can look through it to see what kind of content is in clipboardData?

It will not. But you can find out what kind of content is there with clipboardData.types.
Comment 54 Steve Lancashire 2012-08-01 18:51:46 PDT
Is this still being worked on? Any chance of an update?  Is there a target milestone for this?
Comment 55 :Ehsan Akhgari 2012-08-16 14:41:39 PDT
Comment on attachment 633602 [details] [diff] [review]
Part 1 - add clipboardData to cut/copy/paste events

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

Looks good overall, although I did not do an in depth review.

::: content/base/src/nsCopySupport.cpp
@@ +706,5 @@
>    if (!nsContentUtils::IsSafeToRunScript())
>      return false;
>  
> +  // next, fire the cut, copy or paste event
> +  // XXXndeakin why was a preference added here without a running-in-chrome check?

Worth filing a bug for!

::: content/events/src/nsDOMClipboardEvent.cpp
@@ +104,5 @@
> +{
> +  nsClipboardEvent* event = static_cast<nsClipboardEvent*>(mEvent);
> +
> +  if (!mEventIsInternal && !event->clipboardData) {
> +    // XXXndeakin is this ever called?

Hmm, this should not happen...  Why not MOZ_ASSERT that?

@@ +121,5 @@
> +  nsDOMClipboardEvent* it =
> +    new nsDOMClipboardEvent(aPresContext, aEvent);
> +  if (nsnull == it) {
> +    return NS_ERROR_OUT_OF_MEMORY;
> +  }

Nit: the null check is unnecessary.

::: content/events/src/nsDOMDataTransfer.cpp
@@ +976,5 @@
> +  NS_ASSERTION(mEventType != NS_CUT && mEventType != NS_COPY,
> +               "clipboard event with empty data");
> +
> +  nsCOMPtr<nsITransferable> trans =
> +    do_CreateInstance("@mozilla.org/widget/transferable;1");

Note that you need to init this transferable.

::: content/events/src/nsDOMEvent.cpp
@@ +702,5 @@
> +    case NS_CLIPBOARD_EVENT:
> +    {
> +      nsClipboardEvent* oldClipboardEvent = static_cast<nsClipboardEvent*>(mEvent);
> +      nsClipboardEvent* clipboardEvent = new nsClipboardEvent(false, msg);
> +      NS_ENSURE_TRUE(clipboardEvent, NS_ERROR_OUT_OF_MEMORY);

clipboardEvent will never be null.
Comment 56 :Ehsan Akhgari 2012-08-16 14:44:53 PDT
Comment on attachment 633603 [details] [diff] [review]
Part 2 - global clipboardData property

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

Why do we want to implement the global clipboardData property?
Comment 57 :Ehsan Akhgari 2012-08-16 14:46:59 PDT
(In reply to Ehsan Akhgari [:ehsan] from comment #56)
> Comment on attachment 633603 [details] [diff] [review]
> Part 2 - global clipboardData property
> 
> Review of attachment 633603 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Why do we want to implement the global clipboardData property?

To be clear, I did read comment 34 (and also you blog post here http://enndeakin.wordpress.com/2010/03/04/clipboard-data/) but I am not convinced that this is a good idea.  For usage within the Mozilla applications, chrome code can continue to use nsIClipboard.
Comment 58 :Ehsan Akhgari 2012-08-16 14:52:22 PDT
Comment on attachment 633604 [details] [diff] [review]
Part 3 - clipboard changed checks

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

This looks mostly good as well.

Again, so sorry that it me so long to get back to you on this.

::: widget/qt/nsClipboard.cpp
@@ +563,5 @@
> +  }
> +  else {
> +    *aChanged = false;
> +  }
> +  return NS_OK;

Perhaps return NS_ERROR_NOT_IMPLEMENTED?
Comment 59 Julian Viereck 2012-10-04 01:34:51 PDT
This would be good to have for PDF.JS when copy selected text.

How much is required to make this land? Can we move the "global clipboardData property" decision to another bug and make the "Part 1" and "Part 3" land?
Comment 60 :Ehsan Akhgari 2012-10-04 17:07:34 PDT
(In reply to comment #59)
> This would be good to have for PDF.JS when copy selected text.
> 
> How much is required to make this land? Can we move the "global clipboardData
> property" decision to another bug and make the "Part 1" and "Part 3" land?

We just need someone to finish up the patches here.  Neil, do you have time to do that?
Comment 61 Neil Deakin (away until Oct 3) 2012-10-17 06:04:15 PDT
(In reply to Ehsan Akhgari [:ehsan] from comment #57)

> To be clear, I did read comment 34 (and also you blog post here
> http://enndeakin.wordpress.com/2010/03/04/clipboard-data/) but I am not
> convinced that this is a good idea.  For usage within the Mozilla
> applications, chrome code can continue to use nsIClipboard.

The intent is to remove/deprecate it and nsITransferable in favour of nsIDOMDataTransfer to reduce duplicate and complicated code. That said, I could just add a clipboardData property to nsIClipboard instead.
Comment 62 Neil Deakin (away until Oct 3) 2012-10-17 06:06:10 PDT
(In reply to Julian Viereck from comment #59)
> How much is required to make this land? Can we move the "global
> clipboardData property" decision to another bug and make the "Part 1" and
> "Part 3" land?

Without Part 2, part 3 isn't needed. Part 3 doesn't yet work properly on Linux as described in comment 35 and I don't know how to fix it.
Comment 63 :Ehsan Akhgari 2012-10-17 07:47:16 PDT
I think it's worth trying to land the first part here which is going to provide a lot of value to web developers, and move parts 2 and 3 to another bug and try to work on them to get them landed separately.  Do you agree?  If yes, it would be great if you can address my previous comments on part 1 and submit a patch for review.  Thanks!
Comment 64 Neil Deakin (away until Oct 3) 2012-10-22 12:01:05 PDT
Created attachment 673950 [details] [diff] [review]
Latest patch

OK, here is the latest patch that addresses the feedback above.
Comment 65 Julian Viereck 2012-10-22 12:27:34 PDT
(In reply to Neil Deakin from comment #64)
> Created attachment 673950 [details] [diff] [review]
> Latest patch
> 
> OK, here is the latest patch that addresses the feedback above.

Awesome! Do you want to setup someone for review on the patch?
Comment 66 :Ehsan Akhgari 2012-10-22 13:28:31 PDT
(In reply to comment #65)
> (In reply to Neil Deakin from comment #64)
> > Created attachment 673950 [details] [diff] [review]
> > Latest patch
> > 
> > OK, here is the latest patch that addresses the feedback above.
> 
> Awesome! Do you want to setup someone for review on the patch?

I can do the review if the patch is ready for review, Neil.
Comment 67 Neil Deakin (away until Oct 3) 2012-10-25 10:27:43 PDT
Comment on attachment 673950 [details] [diff] [review]
Latest patch

You can review it if you like. The spec pointed to in comment 36 appears to have changed quite a bit since then so this patch likely matches it less than I mentioned earlier.
Comment 68 :Ehsan Akhgari 2012-10-30 16:45:50 PDT
Comment on attachment 673950 [details] [diff] [review]
Latest patch

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

Apologies for the delay.  r=me with the below addressed.

::: content/base/src/nsCopySupport.cpp
@@ +709,5 @@
>    if (!nsContentUtils::IsSafeToRunScript())
>      return false;
>  
> +  // next, fire the cut, copy or paste event
> +  // XXXndeakin why was a preference added here without a running-in-chrome check?

Please file a follow-up on this, thanks!

@@ +716,4 @@
>    if (Preferences::GetBool("dom.event.clipboardevents.enabled", true)) {
> +    clipboardData = new nsDOMDataTransfer(aType, aType == NS_PASTE);
> +    if (!clipboardData)
> +      return false;

No need for the null check.

::: content/events/src/nsDOMClipboardEvent.h
@@ +33,5 @@
> + * the provisions above, a recipient may use your version of this file under
> + * the terms of any one of the MPL, the GPL or the LGPL.
> + *
> + * ***** END LICENSE BLOCK ***** */
> +

Nit: please use the MPL2 header in the new files.

::: content/events/src/nsDOMDataTransfer.cpp
@@ +223,5 @@
>  {
>    *aFileList = nullptr;
>  
> +  if (mEventType != NS_DRAGDROP_DROP && mEventType != NS_DRAGDROP_DRAGDROP &&
> +      mEventType != NS_PASTE)

Hmm, this seems wrong.  I think you're missing a return statement.

@@ +688,5 @@
> +                                   nsITransferable** aTransferable)
> +{
> +  *aTransferable = nullptr;
> +
> +  nsTArray<TransferItem>& item = mItems[aIndex];

As a precaution against someone modifying |item| by accident in the future, please declare it as const.

@@ +696,5 @@
> +
> +  nsCOMPtr<nsITransferable> transferable =
> +    do_CreateInstance("@mozilla.org/widget/transferable;1");
> +  if (!transferable)
> +    return;

Hmm, I think do_CreateInstance should be infallible these days... But I'm not sure...

@@ +701,5 @@
> +  transferable->Init(aLoadContext);
> +
> +  bool added = false;
> +  for (uint32_t f = 0; f < count; f++) {
> +    TransferItem& formatitem = item[f];

Ditto on const-ness.

@@ +956,5 @@
> +  // there isn't a way to get a list of the formats that might be available on
> +  // all platforms, so just check for the types that can actually be imported
> +  const char* formats[] = { kFileMime, kHTMLMime, kURLMime, kURLDataMime, kUnicodeMime };
> +
> +  for (uint32_t f = 0; f < NS_ARRAY_LENGTH(formats); f++) {

Nit: Please use mozilla::ArrayLength.

@@ +1000,5 @@
> +    do_CreateInstance("@mozilla.org/widget/transferable;1");
> +  if (!trans)
> +    return;
> +
> +  trans->Init(nullptr);

I think it's a good idea to initialize this transferable with the document's load context for consistency.

::: content/events/src/nsDOMDataTransfer.h
@@ +82,5 @@
> +  // paste or a drag that was started without using a data transfer. The
> +  // latter will occur when an external drag occurs, that is, a drag where the
> +  // source is another application, or a drag is started by calling the drag
> +  // service directly.
> +  nsDOMDataTransfer(uint32_t aEventType, bool aIsExternal);

Hmm, please use an enum instead of a bool here, so that the callers look like:

  new nsDOMDataTransfer(foo, nsDOMDataTransfer::external/internal);

@@ +101,5 @@
>                          nsIDOMNode* aDragTarget);
>  
> +  // converts the data for a single item at aIndex into an nsITransferable object.
> +  void GetTransferable(uint32_t aIndex, nsILoadContext* aLoadContext,
> +                       nsITransferable** aTransferable);

Please return an already_AddRefed.

::: dom/interfaces/events/nsIDOMClipboardEvent.idl
@@ +53,5 @@
> +  void initClipboardEventNS(in DOMString namespaceURIArg,
> +                            in DOMString typeArg,
> +                            in boolean canBubbleArg,
> +                            in boolean cancelableArg,
> +                            in nsIDOMDataTransfer aClipboardData);

None of these two methods is mentioned in the spec any more, so we should be able to remove them both.
Comment 69 Neil Deakin (away until Oct 3) 2012-11-01 13:00:41 PDT
> > +    do_CreateInstance("@mozilla.org/widget/transferable;1");
> > +  if (!trans)
> > +    return;
> > +
> > +  trans->Init(nullptr);
> 
> I think it's a good idea to initialize this transferable with the document's
> load context for consistency.
> 

The document is always null here.


> Hmm, please use an enum instead of a bool here, so that the callers look
> like:
> 
>   new nsDOMDataTransfer(foo, nsDOMDataTransfer::external/internal);

Blech!
Comment 70 Julian Viereck 2013-01-18 12:41:01 PST
Any updates on this one?
Comment 71 Julian Viereck 2013-02-07 04:22:15 PST
Created attachment 711261 [details] [diff] [review]
Rebased patch and address some review comments

This is a rebased version of "673950: Latest patch". I don't really understand the code Neil did and therefore "just" made the patches apply. It didn't loooked like the code didn't apply because there were a lot of changes, so this might be all good.

I've also tried to address the review comments made by :ehsan in comment #68. Some notes there:

> ::: content/base/src/nsCopySupport.cpp
> @@ +709,5 @@
> >    if (!nsContentUtils::IsSafeToRunScript())
> >      return false;
> >  
> > +  // next, fire the cut, copy or paste event
> > +  // XXXndeakin why was a preference added here without a running-in-chrome check?
> 
> Please file a follow-up on this, thanks!

I didn't do this one. @Neil, have you done this already/could you please do this?

> ::: content/events/src/nsDOMDataTransfer.cpp
> @@ +223,5 @@
> >  {
> >    *aFileList = nullptr;
> >  
> > +  if (mEventType != NS_DRAGDROP_DROP && mEventType != NS_DRAGDROP_DRAGDROP &&
> > +      mEventType != NS_PASTE)
> 
> Hmm, this seems wrong.  I think you're missing a return statement.

I've just placed a "return NS_OK" but have no clue if that's the right things to do there. Please double check!

> ::: content/events/src/nsDOMDataTransfer.h
> @@ +82,5 @@
> > +  // paste or a drag that was started without using a data transfer. The
> > +  // latter will occur when an external drag occurs, that is, a drag where the
> > +  // source is another application, or a drag is started by calling the drag
> > +  // service directly.
> > +  nsDOMDataTransfer(uint32_t aEventType, bool aIsExternal);
> 
> Hmm, please use an enum instead of a bool here, so that the callers look
> like:
> 
>   new nsDOMDataTransfer(foo, nsDOMDataTransfer::external/internal);

Can you please give me a pointer on how to implement this? I guess it should be done using an enum, but then I don't know how to make it be on the |nsDOMDataTransfer| directly. Should the enum be named something like |nsDOMDataTransferType| instead?

> @@ +101,5 @@
> >                          nsIDOMNode* aDragTarget);
> >  
> > +  // converts the data for a single item at aIndex into an nsITransferable object.
> > +  void GetTransferable(uint32_t aIndex, nsILoadContext* aLoadContext,
> > +                       nsITransferable** aTransferable);
> 
> Please return an already_AddRefed.

I don't understand this one. The function |GetTransferable| returns a void and already_AddRefed expects a pointer, right?
Comment 72 Josh Matthews [:jdm] 2013-02-07 04:24:23 PST
>> @@ +101,5 @@
>> >                          nsIDOMNode* aDragTarget);
>> >  
>> > +  // converts the data for a single item at aIndex into an nsITransferable object.
>> > +  void GetTransferable(uint32_t aIndex, nsILoadContext* aLoadContext,
>> > +                       nsITransferable** aTransferable);
>> 
>> Please return an already_AddRefed.
>
>I don't understand this one. The function |GetTransferable| returns a void and 
>already_AddRefed expects a pointer, right?

For non-XPCOM functions, it's preferable to return an already_AddRefed pointer (instead of void) and avoid using outparams.
Comment 73 Julian Viereck 2013-02-07 04:25:13 PST
Created attachment 711262 [details]
Updated testcase that uses onCut and onCopy as well.
Comment 74 Julian Viereck 2013-02-07 05:14:07 PST
Created attachment 711279 [details]
Updated testcase that uses onCut and onCopy as well. [bugfix]

Same as in "711262: Updated testcase that uses onCut and onCopy as well.", but get the string right when copy/cut the selection.
Comment 75 Julian Viereck 2013-02-07 05:18:14 PST
Created attachment 711280 [details]
Rebase v2 with already_AddRefed

(In reply to Josh Matthews [:jdm] from comment #72)
> >> @@ +101,5 @@
> >> >                          nsIDOMNode* aDragTarget);
> >> >  
> >> > +  // converts the data for a single item at aIndex into an nsITransferable object.
> >> > +  void GetTransferable(uint32_t aIndex, nsILoadContext* aLoadContext,
> >> > +                       nsITransferable** aTransferable);
> >> 
> >> Please return an already_AddRefed.
> >
> >I don't understand this one. The function |GetTransferable| returns a void and 
> >already_AddRefed expects a pointer, right?
> 
> For non-XPCOM functions, it's preferable to return an already_AddRefed
> pointer (instead of void) and avoid using outparams.

This patch moves the outparams as a return value. Thanks Josh for that hint! 

The other open questions from attachment #711261 [details] [diff] [review] still apply. 

I've changed the already existing GetTransferables function as well to return an already_AddRefed and had to do a new change:

+++ b/content/events/src/nsEventStateManager.cpp
@@ -2312,18 +2313,17 @@
     action = nsIDragService::DRAGDROP_ACTION_COPY |
              nsIDragService::DRAGDROP_ACTION_MOVE |
              nsIDragService::DRAGDROP_ACTION_LINK;
 
   // get any custom drag image that was set
   int32_t imageX, imageY;
   nsIDOMElement* dragImage = aDataTransfer->GetDragImage(&imageX, &imageY);
 
-  nsCOMPtr<nsISupportsArray> transArray;
-  aDataTransfer->GetTransferables(getter_AddRefs(transArray), dragTarget);
+  nsCOMPtr<nsISupportsArray> transArray = aDataTransfer->GetTransferables(dragTarget);
   if (!transArray)
     return false;
 
   // XXXndeakin don't really want to create a new drag DOM event
   // here, but we need something to pass to the InvokeDragSession
   // methods.
   nsCOMPtr<nsIDOMEvent> domEvent;
   NS_NewDOMDragEvent(getter_AddRefs(domEvent), aPresContext, aDragEvent);
Comment 76 Neil Deakin (away until Oct 3) 2013-02-07 05:59:39 PST
Thanks! By coincidence I had updated the patch yesterday as well, but your version is fine too. It looks like you generated it by ignoring whitespace though. This makes it harder to read when reviewing so generally it should only be done if the reviewer requests it.

> 
> I didn't do this one. @Neil, have you done this already/could you please do
> this?

Another bug should be filed for this.


> > Hmm, this seems wrong.  I think you're missing a return statement.
> 
> I've just placed a "return NS_OK" but have no clue if that's the right
> things to do there. Please double check!

return NS_OK is correct here.


> Can you please give me a pointer on how to implement this? I guess it should
> be done using an enum, but then I don't know how to make it be on the
> |nsDOMDataTransfer| directly. Should the enum be named something like
> |nsDOMDataTransferType| instead?

I think this is a silly request so I just ignored it.


I never finished this patch as I reviewed in more details compared to the spec at http://www.w3.org/TR/clipboard-apis/ and for compatibility with other browsers.

For part 2 of the patches, I think I'm going to just add a clipboardData property to nsIClipboard instead for now.
Comment 77 Neil Deakin (away until Oct 3) 2013-02-07 18:41:58 PST
I don't think we need another round of review here. I did some testing today and there are a only minor differences between this patch and Chrome/Webkit, all of which could be handled in followup bugs. Julian, could you update the patch, and run it on a tryserver if you haven't already and then we can check this in. Maybe Ehsan can comment if he thinks there are any issues here.

- If you cache the clipboardData object and access it outside of the event path, Chrome clears it and makes changes have no effect. We could just make it readonly.
- oncut="event.preventDefault()" prevents cutting in our patch. On Chrome, this causes an oridinary cut (which is incorrect according to the spec)
- Chrome implements the DataTransferItemList stuff from the drag and drop spec but for some reason, pasted data is often duplicated, meaning two items exist, or an extra empty item exists. Since this is described by a different spec and isn't actually needed for clipboard operations as they only contain one item, this feature doesn't need to be handled here. 
- html is formatted quite differently between both browsers. The spec contains some sanitizing and formatting text that could be done in a followup bug.

Other spec differences:
- A followup bug would be to add the clipboard event constructors. Or, probably just remove them from the spec.
- the spec provides a mechanism to clear the clipboard or remove specific types. Not sure why.
- the spec wants clipboard events to fire on keydown. This section should be removed as this behaviour is platform specific. On Mac, for example, keyboard shortcuts fire on keypress, and repeat if you hold the key down.
Comment 78 Julian Viereck 2013-02-09 08:50:25 PST
Created attachment 712145 [details] [diff] [review]
Rebase v3

This is the patch I wanted to push to try but there seems to be an issue with my account.
Comment 79 Julian Viereck 2013-02-09 09:41:44 PST
:RyanVM pushed the v3 patch to try: https://tbpl.mozilla.org/?tree=Try&rev=3214821a9727
Comment 80 Julian Viereck 2013-02-10 12:00:14 PST
(In reply to Julian Viereck from comment #79)
> :RyanVM pushed the v3 patch to try:
> https://tbpl.mozilla.org/?tree=Try&rev=3214821a9727

There is one test failing on Android that might be related to this patch:

1918 ERROR TEST-UNEXPECTED-FAIL | /tests/dom/tests/mochitest/general/test_clipboard_events.html | paste text/x-moz-url multiple types - got "", expected "http://www.mozilla.org"

See:

  https://tbpl.mozilla.org/php/getParsedLog.php?id=19608455&tree=Try

Neil, any idea?
Comment 81 Neil Deakin (away until Oct 3) 2013-02-10 18:39:24 PST
I would guess that our android clipboard code doesn't handle x-moz-url. If that's the case, we should just disable that line of the test on Android.
Comment 82 Julian Viereck 2013-02-11 06:40:05 PST
From IRC: http://irclog.gr/#show/irc.mozilla.org/mobile/234940

  jviereck	Does the firefox for android clipboard code handle x-moz-url? See this comment: https://bugzilla.mozilla.org/show_bug.cgi?id=407983#c81
  mfinkle	jviereck, it does not explicitly handle it
  jviereck	mfinkle: do you think it's okay to diable the test for Android then and open a follow up?
  mfinkle	jviereck, yes

Will open a follow up bug and disable the unit test.
Comment 83 Julian Viereck 2013-02-11 09:03:39 PST
Created attachment 712483 [details] [diff] [review]
Disable failing test on Android

Disable the failing test as discussed in comment #81.

Push to try: https://tbpl.mozilla.org/?tree=Try&rev=abb606d64923
Comment 84 Julian Viereck 2013-02-12 01:31:19 PST
(In reply to Julian Viereck from comment #83)
>
> Push to try: https://tbpl.mozilla.org/?tree=Try&rev=abb606d64923

Look greenish to me.

@Neil, can we set checkin-needed?
Comment 85 Neil Deakin (away until Oct 3) 2013-02-12 06:40:05 PST
Comment on attachment 712483 [details] [diff] [review]
Disable failing test on Android

Yes, this looks good. Since Ehsan's away, I'm going to ask for superreview first then we should be good to check in.
Comment 86 Olli Pettay [:smaug] (TPAC) 2013-02-12 15:29:30 PST
Comment on attachment 712483 [details] [diff] [review]
Disable failing test on Android


>-  // next, fire the cut or copy event
>+  // next, fire the cut, copy or paste event
>+  // XXXndeakin why was a preference added here without a running-in-chrome check?
Please file a followup for the XXXndeakin

>+  bool doDefault = true;
>+  nsRefPtr<nsDOMDataTransfer> clipboardData;
>   if (Preferences::GetBool("dom.event.clipboardevents.enabled", true)) {
>+    clipboardData = new nsDOMDataTransfer(aType, aType == NS_PASTE);
>+
>     nsEventStatus status = nsEventStatus_eIgnore;
>-    nsEvent evt(true, aType);
>-    nsEventDispatcher::Dispatch(content, presShell->GetPresContext(), &evt, nullptr,
>-                                &status);
>-    // if the event was cancelled, don't do the clipboard operation
>-    if (status == nsEventStatus_eConsumeNoDefault)
>-      return false;
>+    nsClipboardEvent event(true, aType);
>+    event.clipboardData = clipboardData;
>+    presShell->HandleDOMEventWithTarget(content, &event, &status);
>+    doDefault = (status != nsEventStatus_eConsumeNoDefault);
>   }
Why s/nsEventDispatcher::Dispatch/HandleDOMEventWithTarget/ ?


>   
>-  if (presShell->IsDestroying())
>-    return false;
>-
>   // No need to do anything special during a paste. Either an event listener
>   // took care of it and cancelled the event, or the caller will handle it.
>-  // Return true to indicate the event wasn't cancelled.
>-  if (aType == NS_PASTE)
>-    return true;
>+  // Return true to indicate that the event wasn't cancelled.
>+  if (aType == NS_PASTE) {
>+    // Clear and mark the clipboardData as readonly. This prevents someone
>+    // from reading the clipboard contents after the paste event has fired.
>+    clipboardData->ClearAll();
>+    clipboardData->SetReadOnly();
>+    return doDefault;
>+  }
Hmm, how do browsers handle modal dialogs happening during paste event?

>-  if (NS_FAILED(nsCopySupport::HTMLCopy(sel, doc, nsIClipboard::kGlobalClipboard)))
>+    rv = HTMLCopy(sel, doc, nsIClipboard::kGlobalClipboard);
>+    if (NS_FAILED(rv))
>     return false;
if (expr) {
  stmt;
}


>+  }
>+  else {
else { should be in the previous line


>-  // the effect of updating the paste menu item.
>+  // the effect of updating the enabled state of the paste menu item.
>+  if (doDefault || count)
>   piWindow->UpdateCommands(NS_LITERAL_STRING("clipboard"));
if (expr) {
  stmt;
}

> 		nsDOMNotifyAudioAvailableEvent.cpp \
> 		nsDOMSimpleGestureEvent.cpp \
> 		nsDOMEventTargetHelper.cpp \
> 		nsDOMScrollAreaEvent.cpp \
> 		nsDOMTransitionEvent.cpp \
> 		nsDOMAnimationEvent.cpp \
> 		nsDOMTouchEvent.cpp \
> 		nsDOMCompositionEvent.cpp \
>+        nsDOMClipboardEvent.cpp \
Align properly


>+nsDOMClipboardEvent::nsDOMClipboardEvent(nsPresContext* aPresContext,
>+                                         nsClipboardEvent* aEvent)
>+  : nsDOMEvent(aPresContext, aEvent ? aEvent :
>+               new nsClipboardEvent(false, 0))
>+{
>+  if (aEvent) {
>+    mEventIsInternal = false;
>+  }
>+  else {
else { to the previous line


>+nsDOMClipboardEvent::~nsDOMClipboardEvent()
>+{
>+  if (mEventIsInternal) {
>+    if (mEvent->eventStructType == NS_CLIPBOARD_EVENT) {
Perhaps merge the expressions within the ifs so that there would be just
one if


>+class nsDOMClipboardEvent : public nsIDOMClipboardEvent,
>+                            public nsDOMEvent
Inherit nsDOMEvent first



>+  else if (mIsExternal) {
should be in the previous line

>+    if (aEventType == NS_PASTE) {
>+      CacheExternalClipboardFormats();
>+    }
>+    else if (aEventType >= NS_DRAGDROP_EVENT_START && aEventType <= NS_DRAGDROP_LEAVE_SYNTH) {
ditto

>+  // only the first item is valid for clipboard events
>+  if (aIndex > 0 &&
>+      (mEventType == NS_CUT || mEventType == NS_COPY || mEventType == NS_PASTE))
>+    return NS_ERROR_DOM_INDEX_SIZE_ERR;
if (exptr) {
  stmt;
}

>+

>+  // only the first item is valid for clipboard events
>+  if (aIndex > 0 &&
>+      (mEventType == NS_CUT || mEventType == NS_COPY || mEventType == NS_PASTE))
>+    return NS_ERROR_DOM_INDEX_SIZE_ERR;
ditto.
Also, s/only/Only/


>+  // only the first item is valid for clipboard events
>+  if (aIndex > 0 &&
>+      (mEventType == NS_CUT || mEventType == NS_COPY || mEventType == NS_PASTE))
>+    return NS_ERROR_DOM_INDEX_SIZE_ERR;
ditto


>+  // only the first item is valid for clipboard events
>+  if (aIndex > 0 &&
>+      (mEventType == NS_CUT || mEventType == NS_COPY || mEventType == NS_PASTE))
>+    return NS_ERROR_DOM_INDEX_SIZE_ERR;
ditto



>+nsDOMDataTransfer::GetTransferable(uint32_t aIndex, nsILoadContext* aLoadContext)
>+{
>+  nsTArray<TransferItem>& item = mItems[aIndex];
>     uint32_t count = item.Length();
>     if (!count)
>-      continue;
>+    return nullptr;
missing indentation, and while you're here,
if (expr) {
  stmt;
}


>+  bool added = false;
>     for (uint32_t f = 0; f < count; f++) {
>-      TransferItem& formatitem = item[f];
>+    const TransferItem& formatitem = item[f];
Missing indentation?

>       nsresult rv = transferable->SetTransferData(format, convertedData, length);
>       if (NS_FAILED(rv))
>-        return;
>+      return nullptr;
same here


>+  for (uint32_t f = 0; f < mozilla::ArrayLength(formats); f++) {
++f please


>+[scriptable, builtinclass, uuid(8D92944A-F2E5-41F4-9CF3-D85043B90CAC)]
>+interface nsIDOMClipboardEvent : nsIDOMEvent
>+{
>+  readonly attribute nsIDOMDataTransfer clipboardData;
>+
>+};

Per spec there should be a dictionary for ClipboardEvent
Comment 87 Julian Viereck 2013-02-14 11:53:45 PST
Created attachment 714018 [details]
Right patch to disable failing test on Android

The patch "712483: Disable failing test on Android" was the same as "712145: Rebase v3". This patch is the patch you're looking for to disable the failing test.  

This is the second patch together with "712145: Rebase v3" that I've pushed to try in https://tbpl.mozilla.org/?tree=Try&rev=abb606d64923.
Comment 88 Julian Viereck 2013-02-14 13:00:43 PST
(In reply to Olli Pettay [:smaug] from comment #86)
> Comment on attachment 712483 [details] [diff] [review]
> [...]

Neil, what's your plan on how to handle the feedback from :smaug? I can improve the patch in terms of the "missing indentions", "if (expr) { ...}" and such. Can you handle:

- Please file a followup for the XXXndeakin
- Hmm, how do browsers handle modal dialogs happening during paste event?
- Per spec there should be a dictionary for ClipboardEvent
- Why s/nsEventDispatcher::Dispatch/HandleDOMEventWithTarget/ ?

then? Or do you think it's makes more sense if you do all the fixes on your own?
Comment 89 Neil Deakin (away until Oct 3) 2013-02-19 09:40:25 PST
Julian, if you could please update the patch as much as you can, then I can update the remaining issues, thanks.
Comment 90 Julian Viereck 2013-02-23 03:01:12 PST
Created attachment 717492 [details] [diff] [review]
WIP 4 - fix nits

This patch fixes the review-style comments made by :smaug in comment #86.

Neil, I leave the open issues raised by :smaug in comment #86 to you as per comment #89.
Comment 91 Neil Deakin (away until Oct 3) 2013-02-27 06:33:12 PST
Created attachment 718975 [details] [diff] [review]
Add support for clipboardData in cut/copy/paste events
Comment 92 Neil Deakin (away until Oct 3) 2013-02-27 06:36:20 PST
Created attachment 718977 [details] [diff] [review]
Add clipboard constructor
Comment 93 Olli Pettay [:smaug] (TPAC) 2013-03-01 05:47:04 PST
Comment on attachment 718977 [details] [diff] [review]
Add clipboard constructor

Could you  add [noscript] void InitClipboardEvent to nsIDOMClipboardEvent interface and just call that in InitFromCtor.
That would be consistent with other events.
Comment 94 Olli Pettay [:smaug] (TPAC) 2013-03-01 06:21:28 PST
Comment on attachment 718975 [details] [diff] [review]
Add support for clipboardData in cut/copy/paste events

>+NS_IMETHODIMP
>+nsDOMClipboardEvent::GetClipboardData(nsIDOMDataTransfer** aClipboardData)
>+{
>+  nsClipboardEvent* event = static_cast<nsClipboardEvent*>(mEvent);
>+
>+  MOZ_ASSERT(mEventIsInternal || event->clipboardData);
>+  if (!mEventIsInternal && !event->clipboardData) {
>+    event->clipboardData =
>+      new nsDOMDataTransfer(event->message, event->message == NS_PASTE);
>+  }
I don't understand this. We assert that either event is internal or we have
clipboard data, yet we have 'if' for the case when we have non-internal event
missing clipboard data.

>+nsDOMDataTransfer::GetTransferable(uint32_t aIndex, nsILoadContext* aLoadContext)
>+{
>+  nsTArray<TransferItem>& item = mItems[aIndex];
This is tiny bit error prone. Could you add a check that aIndex valid
>+    if (supported) {
>+      if (strcmp(formats[f], kUnicodeMime) == 0) {
>+        SetDataWithPrincipal(NS_LITERAL_STRING("text/plain"), nullptr, 0, nullptr);
>+      }
>+      else {
if (expr) {
  stmt;
} else {
  stmt2;
}


>+  if (mEventType == NS_PASTE) {
>+    MOZ_ASSERT(aIndex == 0, "index in clipboard must be 0");
>+
>+    nsCOMPtr<nsIClipboard> clipboard = do_GetService("@mozilla.org/widget/clipboard;1");
>+    if (!clipboard) {
>+      return;
>+    }
>+
>+    clipboard->GetData(trans, nsIClipboard::kGlobalClipboard);
>+  }
>+  else {
ditto

>+  // converts the data for a single item at aIndex into an nsITransferable object.
>+  already_AddRefed<nsITransferable>  GetTransferable(uint32_t aIndex,
>+                            nsILoadContext* aLoadContext);
align parameters
Comment 95 Neil Deakin (away until Oct 3) 2013-03-01 14:20:55 PST
Created attachment 720140 [details] [diff] [review]
Part 1 -  Add support for clipboardData in cut/copy/paste events
Comment 96 Olli Pettay [:smaug] (TPAC) 2013-03-04 11:56:30 PST
Comment on attachment 720140 [details] [diff] [review]
Part 1 -  Add support for clipboardData in cut/copy/paste events


>+    // if the format is supported, add an item to the array with null as
>+    // the data. When retrieved, GetRealData will read the data.
>+    if (supported) {
>+      if (strcmp(formats[f], kUnicodeMime) == 0) {
>+        SetDataWithPrincipal(NS_LITERAL_STRING("text/plain"), nullptr, 0, nullptr);
>+      } else {
>+        if (strcmp(formats[f], kURLDataMime) == 0)
>+          SetDataWithPrincipal(NS_LITERAL_STRING("text/uri-list"), nullptr, 0, nullptr);
>+        SetDataWithPrincipal(NS_ConvertUTF8toUTF16(formats[f]), nullptr, 0, nullptr);
>+      }
I really wish we wouldn't almost copy-paste this code from nsDOMDataTransfer::CacheExternalFormats()
Also, why null principal?
Comment 97 Piyush Agarwal 2013-03-05 10:00:15 PST
@Neil Deakin - Is this patch being considered for the ESR and Latest Stable releases? If so is there an ETA for adding this support?

Thanks!
Piyush
Comment 98 Olli Pettay [:smaug] (TPAC) 2013-03-05 10:08:28 PST
This is certainly a feature which shouldn't go to stable ESR17, nor FF19/20/21.
Needs normal testing.
Comment 99 Piyush Agarwal 2013-03-05 14:14:41 PST
@Olli Pettay, I understand. Unfortunately we have a use case where we have users pasting text into a div that is editable, but for the paste content we would like the paste to go through as plain text rather than keeping the full HTML styles etc.. (Which I can do via clipboard in the other browsers.) Unfortunately I have been unable to figure out what code a textarea runs when rich/html text is pasted in it to format it, so I cannot reproduce the same formatting when trying to do it after the default paste goes through. Any thoughts? (Or I can open a new topic somewhere if that should be discussed elsewhere).
Comment 100 Neil Deakin (away until Oct 3) 2013-03-06 12:14:19 PST
Created attachment 721845 [details] [diff] [review]
Part 1 - Add support for clipboardData in cut/copy/paste events
Comment 101 Neil Deakin (away until Oct 3) 2013-03-06 12:19:08 PST
Created attachment 721848 [details] [diff] [review]
Part 2 - add clipboard constructor
Comment 102 :Ehsan Akhgari 2013-03-06 12:29:26 PST
(In reply to comment #99)
> @Olli Pettay, I understand. Unfortunately we have a use case where we have
> users pasting text into a div that is editable, but for the paste content we
> would like the paste to go through as plain text rather than keeping the full
> HTML styles etc.. (Which I can do via clipboard in the other browsers.)
> Unfortunately I have been unable to figure out what code a textarea runs when
> rich/html text is pasted in it to format it, so I cannot reproduce the same
> formatting when trying to do it after the default paste goes through. Any
> thoughts? (Or I can open a new topic somewhere if that should be discussed
> elsewhere).

A common workaround for doing this kind of thing before this bug is fixed to put focus into a textarea immediately when pasting to make sure that the clipboard contents are pasted into the textarea, and then extract the value of the textarea and use it inside your contentEditable element (see <http://stackoverflow.com/questions/15125217/convert-html-to-plain-text-in-contenteditable> for example).  This sucks but unfortunately there is no better workaround that I know of, and this fix is definitely not something that we can take on stable branches...
Comment 103 Piyush Agarwal 2013-03-06 12:54:41 PST
(In reply to :Ehsan Akhgari from comment #102)
> A common workaround for doing this kind of thing before this bug is fixed to
> put focus into a textarea immediately when pasting to make sure that the
> clipboard contents are pasted into the textarea, and then extract the value
> of the textarea and use it inside your contentEditable element (see
> <http://stackoverflow.com/questions/15125217/convert-html-to-plain-text-in-
> contenteditable> for example).  This sucks but unfortunately there is no
> better workaround that I know of, and this fix is definitely not something
> that we can take on stable branches...

Hello Ehsan, yep we actually do that for the keyboard ctrl-v/shift-ins paste event. But we need this functionality to work for the right click and edit-paste menu options as well. If I try to do the focus redirection in the actual paste event (which is the only event I see from the menu pastes), then the redirect never happens. The paste still occurs on the original div.
Comment 104 :Ehsan Akhgari 2013-03-06 14:49:15 PST
(In reply to comment #103)
> (In reply to :Ehsan Akhgari from comment #102)
> > A common workaround for doing this kind of thing before this bug is fixed to
> > put focus into a textarea immediately when pasting to make sure that the
> > clipboard contents are pasted into the textarea, and then extract the value
> > of the textarea and use it inside your contentEditable element (see
> > <http://stackoverflow.com/questions/15125217/convert-html-to-plain-text-in-
> > contenteditable> for example).  This sucks but unfortunately there is no
> > better workaround that I know of, and this fix is definitely not something
> > that we can take on stable branches...
> 
> Hello Ehsan, yep we actually do that for the keyboard ctrl-v/shift-ins paste
> event. But we need this functionality to work for the right click and
> edit-paste menu options as well. If I try to do the focus redirection in the
> actual paste event (which is the only event I see from the menu pastes), then
> the redirect never happens. The paste still occurs on the original div.

Unfortunately I don't know of a work-around for that.
Comment 107 Alex Keybl [:akeybl] 2013-04-05 11:59:28 PDT
This bug has been listed as part of the Aurora 22 release notes in either [1], [2], or both. If you find any of the information or wording incorrect in the notes, please email release-mgmt@mozilla.com asap. Thanks!

[1] https://www.mozilla.org/en-US/firefox/22.0a2/auroranotes/
[2] https://www.mozilla.org/en-US/mobile/22.0a2/auroranotes/
Comment 108 Steven Saviano 2013-04-10 17:16:22 PDT
Quick question: I am looking into using this API in Google Docs.

It does not appear this supports writing a custom mimetype to the clipboardData object. Is that true?

For instance, on copy:
e.clipboardData.setData('text/custom-mimetype',  'data');

The on paste that data is not available. Is this expected? Are custom mimetypes planned to be supported similar to Chrome? If it should work, shall I open a bug?

Thanks!
Comment 109 Neil Deakin (away until Oct 3) 2013-04-10 18:26:03 PDT
Only text, urls, html and files are currently supported for pasting. Other types may be supported to be copied to the clipboard, but these are platform specific.

Feel free to file a bug on support for other types of data. There's lots of work to be done to improve the situation for clipboard and drag/drop so that there is more consistency between platforms.
Comment 110 Steven Saviano 2013-04-11 11:56:55 PDT
Filed https://bugzilla.mozilla.org/show_bug.cgi?id=860857
Comment 111 Alfonso Martinez 2013-04-20 11:24:15 PDT
Filed Bug 864052
Comment 112 Spocke 2013-06-24 08:49:49 PDT
I think this https://bugzilla.mozilla.org/show_bug.cgi?id=850663 bug report should be blocking for this feature. Since the clipboard data would be pretty useless from a rich text editor standpoint if contents can't be extracted when pasting from MS Office etc.
Comment 113 Jesse Ruderman 2013-06-24 12:26:13 PDT
Spocke, are you saying we should disable this feature until bug 850663 is fixed?  Or just that you'd like us to prioritize bug 850663?
Comment 114 Neil Deakin (away until Oct 3) 2013-06-24 19:04:21 PDT
Bug 850663 is just a bug in this feature, and not a regression, and doesn't need to block this bug. For whatever reason, html data from word isn't being reflected in the clipboardData object.
Comment 115 Spocke 2013-06-25 01:55:34 PDT
I think solving pasting from Word is a very important issue since we had to add browser sniffs for Gecko in our feature detect for ClipboardEvent and disable it. Since we would otherwise get bug reports on existing logic that they can no longer paste rich content from word.
Comment 116 Vincent Lefevre 2013-07-18 02:19:50 PDT
I suspect that this enhancement introduced a new bug under Linux, which has security implications. See bug 894736 (bug 894736 comment 7 for the test with mozregression).

Note You need to log in before you can comment on or make changes to this bug.