234427 - CDATA sections not copied for copy&paste

Status: RESOLVED FIXED

(Core :: DOM: Serializers, defect)

Description

[Sebastian Redl]

User-Agent:       
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.6) Gecko/20040127

The example page is part of my C++ tutorial. All content is XHTML 1.1 and
delivered as text/xml by the server. The code examples are CDATA sections.
Mozilla doesn't let me copy selected text which contains text from the CDATA
sections into the clipboard. It doesn't simply ignore the CDATA text, it ignores
any selection which spans into CDATA sections. The copy option is active, so
apparently it knows that there is selected text, but using the option has no effect.
At first I blamed nsPlainTextSerializer, because it does nothing on encountering
a CDATA, but some code changes and the fact that it doesn't copy normal text
nodes if CDATA is also in the selection proved me wrong. Or maybe I only solved
part of the problem.

I'd be willing to help in solving the problem, if someone can point me to a
resource explaining the way copy & paste works in Mozilla, or better yet, how
the whole of the Mozilla code "works".

The problem occurs in every Gecko version I've tried: Mozilla 1.4a to 1.6b on
Win2k, Mozilla 1.6 on Linux (GTK2) and Firefox 0.8 on Linux (GTK2).

Of course, this might be a problem in that my XML declaration states UTF-8 as
character set while the server doesn't report any character set and the text/xml
content should therefore default to usascii. But is Mozilla really affected by this?

Reproducible: Always
Steps to Reproduce:
1. Go to the example page.
2. Select some text that at least partly includes something from the code
samples (the blue boxes). Use any action to get it into the clipboard (Ctrl+C,
right-click->Copy, Edit->Copy)
3. Paste into a text editor.

Actual Results:  
The pasted text is whatever was previously in the clipboard. The contents of the
clipboard haven't changed.

Expected Results:  
The pasted text should be the text selected on the web page.

Comment 1

[Sebastian Redl]

Forgot to add: the problem occurs on every page in this tutorial where I use
CDATA sections. Haven't tried anything else.

Comment 2

[Boris Zbarsky [:bzbarsky]]

So I just tried this in a debug build -- highlight some text including CDATA
text on that page.  That should place the text in PRIMARY, so it invokes the
selection code.  On the console I see:

WARNING: NS_ENSURE_TRUE(content) failed, file
/home/bzbarsky/mozilla/debug/mozilla/content/base/src/nsDocumentEncoder.cpp,
line 629
WARNING: NS_ENSURE_TRUE(NS_SUCCEEDED(rv)) failed, file
/home/bzbarsky/mozilla/debug/mozilla/content/base/src/nsDocumentEncoder.cpp,
line 728
WARNING: NS_ENSURE_TRUE(NS_SUCCEEDED(rv)) failed, file
/home/bzbarsky/mozilla/debug/mozilla/content/base/src/nsDocumentEncoder.cpp,
line 850
WARNING: NS_ENSURE_TRUE(NS_SUCCEEDED(rv)) failed, file
/home/bzbarsky/mozilla/debug/mozilla/content/base/src/nsDocumentEncoder.cpp,
line 894

So for some reason the aNode there is probably null... Is that a reasonable
start?  ;)

Comment 3

[Sebastian Redl]

That would mean that the call to GetChildAt in
nsDocumentEncoder::SerializeRangeNodes (nsDocumentEncoder.cpp:721) in the first
call returns 0, or the return value can't be cast to a nsIDOMNode. Though I
think the latter would generate a warning too. The recursive call on line 724
would then be the one where the ENSURE_TRUE fails.

Comment 4

[Boris Zbarsky [:bzbarsky]]

> the call to GetChildAt in nsDocumentEncoder::SerializeRangeNodes in the first
> call returns 0

That really shouldn't happen.

> or the return value can't be cast to a nsIDOMNode

That shouldn't happen either (CDATA sections certainly QI to nsIDOMNode).

It sounds like stepping through this in a debugger is the way to go....
(breaking in that function and seeing what's going on).

Comment 5

[Sebastian Redl]

Did that. Apparently the code tries to go through the children of the CDATA
section, because the code branch for elements is taken. The code branch for text
nodes should be taken. There is no code branch for CDATA sections.

nsDocumentEncoder.cpp:654
    // due to implementation it is impossible for text node to be both start and
end of 
    // range.  We would have handled that case without getting here.
    if (IsTextNode(aNode))   // Line 654
    {
... (code for handling text)
    }
    else
    {
... (code for handling elements)

IsTextNode only recognizes text nodes, and is implemented on line 586ff.
static PRBool IsTextNode(nsIDOMNode *aNode)
{
  if (!aNode) return PR_FALSE;
  PRUint16 nodeType;
  aNode->GetNodeType(&nodeType);
  if (nodeType == nsIDOMNode::TEXT_NODE)
    return PR_TRUE;
  return PR_FALSE;
}

I looked at every usage of this function in the file and I believe it would be
safe, even beneficial, to simply modify the function to also accept
nsIDOMNode::CDATA_SECTION_NODE as valid value.


I also believe that the whole Mozilla code in the content subdir is full of
forgetting to handle CDATA sections.

Comment 6

[Sebastian Redl]

Attachment: Archive of three patches

Don't know how to make a single diff file for multiple sources :o, so I tarred
the three files up.

The other two patches are for nsPlainTextSerializer, which would simply ignore
CDATA sections without this.

By applying this to my files, I was able to resolve this bug, and maybe a few
others ;), but I don't know if I introduced any.

Comment 7

[Boris Zbarsky [:bzbarsky]]

Sebastian, just do the diff from a common root.  Eg from the tree root:

cvs diff -u content/base/src/foo content/html/content/src/bar
content/base/public/whatever > patchfile.txt

Comment 8

[Sebastian Redl]

Attachment: Patch for bug.

Ok, it's a proper patch now.

Comment 9

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 141542 [details] [diff] [review]
Patch for bug.

akkana, could you take a look?

One thing that worries me here is the IsTextNode call in
nsHTMLCopyEncoder::EncodeToStringWithContext -- should that really just grab
the text of the CDATA?	Or also the <![CDATA[ ]]> part?

Sebastian, for future reference, patches are easier to read with more context
and using the -p option (eg diff -pu8 or so).

Comment 10

[Sebastian Redl]

(In reply to comment #9)
> (From update of attachment 141542 [details] [diff] [review])
> akkana, could you take a look?
> 
> One thing that worries me here is the IsTextNode call in
> nsHTMLCopyEncoder::EncodeToStringWithContext -- should that really just grab
> the text of the CDATA?	Or also the <![CDATA[ ]]> part?

I don't think so. When I copy text, I want to copy what I see on the screen, not
what is in the source. If the (likely XML-unaware) readers of my tutorial copy
the example code I don't want them to have <![CDATA[ somewhere in there.

> 
> Sebastian, for future reference, patches are easier to read with more context
> and using the -p option (eg diff -pu8 or so).
> 

Ok, thanks.

Comment 11

[Boris Zbarsky [:bzbarsky]]

> When I copy text, I want to copy what I see on the screen

Oh, ok.  So that code is only used for serialization when copying in the
plaintext flavor?

Comment 12

[Akkana Peck]

Comment on attachment 141542 [details] [diff] [review]
Patch for bug.

> So that code is only used for serialization when copying in the
> plaintext flavor?

Yes.  I'm curious what happens when pasting to an html-aware client.  Does it
preserve the CDATA?

On the patch:

>   // Consume the last bit of the string if there's any left
>-  if (NS_SUCCEEDED(rv) & (start < length)) {

Good catch!

>+NS_IMETHODIMP 
>+nsPlainTextSerializer::AppendCDATASection(nsIDOMCDATASection* aCDATASection, 
>+                                  PRInt32 aStartOffset,
>+                                  PRInt32 aEndOffset, 
>+                                  nsAString& aStr)

The last three arguments don't seem to line up with the first one.  Can you fix
that?  (Using spaces -- no tabs, please.)

There's so much in common between AppendText and AppendCDATASection -- I wonder
if they're similar enough that we could just make AppendText take an
nsISupports or some other base class, and avoid having to add this new code
block?	It looks like the code is just the same after you do the QI to
nsITextContent.  But then all callers of AppendText would have to QI first ...
might be a hassle tracking that down.

The code looks fine, and I'm going to go ahead and mark it r+ (please check
that indentation, though), but if you think you might be able to change it to
make one routine handle both cases, that would be even better and would help
keep mozilla on a diet.  I'll leave it to the sr to decide how important this
is.

Comment 13

[Boris Zbarsky [:bzbarsky]]

> I wonder if they're similar enough that we could just make AppendText take an
> nsISupports or some other base class

nsIDOMText is a base class for nsIDOMCDATASection.  See
http://lxr.mozilla.org/seamonkey/source/dom/public/idl/core/nsIDOMCDATASection.idl#56
(which is frozen, so this is not going to change).  So AppendCDATASection could
just call straight through to AppendText in the plaintext case, no?

And please do test what happens when pasting into composer (preferably composer
running in a separate process to ensure that we hit the OS clipboard instead of
passing around some internal data).

Comment 14

[Akkana Peck]

Comment on attachment 141542 [details] [diff] [review]
Patch for bug.

The html paste case shouldn't go through this code at all, so I'm not worried
about this patch making anything worse for html -- but I do wonder if we need a
similar patch for the html case.

And of course, Boris is right, having the CDATA method call through to
AppendText is the easy way to go.  Sebastian, can you make a patch that does it
that way instead?

Comment 15

[Sebastian Redl]

Should I QI to IDOMText or just cast?

Comment 16

[Boris Zbarsky [:bzbarsky]]

Neither.  Just pass it.  The compiler will do an implicit static cast to a
superclass.

Comment 17

[Sebastian Redl]

Attachment: Reviewer comments implemented

-pU8 to diff, indentation corrected and AppendCDATASection simply calls
AppendText.

Comment 18

[Sebastian Redl]

You were right btw, the code was indeed exactly the same, except for the single
changed argument. It was just a quick hack.

Comment 19

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 141590 [details] [diff] [review]
Reviewer comments implemented

sr=bzbarsky, if you remove the comment in AppendCDATASection.  It's not really
needed -- it's pretty clear what's going on.

Akkana, this look ok to you?

Comment 20

[Sebastian Redl]

Attachment: Comment removed

Done

Comment 21

[Akkana Peck]

Comment on attachment 141590 [details] [diff] [review]
Reviewer comments implemented

Looks great!  Thanks.
The final patch had some extra stuff (seems to be removing the previous version
of AppendCDATASection) so I'm putting my r+ on this one instead.

Comment 22

[Boris Zbarsky [:bzbarsky]]

Attachment: Merged to current tip

Comment 23

[Boris Zbarsky [:bzbarsky]]

Fix checked in for 1.7b

Comment 24

[Boris Zbarsky [:bzbarsky]]

Note that this doesn't seem to work, still... See bug 270145