134439 - Plain text compose window should be non wysiwyg by default

Status: RESOLVED WONTFIX

(MailNews Core :: Composition, defect)

Description

[Dimitrios]

I am filing this bug per Akkana's comment in
http://bugzilla.mozilla.org/show_bug.cgi?id=83378#c45
Since bug 83378 seems hard to fix in 1.0 timeframe and I think this milestone
should not be released with such an issue unresolved. I for one prefer the
simple approach of other mail clients (including NS4) than the complex, yet
buggy current solution.

Comment 1

[Dimitrios]

Re-assign bug to Akkana, as she already requested.

Comment 2

[Akkana Peck]

Cc some people who care about plaintext mail or the plaintext serializer.

Folks, here's the issue: as we all know, plaintext mail compose has a problem
where there's a tendency for the caret to get stuck inside the quoted text,
meaning that anything the user types won't wrap normally (bug 83378).

The reason for this is that we make quotes immune from wrapping, since they're
typically just slightly longer than our wrap column (since they have the "> "
added), so if they were wrapped to our normal wrap column, they'd end up with
the last word of each quoted line on a line by itself.

The way 4.x used to deal with this is that plaintext mail compose was not
wysiwyg: text wrapped to the size of the compose window, not the output wrap
column, and most users sized their compose windows (or used the default size)
considerably wider than 80 columns, so quoted text didn't appear wrapped and
there was no indication of where the message would actually be wrapped at send time.

It doesn't look like bug 83378 has much chance of being fixed any time soon, and
I suggested in that bug that one option might be to make a plaintext mail
compose mode that is non-wysiwyg, like 4.x used to be: all text wraps to the
window width during compose, and at send time, the non-quoted text is wrapped to
the normal wrap column (default 72).  I suggest that this be on a pref, so that
we can go back to wysiwyg mode by default if bug 83378 ever gets fixed (and
users who prefer it, if any, can still use it).

Two questions:
1. I've marked this moz 1.0.1, but I don't expect it to be difficult.  If
there's a chorus of "Yes, we want this and I'm willing to talk drivers into
putting it in 1.0!", I can do it in that timeframe.  OTOH, if mozilla folks feel
that this is the wrong approach and should be futured, I'd like to know that too.
2. Should non-wysiwyg be the default?  (I know the submitter thinks so and put
it in the bug title; I'm soliciting other opinions.)

Comment 3

[Jonas Jørgensen]

One strong argument for turning off the so-called wysiwyg wrapping is that since
Mozilla uses format=flowed rather than hard line breaks, it is actually not
wysiwyg at all.

Comment 4

[Brendan Eich [:brendan]]

Let's be 4xp and better; as Jonas says, we're not wysiwyg now anyway.  Most of
us have never been bothered by 4.x's behavior (did 3.x do something different
still from 4.x and Mozilla?).  Pre-approving for mozilla1.0.

/be

Comment 5

[Jesse Glick]

Comment as an end-user: I guess I did notice the long-short wrapping problem in
NS 4, but at worst it was a minor annoyance. When using NS 4 I can write email
without paying attention to the tool. If the result when sent occasionally looks
funny, oh well.

In the current Mozilla mail client, *every time* I write a message involving
quoting I get bit by #83378 and have to use various tricks and undo some edits
to try to get it to wrap where I want and not do bizarre things to my message. I
believe you if you say that the behavior is now "correct", but it is not at all
what I expect it to do - violating the basic rule of UI. Even knowing about
details of the issue, I still find the wrapping behavior frustrating and pull my
hair out over it.

Please make the NS 4 style an option, at least until the new system has gone
through a much more thorough usability check. It would be a relief. I don't so
much care if it is the default or not, so long as the option is easily discoverable.

Comment 6

[Akkana Peck]

Attachment: Make it so

I think this is all that's needed.  This does the following:

- Add a new pref, "mail.compose.wrap_to_window_width" (would "mailnews.wrap..."
be better?  I couldn't figure out a consistency between when mailnews. is used
vs. mail.compose.)  True by default for all platforms.
- Save the value of this pref in the plaintext editor, and also save the wrap
column (previously we didn't need to save it because the information was stored
in the editor document's body node style).
- Change GetWrapWidth to return the value of the variable instead of getting
the info from the body node.
- Change SetWrapColumn: always save the argument in mWrapColumn, but set style
to wrap to body width if we're a mail editor and mWrapToWindow is set. 
- In nsHTMLEditor::InsertAsPlaintextQuotation, don't wrap in the pre or span if
mWrapToWindow is set; instead, fall back on the inherited
nsPlaintextEditor::InsertAsQuotation.
- Fix a warning while I was there because they drive me crazy.

Seeking review (probably from jfrancis) and additional testing (from anyone who
uses plaintext mail and cares about the issue).

Comment 7

[Akkana Peck]

Whoops, we'll need one more thing: a change to the plaintext serializer, to make
it smarter about recognizing quoted text and not wrapping it.  With only the
current patch, we miswrap quotes at send time.

Comment 8

[selmer (gone)]

Please update this bug with an [adt1] - [adt3] impact rating (or take it off the
list if it doesn't even rate adt3.)  Thanks!

Comment 9

[Akkana Peck]

Attachment: Patch including serializer change

Here's the same patch as before with the addition of the serializer change.  I
extended the test for whether we're in a quote (and therefore don't do
"intelligent wrapping") to include any line starting with > if the new pref is
set.

Comment 10

[Joe Francis]

Comment on attachment 77556 [details] [diff] [review]
Patch including serializer change

I've thought about the editor changes and how they will impact mail.  I think
they should be ok.  I looked at the serializer changes too, but I dont know
that code very well.

Comment 11

[Joe Francis]

5 minutes after r'ing this I thought of an issue needing investigation.  Akkana 
is investigating and will report back.  Issue in a nutshell: ">" anywhere in 
message could conceivably kill wrapping.

Comment 12

[Akkana Peck]

Here are the results of the investigating so far:

First, if in the plaintext compose window you type return, followed by a >, and
then continue your typing, the serializer will see that as a quoted line and
will not wrap that line.  So if you hit return, greater than, and then go on
typing a whole paragraph, that paragraph will be sent as typed (unwrapped). 
That's expected behavior and I wouldn't think users would be likely to do that;
but if they are likely, then perhaps we should consider a different treatment
for quoted lines, like wrap at wrapcol+8 or something like that.  (That might
not be a bad idea anyway, but it would involve some tricky surgery in the
serializer and is therefore higher risk.  I'll file a separate bug.)

Second, the serializer looks for > as the first character in a text node, but
doesn't actually check whether it's at the beginning of a line, so it is
possible for a line that should be wrapped to be mistakenly thought a quote, and
not wrapped.  We tried lots of different ways to make this happen in the editor
window (preceeding > by two spaces so it would be an nbsp, or by & so it would
be an amp entity) without breaking things, but Joe was able to come up with a
scenario that broke the algorithm: make a line that's just barely long enough to
wrap, end it with a space then hit return and type for several more lines with
no more returns.  Now copy the string "& foo" (no quotes) from somewhere.  Put
the cursor at the end of the line with the space and drag to the beginning of
the next line (thus selecting only the br, nothing else).  Now paste the > foo
that you copied before.  When you send this, the line isn't wrapped at > as it
should be.

The problem here is that the code that tests whether the first character of the
text node is > doesn't also test to make sure we're at the beginning of a line.
 The solution is to add a check of mAtFirstColumn into that if statement in
nsPlainTextSerializer::Write.

Patch coming, along with one other minor change Joe suggested, guard against
SetWrapWidth setting the style unless the editor flags say we're plaintext;
we're both convinced that's not happening now since it would be very noticable
to the user, but it doesn't hurt to guard against it.

Comment 13

[Akkana Peck]

Attachment: Patch with changes from discussion with Joe

It turns out that mAtFirstColumn isn't trustworthy -- it can be true even when
we're already 72 lines into the string.  So I'm doing what I mentioned in the
last comment, but using mCurrentLine.IsEmpty() as a more reliable indicator of
column position.  With this patch, Joe's edge case does the right thing and
wraps the line with the > foo.	It also puts a space before the > at the
beginning of a line, which it also did in some of the other test cases I saw
where > was not a valid quote character.

Comment 14

[Akkana Peck]

Cc Daniel (sorry, I should have cc'ed you before this), the expert on the
wrapping of quoted text in the plaintext serializer.  Daniel, if you have time
to look at this and say whether you think it's safe, or whether there's a better
way to do the serializer portion, I'd appreciate it. 

Tanu, Daniel or anyone else: any idea whether it's correct, or a bug, that
mAtFirstColumn was false in the test case I described, even though mCurrentLine
contained 72 characters?  The patch seems to work fine checking
mCurrentLine.IsEmpty(), so mostly I'm just curious why mAtFirstColumn didn't
work for that purpose (when I tried to use it, I got into into "no intelligent
wrapping" for the "> foo" in the middle of the line, then hit the assert about
mixed wrapping and nonwrapping data).

Comment 15

[Joe Francis]

Comment on attachment 77759 [details] [diff] [review]
Patch with changes from discussion with Joe

r=jfrancis

Comment 16

[Daniel Bratell]

mAtFirstColumn descibes the output, not the input. So mAtFirstColumn means that
the next character output would end up at the first column, i.e. we need to
insert indentation and stuff in front of that character.

Akkana, can you add some comments describing the new member variables as Tamu
did with his/her last additions. The number of knobs and throttles in the
nsPlainTextSerializer is running little out of hand.

Also, can you look at bug 119850. You're touching the exact line that causes
that bug so we should at least consider it. I was going to remove the ">" check
completely because I didn't understand the use for it, but after this it might
make more sense. 

I wonder if it would be more reliable checking the mEmptyLines variable instead
of if mCurrentLine.isEmpty(). I think we sometimes might go into the wrong
codepath with the current check. mEmptyLines will be 0 or greater if we are on a
new line, -1 if we are on the middle of a line. It is not maintained on the
"unintelligent" code path though, so it will only work if the rest of the text
goes through the "intelligent" code path.

I'm a little sorry that more text will enter the "unintelligent, just output
whatever comes in" code path. It was not designed to be mixed with the other
code path and it is much more limited. My long term (long term because I have so
little time) plan was to move that little functionality that was there to the
other code path so that the number of duplicated code was limited.

Comment 17

[Akkana Peck]

Attachment: Check mEmptyLines instead of mCurrentLine.isEmpty(), as suggested by Daniel

Thanks for the suggestions, Daniel.  I've added comments describing the quote
variables in nsPlainTextSerializer, and changed the if to check mEmptyLines
instead.  It seems to work fine -- handles Joe's test case and doesn't hit the
mixed wrapping/nonwrapping assert.

Regarding bug 119850: the cause may be that we're checking the level of any
span, not just quote spans.  The editor does mark the spans, with
_moz_quote=true, so the serializer could check that and only treat those spans
specially.  I'll make a patch and attach it to that bug.

> mAtFirstColumn descibes the output, not the input.

Output was what I'm trying to test there -- aren't mCurrentLine and mEmptyLines
also measuring output?	So I still think mAtFirstColumn should have done it,
but (mEmptyLines > 0) should be fine.

Comment 18

[Daniel Bratell]

Comment on attachment 77895 [details] [diff] [review]
Check mEmptyLines instead of mCurrentLine.isEmpty(), as suggested by Daniel

r=bratell for the nsPlainTextSerializer and you've already got r for the rest.

mAtFirstColumn wont work because it will be set whenever a new line is started
in wrapping. So if I have the text. "I'm sure that x > 3!" and it wraps to:

I'm sure that x
> 3!

Then mAtFirstColumn will be set when encountering the '>'. mEmptyLines on the
other hand only reacts to explicit line breaks.

Comment 19

[Akkana Peck]

Thanks, Daniel!

Any super-reviewers interested in this?  Shaver?  Brendan?  (Blizzard
says he
doesn't have time.)  How about testers -- do any of the people who have been
asking for this want to test it out?

Comment 20

[Dimitrios]

Finally I managed to compile Mozilla at home, so I'm willing to test your patch
(if first I find how to apply the patch to the Win32 Mozilla source :-) ).

Comment 21

[Dimitrios]

After building Mozilla with the patch, I noticed the following:
1. I get occasional "unreferenced memory" crashes. This error might be my fault,
although I used a tarball from 6 April.
2. PLAIN TEXT REPLY: When I type something in the mailquote (without hitting
Return), the line is wrapped at the window boundary, as expected. However, the
message sent leaves the text in one line, regardless of the wrap setting (does
not insert hard return). From what I understand from the discussion above, this
is intended. At least it gives the best optical result on receive, when "Wrap
text to fit window width" is checked (all quoted lines remain quoted). 
However, if "Wrap text to fit window width" is not checked, the result is rather
ugly (a long unwrapped line that cause horizontal scrollbar to appear).
3. HTML REPLY: If I type in the mailquote and I exceed the wrap preference, the
line is always wrapped with hard return on sending. Quoting is correctly
preserved, so everything is fine.

So, it seems to exist an inconsistency between html and plain text reply, the
first always hard wraps long mailquotes while the latter doesn't. FYI, Outlook
Express always inserts hard return on sent message, when line width exceeds the
limit set in it's sending preferences. This is true even for mailquotes, for
both html and plain text reply.

Comment 22

[Timo Pietilä]

Just want to say that this is *NEEDED*. This is the only reason I don't use 
Mozilla. And before bug 83378 or this is resolved I won't use mozilla (or 
netscape). This is most gruesome "non-critical" bug I have ever seen. IMO even 
crashes are more acceptable (unless they involve crash of OS too).

Comment 23

[Akkana Peck]

Still looking for an sr.  Cc'ing alecf -- Alec, could you please look at this or
suggest a better victim?  Thanks!

Comment 24

[Alec Flett]

Comment on attachment 77895 [details] [diff] [review]
Check mEmptyLines instead of mCurrentLine.isEmpty(), as suggested by Daniel

overall this looks good.. I don't know this code well enough to know if we're
making sure to only follow this pref in the mail case...so assuming we do
(mostly worried about the places where we call
GetBoolPref("mail.compose.wrap_to_window_width"..)

sr=alecf

Comment 25

[Akkana Peck]

It's in on the trunk.  Please test it and report any problems!

Leaving open for now until I figure out the appropriate keywords and such so
that it still gets branch consideration.

Comment 26

[Akkana Peck]

One side effect of this is that the output sink no longer does space stuffing
before >, because real quotes come through as > at the beginning of a line.

Daniel, is this a problem?  I've put an   before the > in the simplemail
output test to fix the output tests for now.

Comment 27

[Akkana Peck]

Another problem: there's been a regression since we were all testing it last,
and now, the space between the > and the first nonspace character is lost (so
you get quotes like
>This is quoted text
)  Blech, bitrot!  I'm sure no one would like that regression, so I've turned
the pref off for now.  You can still test the behavior with: 
user_pref("mail.compose.wrap_to_window_width",  true);

I'm looking into what's going on and will attach a new patch to fix that behavior.

Comment 28

[Daniel Bratell]

Losing the space stuffing in f=f mails would be quite serious. That would make
the quote detection really broken so if that's the case and we haven't got a fix
right now we better back this out before too many people start filing bugs. 

Comment 29

[Akkana Peck]

The pref is off, so only people who explicitly want to test it will get it.  The
editor part is in and probably won't change much (and people can test it, but be
aware of the serializer problems), so we just need to make a plan for the
serializer.

On space stuffing of '>':  a major part of this modification was that quotes
aren't wrapped with anything special, because wrapping them in any html tag
causes bad composer behavior.  This means that > at the beginning of a line is
the only sign that we're in a quote, so we can't space-stuff > at the beginning
of a line or we'll lose all of our quotes (they'll start with " > " instead of "> ".

I'm unclear why this is a problem for f=f -- we won't be reflowing quoted text
anywy, will we?  I suppose we could make it not happen when the flowed pref is
on, but that would mean few users would actually get the benefit and most would
be stuck with the current problems.

Here's a new suggestion: Joe, would composer behave okay if we wrapped the quote
in a span rather than a div?  Would this get around all the bad behavior we were
seeing with pre and div?

If we can go with the div solution, that makes the code quite simple since it's
not very different from what we were doing before with pre and div.  (We have
too many different quote options, though; for the next iteration, how about we
pick two and get rid of all the rest?)

Further coding (like finding out why this regressed in the last week) is on hold
until we come up with a plan.

Comment 30

[Daniel Bratell]

The space stuffing is there so that a f=f decoder doesn't confuse a line
beginning with a > with a line that's really, without ambiguity a quote. If we
don't space stuff we're in clear violation of the spec.

The problem here is that we have moved away from the "intelligent" code path
which does such things as space stuffing. There's other things also done on that
code path that we probably need. I wonder how hard it would be to get everything
going through that code path and instead of a big switch at the entrance, having
small switches around white space compression, wrapping, space stuffing...

Comment 31

[Akkana Peck]

I've been playing around with the idea of sending quoted text through the
intelligent wrapping fork, but have the "bonuswidth" in AddToLine be larger
(perhaps as large as 15 or so) when we're in quoted text.  That way, completely
unwrapped quotes (500-char lines) would still be wrapped, but quoted lines that
were just a little too long because of the indent chars or because the sender
used a slightly longer wrap width would be passed through.

But I don't think we can do this safely in the "quotes aren't special, they're
just lines that happen to start with >" case; I think we need to wrap in
something, at least a span, so we need Joe to tell us whether the edit rules can
handle that.

Comment 32

[Boris 'pi' Piwinger]

I strongly oppose non-WYSIWYG plain text editor. The big problem you see quite
often with Netscape 4 users is that the mean to break there seemingly too long
lines by hand and then Netscape does another line break -- ending in a sawtooth
situation.

All those effects happen because of the fact that people don't see what it will
look like once it is mailed or posted.

In particular, people will try to use (and I think this is legitimate) quote
symbols to make long lines not wrap. Without WYSIWYG the get the impression it
works, but the result will be horrible.

And so far Mozilla does a pretty good job at WYSIWYG. Don't fix what isn't
broken, please!

pi

Comment 33

[Vidar Haarr (not reading bugmail)]

I /fully/ agree with Boris. If implemented, please make the default option
'WYSIWYG'.

Comment 34

[Jonas Jørgensen]

> If implemented, please make the default option 'WYSIWYG'.

Mozilla uses format=flowed for sending, i.e., soft line breaks rather than hard
ones. So the current behavior is not WYSIWYG.

Comment 35

[Akkana Peck]

BTW, my comments regarding <span> instead of <div> were delerium from working
too long that day.  Or something.  We already use a span, not a div, and
obviously we still have edit rules problems.

So if we implement this non-wysisyg code, it will be at the cost of
space-stuffing lines beginning with > -- in other words, all lines beginning
with > will be taken to be quoted lines and wrapped (or not) accordingly, and >
will need to be removed from the list of characters which cause space stuffing.

Obviously people feel strongly about both sides of this and there's no good
solution.  Boris is quite right about the drawback of non-wysiwyg; I used to see
that a lot, and still see it fairly often from people who compose a message in
some other app then paste it into a mail window.

Comment 36

[Boris 'pi' Piwinger]

Regarding comment 34: Even though Mozilla uses by default format flawed, itself
it displays the lines as sent. Rewrapping using f=f might be at hand, but is not
used. So actually we do have WYSIWYG. Of course, other readers might implement
this differently, but we don't. And don't forget those readers which do not use
f=f at all, the also see what we see before sending.

Please: WONTFIX

pi

Comment 37

[Jonas Jørgensen]

> Even though Mozilla uses by default format flawed, itself it displays the
> lines as sent. Rewrapping using f=f might be at hand, but is not used.

Huh? You wouldn't by any chance happen to have
user_pref("mailnews.display.disable_format_flowed_support", true); in your
prefs.js, would you?

Comment 38

[Akkana Peck]

Not hearing any suggestions from the folks who wanted this ... sounds like
people have lost interest.  Bumping this off.

Comment 39

[Dimitrios]

I'm still interested in this bug but that doesn't count much (and I don't
understand much of it's technicalities). But the real objective of this bug was
to find a workaround for bug 83378, in Mozilla 1.0 timeframe.

Comment 40

[Akkana Peck]

I'm going to close this -- sounds like we have irreconcilable differences
between this bug and format=flowed.  If someone has a suggestion how to resolve
the differences, we can reopen it.