39728 - Don't generate   for flowed

Status: RESOLVED FIXED

(MailNews Core :: MIME, defect, P3)

Description

[Ben Bucksch (:BenB)]

Split-off from bug 39370.

At least in the quoting case (better always IMO), we really shouldn't use nbsp
just to get fixed lines. Use e.g.
  - quoting
    - flowed paragraphs*: tt + br
    - fixed lines: pre or pre wrap
  - viewing wrap=on
    - flowed paragraphs*: tt + br or pre wrap
    - fixed lines: pre wrap
  - viewing wrap=off
    - flowed paragraphs*: tt + br or pre wrap
    - fixed lines: pre
  *the whole paragraph is one ling line; the same as the qp text converter does

The proposed tags are open for discussion.

Comment 1

[Daniel Bratell]

I just remembered another reason why   was used originally:
* Multiple whitespace, for instance "Sentance    with    large    spaces"

Without   the whitespaces are compressed. (Bye, bye ASCII-art).

Comment 2

[Ben Bucksch (:BenB)]

Daniel, no, pre preserves whitespace (at least it should).

Comment 3

[Daniel Bratell]

But it removes the <tt> option above. There seems to be alternatives everywhere 
but for one place so if only that can be dealt with, maybe it's still possible. 

Comment 4

[Ben Bucksch (:BenB)]

Daniel, there is no ASCII-art in flowed paragraphs, is there?

Comment 5

[Daniel Bratell]

Could be. There is no way to align two lines with eachother without explicit 
line breaks, but it's sure possible to send mails with multiple spaces anyway, 
and Mozilla should display them with multiple spaces. It could for instance be 
an indented first line in a paragraph. Like:

      This is a text that goes on and on and on and on and on and on and on and 
on and on and on and on and on and on and on and on...

Comment 6

[Ben Bucksch (:BenB)]

ic, you could substitute all spaces in a row but the first one with &nsbr;.
That's what the editor does.

Comment 7

[Daniel Bratell]

Or make every other SPACE a   ? That way you could wrap even if there were 
many spaces.

Comment 8

[Ben Bucksch (:BenB)]

After private discussion pre email with Daniel, it turned out to be to just
change the nbsp generation and not to use pre. (Comments are stil welcome, of
course.)

We can't use <pre wrap>, because it is a non-standard HTML extension, which MSIE
doesn't support. Paragraphs are displayed as one long line. So, we indeed have
to use tt for flowed paragraphs.

Even flowed paragraphs can contain several spaces in a row, e.g. after a full
stop or as indention of the first line. Editor decided to allow that in new HTML
text, so we should do the same (for quoting, which was the issue. For display,
noone but us reads the source :) ). We should duplicate the editor strategy
which is, IIRC, to leave the forst space alone and turn all following (in a row)
into nbsp.

This strategy would also work for fixed lines. The only issue is that they would
always wrap. It also is somewhat readable, so I see no big advantage in changing
them to <pre>s.

So, all we would have to do for this bug is to
- change the space conversion strategy
- check (and fix if needed), if we insert to many real linebreaks (I think,
we're adding to extra ones) in order to make the source more readable.

Looks easy!

Comment 9

[Ben Bucksch (:BenB)]

Fixed this. REASSIGNing to myself. I'll attach the full source file mimetpfl.cpp
(includes fix for bug 31906, thus depends on latest patch for that bug).

It also fixes:
- It seems like we collapsed multiple spaces/tabs in a row in flowed paragraphs
(i.e. "  " displayed as " ").
- For fixed lines, the wrap pref wasn't honored, but hardcoded to "don't wrap".

The changes for flowed paragraphs are untested, as this mode is broken on Unix
(all lines are interpreted as fixed). Daniel, can you test and review, please?

Example output:
div class=text-flowed style="font-family: adobe-helvetica-iso8859-1; font-size
14px;"><tt>
<br></tt><blockquote type=cite><tt>&gt; Ah, the problem wasn't the font size bu
something much more
<br>&gt; fundamental. Your code don't honor the mail.fixed_width_messages
pref.<br>&gt; (That made your code display with a fixed font which was much
larger
<br>&gt; than my usual proportional width font). This is a MUSTFIX (obviously)
<br>&gt; but I haven't looked into how to fix it and why it don't work. I
think<br>&gt; the same problem is in both mimetpfl.cpp and mimetpla.cpp.
<br>
<br>WORKSFORME, I nearly all the time use variable width fonts. Dunno, why
<br>it doesn't work for you. Maybe the *_begin function(s) give you a clue.
<br>IIRC, in mimetpfl, it is queried directly and in mimetpla indirectly via
<br><code class=txt-verticalline><span
class=txt-tag>|</span>obj-&gt;p_variablewidth_font<span
class=txt-tag>|</span></code> or so.
<br></tt></blockquote><tt>
<br>I found the problem as you've might have seen in bug 39963. &nbsp;It's
<br> &nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;openingDiv += "
styl=\"";
<br> &nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;openingDiv +=
fontstle;
<br> &nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;openingDiv += '\"';
<br>
<br>that's referred to. The middle part is not working so no fontstyle is 
<br>added.
[...]

Comment 10

[Ben Bucksch (:BenB)]

Some chars in the example output have been truncated - don't worry.

Comment 11

[Ben Bucksch (:BenB)]

Attachment: mimetpfl.cpp

Comment 12

[Ben Bucksch (:BenB)]

Attachment: diff -u mimetpfl.cpp.31906 mimetpfl.cpp

Comment 13

[Ben Bucksch (:BenB)]

Fix checked in by rhp.

Comment 14

[Daniel Bratell]

Reopening, because the conversion SPACE ->   doesn't work for flowed 
paragraphs/lines in format=flowed mails.

Comment 15

[Daniel Bratell]

Taking the bug because I have a fix.

Comment 16

[Daniel Bratell]

The problem was that the nextSpaceIsNbsp variable was initialized _inside_ the 
loop (every iteration). Just moving two lines outside the loop fixes this. The 
fix is in the fix for bug 46173 (attachment 11763 [details] [diff] [review] 
<http://bugzilla.mozilla.org/showattachment.cgi?attach_id=11763>).

Rich, can you review this in one piece or should I break it in two?

Comment 17

[Ben Bucksch (:BenB)]

It is better to file a new bug for this. What you see is the exact
opposite of this bug, just a bug with the fix of this bug. Filed bug 46188.

It matters, because I want to keep track of which bugs I fixed via
bugzilla.

Comment 18

[Ben Bucksch (:BenB)]

taking bug back.

Comment 19

[Ben Bucksch (:BenB)]

reset to fixed.

sorry for the inconvience.

Comment 20

[esther]

changing qa