598289 - Unable to paste CF_HTML from clipboard if StartHTML/EndHTML set to -1

Status: RESOLVED FIXED

(Core :: Widget: Win32, defect)

Description

[ephox]

User-Agent:       Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10_6_4; en-gb) AppleWebKit/533.18.1 (KHTML, like Gecko) Version/5.0.2 Safari/533.18.5
Build Identifier: Mozilla/5.0 (Windows NT 6.1; rv:2.0b6) Gecko/20100101 Firefox/4.0b6

If the HTML format on the clipboard has the StartHTML and EndHTML fields set to -1 (to indicate "no context" as per http://msdn.microsoft.com/en-us/library/aa767917(VS.85).aspx) pasting into a contenteditable section (or designMode iframe) fails with no output or error messages.

This is a significant problem because Java always sets StartHTML and EndHTML to -1 when copying HTML to the clipboard (in a low-level, sealed class).  As such, it's impossible to copy HTML from a Java application and paste it into Firefox on Windows.

Reproducible: Always

Steps to Reproduce:
1. Copy text from a Java JTextPane which has it's contentType set to text/html
2. Attempt to paste into a contenteditable div or editor such as TinyMCE within Firefox.
Actual Results:  
Nothing happens.

Expected Results:  
HTML content from the clipboard should have been pasted.

Comment 1

[ephox]

Attachment: Test case with Java application and html page

Comment 2

[Brian R. Bondy [:bbondy]]

Attachment: Patch for CF_HTML handling when whole context is specified with -1

Fixed, added support for paste operations from clipboard data that include the whole context.

I will be submitting a third patch (second patch is a fix in win32 widget) for a mochitest for review by you as well separately.

Comment 3

[Brian R. Bondy [:bbondy]]

Attachment: Patch for parsing CF_HTML in win32 widget code

Clipboard operations for CF_HTML was doing bad parsing for the CF_HTML values.

Comment 4

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I'm confused. We're parsing as %u and then checking for -1. So Windows actually sends us 2^32-1 here?

Comment 5

[Brian R. Bondy [:bbondy]]

Yes it does. I think it would be better to do %d though so I'll submit a new patch.

Comment 6

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Will 2^32-1 actually be parsed by %d?

Comment 7

[Brian R. Bondy [:bbondy]]

Well I was going to use an int instead of an unsigned int.  So 2^31 would be the max (~2GB text).  Do you want me to use a 64bit int and format specifier instead?

Comment 8

[Brian R. Bondy [:bbondy]]

2^31-1 that is

Comment 9

[Brian R. Bondy [:bbondy]]

I could also do like the other patch and not even use sscanf and do something like FindIntegerAfterString.

Comment 10

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I mean, is Windows sending "StartHTML:4294967295\n" or "StartHTML:-1\n"?

Comment 11

[Brian R. Bondy [:bbondy]]

Windows always sends it as "StartHTML:-1" and never as "StartHTML:4294967295".

MSVC++ also parses both of these the same with a return value of 4294967295 inside &ui.

sscanf("-1", "%u", &ui);
sscanf("4294967295", "%u", &ui);
Both return values are also 1.

But I'm not sure if that behaviour is the same across other compilers so that's why I suggested %d.
I know that code is only hit in Windows only though so let me know how you want the modified patch.

Comment 12

[Robert O'Callahan (:roc) (email my personal email if necessary)]

%d into PRInt32s sounds right.

Comment 13

[Brian R. Bondy [:bbondy]]

Attachment: Patch for parsing CF_HTML in win32 widget code v2

Changed %u to %d, old code worked but it makes more sense to be %d.

Comment 14

[(no longer active)]

Comment on attachment 552901 [details] [diff] [review]
Patch for CF_HTML handling when whole context is specified with -1

>+  if (endHTML == -1) {
>+    const char endFragmentMarker[] = "<!--EndFragment-->";
>+    endHTML = aCfhtml.Find(endFragmentMarker);
>+    if (endHTML == -1)
>+      return PR_FALSE;
>+    endHTML += sizeof(endFragmentMarker);

Use NS_ARRAY_LENGTH please.  r=me with that.

For the test, I suggest you grab the CF-HTML data on the clipboard with Windows Clipboard Viewer or a similar tool, dump it into a file, and then just extend <http://mxr.mozilla.org/mozilla-central/source/editor/libeditor/html/tests/test_CF_HTML_clipboard.html?force=1> to test the new CF-HTML data.

Thanks for your patch!

Comment 15

[Brian R. Bondy [:bbondy]]

Attachment: Patch for CF_HTML handling when whole context is specified with -1 v2

Changed sizeof to NS_ARRAY_LENGTH, I also subtracted 1 from this length because I didn't mean to include the null termination char.

Comment 16

[Brian R. Bondy [:bbondy]]

Attachment: Test case for CF_HTML parsing with startHTML and EndHTML set to -1

Comment 17

[(no longer active)]

Comment on attachment 553892 [details] [diff] [review]
Test case for CF_HTML parsing with startHTML and EndHTML set to -1

Looks great, thanks!

Comment 18

[Brian R. Bondy [:bbondy]]

Pushed to try:
https://tbpl.mozilla.org/?tree=Try&usebuildbot=1&rev=9bfc89c2e2c8

Comment 19

[Brian R. Bondy [:bbondy]]

Pushed to inbound:
http://hg.mozilla.org/integration/mozilla-inbound/rev/16c9cfa0cf84
http://hg.mozilla.org/integration/mozilla-inbound/rev/b7486f31f6b1
http://hg.mozilla.org/integration/mozilla-inbound/rev/9e3562fefd4c

Comment 20

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/9e3562fefd4c
https://hg.mozilla.org/mozilla-central/rev/b7486f31f6b1
https://hg.mozilla.org/mozilla-central/rev/16c9cfa0cf84

Comment 21

[Olivier Vit (just a reporter)]

Hi
Should this also fix the following scenario FF 7.0.1 release on Windows Vista 32):
Copy text containing an HTML table from a web page (Gmail) and pasting it into another web page (Google Docs) : fails silently without any content being pasted.
While pasting to MS Word, copying again from MS Word, and pasting to Google Docs works fine.

Comment 22

[Olivier Vit (just a reporter)]

Just tested in Aurora 9.0a2 (2011-10-16) and the bug is still there, worth a bug report then ?

Comment 23

[Brian R. Bondy [:bbondy]]

I tested with v9 and v10 and it is fixed for me.
I tested with v8 and v7 and it does not work (as the fix has not hit there yet).

Please post more detailed steps in a new bug and CC me if it still doesn't work for you.  
Note: I did find that in java 1.7 java fixed the -1 context CF_HTML so you need to use 1.6 to reproduce on the older browsers in the first place.

Comment 24

[Brian R. Bondy [:bbondy]]

> Going with Brian's recommendation to backout bug 598289. Feel free to use either  > bug for the backout (we're now tracking both).

I'll backout this task from Beta within the context of this bug. 
The changesets which will be backed out are: 

https://hg.mozilla.org/mozilla-central/rev/9e3562fefd4c
https://hg.mozilla.org/mozilla-central/rev/b7486f31f6b1
https://hg.mozilla.org/mozilla-central/rev/16c9cfa0cf84

Comment 25

[Brian R. Bondy [:bbondy]]

As described in bug 714115, I pushed the backout of the above 3 revisions to beta. They can be found aggregated in this changeset:
http://hg.mozilla.org/releases/mozilla-beta/rev/d53d327daa8d