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.

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

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.)

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.

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 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.

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).

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.

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

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 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.

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.

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.

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.

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 on attachment 77759 [details] [diff] [review] Patch with changes from discussion with Joe r=jfrancis

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.

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 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.

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?

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 :-) ).

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.

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).

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

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

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.

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.

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.

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.

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.

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...

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.

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

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

> 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.

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.

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

> 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?

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

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.

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.