The default bug view has changed. See this FAQ.

Add support clipboardData object for the onpaste, oncopy, oncut events

RESOLVED FIXED in mozilla22

Status

()

Core
DOM: Core & HTML
--
enhancement
RESOLVED FIXED
9 years ago
a year ago

People

(Reporter: spam, Assigned: Neil Deakin)

Tracking

(Blocks: 1 bug, {dev-doc-complete})

unspecified
mozilla22
dev-doc-complete
Points:
---
Dependency tree / graph
Bug Flags:
wanted-next +
blocking1.9 -
in-testsuite +

Firefox Tracking Flags

(relnote-firefox 22+)

Details

(Whiteboard: [evang-wanted][devtools-needed])

Attachments

(3 attachments, 22 obsolete attachments)

747 bytes, text/html
Details
65.87 KB, patch
Details | Diff | Splinter Review
15.37 KB, patch
smaug
: review+
Details | Diff | Splinter Review
(Reporter)

Description

9 years ago
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
(Reporter)

Updated

9 years ago
Component: General → DOM: Core
Depends on: 280959
Product: Firefox → Core
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.
Status: UNCONFIRMED → NEW
Ever confirmed: true
(Reporter)

Comment 2

9 years ago
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.
(Reporter)

Comment 3

9 years ago
Here are some references of the IE and WebKit implementations:

http://msdn2.microsoft.com/en-us/library/ms535220.aspx
http://developer.apple.com/documentation/AppleApplications/Reference/SafariJSRef/Classes/Events.html
QA Contact: general → general

Updated

9 years ago
Flags: blocking1.9?

Comment 4

9 years ago
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.
(Reporter)

Comment 5

9 years ago
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

9 years ago
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.

Updated

9 years ago
Flags: tracking1.9? → blocking1.9?
Odds are I can't clear the flag, but given that onpaste and friends are being backed out, this doesn't block.
Flags: blocking1.9?
Blah, I suck -- it's only the onbefore* versions that are being removed.  Restoring blocking request...
Flags: blocking1.9?
New support this late?  -'ing.

And, can you explain why you are requesting blocking again?
Flags: blocking1.9? → blocking1.9-
Dan, can you answer that?

(Note I was just restoring a flag I accidentally removed, not seriously requesting blocking here!)

Comment 11

9 years ago
(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.

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.
Flags: wanted-next+

Updated

9 years ago
Component: DOM: Core → DOM: Core & HTML

Comment 13

8 years ago
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

8 years ago
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

8 years ago
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

8 years ago
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).
(Reporter)

Comment 17

8 years ago
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
(Assignee)

Comment 18

8 years ago
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

8 years ago
(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.
(Reporter)

Comment 20

8 years ago
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.
(Assignee)

Updated

8 years ago
Depends on: 501154

Comment 21

8 years ago
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?
(In reply to comment #21)
> So, what are we still waiting for?

Someone doing the work. (Bug 501154 looks promising.)
(Assignee)

Comment 23

8 years ago
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.
Assignee: nobody → enndeakin
Status: NEW → ASSIGNED
(Assignee)

Comment 24

8 years ago
Builds are at https://build.mozilla.org/tryserver-builds/neil@mozilla.com-clipboard4/

Comment 25

8 years ago
The build seems lost. Anywhere to try it out now?

Comment 26

8 years ago
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

7 years ago
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

7 years ago
This must be implemented in order for Bespin to work properly as a text editor.

Where are we for now ?
Whiteboard: [evang-wanted]

Comment 29

7 years ago
Now that bug 501154 is fixed, I guess we only need to wait for Neil Deakin to update his patch.

Comment 30

7 years ago
@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

7 years ago
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

7 years ago
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!
(Assignee)

Comment 33

6 years ago
Created attachment 541671 [details] [diff] [review]
patch part 1 - add clipboardData to cut/copy/paste events
Attachment #386637 - Attachment is obsolete: true
(Assignee)

Comment 34

6 years ago
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.
(Assignee)

Comment 35

6 years ago
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.
How close is the implementation to http://dev.w3.org/2006/webapi/clipops/ ?
(I haven't reviewed the draft spec )
(Assignee)

Comment 37

6 years ago
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.

Updated

6 years ago
Duplicate of this bug: 689590
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.
Created attachment 574933 [details] [diff] [review]
patch part 1 - add clipboardData to cut/copy/paste events
Attachment #541671 - Attachment is obsolete: true
Created attachment 574934 [details] [diff] [review]
patch part 2 - global clipboardData property
Attachment #541672 - Attachment is obsolete: true
Created attachment 574935 [details] [diff] [review]
patch part 3 - clipboard changed checks
Attachment #541673 - Attachment is obsolete: true
Fabien, what is the status of these patches?
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.
Whiteboard: [evang-wanted] → [evang-wanted][devtools-needed]
Guys, is there any update on this issue by chance?
Neil, do you think you'll have a chance to work on this in the near future?
(Assignee)

Comment 47

5 years ago
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)
I can try to provide some feedback for you.  :-)
(Assignee)

Comment 49

5 years ago
Created attachment 633602 [details] [diff] [review]
Part 1 - add clipboardData to cut/copy/paste events
Attachment #574933 - Attachment is obsolete: true
Attachment #633602 - Flags: feedback?(ehsan)
(Assignee)

Comment 50

5 years ago
Created attachment 633603 [details] [diff] [review]
Part 2 - global clipboardData property

See comment 34 for info about this part.
Attachment #633603 - Flags: feedback?(ehsan)
(Assignee)

Comment 51

5 years ago
Created attachment 633604 [details] [diff] [review]
Part 3 - clipboard changed checks

This is really only implemented on Windows and Mac.
Attachment #574934 - Attachment is obsolete: true
Attachment #574935 - Attachment is obsolete: true
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?
(Assignee)

Comment 53

5 years ago
(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

5 years ago
Is this still being worked on? Any chance of an update?  Is there a target milestone for this?
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.
Attachment #633602 - Flags: feedback?(ehsan) → feedback+
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?
(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.

Updated

5 years ago
Attachment #633603 - Flags: feedback?(ehsan) → feedback-
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?
Attachment #633604 - Flags: feedback+

Comment 59

5 years ago
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?
(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?
(Assignee)

Comment 61

5 years ago
(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.
(Assignee)

Comment 62

5 years ago
(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.
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!
(Assignee)

Comment 64

5 years ago
Created attachment 673950 [details] [diff] [review]
Latest patch

OK, here is the latest patch that addresses the feedback above.

Comment 65

5 years ago
(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?
(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.
(Assignee)

Comment 67

5 years ago
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.
Attachment #673950 - Flags: review?(ehsan)
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.
Attachment #673950 - Flags: review?(ehsan) → review+
(Assignee)

Comment 69

5 years ago
> > +    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

4 years ago
Any updates on this one?

Comment 71

4 years ago
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?
Attachment #711261 - Flags: review?(ehsan)
Flags: needinfo?(neil)

Updated

4 years ago
Flags: needinfo?(neil) → needinfo?(enndeakin)
>> @@ +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

4 years ago
Created attachment 711262 [details]
Updated testcase that uses onCut and onCopy as well.

Comment 74

4 years ago
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.
Attachment #711262 - Attachment is obsolete: true

Comment 75

4 years ago
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);
Attachment #711261 - Attachment is obsolete: true
Attachment #711261 - Flags: review?(ehsan)
Attachment #711280 - Flags: review?
(Assignee)

Comment 76

4 years ago
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.
Flags: needinfo?(enndeakin)
(Assignee)

Comment 77

4 years ago
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

4 years ago
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.
Attachment #711280 - Attachment is obsolete: true
Attachment #711280 - Flags: review?

Comment 79

4 years ago
:RyanVM pushed the v3 patch to try: https://tbpl.mozilla.org/?tree=Try&rev=3214821a9727

Comment 80

4 years ago
(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?
(Assignee)

Comment 81

4 years ago
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

4 years ago
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

4 years ago
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

4 years ago
(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?
Flags: needinfo?(enndeakin)

Updated

4 years ago
Blocks: 810636
(Assignee)

Comment 85

4 years ago
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.
Attachment #712483 - Flags: superreview?(bugs)
Flags: needinfo?(enndeakin)
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
Attachment #712483 - Flags: superreview?(bugs) → superreview-

Comment 87

4 years ago
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

4 years ago
(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?
Flags: needinfo?(enndeakin)
(Assignee)

Comment 89

4 years ago
Julian, if you could please update the patch as much as you can, then I can update the remaining issues, thanks.
Flags: needinfo?(enndeakin)

Comment 90

4 years ago
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.
Attachment #712145 - Attachment is obsolete: true
Attachment #712483 - Attachment is obsolete: true
(Assignee)

Updated

4 years ago
Blocks: 844941
(Assignee)

Comment 91

4 years ago
Created attachment 718975 [details] [diff] [review]
Add support for clipboardData in cut/copy/paste events
Attachment #633602 - Attachment is obsolete: true
Attachment #673950 - Attachment is obsolete: true
Attachment #711279 - Attachment is obsolete: true
Attachment #714018 - Attachment is obsolete: true
Attachment #717492 - Attachment is obsolete: true
Attachment #718975 - Flags: superreview?(bugs)
(Assignee)

Comment 92

4 years ago
Created attachment 718977 [details] [diff] [review]
Add clipboard constructor
Attachment #718977 - Flags: review?(bugs)
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.
Attachment #718977 - Flags: review?(bugs) → review-
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
Attachment #718975 - Flags: superreview?(bugs) → superreview+
(Assignee)

Comment 95

4 years ago
Created attachment 720140 [details] [diff] [review]
Part 1 -  Add support for clipboardData in cut/copy/paste events
Attachment #718975 - Attachment is obsolete: true
Attachment #720140 - Flags: superreview?(bugs)
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?
Attachment #720140 - Flags: superreview?(bugs) → superreview-

Comment 97

4 years ago
@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
This is certainly a feature which shouldn't go to stable ESR17, nor FF19/20/21.
Needs normal testing.

Comment 99

4 years ago
@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).
(Assignee)

Comment 100

4 years ago
Created attachment 721845 [details] [diff] [review]
Part 1 - Add support for clipboardData in cut/copy/paste events
Attachment #721845 - Flags: superreview?(bugs)
(Assignee)

Comment 101

4 years ago
Created attachment 721848 [details] [diff] [review]
Part 2 - add clipboard constructor
Attachment #633603 - Attachment is obsolete: true
Attachment #633604 - Attachment is obsolete: true
Attachment #718977 - Attachment is obsolete: true
Attachment #720140 - Attachment is obsolete: true
Attachment #721848 - Flags: review?(bugs)
(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

4 years ago
(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.
(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.

Updated

4 years ago
Attachment #721848 - Flags: review?(bugs) → review+

Updated

4 years ago
Attachment #721845 - Flags: superreview?(bugs) → superreview+
(Assignee)

Comment 105

4 years ago
Checked into inbound:

https://hg.mozilla.org/integration/mozilla-inbound/rev/1cc899b40731
https://hg.mozilla.org/integration/mozilla-inbound/rev/b123c8210d41
Flags: in-testsuite+
(Assignee)

Updated

4 years ago
Keywords: dev-doc-needed
https://hg.mozilla.org/mozilla-central/rev/1cc899b40731
https://hg.mozilla.org/mozilla-central/rev/b123c8210d41
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla22

Updated

4 years ago
Depends on: 850198

Updated

4 years ago
Depends on: 850663
(Assignee)

Updated

4 years ago
No longer depends on: 850663

Updated

4 years ago
relnote-firefox: --- → ?

Updated

4 years ago
relnote-firefox: ? → 22+
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

4 years ago
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!
(Assignee)

Comment 109

4 years ago
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

4 years ago
Filed https://bugzilla.mozilla.org/show_bug.cgi?id=860857

Updated

4 years ago
Depends on: 861512

Comment 111

4 years ago
Filed Bug 864052

Updated

4 years ago
Blocks: 860857

Comment 112

4 years ago
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.

Updated

4 years ago
Blocks: 850663

Comment 113

4 years ago
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?
(Assignee)

Comment 114

4 years ago
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.
No longer blocks: 850663, 860857

Comment 115

4 years ago
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.

Updated

4 years ago
Blocks: 891247
(Assignee)

Updated

4 years ago
No longer blocks: 891247

Comment 116

4 years ago
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).

Updated

4 years ago
Depends on: 894736
Documentation updated:
https://developer.mozilla.org/en-US/docs/Web/API/ClipboardEvent.clipboardData
https://developer.mozilla.org/en-US/docs/Web/API/ClipboardEvent
https://developer.mozilla.org/en-US/docs/Web/Reference/Events/copy
https://developer.mozilla.org/en-US/docs/Web/Reference/Events/cut
https://developer.mozilla.org/en-US/docs/Web/Reference/Events/paste

(Thanks to Jesper Kristensen who did most of it)
Keywords: dev-doc-needed → dev-doc-complete
Depends on: 920405

Updated

a year ago
Summary: Add support clipboardData object for the onpaste event → Add support clipboardData object for the onpaste, oncopy, oncut events
You need to log in before you can comment on or make changes to this bug.