50497 - Missing newlines in TXT output

Status: VERIFIED FIXED

(Core :: DOM: Serializers, defect, P3)

Description

[Ben Bucksch (:BenB)]

Reproduce:
1. Open new html msg
2. Enter 5 lines of text
3. mark (parts of) lines 2-4
4. ul
5. goto end of line 4
6. hit return twice
7. enter some text
8. Debug|OutputText

Actual result:

zuitzuitzuizui

    * tzu7izuizui
    * tzuitzuitzui
    * tzuitzuizui

zuitzuitzuitzui

Expected result:

zuitzuitzuizui

    * tzu7izuizui
    * tzuitzuitzui
    * tzuitzuizui

zuitzuit
zuitzui

Additionoal comments:
Debug->OutpufXIF shows:

<content>zuitzuit</content>
<leaf isa="br">
<attr name="_moz_dirty" value=""/>
<attr name="type" value="_moz"/>
</leaf><!--br-->
<content>zuitzui</content>
<leaf isa="br">
<attr name="_moz_dirty" value=""/>
</leaf><!--br-->

Note the "type=_moz". What's that??? It seems that this won't output in the TXT
sink, but the HTML sink.

WHo I found this: I just sent a mail with an ul and a line directly after it
(created similar to above), but out came
  * foo
  * bar
  baz
(note the indention of the last line).

Comment 1

[Akkana Peck]

If there's a br type=_moz being inserted where it shouldn't be, that would be
Joe's domain (but I'm staying on the cc list in case I misunderstood something).

Comment 2

[rubydoo123]

moving this to m19

Comment 3

[rubydoo123]

marking nsbeta3-

Comment 4

[Ben Bucksch (:BenB)]

Beppe, this is data corruption, and not even hard to run in.

Comment 5

[rubydoo123]

there isn't loss of data, there is a workaround (as painful as it may be) and 
joe is just too overloaded for b3, if we can get help -- great. i did put this 
to m19 and not future

Comment 6

[Ben Bucksch (:BenB)]

beppe, the problem is that you won't see that anything is wrong until you sent
it.

Comment 7

[Hixie (not reading bugmail)]

The Debug->OutputXIF stuff is internal, don't worry about the _moz_xxx stuff.

BenB: The problem is that HTML->Text conversion is failing to render newlines 
correctly? Does this happen with all newlines? What is the exact pattern?

Comment 8

[Ben Bucksch (:BenB)]

Ian, the HTML->TXT converter is correct, I checked it.

Comment 9

[Joe Francis]

I'll try to look at this when I get the chance...

Comment 10

[rubydoo123]

marking as future, this is not a pull-it-off-the-wire bug

Comment 11

[Joe Francis]

moz 0.9

Comment 12

[Joe Francis]

Attachment: bug fix patch to various layout files

Comment 13

[Joe Francis]

Attachment: new source file: layout/base/src/nsCopySupport.h

Comment 14

[Joe Francis]

Attachment: new source file: layout/base/src/nsCopySupport.cpp

Comment 15

[Joe Francis]

attached patch + new files is the fix.  New files are because I took this august
occassion to factor the redundant copy code out of nsPresShell.cpp and
nsAutoCopy.cpp and put it in one place.  Only there wasn't a good place, so I
created a new source file that implements an nsCopySupport class (very simple
class: one static method).

The fix is to make sure that we query the encoder for what mime type is REALLY
used, since it may change it to plaintext out from under you.

requesting review and sr.  akkana?  simon?

Comment 16

[Simon Fraser [no longer active]]

You could replace

    rv = nsComponentManager::CreateInstance(kCTransferableCID, nsnull, 
                                            NS_GET_IID(nsITransferable), 
                                            getter_AddRefs(trans));

with 
  trans = do_CreateInstance(kCTransferableCID, &rv);

Otherwise, it looks fine. sr=sfraser.

Comment 17

[Akkana Peck]

Should kHTMLContext and kHTMLInfo be defined in a more global place in case
anyone else might want to try using them?

In nsAutoCopy, you don't need to do
  if (NS_FAILED(rv))
    return rv;
  return rv;
you can just return rv, or return nsCopySupport::HTMLCopy(aSel, doc).

Uh-oh -- I'm getting asserts every time I mouse over the window:
###!!! ASSERTION: NS_ENSURE_TRUE(NS_SUCCEEDED(rv)) failed: '(!((rv) &
0x80000000))', file nsDocumentEncoder.cpp, line 908
Sorry, I told Joe earlier that the patch was working fine in my tree, but it
turns out that I had backed it out and hadn't re-applied it.  This assert is
complaining that it didn't get the unicode encoder; what that has to do with
Joe's code I don't see, but it was the only change I made this time.

I'll poke around and try to figure out what's going on.

Comment 18

[Akkana Peck]

I bet the asserts are because I didn't rebuild outside layout, so all the other
libraries are confused about the atom definitions due to the new atoms that were
added.  I'm rebuilding from the top.

Comment 19

[Akkana Peck]

Rebuilt, the assert is gone, but copy can no longer paste to external apps. 
Paste to editors inside mozilla still works.

Comment 20

[Akkana Peck]

Autocopy broke because autocopy used to do
 mClipboard->SetData(trans, nsnull, nsIClipboard::kSelectionClipboard);
while now it calls nsCopySupport::HTMLCopy() which does:
 clipboard->SetData(trans, nsnull, nsIClipboard::kGlobalClipboard);
It's copying to the wrong clipboard.  Looks like HTMLCopy() needs to take an
argument indicating which of the two clipboards to copy to, and then nsAutoCopy
can pass in nsIClipboard::kSelectionClipboard while nsPresShell can pass
nsIClipboard::kGlobalClipboard.

Comment 21

[Joe Francis]

ok, so maybe that common code wasslightly less common than I thought.  Here is a
tweaked set of diffs/new files.

Comment 22

[Joe Francis]

Attachment: updated diffs

Comment 23

[Joe Francis]

Attachment: updated nsCopySupport.cpp

Comment 24

[Joe Francis]

Attachment: updated nsCopySupport.h

Comment 25

[Akkana Peck]

I'm having trouble getting the patch to compile, because I think it's dependant
on my having patches for some of the other bugs Joe is working on, and I just
deleted some of those so that I could re-apply this one.

So I can't test it, but it looks like exactly the right thing.
r=akkana (but if you can mail me a copy of nsDocumentEncoder.cpp I can probably
test it -- I think I probably have the rest).

Comment 26

[Joe Francis]

fixed

Comment 27

[R.K.Aa.]

nsCopySupport.h:33: warning: file does not end in newline

Comment 28

[Ben Bucksch (:BenB)]

Using the reproduction steps mentioned in the description, this is fixed for me
(i.e. "vrfyed"). Tested with Mozilla 0.8 Linux build.

Comment 29

[sujay]

marking verified.