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

RESOLVED FIXED in Firefox 10

Status

()

Core
Widget: Win32
RESOLVED FIXED
7 years ago
6 years ago

People

(Reporter: ephox, Assigned: bbondy)

Tracking

(Depends on: 1 bug)

unspecified
mozilla9
x86
Windows 7
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox10+ fixed)

Details

Attachments

(4 attachments, 2 obsolete attachments)

(Reporter)

Description

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

Comment 1

7 years ago
Created attachment 477091 [details]
Test case with Java application and html page
(Assignee)

Updated

6 years ago
Assignee: nobody → netzen
(Assignee)

Comment 2

6 years ago
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.
Attachment #552901 - Flags: review?(ehsan)
(Assignee)

Updated

6 years ago
Status: UNCONFIRMED → NEW
Ever confirmed: true
(Assignee)

Comment 3

6 years ago
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.
Attachment #552902 - Flags: review?(roc)
I'm confused. We're parsing as %u and then checking for -1. So Windows actually sends us 2^32-1 here?
(Assignee)

Comment 5

6 years ago
Yes it does. I think it would be better to do %d though so I'll submit a new patch.
Will 2^32-1 actually be parsed by %d?
(Assignee)

Comment 7

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

Comment 8

6 years ago
2^31-1 that is
(Assignee)

Comment 9

6 years ago
I could also do like the other patch and not even use sscanf and do something like FindIntegerAfterString.
I mean, is Windows sending "StartHTML:4294967295\n" or "StartHTML:-1\n"?
(Assignee)

Comment 11

6 years ago
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.
%d into PRInt32s sounds right.
(Assignee)

Comment 13

6 years ago
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.
Attachment #552902 - Attachment is obsolete: true
Attachment #553096 - Flags: review?(roc)
Attachment #552902 - Flags: review?(roc)
Attachment #553096 - Flags: review?(roc) → review+
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!
Attachment #552901 - Flags: review?(ehsan) → review+
(Assignee)

Comment 15

6 years ago
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.
Attachment #552901 - Attachment is obsolete: true
Attachment #553209 - Flags: review?(ehsan)
Attachment #553209 - Flags: review?(ehsan) → review+
(Assignee)

Comment 16

6 years ago
Created attachment 553892 [details] [diff] [review]
Test case for CF_HTML parsing with startHTML and EndHTML set to -1
Attachment #553892 - Flags: review?(ehsan)
(Assignee)

Updated

6 years ago
Depends on: 574005
Comment on attachment 553892 [details] [diff] [review]
Test case for CF_HTML parsing with startHTML and EndHTML set to -1

Looks great, thanks!
Attachment #553892 - Flags: review?(ehsan) → review+
(Assignee)

Comment 18

6 years ago
Pushed to try:
https://tbpl.mozilla.org/?tree=Try&usebuildbot=1&rev=9bfc89c2e2c8
(Assignee)

Comment 19

6 years ago
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
https://hg.mozilla.org/mozilla-central/rev/9e3562fefd4c
https://hg.mozilla.org/mozilla-central/rev/b7486f31f6b1
https://hg.mozilla.org/mozilla-central/rev/16c9cfa0cf84
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla9
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.
Just tested in Aurora 9.0a2 (2011-10-16) and the bug is still there, worth a bug report then ?
(Assignee)

Comment 23

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

Updated

6 years ago
Depends on: 714115

Updated

6 years ago
tracking-firefox10: --- → +
(Assignee)

Comment 24

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

Comment 25

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

Updated

6 years ago
status-firefox10: --- → fixed
You need to log in before you can comment on or make changes to this bug.