Bug 19893

<rdar://problem/6055546>

Created attachment 50877 [details] Patch

(In reply to comment #2) > Created an attachment (id=50877) [details] > Patch It turns out that text/html wasn't returning anything because webkit on mac doesn't add text/html to the pasteboard (it uses WebArchive for html). In fact, it was currently possible to copy from chrome mac and paste into safari and use getData('text/html')! I originally tried to handle reading from LegacyWebArchive in the getData method, but it seemed non-trivial (the WebArchive code I needed was in WebKit/mac/). I'm not sure which other platforms this will fail on, but I will try to fix win and chromium platforms after these tests land.

Comment on attachment 50877 [details] Patch You need to test the effect of copy/pasting say <div onload="alert(1)"></div> I suspect the getData("text/html") query will currently produce a string that contains the event handler which is _probably_ unsafe.

Comment on attachment 50877 [details] Patch This reverses a change we made in 2004, <http://trac.webkit.org/changeset/6793>. The symptom of that bug was: - Copy some text from any web page. - Try to paste into an Excel spreadsheet. - Get error message: "Microsoft Excel cannot paste the data." Please don't make this change to WebKit unless you have some other solution for that problem. We want people to be able to copy in Safari and paste into Excel.

(In reply to comment #5) > Please don't make this change to WebKit unless you have some other solution for > that problem. We want people to be able to copy in Safari and paste into Excel. Thanks for letting me know about the history of this behavior. I am able to copy and paste with this patch into Excel 2008. Which version of Excel do we want to make sure still works? It looks like the versions before 2008 were 2004 and 2001. (In reply to comment #4) > (From update of attachment 50877 [details]) > You need to test the effect of copy/pasting say > <div onload="alert(1)"></div> > > I suspect the getData("text/html") query will currently produce a string that > contains the event handler which is _probably_ unsafe. Yes, the string will contain the event handler, which the website could do something unsafe with (say, assign to innerHTML). I think it's up to the site to sanitize the string or just use text/plain. Lots of stuff gets removed by webkit when pasted (css, event handlers, smart handling of some formatting), but perhaps the site has some other good reason to want to use the original html. Julie, do you have some example uses cases of wanting text/html?

Tony - Nearly every product team at Google that uses rich text has asked for this. They currently implement it in a number of hacky ways, like putting the user's cursor in another contentEditable region so the paste goes there, then grabbing those contents, sanitizing them, and inserting back into the original element. Or letting the paste go through, and processing the changes afterwards, which can make the contents change out from under the user. The use cases is to be able to change the pasted contents before it is pasted into the final destination, for any number of reasons: some want to strip out certain types of content (for example, they don't want to allow images in their rich text region), others want to clean up the HTML (for example, convert <strong> to <b> because execCommand('bold') can't unbold <strong> tags).

(In reply to comment #7) > The use cases is to be able to change the pasted contents before it is pasted > into the final destination, for any number of reasons: some want to strip out > certain types of content (for example, they don't want to allow images in their > rich text region), others want to clean up the HTML (for example, convert > <strong> to <b> because execCommand('bold') can't unbold <strong> tags). As far as I can tell, clipboardData.setData doesn't change the clipboard contents during onpaste (it's probably too late, you have to do it in onbeforepaste). You can however cancel the paste and manually handle the paste into the DOM. Be aware that at this point, you lose all the code in webkit's for handling the paste (i.e., removing the extra style attributes, removing event handlers, special merge handling that happens for lists, tables, etc). I guess the main benefit of this is that you can work around webkit bugs without having to wait for new browser releases, although it'll be a lot of duplicated effort with what the browser does.

It seems like we should fix this bug given Tony's new research. It's not great that getData('text/html') isn't available.

Bug 104179 is the similar bug for setData.

Is this still a bug on just the Mac port of WebKit or is it a problem for other platforms as well? I'm fairly certain that the WebKit port of Chromium supports getData() and setData() for HTML.

(In reply to comment #11) > Is this still a bug on just the Mac port of WebKit or is it a problem for other platforms as well? I'm fairly certain that the WebKit port of Chromium supports getData() and setData() for HTML. (I mean the Chromium port of WebKit of course...)

Oh, I suppose this bug as listed is fixed. I couldn't get at the clipboardData from the copy event though, which seems unnecessarily different.

I also met this issue working on drag and drop in the contenteditable element. I am not able to get html data when user drop same html into the contenteditable.

For the first time, I've seen this ticket about 3-4 years ago and even then it was very sad that Webkit still didn't provide HTML on paste. I'm now working on a totally new version of CKEditor and, in the meantime, I totally forgot about this problem (I kinda optimistically believed that it must all work by now). Imagine my surprise when I found out that it's still open :(. Any chance that this gets under your radar again? I see (https://bugs.webkit.org/show_bug.cgi?id=19893#c9) that it's been already accepted by Ryosuke.

(In reply to comment #15) > For the first time, I've seen this ticket about 3-4 years ago and even then > it was very sad that Webkit still didn't provide HTML on paste. I'm now > working on a totally new version of CKEditor and, in the meantime, I totally > forgot about this problem (I kinda optimistically believed that it must all > work by now). Imagine my surprise when I found out that it's still open :(. > > Any chance that this gets under your radar again? I see > (https://bugs.webkit.org/show_bug.cgi?id=19893#c9) that it's been already > accepted by Ryosuke. @rniwa / @darin: Any opposition to reviving this assuming fixing this does not break copy/pasting to Excel anymore as indicated by Tony in [1]? [1] https://bugs.webkit.org/show_bug.cgi?id=19893#c6

No objection. My objection was reversing the change without reading the change history and figuring out what kind of testing was needed.

(In reply to comment #17) > No objection. My objection was reversing the change without reading the > change history and figuring out what kind of testing was needed. Using Excel 2011 Mac, copying from Safari and pasting to Excel still works with the patch. There is a behavior change though, it now pastes as HTML instead of simple text. Excel supports displaying HTML so this is not an issue.

Sounds fine.

Created attachment 292643 [details] Patch

While you are at it, could you please also test other Office products?

Comment on attachment 292643 [details] Patch r=me assuming this doesn't let web content insert an arbitrary HTML content into the system's pasteboard by setData, or we don't start allowing getData to access text/html type in the pasteboard both of which pose privacy and security problems.

(In reply to comment #22) > Comment on attachment 292643 [details] > Patch > > or we don't start allowing getData > to access text/html type in the pasteboard both of which pose privacy and > security problems. Isn't this exactly what this patch is doing though? (allowing JS to access call getData('text/html') to get it from the pasteboard)

(In reply to comment #23) > (In reply to comment #22) > > Comment on attachment 292643 [details] > > Patch > > > > or we don't start allowing getData > > to access text/html type in the pasteboard both of which pose privacy and > > security problems. > > Isn't this exactly what this patch is doing though? (allowing JS to access > call getData('text/html') to get it from the pasteboard) To be clear though, this patch merely add HTML to the pasteboard (in addition to what WebKit already added, such as RTFD). As a result, getData('text/html') will be able to read the HTML we now write. Therefore, I do not think I am exposing more information, I am merely providing an extra format on the pasteboard.

(In reply to comment #21) > While you are at it, could you please also test other Office products? I will do so before landing (FYI, I also tried Apple's Keynote and it worked).

(In reply to comment #25) > (In reply to comment #21) > > While you are at it, could you please also test other Office products? > > I will do so before landing (FYI, I also tried Apple's Keynote and it > worked). Works in Powerpoint 2011 and Word 2011 too.

Comment on attachment 292643 [details] Patch Clearing flags on attachment: 292643 Committed r207797: <http://trac.webkit.org/changeset/207797>

All reviewed patches have been landed. Closing bug.

Thanks for taking care of this so quickly :)! Does it mean that the following is going to work now? 1. Open https://jsfiddle.net/kamxex2q/3/ 2. Copy some text from the editable element. (On #paste we write some content to the clipboard) 3. Paste it. Expected: 'x <strong>y</strong z' was pasted. Actual: the text that was selected was pasted. Or does it require a new ticket?

(In reply to comment #29) > Thanks for taking care of this so quickly :)! > > Does it mean that the following is going to work now? > > 1. Open https://jsfiddle.net/kamxex2q/3/ > 2. Copy some text from the editable element. (On #paste we write some > content to the clipboard) > 3. Paste it. > > Expected: 'x <strong>y</strong z' was pasted. > Actual: the text that was selected was pasted. > > Or does it require a new ticket? This does not seem to work in *any* browser I tested?

(In reply to comment #30) > (In reply to comment #29) > > Thanks for taking care of this so quickly :)! > > > > Does it mean that the following is going to work now? > > > > 1. Open https://jsfiddle.net/kamxex2q/3/ > > 2. Copy some text from the editable element. (On #paste we write some > > content to the clipboard) > > 3. Paste it. > > > > Expected: 'x <strong>y</strong z' was pasted. > > Actual: the text that was selected was pasted. > > > > Or does it require a new ticket? > > This does not seem to work in *any* browser I tested? I believe you need to cancel the event if you want your call to setData not to be ignored. Like so: https://jsfiddle.net/kamxex2q/4/ In latest WebKit, this pastes as "x y z" with "y" in bold, So it looks correct to me.

(In reply to comment #31) > (In reply to comment #30) > > (In reply to comment #29) > > > Thanks for taking care of this so quickly :)! > > > > > > Does it mean that the following is going to work now? > > > > > > 1. Open https://jsfiddle.net/kamxex2q/3/ > > > 2. Copy some text from the editable element. (On #paste we write some > > > content to the clipboard) > > > 3. Paste it. > > > > > > Expected: 'x <strong>y</strong z' was pasted. > > > Actual: the text that was selected was pasted. > > > > > > Or does it require a new ticket? > > > > This does not seem to work in *any* browser I tested? > > I believe you need to cancel the event if you want your call to setData not > to be ignored. Like so: > https://jsfiddle.net/kamxex2q/4/ > > In latest WebKit, this pastes as "x y z" with "y" in bold, So it looks > correct to me. This is explained in this section of the specification: - https://www.w3.org/TR/clipboard-apis/#the-copy-action

Right! Thanks :D It's been so long since the last time that I forgot about preventing the default action. I only new it was working in CKEditor 4, but I couldn't find why :D. Anyway, I confirm that I can set the data in Safari 10.0.1 and that this data is used on paste. Playground if anyone's interested: https://jsfiddle.net/kamxex2q/5/. Thanks!

Good job with fixing it on a desktop version. It still doesn't work on ios though, as it does not include text/html. It includes type `public.html` with no content. Only types `text/plain` and `public.rtf` have some content while copying and pasting formatted text within Safari.