142855 - Support pasting of CF_HTML in editor (flavor application/x-moz-nativehtml)

Status: RESOLVED FIXED

(Core :: DOM: Editor, defect)

Description

[Mike Pinkerton (not reading bugmail)]

Since 69566 has gone in, you can now request "application/x-moz-nativehtml" 
from the clipboard service and get the CF_HTML data format on win32. Editor 
needs to do some work to correctly map this flavor into our own format and 
insert it.

This work is needed for embedding.

Comment 1

[Dave Barrowman]

setting keywords to "topembed-, embed" since this does not block any current
embedding customer, but this area of the product is getting more attention and
will block future endeavours.

Comment 2

[Joe Francis]

stealing this bug.

Comment 3

[Joe Francis]

The trunk is the wave of the future!

Comment 4

[Joe Francis]

Attachment: patch for nsHTMLDataTransfer.cpp and nsHTMLEditor.h

first cut at this.  written on a mac at home so I cant even test it yet
(cf_html is windows only at present).

Comment 5

[Joe Francis]

cc'ing Jag for string usage review and suggestions.
i'll try to hit kin up for some testing tomorrow.

Comment 6

[Joe Francis]

Here is the spec for CF_HTML so reviewers can make some sense of what i'm doing:
http://msdn.microsoft.com/workshop/networking/clipboard/htmlclipboard.asp

Comment 7

[Joe Francis]

Attachment: take #2

try this one kin

Comment 8

[Joe Francis]

Attachment: take #3

This one doesn't abuse the clipboard code as badly.

Comment 9

[Kathleen :Brade]

Comment on attachment 90205 [details] [diff] [review]
take #3

for FindPositiveIntegerAfterString :
 * why create "crlf" instead of using the constant CRLF which is already
defined elsewhere?
 * I'm concerned about ignoring the errorCode from ToInteger.  In particular,
if there is an error, it returns 0 so  ParseCFHTML won't return
NS_ERROR_FAILURE.  Is that ok?

I'd like to see a better local variable name for "char* text" that you are
adding (since the block below that declares a PRUnichar* text variable).

Comment 10

[Joe Francis]

Attachment: take #4

I've done some testing on windows and fixed some snafus.  Brade's requested
changes are not in yet but will be next rev.

This version actually "works" on windows, sorta.  I will have to make a trivial
patch to the clipboard code because it is not properly generating CFHTML. 
presently this code works with the bad version mozilla makes rather than the
real version, so you still wont be able to paste in cfhtml from other apps.

Which brings us to the next problem: I need to talk to Pink about how to find
out if cfhtml is really on the clipboard.  Presently if I ask for it on
windows, our clipboard code creates it for me.	That's not what I want. 
Internally I need mozilla to use it's own html formats rather than cfhtml (and
besides, our internal are more efficient and dont require as much string
searching or string copying or ucs2->utf conversion).

Then once all that is fixed we have to make the clipboard code properly create
cfhtml for other apps consumption.  Right now we have no legitimate context
information in cfhtml we create.  So for instance, with this patch, if you copy
the middle list item of a list and paste into another app, it won't know what
kind of list to create.  It may not even know to put the list item in a list at
all.  

I will file bugs on all of these issues.  Mike and I will have to educate each
other on how things work in order to fix things.

Comment 11

[Joe Francis]

The first issue above (cfhtml syntax slightly messed up) is bug 157552.  Easy fix.

Comment 12

[Joe Francis]

*** Bug 141063 has been marked as a duplicate of this bug. ***

Comment 13

[Joe Francis]

Attachment: take #5

a whole host of changes.  properly strips the fragment comments now.  works
around parser limitations.  properly detects when to use internal data flavors
or cfhtml.  make money fast!  act now while supplies last!

This solves all the probs mentioned earlier, plus a few more, except for the
issue of our own copy not creating context info for the cfhtml flavor.	But
paste should work with this patch.  I was able to paste in from IE just fine.

Comment 14

[timeless]

can you change == -1 to == kNotFound, >= 0 to > kNotFound and return -1 to
return kNotFound where appropriate?

Comment 15

[jag (Peter Annema)]

>= 0 is better than > kNotFound. In the latter case you're depending on the fact
that kNotFound happens to be -1. Theoretically it could be any negative number
that fits within PRInt32. >= 0 says "I want to know whether the character was
somewhere in this string". Another way of saying that is != kNotFound.

Comment 16

[Joe Francis]

The days of having a half dozen milestones out in front of us to divide bugs 
between seem to be gone, though I dont know why.  Lumping everything together as 
far out as I can.  I'll pull back things that I am working on as I go.

Comment 17

[Joe Francis]

Attachment: take #6

always post the exact versions you tested....

Comment 18

[Joe Francis]

Attachment: take #7

more workarounds for parser woes.  With this patch I was able to successfully
copy pages in IE that have complex <head> tags, and still paste them in
mozilla.  www.netscape.com and news.yahoo.com both work now, for instance.

Comment 19

[Judson Valeski]

shouldn't this have a different milestone?

Comment 20

[Joe Francis]

Attachment: take #8

This is updated to take adbantage of parser/sink/dtd work done in patch for bug
159842.  You must apply that patch first to test this one.

Comment 21

[Joe Francis]

Attachment: take #9

includes suggested changes from kin

Comment 22

[Joe Francis]

Attachment: take #10

more changes from kin

Comment 23

[kinmoz]

This patch looks good, but I didn't realize how many times the CFHTML data is
actually copied and converted in ParseCFHTML() and it's caller ... yikes!

I'm no string guru, so those who know better, please chime in ...

You can probably avoid this copy in the caller by using nsDependentCString, or
perhaps use an nsXPIDLCString to manage and destroy |text|:


+        char* text = nsnull;
+        textDataObj->ToString ( &text );
+        nsCAutoString cfhtml(text, len);


but that would change the signature of ParseCFHTML() having a ripple affect down
into FindPositiveIntegerAfterString() etc.

Would the following be more efficient if you instead just added
|SubString(startHTML, startFrag - startHTML) + SubString(endFrag, endHTML -
endFrag)|?


+  // create context string
+  nsCAutoString contextUTF8(Substring(aCfhtml, startHTML, endHTML-startHTML));
+  
+  // cut fragment string out of context
+  contextUTF8.Cut(startFragment-startHTML, endFragment-startFragment);


This does both a conversion+copy and a copy for each line:


+  aStuffToPaste.Assign(NS_ConvertUTF8toUCS2(fragmentUTF8));
+  aCfcontext.Assign(NS_ConvertUTF8toUCS2(contextUTF8));


Since both aStuffToPaste and aCfcontext both get overwritten below anyways, I
think you can get away with just doing something like this for both fragmentUTF8
and contextUTF8:

  const nsAFlatString& ucs2Str = NS_ConvertUTF8toUCS2(fragmentUTF8);

and use that as an arg to |ConvertUnicharLineBreaks(ucs2Str.get() ...)| thus
avoiding an extra 2 copies?

Also, if we made aStuffToPaste and aCFContext PRUnichar**, we could avoid this
copy, by simply handing the |newStr| buffer over to them. If you did this, you
could use XPIDLStrings in the caller to manage/free these buffers when they go
out of scope in the caller:


+  if (newStr)
+  {
+    aStuffToPaste.Assign(newStr, newLengthInChars);
+    nsMemory::Free (newStr);
+  }


Here are some URLs to the string docs, some of this info isn't in those docs, so
I gleaned info from just using LXR and grep.

http://www.mozilla.org/projects/xpcom/string-quickref.html
http://www.mozilla.org/projects/xpcom/string-guide.html

Comment 24

[kinmoz]

Comment on attachment 96374 [details] [diff] [review]
take #10

I forgot to mention that there are a couple of places where you missed the
cfcontext to aCFContext and stuffToPaste to aStuffToPaste conversion in the
patch.

Comment 25

[kinmoz]

... and outNode should be aOutNode:

+nsresult nsHTMLEditor::ParseFragment(const nsAString & aFragStr,
nsCOMPtr<nsIDOMNode> *outNode)

And some of the args in the methods you added into nsHTMLEditor.h need the 'a'
prefix too.

Comment 26

[Joe Francis]

Kin's string comments rock.  Although I read string guide info when doing this,
I didnt encounter XPIDLString info.  Hadn't heard of that and don't know what it
does.  I'll have to study it.

I do disagree with changing outNode to aOutNode, though.  But I'll do it anyway.
(To me aFoo, inFoo, outFoo, ioFoo are all in the "namespace" of parameters. 
aOutFoo is redundant.)

Comment 27

[Joe Francis]

Attachment: take #11

work in progress.  merged with recent string api changes.

Comment 28

[Joe Francis]

Attachment: patch to content

changes to nsHTMLFragmentContentSink

Comment 29

[Joe Francis]

Attachment: patch to htmlparser

changes to nsHTMLFragmentContentSink (in htmlparser)

Comment 30

[Joe Francis]

Attachment: patch to layout

changes to nsHTMLFragmentContentSink (in layout)

Comment 31

[Joe Francis]

Attachment: patch to editor

work in progress.  When combined with the other patches
(content/layout/htmlparser) paste is once again functional, and is using the
parser directly instead of nsRange->CreateContextualFragment().

I now know more than I ever wanted to know about nsHTMLFragmentContentSink.

Stay tuned for an updated editor patch that gets cf_html working again.

Comment 32

[Kathleen :Brade]

Comment on attachment 100204 [details] [diff] [review]
patch to content

r=brade

Comment 33

[Kathleen :Brade]

Comment on attachment 100205 [details] [diff] [review]
patch to htmlparser

r=brade

Comment 34

[Kathleen :Brade]

Comment on attachment 100206 [details] [diff] [review]
patch to layout

r=brade

Comment 35

[Joe Francis]

Attachment: patch to content

additional tweaks to nsHTMLFragmentContentSink2

Comment 36

[Joe Francis]

Attachment: patch to editor

further tweaks to editor paste code to track behavior of parser and content
sink.

This one works, folks.	Regular intra-application paste, and also cf_html paste
from outside sources both should do the right thing here (and do in my
testing).

You will need all of the non-obsolete patches installed to try this, and you
will have to rebuild the universe, more or less.

Comment 37

[Joe Francis]

Reviewing can begin.

I know I need to add comments to the nsHTMLFragmentContentSink2 class explaining
why we need a 2nd implementation.  And I also should try to reduce string
copying a la kin's comment #23.  

Comment 38

[Kathleen :Brade]

Comment on attachment 100334 [details] [diff] [review]
patch to content

r=brade

Comment 39

[Kathleen :Brade]

Comment on attachment 100339 [details] [diff] [review]
patch to editor

add a blank line to the end of the file in nsEditor.cpp

in nsEditor.h, be sure that it has the same #ifdef that the cpp file has
(DEBUG_JOE)

I prefer this on two lines:
+      if (!aHavePrivFlavor)  (*aTransferable)->AddDataFlavor(kNativeHTMLMime);

isn't there a #define somewhere we can use for this:
+  char crlf[] = {nsCRT::CR, nsCRT::LF, 0};
 maybe CRLF or something similar?

The code here seems a little odd to me:
+  PRUnichar* newStr=0;
+  newStr = nsLinebreakConverter::ConvertUnicharLineBreaks( ...
+  newStr=0;
+  newStr = nsLinebreakConverter::ConvertUnicharLineBreaks( ...
is it useful to set that to 0?	Is there an autostring that can be used
instead?

Make this have the same formatting (alignments) as neighboring lines:
+  PRBool HavePrivateHTMLFlavor( nsIClipboard *clipboard );


+   res = StripFormattingNodes(contextAsNode);
needs another space before "res"

Comment 40

[Roger Chickering]

I merged these patches onto our branch, and got the following
feedback from a tester:

Okay. I tried the feature. It works, but it works differently than I would
expect. I was hoping for parity with what happens using the MSHTML component.
That's not what happens.

When I select a range of cells in Excel and paste into an MSHTML control, the
table formatting is retained, along with all attributes of the table (e.g,
cell colors, the ability to center the table as a unit, cell formatting, table
grid lines, etc. When I paste into Gecko, the table cells exist, but all other
formatting is lost (e.g., cell colors, ability to center the table as a unit,
table grid lines, etc.)

I also tried content copied from Word, and again the Gecko result was
significantly different than the MSHTML result.  MSHTML retained the exact
formatting of the original document, Gecko did not.

Reopening bug, meaning we need to reopen discussion with Gecko team to see if
they can't provide a fix that gets us closer to the expected behavior. The
measure of success is the answer to the question, "Does copy/paste from Office
documents into a Gecko-editor window behave exactly the same as Outlook
Express?"

Comment 41

[Joe Francis]

i can't look at this for a while.  but i might be able to identify what is
happening with some help from those reporting the issue.  Can you examine the
html source for the table case, for instance, both in the source doc and the
target doc after paste, and tell me if the missing styles on the table
correspond to attributes that are present in the source but not pasted in the
destination?  Or is the missing style being applied by a style sheet in the
source doc?  right now we are not picking up style statements or imported
stylesheets in the <head> of the source doc.

Comment 42

[Roger Chickering]

OK, here's another problem.  If I open a document containing the following
table using IE:

<table x:str="" border="0" cellpadding="0" cellspacing="0" width="480" >
  <col width="181">  <col width="235" >  <col width="64">  <tr  height="17">
   <td height="17" width="181">Here's
a   spreadsheet row</td>
   <td width="235">with a value</td>
   <td width="64">or two</td>
  </tr>
  <tr height="17">
   <td height="17">Here's another row</td>
   <td>with some value</td>
   <td>else</td>
  </tr>
</table>

...and paste it into Gecko editor, Gecko editor gets really lost.

This is a proxy for what happens when you paste from Excel.

Any ideas?

Comment 43

[Juan José Mata]

Can you elaborate?  I tried the case you mention (put some basic HTML around
your table -see below-, opened the file with IE, selected and copied into Gecko
editor where items got copied)

The behavior I expected wasn't met (the table was copied as text, with no HTML
markup) but the editor didn't get "really lost".  What did you mean by that
comment, Roger?

<html>
<head>
<title>
Test
</title>
</head>
<body>
<table x:str="" border="0" cellpadding="0" cellspacing="0" width="480" >
  <col width="181">  <col width="235" >  <col width="64">  <tr  height="17">
   <td height="17" width="181">Here's
a   spreadsheet row</td>
   <td width="235">with a value</td>
   <td width="64">or two</td>
  </tr>
  <tr height="17">
   <td height="17">Here's another row</td>
   <td>with some value</td>
   <td>else</td>
  </tr>
</table>
</body>
</html>

Comment 44

[Juan José Mata]

Ignore my (last) comment #43 .  This hasn't landed on the trunk where I was
testing ...

Comment 45

[Kathleen :Brade]

jjmata--I don't understand.  I'm pretty sure jfrancis checked this into the trunk.

With my debug build from today, I created an html document (which I'll attach
momentarily).  I loaded this file into the browser.  I selected all of the table
with the mouse (top left to bottom right).  I chose Copy.  I created a new
Composer window.  I chose Paste.

The result is a table with 6 cells (2 rows, 3 columns).

RogC, JJMata--What is expected?

Comment 46

[Kathleen :Brade]

hmmm... it seems Joe hasn't landed the latest patches on the trunk.  Regardless,
I can copy and paste from the file I'll attach next.

Comment 47

[Joe Francis]

FYI: This patch has not landed on trunk.  An internal client has been using it
on a seperate branch.  The plan is to land on trunk pretty soon (over next week
or so).  Remaining work for trunk landing is to redo the string handling per
kin's comment above.

Comment 48

[Roger Chickering]

What browser did you use?  The problem is with these patches
on our branch copying that table from IE and pasting into a compose
window results in Gecko confusion.  We are in the process of
merging changes from the 1.2 branch which hopefully will address
the problem.

Comment 49

[Joe Francis]

Attachment: patch to editor

Kin, here is a new editor patch with better string foo.  I'm pretty weak with
the string foo, so I need careful review of the changes I made.  Mostly it is a
direct implementation of your earlier suggestions.  Fortunately an
nsXPIDLString is also a nsAString, so the changes I made didn't explode out
into the signatures of all the other methods.

Comment 50

[Joe Francis]

this last patch is on a stale tree.  as soon as i have a new build i'll merge it
to tip.

Comment 51

[Joe Francis]

Attachment: editor patch merged to tip

Comment 52

[Joe Francis]

Attachment: editor patch with fixes

I did testing with this on wondows, and found a couple of problems with the
string changes.  I also cleaned up some bad indention and added and "ifdef
DEBUG_JOE" to the declaration of a static debugging utility function.

Comment 53

[kinmoz]

Comment on attachment 106389 [details] [diff] [review]
editor patch with fixes

sr=[e-mail] ... nice job jfrancis. Just some minor formatting nits and
one typo:

==== Why all the different spacings for the arg types/names? :-)


+// some little helpers
+static PRInt32 FindPositiveIntegerAfterString( const char * aLeadingString,
nsCString & aCStr );
+static nsresult RemoveFragComments(nsCString & theStr);
+static void RemoveBodyAndHead(nsIDOMNode *aNode);


==== Fix "incllude" typo:


+  PRInt32 oldLengthInChars = fragUcs2Str.Length() + 1;  // +1 to incllude null
terminator


==== My eyes hurt after looking at |HavePrivateHTMLFlavor()|. :-)  Feel free to
use blank lines so it doesn't look like a wall of text. :-P


==== I'm seeing several places where spaces are placed between the parens and
the args in some function calls/exprs but not others, is this intentional?



+  PRBool bHavePrivateHTMLFlavor = HavePrivateHTMLFlavor( clipboard );
+

Comment 54

[kinmoz]

Comment on attachment 100206 [details] [diff] [review]
patch to layout

sr=[e-mail]

Comment 55

[kinmoz]

Comment on attachment 100205 [details] [diff] [review]
patch to htmlparser

sr=[e-mail]

Comment 56

[kinmoz]

Comment on attachment 100334 [details] [diff] [review]
patch to content

sr=[e-mail]

Comment 57

[Kathleen :Brade]

Comment on attachment 106389 [details] [diff] [review]
editor patch with fixes

add the blank line back in:
-
+#if DEBUG_JOE
+nsresult
+nsEditor::DumpNode

This block seems odd to me; you don't assign res" but you check it:
+    aNode->GetChildNodes(getter_AddRefs(childList));
+    if (NS_FAILED(res)) return res;

Add another line to this file to remove the warning about no newline:
+#endif
\ No newline at end of file

inconsistent whitespace and too long of line (try to keep under 80 columns):
+NS_IMETHODIMP nsHTMLEditor::PrepareHTMLTransferable(nsITransferable
**aTransferable , PRBool aHavePrivFlavor )


put this on 2 lines:
+      if (!aHavePrivFlavor)  (*aTransferable)->AddDataFlavor(kNativeHTMLMime);


Isn't this defined somewhere else that you could use?
+  char crlf[] = {nsCRT::CR, nsCRT::LF, 0};


preference is now to use explicit assignment (e.g. foo = do_QI(bar)):
+      nsCOMPtr<nsISupportsCString> textDataObj (
do_QueryInterface(genericDataObj) );


inconsistent whitespace on this line:
+	 textDataObj->GetData ( cfhtml );


inconsistent whitespacing (how about adding some blank lines within that method
too?):
+PRBool nsHTMLEditor::HavePrivateHTMLFlavor( nsIClipboard *aClipboard )


sorry I don't have time to finish :-(
Maybe Akkana or Charley can review the 2nd half?

Comment 58

[Akkana Peck]

> Isn't this defined somewhere else that you could use?
> +  char crlf[] = {nsCRT::CR, nsCRT::LF, 0};

nsCRT has:
#define CRLF "\015\012"     /* A CR LF equivalent string */

Comment 59

[Charles Manske]

Comment on attachment 106389 [details] [diff] [review]
editor patch with fixes

cool!

Comment 60

[Joe Francis]

I have landed this, along with all the requested changes in kin and brade's
final review comments, save for the style nit of constructor-style verses
assignment style initialization for local variables.  Since I've used
constructor style throughout editor, I don't want to start changing it
piecemeal.  Besides, it doesn't matter.

Comment 61

[Charles Rosendahl]

Renominated topembed.  Recommend this be plussed and verified.

Comment 62

[Juan José Mata]

Adding 69566 dependency to make it easier to find next time around.