Last Comment Bug 598289 - Unable to paste CF_HTML from clipboard if StartHTML/EndHTML set to -1
: Unable to paste CF_HTML from clipboard if StartHTML/EndHTML set to -1
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Widget: Win32 (show other bugs)
: unspecified
: x86 Windows 7
: -- normal (vote)
: mozilla9
Assigned To: Brian R. Bondy [:bbondy]
:
Mentors:
Depends on: 574005 714115
Blocks:
  Show dependency treegraph
 
Reported: 2010-09-21 03:35 PDT by ephox
Modified: 2012-01-23 14:21 PST (History)
7 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
+
fixed


Attachments
Test case with Java application and html page (2.12 KB, application/zip)
2010-09-21 03:44 PDT, ephox
no flags Details
Patch for CF_HTML handling when whole context is specified with -1 (4.75 KB, patch)
2011-08-13 14:04 PDT, Brian R. Bondy [:bbondy]
ehsan: review+
Details | Diff | Review
Patch for parsing CF_HTML in win32 widget code (3.04 KB, patch)
2011-08-13 14:06 PDT, Brian R. Bondy [:bbondy]
no flags Details | Diff | Review
Patch for parsing CF_HTML in win32 widget code v2 (3.06 KB, patch)
2011-08-14 21:05 PDT, Brian R. Bondy [:bbondy]
roc: review+
Details | Diff | Review
Patch for CF_HTML handling when whole context is specified with -1 v2 (4.77 KB, patch)
2011-08-15 10:13 PDT, Brian R. Bondy [:bbondy]
ehsan: review+
Details | Diff | Review
Test case for CF_HTML parsing with startHTML and EndHTML set to -1 (2.21 KB, patch)
2011-08-17 13:42 PDT, Brian R. Bondy [:bbondy]
ehsan: review+
Details | Diff | Review

Description ephox 2010-09-21 03:35:27 PDT
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 2010-09-21 03:44:30 PDT
Created attachment 477091 [details]
Test case with Java application and html page
Comment 2 Brian R. Bondy [:bbondy] 2011-08-13 14:04:15 PDT
Created attachment 552901 [details] [diff] [review]
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] 2011-08-13 14:06:32 PDT
Created attachment 552902 [details] [diff] [review]
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) (Exited; email my personal email if necessary) 2011-08-14 18:08:46 PDT
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] 2011-08-14 18:10:49 PDT
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) (Exited; email my personal email if necessary) 2011-08-14 18:13:33 PDT
Will 2^32-1 actually be parsed by %d?
Comment 7 Brian R. Bondy [:bbondy] 2011-08-14 18:16:59 PDT
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] 2011-08-14 18:17:18 PDT
2^31-1 that is
Comment 9 Brian R. Bondy [:bbondy] 2011-08-14 18:19:37 PDT
I could also do like the other patch and not even use sscanf and do something like FindIntegerAfterString.
Comment 10 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2011-08-14 20:36:54 PDT
I mean, is Windows sending "StartHTML:4294967295\n" or "StartHTML:-1\n"?
Comment 11 Brian R. Bondy [:bbondy] 2011-08-14 20:41:57 PDT
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) (Exited; email my personal email if necessary) 2011-08-14 20:45:43 PDT
%d into PRInt32s sounds right.
Comment 13 Brian R. Bondy [:bbondy] 2011-08-14 21:05:20 PDT
Created attachment 553096 [details] [diff] [review]
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 :Ehsan Akhgari (out sick) 2011-08-15 08:11:21 PDT
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] 2011-08-15 10:13:03 PDT
Created attachment 553209 [details] [diff] [review]
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] 2011-08-17 13:42:37 PDT
Created attachment 553892 [details] [diff] [review]
Test case for CF_HTML parsing with startHTML and EndHTML set to -1
Comment 17 :Ehsan Akhgari (out sick) 2011-08-17 13:55:26 PDT
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] 2011-09-20 12:57:59 PDT
Pushed to try:
https://tbpl.mozilla.org/?tree=Try&usebuildbot=1&rev=9bfc89c2e2c8
Comment 21 Olivier Vit (just a reporter) 2011-10-17 02:11:41 PDT
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) 2011-10-17 03:21:21 PDT
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] 2011-10-22 18:45:36 PDT
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] 2012-01-17 12:15:59 PST
> 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] 2012-01-17 18:54:10 PST
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

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