58377 - Lines after lists messed up

Status: VERIFIED FIXED

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

Description

[Ben Bucksch (:BenB)]

Anthony: Please triage, because if you don't get to it in mozilla0.9, I'll
probably fix it myself.

Comment 1

[Ben Bucksch (:BenB)]

Reproduce:
1. Mailnews HTML composer
2. ul, then normal text directly on the next line after the list
3. Send
4. View in Mozilla

Actual result:
No wrap between last list item and next line (outside the list), e.g.
bla

 * bla
 * bla     bla
bla

Expect result:
bla

 * bla
 * bla
   
bla
bla

Reason:
We indent the empty line after the list, which make the view interpret it
correctly as flowed line and remove the line wrap.

Solution: Make sure that an empty line has no spaces.

Additional comments:
I might take it, if I'm in the right mood. However, would be nice, if somebody
else fixed it :).

[OT]: Akk, you might want to change the default owner of this component.

Comment 2

[anthonyd]

0.9 for embedding story.

anthonyd

Comment 3

[Akkana Peck]

You might want to check with the patch in 42452 -- it might fix this problem too.

Comment 4

[rubydoo123]

setting to nsbeta1

Comment 5

[Ben Bucksch (:BenB)]

This bug annoyed me and I fixed it. Patch follows.

I also fixed a few other problems I saw on the way.
- Over-eager space-stuffing
We space-stuff *all* quotes lines (that's, strictly, the our space after the
">"s), so don't do it again for the special cases like " ", ">", "From " - you
will end up adding an extra space.
- Lines before/after ul/ol
We inserted one empty line before each ol/ul, none after. Gecko inserts one
before each ul/ol of the first level, none for nested uls/ols. I emulate Gecko's
behaviour now (well, nearly: I will insert an extra line for an ul nested in a
ol and vice versa - I need to keep some cooding fun for later ;-P ).

I also found another bug, which I didn't fix: Lines starting with 1 or 2 spaces,
quoted or not quoted, behave odd. Marked the code with XXX.

Daniel, please review.

Comment 6

[Ben Bucksch (:BenB)]

Attachment: Fix, version 1, diff -uw

Comment 7

[Ben Bucksch (:BenB)]

> Gecko inserts one before each ul/ol of the first level

s/before/before and after/

Comment 8

[Daniel Bratell]

I just glanced at the patch and the new logic looks reasonable. I will have to 
apply it and test some too, before I'm satisfied. But... I don't like you 
putting logical operators first on a line instead of last on the previos line. 
It's much more readable keeping hanging operators at the end of a line, and I 
think you will see that is how (almost) everyone else is doing it.

Comment 9

[Akkana Peck]

Personally, I find it more readable with the operator at the beginning of the
continuation line, where it's easy to see.  But I'm not religious about it (and
being consistent with the rest of the current clause makes sense).

The logic looks fine as far as I can tell.

Comment 10

[Ben Bucksch (:BenB)]

I have a better patch.

> I emulate Gecko's
> behaviour now (well, nearly: I will insert an extra line for an ul nested in a
> ol and vice versa - I need to keep some cooding fun for later ;-P )

Now is "later" :-). I emulate Gecko exactly now, I think.

I also found and fixed another bug: Unlike Gecko, we outputted no empty line
after <pre>. We do now.

I am not religious about the operator placement either. I will change it, if you
want. The code uses both styles, so consistency is no issue here.

For you convience, a diff of the diffs:

--- 58377-1.diff	Thu Feb 15 23:39:45 2001
+++ 58377-2.diff	Fri Feb 16 01:06:09 2001
[...]
@@ -21,34 +21,44 @@
 -    // Indent here to support nested list, which aren't included in li :-(
 -    EnsureVerticalSpace(1); // Must end the current line before we change indent.
 +    // Indent here to support nested lists, which aren't included in li :-(
-+    EnsureVerticalSpace(mULCount++ == 0 ? 1 : 0);
++    EnsureVerticalSpace(mULCount++ + mOLStackIndex == 0 ? 1 : 0);
 +         // Must end the current line before we change indention
      mIndent += kIndentSizeList;
    }
    else if (type == eHTMLTag_ol) {
 -    EnsureVerticalSpace(1); // Must end the current line before we change indent.
-+    EnsureVerticalSpace(mOLStackIndex == 0 ? 1 : 0);
++    EnsureVerticalSpace(mULCount + mOLStackIndex == 0 ? 1 : 0);
 +         // Must end the current line before we change indention
      if (mOLStackIndex < OLStackSize) {
        mOLStack[mOLStackIndex++] = 1;  // XXX should get it from the node!
      }
-@@ -734,11 +738,14 @@
+@@ -727,18 +731,23 @@
    }
+   else if ((type == eHTMLTag_tr) ||
+            (type == eHTMLTag_li) ||
+-           (type == eHTMLTag_pre) ||
+            (type == eHTMLTag_dt)) {
+     // Items that should always end a line, but get no more whitespace
+     EnsureVerticalSpace(0);
+   }
++  else if (type == eHTMLTag_pre) {
++    EnsureVerticalSpace(1);
++  }
    else if (type == eHTMLTag_ul) {
      mIndent -= kIndentSizeList;
-+    if (--mULCount == 0)
++    if (--mULCount + mOLStackIndex == 0)
 +      EnsureVerticalSpace(1);
    }
    else if (type == eHTMLTag_ol) {
      FlushLine(); // Doing this after decreasing OLStackIndex would be wrong.
 -    --mOLStackIndex;
      mIndent -= kIndentSizeList;
-+    if (--mOLStackIndex == 0)
++    if (mULCount + --mOLStackIndex == 0)
 +      EnsureVerticalSpace(1);
    }
    else if (type == eHTMLTag_dd) {
      mIndent -= kIndentSizeDD;
-@@ -1045,9 +1052,11 @@
+@@ -1045,9 +1054,11 @@
[...]

Comment 11

[Ben Bucksch (:BenB)]

Attachment: Fix, version 2, diff -w

Comment 12

[Daniel Bratell]

Is it just me, or is MailNews in a really bad shape? I get assertion twice a 
minute. I am not able to output nested lists formatted. How do you do that? 
Anything I do looks "flat". Anyway, I have a couple of comments.

* This breaks the automatic tests for me. Either there is a bug introduced or,  
more likely, the tests have to be modified for the new output.

* Mixing the increment operator and arithmetics are ugly and error prone, and 
won't produce any better code.

* The new comment is wrong. It says "Better don't output a space..." and then we 
do output a space at the next line.

* Do we need any new test cases for this so it doesn't regress?

Comment 13

[Ben Bucksch (:BenB)]

> This breaks the automatic tests for me.

*mumbling* *grumble* Will check them.

> * Mixing the increment operator and arithmetics are ugly and error prone, and
> won't produce any better code.

You mean |mULCount++ + mOLStackIndex == 0 ? 1 : 0|? Will change it to

[...]mULCount + mOLStackIndex == 0 ? 1 : 0
[...]
mULCount++;

> The new comment is wrong. It says "Better don't output a space..."

Oh, yes. I meant 'don't output, if the line is empty'. Will change it.

Comment 14

[Ben Bucksch (:BenB)]

> I am not able to output nested lists formatted. How do you do that?

Debug|Output Text, sending the msg as plaintext all works fine for me. Pulled
15th. Maybe you just have a bad build?

Comment 15

[Ben Bucksch (:BenB)]

Attachment: Fix, version 3, addressing Daniel's feedback

Comment 16

[Ben Bucksch (:BenB)]

> Will change it to
> [...]mULCount + mOLStackIndex == 0 ? 1 : 0
> [...]
> mULCount++;

> Oh, yes. I meant 'don't output, if the line is empty'. Will change it.

Done.

Can I have an r=? Daniel? Anthony?

> This breaks the automatic tests for me.

Where are they now? BTW: Last year, I had a hard time getting them to work.
Hope, it improved since then.

Comment 17

[Daniel Bratell]

I have not applied the patch yet, but about the tests, all I have to do on
Windows is to write TestOutSinks.pl in the mozilla bin directory. (It's a perl
script named TestOutSinks.pl)

The tests are part of the tinderbox tests so if they don't work we would have a
nice orange tree. Maybe you can look at what the tinderboxes do if you have
trouble with them?

Regarding the patch, it looks like good changes. mCurrentLine could of course be
filled after the call to OuputQuotesAndIndent but I don't think that can happen
right now and it's better than what we got. I will run with them, but that
requires me to restart Mozilla so I will start with just this comment. 

Comment 18

[Daniel Bratell]

I fail the first three tests but the two last succeeds. I wouldn't be surprised 
if it's the tests that have to be updated.

Comment 19

[Ben Bucksch (:BenB)]

Daniel, *which* tests? I don't know, where their source lives.

Comment 20

[Ben Bucksch (:BenB)]

Oh, missed your earlier comment. Trying now...

Comment 21

[Ben Bucksch (:BenB)]

wow, the tests actually did show a problem. For
<ul>
<li>item</li>
<li><ol>
    <li>item one</li>
    <li>item two</li>
    </ol>
</li>
<li>item</li>
</ul>
Gecko shows
    * item
    *
         1. item one
         2. item two
    * item
and this is what the converter gave, too - *before* my change. Now, it gives
    * item
       * 1. item one
         2. item two
    * item

I think, the culprit is that I now effectively do |EnsureVerticalSpace(0)|
(instead of |EnsureVerticalSpace(1)|, which does the wrong thing in the normal
case).

Daniel, I thought, |EnsureVerticalSpace(0)| was supposed to ensure a newline
(while |EnsureVerticalSpace(1)| ensures em empty line, i.e. 2 newlines), not?

How can we fix this? I don't think, I can do anything about that in my code,
since ...(0) does the right thing, if there is text on the line, and I don't
know, if there is text on the line.
[pause]
I looked into your code, and it definitely seems to be the culprit. Lots of
checks, if the line is empty, and behaviour based on that. This is all a bit too
fancy for me too touch. Can you propose or create a solution?

Comment 22

[Ben Bucksch (:BenB)]

BTW: I hacked TestOutput, so I can make it *write* to the "should" output file,
not compare with it. You'd just need to use an additional "-g" (generate) on the
commandline. Will attach patch.

Comment 23

[Ben Bucksch (:BenB)]

Attachment: Patch für test app, version 1. Note: It depends on std C++ lib now.

Comment 24

[Daniel Bratell]

EnsureVerticalSpace(0) says that we want a new line.
EnsureVerticalSpace(1) says that we want 1 empty line.

What do you think is the problem more exactly?

Comment 25

[Ben Bucksch (:BenB)]

The fact the |EnsureVerticalSpace(0)| doesn't do what I say. I do say that when
the nested <ol> is started, nevertheless, the first <li> of the <ol> starts at
the same line as the <li> of the <ul> it is nested in.
Expected result:
 * item
    *
         1. item one
         2. item two
    * item
Actual result:
    * item
       * 1. item one
         2. item two
    * item
In other words, my |EnsureVerticalSpace(0)| is simply ignored, just because the
line before it contains no real text.

Comment 26

[Daniel Bratell]

You are right. EnsureVerticalSpace doesn't take quotes and list bullets into
consideration. That is often right in case of inserted quotes, but might not be
so when there is other stuff inserted. 

Ach. I don't know how to solve that without digging into the source. You're
probably more familiar with the problem than I am so I don't think I can come
with any wise words right now.

(As a side note, I think I like the look you get better than the "correct" one.)

Comment 27

[Ben Bucksch (:BenB)]

Daniel, I can't do much about the problem. I don't know, how to work around that
from my code, and I don't want to touch your code (the general output system
with EnsureVericalSpace, EndLine, AddToLine etc.), because I fear side-effects.

> (As a side note, I think I like the look you get better than the "correct" one.)

At the very least, the increased indention of "*" is wrong.

Comment 28

[Ben Bucksch (:BenB)]

Ops, I just noted that the spacing went wrong:
> Expected result:
>  * item
>     *
>          1. item one
>          2. item two
>     * item
should have been:
Expected result:
    * item
    *
         1. item one
         2. item two
    * item

Comment 29

[Akkana Peck]

Nintpick: I actually would have expected something different:  the output would
look better if it looked like this:
    * item
         1. item one
         2. item two
    * item
I don't think users would expect to see the bullet before the sublist, would they?
Maybe starting a list should remove the last item in the current indent string.

As long as the extra bullet is there, it looks like a mistake whether it's on
its own line or on the same line with the first sublist item.

Comment 30

[Ben Bucksch (:BenB)]

akk, I disagree, there is an important semantical difference. In one case, the
ol is a sub of the "item", in the other case, the ol is its own section. I don't
know a good example off-hand, but I'm sure that there are cases, where omitting
the star would alter the meaning.

Comment 31

[Daniel Bratell]

This is not trivial. I thought a little more on it and for the indentation we
probably have to be more intelligent about how we build the mInIndentString.
Right now we just append all the list stuff with no more space then necessary. 

So after a <ol> and a <ul> we have mInIndentString = "* 1. ". Had it been "*   
1. ", that would have been better. i.e. Add the correct number of spaces before
adding "1.". 

On the other hand, that would only solve the problem if we started to use the
wrong way. If I understand correctly, we want the EnsureVerticalSpace(0) to
insert a new line also if we already have a new line, but we also have something
in the indentation we want do display?

This is a completely untested and handwritten patch you could play with, Ben. I
don't if I like it myself, but maybe it can get you further:

  void
  nsPlainTextSerializer::EnsureVerticalSpace(PRInt32 noOfRows)
  {
+   // If we have something in the indent we probably want to output
+   // it and it's not included in the count for empty lines so we don't
+   // realize that we should start a new line.
+   if(noOfRows >= 0 && mInIndentString.Length() > 0) {
+     EndLine(PR_FALSE);
+   }
+
    while(mEmptyLines < noOfRows) {
      EndLine(PR_FALSE);
    }
  }


Also something will need to be done in EndLine to reset the mEmptyLine variable
to 0 (or -1) if we have something in the indentation that we just output. Note
that quotes are never counted as text, and are not in the mInIndentString.


I would really like us to get over these last bumps because this patch solves
quite a few whitespace bugs, not only with lists.

Comment 32

[Ben Bucksch (:BenB)]

> If I understand correctly, we want the EnsureVerticalSpace(0) to
> insert a new line also if we already have a new line, but we also have
> something in the indentation we want do display?

Yes, for the purposes of that function, I would count the indentstring as
content. At least that is how I expected it to work.

> I don't if I like it myself, but maybe it can get you further:

I don't understand.

I applied your change and it worked right out of the box. All tests now
complete, modulo the changes I intentionally caused.

> I would really like us to get over these last bumps because this patch solves
> quite a few whitespace bugs, not only with lists.

Thanks :).

One other thing I noticed: We use "* " as indentstring. This means that, in the
case of the above item with no content on that line, we end up with a line
"     * ". This would be interpreted as flowed line, which it isn't. In other
words, after all that hassle, a flowed-aware mailer would display
"     *     1. item one" for the example discussed above. I think, I should file
a new bug on that problem.

Comment 33

[Ben Bucksch (:BenB)]

> Also something will need to be done in EndLine to reset the mEmptyLine variable
> to 0 (or -1) if we have something in the indentation that we just output.

Considering that it works(TM), should we ignore that (for now, maybe add a
comment), or what should we do about that?

Comment 34

[Daniel Bratell]

>> I don't if I like it myself, but maybe it can get you further:
>
>I don't understand.

I missed a "know". The change I attached is kind of hackish, and the new 
attachment (also untested) I will attach is too. The output looks alright in the 
nested lists case but I have not tested it thouroghly and there are a lot of 
special cases. The attachment contains both your and my changes. I think you see 
what I've done.

Comment 35

[Daniel Bratell]

Attachment: Patch including changes to EnsureVerticalSpace, EndLine and OutputQuotesAndIndent

Comment 36

[Ben Bucksch (:BenB)]

I can't review Daniel's changes, because I don't understand *all* their effects.
I ran them, and they worked fine (IIRC - but I'll double-check before check-in).

Could anybody (Anthony, Akk) review Daniel's changes, please?

Comment 37

[Akkana Peck]

I've been swamped trying to land an editor change (and being out of town in the
middle of that for a family emergency), but I guess anthony is also swamped
helping Mike land his editor change.  Sorry no one was available to review.

I don't understand all the changes either, but they look reasonable to me and
should be fine to go in.  They do require changing the output tests, as you
noted, but in good ways, e.g. simplecopy.out was missing part of simple.html
which it should have included, and now it includes it; and removing all the
extra spaces on blank lines and at the ends of lines is also a good thing.

r=akkana.  Sorry again for the delay.

Comment 38

[Ben Bucksch (:BenB)]

akk, thanks for the review. Requested sr from jst.

Comment 39

[Johnny Stenback (:jst)]

Patch looks good to me, but I would've left:

 FlushLine(); // Doing this after decreasing OLStackIndex would be wrong.
-    --mOLStackIndex;
^- this here
     mIndent -= kIndentSizeList;
+    if (mULCount + --mOLStackIndex == 0)
and not done the decrement inline here
+      EnsureVerticalSpace(1);

... for readability. And:

+    while(lineLength > 0 &&
+          ' ' == stringToOutput[lineLength-1]) {

I personally hate constant == variable comparisons, variable == constant makes
so much more sense.

+      --lineLength;
+    }

But either way, it's your call if you wanto change any of this.

sr=jst

Comment 40

[Ben Bucksch (:BenB)]

Attachment: Differences in testoutput

Comment 41

[Ben Bucksch (:BenB)]

jst, thanks for the fast sr.

I created some complex testcase and converted it with both the proposed fix and
0.8.1. Note that the header prefs for the 2 binaries were different (which
accounts for the differentces with "1." / indention), and there's a save/load
between it (which is propably the reason for the diff in the japanese chars). I
attched the diff -u between the 2 outputs. I don't see any problems introduced
by the fix. (I did find unrelated problems, see bug 75301, where you can also
find the full testcase.) Now up to the standard tests...

Comment 42

[Ben Bucksch (:BenB)]

Attachment: Changes for automated tests, version 1

Comment 43

[Ben Bucksch (:BenB)]

OK, for the testapp to work (again) and checked the automated tests again. The
only changes required are intended and an improvement. Patch attached.

Also, I addressed jst's feedback. I very much agree with his second point.

I'm going to check in soon.

Comment 44

[Ben Bucksch (:BenB)]

Checked in, incl. changes to automated tests. Marking FIXED. Yeah.

Comment 45

[Ben Bucksch (:BenB)]

Attachment: Fix, version 5 (which went in). Incorporates jst's feedback.

Comment 46

[sujay]

marking verified.