17883 - [nsHTMLToTXTSinkStream] Indention with heading spaces

Status: VERIFIED FIXED

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

Description

[Ben Bucksch (:BenB)]

Trying to work on bug #16800, I found the following strange behaviour:

Reprocude:
const  PRInt32 gOLNumberWidth = 3;
const  PRInt32 gIndentSizeList = MaxInt(gTabSize, gOLNumberWidth + 3);
                               // Indention of non-first lines of ul and ol

OpenContainer:
  if (type == eHTMLTag_ul)
  {
    // Indent here to support nested list, which aren't included in li :-(
    mIndent += gIndentSizeList;
  }
  else if (type == eHTMLTag_li)
  {
    nsAutoString temp = Spaces(gIndentSizeList - gOLNumberWidth - 2);
    temp += Spaces(gOLNumberWidth) + "*";
    temp += ' ';

    mIndent -= gIndentSizeList;    // don't indent first line so much
    Write(temp);
    mIndent += gIndentSizeList;
  }
ol deleted for clearance

Actual Result:
        * Status reportst item 2s some very run-o-the-mill text in a
        paragraph tag, for those of you who are into that kind of thing.
        I think at this point I shall regale you with some of my
        inestimable poetry, which will be presented in blockquote mode
        for your reading pleasure. Th
(8 spaces before every line)

Expected Result:
      * Status reportst item 2s some very run-o-the-mill text in a
        paragraph tag, for those of you who are into that kind of thing.
        I think at this point I shall regale you with some of my
        inestimable poetry, which will be presented in blockquote mode
        for your reading pleasure. Th
(6 spaces before the first line, 8 before the other lines)

This was introduced with format=flowed.

Comment 1

[Akkana Peck]

Yes, we need more of an indent when we're inside a list item (indent to the
width of the bullet or number character plus the spaces after it).  Ben, want me
to work on this first, or wait for your fix to 16800?  You marked 16800
dependant on this one rather than the other way around, so I'm not sure.

Comment 2

[Ben Bucksch (:BenB)]

Akk,

I'm working on lists as my code fragment shows. With Daniel's mail, it's clear,
that this doesn't work, since he chaged the behaviour of Write. I assigned all
bugs to you, because I didn't dare to bug Daniel.

As state of now, this isn't a real bug. I just need a hint from Daniel, how to
do what I want, because I don't really understand, what all his functions do at
the moment.

Comment 3

[Daniel Bratell]

Does the mail clear things up?

Comment 4

[Ben Bucksch (:BenB)]

Daniel,
as said: I know now, that it doesn't work and why, but I'm still lacking a
solution. Could you please give one?

It is clear to you, what I want to do, isn't it?

Comment 5

[Ben Bucksch (:BenB)]

see the last patches attached to bug #16800 for the full context.

Comment 6

[Daniel Bratell]

Ben, your mailserver has decided that you don't exist but I guess you'll read
this sometime:
"I just thought of something else that might be confusing. Write does whitespace
compression in certain modes and then you can't use Write(" ") to indent a line
for instance."

Comment 7

[Ben Bucksch (:BenB)]

Daniel,
finally read it, mailserver is working again.
Yes, I thought, that it does something like this. This was my only explanation
for the strange behaviour.
Could you please provide a solution? It should be much easier for you than for
me, and I spent already enought time on the fix...

Akk's patch is the current version IIRC.

Comment 8

[Ben Bucksch (:BenB)]

Daniel,
I'm currently tring to implement bug #17723, so you may not have to help me
here.
I hope, won't have to rewrite format=flowed for that, but it looks like I have
to :-(. I'll keep you updated.

I let this open, because I might give up at 17723 and depending bug #18012.

Comment 9

[Daniel Bratell]

Ok. I'm with you. I'm not planning any major work on mozilla this weekend but
I'll be glad to help with testing, ideas and such. Bug 18012 could be a really
hard one. I added support for really simple data tables in what I did last week
so that one could copy and paste from them. The problem comes when a table
consists of a whole "document" in each cell. That could be really hard to do
good with plain text.

Regarding that indentation problem I've been thinking a little more of that and
I think adding one more boolean flag (mLineHasNumber or something like
that) could be a solution. Then you don't have to decrease the indention value
and then increase it again immediatly after which anyway didn't work when the
line was cached. You just set the flag and go in. The WriteQuotesAndIndents
function should than take that boolean flag into account when calculating the
indentation and then reset it.

Comment 10

[Akkana Peck]

Bumping the milestone out on this so it doesn't conflict with Beth's radar.

Comment 11

[Daniel Bratell]

What about keeping an "mIndentString" that keeps the "*" and numbers and so on 
until it's time to emit the indent. Then you can calculate the correct number of 
spaces in nsHTMLToTXTSinkStream::WriteQuotesAndIndent() and output the 
mIndentString and voila, the bug is fixed. I could do this as soon as I know 
that the changes for 16398 are in and works.

Comment 12

[Akkana Peck]

Yes, that sounds ideal.  If you want to help with this, Daniel, I'll be very
happy to take your changes.

Comment 13

[Ben Bucksch (:BenB)]

Yes, that would be a solution. Would you mind to change the code for the tags as
well? It's very easy, you're on it anyway, and I I have no time currently.

Comment 14

[Daniel Bratell]

Done. 
I'll attach the patch waiting for someone (Akkana?) to review it and check it 
in.

Comment 15

[Daniel Bratell]

Attachment: A patch to fix the bug.

Comment 16

[Ben Bucksch (:BenB)]

The patch doesn't apply for me. Are your nsHTMLToTXTSinkStream.*
current or is that a problem on my side?

Comment 17

[Daniel Bratell]

I think it's a problem on your side. This patch is against a clean file from 
CVS. Maybe you have own changes on the very lines that are changed. 

Comment 18

[Ben Bucksch (:BenB)]

The problem was, that there were CRLRs linebreaks. Attaching the fixed patch for
akk.

Comment 19

[Ben Bucksch (:BenB)]

Attachment: Same patch, but without CRs

Comment 20

[Ben Bucksch (:BenB)]

I took a quick look and it looks good to me.

I account the quirkiness with indention (I managed to get 2 or 3 different
intentions for the same indention level) to the editor.

Comment 21

[Daniel Bratell]

Ah, thank you. I think I will stop using cmd.exe unless it's something else that 
loves the CRLF:s.

Comment 22

[Akkana Peck]

The patch looks fine to me.  I'll check it in when the tree opens.  Thanks!

Comment 23

[leger]

Bulk move of all "Output" component bugs to new "DOM to Test Conversion" 
component.  Output will be deleted as a component.

Comment 24

[Akkana Peck]

Uh-oh -- I should have checked this earlier, but I was sidetracked on a PDT+
bug.  Anyway, this patch breaks the output test, because it wraps in the copy
case (flags=0).  This is bad -- flags=0 should not wrap at all.

I won't have time until after M14 to debug it and try to fix this problem in the
patch, but if one of you has a fix, I'll take it and try to get it in for M14.

Comment 25

[Daniel Bratell]

Do you have any other changes in your tree that could cause that? I see no wraps 
at all and can't understand how these changes could affect the wrapping. 

It's more likely that the patches in bug 27062 or bug 16398 causes this if 
anything.

Comment 26

[Akkana Peck]

Marking M15 since this isn't in on the beta radar.

Comment 27

[Daniel Bratell]

I reimplemented the fix and tested it and I get no line breaks with flag == 0. 
That is without any other changes in the tree. I went through the tests and one 
failed because the "correct output" is wrong which this patch fixes. I will 
attach the new patch (probably with CRLF, but a "dos2unix" should fix that if 
needed)

The patch includes changes to the simplefmt.out file used in the tests so that 
it works on my computer. 

Comment 28

[Daniel Bratell]

Attachment: The final (?) fix including test changes

Comment 29

[Akkana Peck]

The fix has been checked in (all hail the M15 tree opening!)

Comment 30

[sujay]

need help marking this bug verified-fixed...

Comment 31

[Ben Bucksch (:BenB)]

sujay,

Use the HTML editor to create a mail with an ordered or unordered list. Create

listitems with text longer than one line (just write a lot of text without

hitting return, only hit return to end one listitem).



Compare the result with the ones shown in the bug description.

Comment 32

[sujay]

verified in 3/14 build.