Plaintext compose, linux. Reply to a mail like this one: Mike Shaver wrote: > foobar Place your cursor to the left of the > on the 2nd line and hit backspace. It should do this: Mike Shaver wrote:> foobar instead, it moves the cursor and leaves the 2nd line there. I don't have any line wrapping on, and it happens for very narrow messages also. I think there are other instances of this bug when i am in mail compose, this is just an easy, reproduceable example.

I think the problem here is that when we do the reply-to-message procedure, we don't paste a "real" <enter> after the "blah wrote:".. I will investigate further. Ducarroz: is bug 21322 possibly in the same code and/or related?

this might be a bug for Joe...

no idea

snarf

test

test

I think the "plaintext" mail is using a pre block with a special nowrap attribute to do the quoting. So there is no way to join the lines as you would like, without changing the wrapping of the line that gets pulled up. I wish mail would just dump the quoted text straight in with hard wrapping, and let it soft wrap to a narrower window width like the rest of your composed message.

assigning to ducarroz for his comments.

reassign to varada

The editor is the one that brackets the quoted text in a <pre> (no special attribute) so that it won't wrap. Mail just calls InsertAsQuotation, as far as I know. That's because otherwise, quotes will all exhibit the long-short-long-short problem, e.g. > I wish mail would just dump the quoted text straight in with hard wrapping, and > let it soft wrap to a narrower window width like the rest of your composed message. It looks ugly and hard to read, and you can't tell visually what's quoted and what isn't. This seems like an editor bug, not a mailnews bug. I'm not clear why you'd want to do this, though, or what the expected result should be. Should the first quoted line be pulled out of the pre and joined to the line before it, but the rest of the quote remain in the pre? Should the line before become part of the pre? I'm sure a rule could be written to do something specific if we could figure out what that something ought to be. Chris? As the submitter, what do you think it should do?

the user cant see the pre. the user won't understand why part of their message doesn't soft wrap, and the rest does. we should nuke this. If the user wants to see the quote hard wrapped, they should make their window wide enough. otherwsie it's just a rats nest, and this bug is just one example. remember, this is "plaintext". There is no feedback to the user about html structure hidden away in the message. There is no reason for the user to think such structure is there. Everything will work much better if we just get rid of using html inside of plaintext mail.

Making the window wider won't help; we wrap to the wrap column we're going to use on output (72, by default). For me, letting quotes wrap as long-short-long-short would make plaintext mail unusable (I wouldn't like to edit messages that looked like that, and I wouldn't be willing to send messages that looked like that). And it was a continual source of bug reports and regressions in 4.x (which was not supposed to wrap quoted text, but now and then it regressed). I still get complaints from people outside who have this happen to them in 4.x. But I don't currently use our plaintext mailer for other reasons. Cc'ing some people who do, or who have expressed opinions on this in the past.

I recommend don't do anything fancy. 4.x did this. quoted message came in, it forgot what all the format was and when you added content it would just blindly wrap things. I usually don't wrap stuff when typing, it all worked out.

removing 0.9.1 since it was reassigned and adding nomination for reconsideration.

mcafee, I cant grok your comment. We are already doing something fnacy. I'm advocating doing nothing fancy. So the first line of your comment seems to be agreement. But the rest seems to be disagreement... am I missing something?

My very first test case still fails as I described. I find plain text compose to be just plain annoying for a few reasons, this is one of them. jfrancis, please call me and I'll clear up any questions you have. x6705 or 415.641.1667.

I agree this isn't ideal behavior but it seems that delete works fine in the rest of the message. Marking nsbeta1-.

I've been told I should reassign this back to the editor team. So I'm removing my previous markings and reassigning to jfrancis.

i cannot proceed on this without buyin from mail team. To fix this I either have to get rid of the seperate wrapping for plaintext mailquotes (a la 4.x, which I think is doing the right thing) or make some relaly fruity rules for pulling lines out of mail quotes which will be a nightmare.

4.x did NOT do long-short-long-short rewrapping, in most cases; that was considered a bug, and was fixed in every case but one, a complicated case involving mixing html and plaintext whose details weren't discovered until after the mail team had switched over to working on 5.0. (There's a bug filed on that case but AFAIK it's still open.) Please, let's not introduce such a huge regression in mozilla. If <pre> causes such problems, perhaps we could come up with some other way of wrapping quotes to keep them distinct from unquoted text (as long as we strive to keep the compose window wysiwyg so people know what they'll be sending).

The long-short wrapping i'm talking about is not "real". It's only due to soft wrapping if your window is narrower than the quote. It doesn't affect how your message gets delivered. It allows you to see everything in your window. It's really weird to get scroll bars in an edit view that is otherwsie window wrapped. And it prevents surprise when pulling up lines out of the quote. I don't see how this is a regression at all. I call it desireable.

The only HTML I know of that we add is <html>, a </html> and in between that a bunch of <br>'s. This is all hard-coded (as all other strings, yay) in nsMsgCompose.cpp

Joe: the wrapping that is done in the plaintext message compose window is to the user's line length (defaulting to 72). It has nothing to do with the window width; it's supposed to be a WYSIWYG preview of what will be sent, definitely including wrapping. (It's not 100% wysiwyg, because the wrapping in layout and the wrapping in the plaintext serializer, alas, don't share code; but it's pretty close.) What is sent comes from the plaintext serializer, which uses the tags in the document as a clue to find out what needs to be wrapped and what doesn't. The serializer wraps the new text to the wrap column setting; it doesn't wrap the quoted text because it sees that it's wrapped in a pre, and suspends wrapping for the duration of the pre. If you have a better way to mark quoted text as being different from unquoted, so that the serializer will know not to miswrap that portion of the message, please elaborate. I suppose we could build mailnews-specific smarts into the serializer to make it try to recognize lines starting with "> " if it's already inside a moz-pre style, and throw out our embryonic support for other types of quoting (like the AOL-style quoting that some users had asked for, which is currently supported but wouldn't be handled right by that algorithm).

I'm not proposing a better way to differentiate. I'm proposing we not differentiate. But I'm guessing that everyone will hate that. On my mac NS6.01 also shows plaintext mailquotes in a different font size from the rest of the message. Is that intended? To me it seems that plaintext should be as plain looking as possible. The more html'isms are in it the harder it is to do something reasonable without dragging in all the html editor code. So... what people want is for backspacing from inside the beginning of a block element to: 1) join the block to it's predecessor if they are compatible (like both list items in the same list) 2) delete what is before the block otherwise 3) unless... it's a mailquote, in which case the first "line" should be pulled out of the block and into the preceding content. Is that right? In my NS6 this could cause the line pulled out to both rewrap and to change font size. But that's what everyone wants, right?

The different font is a bug. It even used to be filed in bugzilla, I think (don't know the number or whether it's still open). I think it was basically an issue of no one having the time to unravel all the CSS files to figure out where that font was coming from, and fix it, and no one who actually understood the CSS being willing to admit to it. Curious as to what we do in similar situations in the html editor, I just tried creating some normal text, followed by a few lines of preformatted, and deleting backward from the beginning of the first preformatted line. It doesn't do anything intuitive either. The first time I hit backspace, nothing visible happens at all. If I hit backspace again, the caret jumps to the end of the last line of the non-preformatted text and deletes the character there. This doesn't seem any clearer than what Chris described for the plaintext quote case. No joining happens, the first nop keypres is confusing, and the caret position before the second keypress doesn't give a hint as to what's going to happen. Honestly, I'm not sure what we should do in cases like this, and I wish someone else would join in the discussion (Chris?) as far as what elements should be joined. It's fairly clear that what we're doing now, both in plaintext mail and in html compose, is unintuitive and could be improved; we just need to decide what it should be doing. Perhaps we should discuss it in the editor group.

well, I would like things to be joined as akkana says, I guess. I really don't know the internals here. I would even prefer a braindead solution: render the reply once, and then turn it over to me as just a plain text area, I'll do all the reformatting manually.

moving to 1.0, we should discuss this in detail before moving forward, we need to have a clear plan on to what should and should not get collapsed, etc.

Chris, I totally agree with your proposal: Basically, make plaintext mail really be plaintext. I already know that few others agree with that, though. :-( I'm working on changing deletion behavior to reorganize disimilar blocks. Once that's done you should be able to suck things out of mailquotes and into the earlier body text. Another benefit will be the ability to delte things "into" a preceding list item, which 4x does.

Beth, can I move this to 092? I think a number of folks would like to see this make that milestone.

Jfrancis, great to see some traction on this bug. It's a common complaint. Thanks!

Joe, if you have a solid plan on how to handle all of the differeent scenarios, cool. Let's talk

When you label an editor as plaintext, people expect it to behave like Notepad or something equivalent. The way it works now is definitely nonintuitive, and it drove me nuts until I found this bug and realized it was doing all this secret HTML behind the scenes. It's just trying to be too clever. The specific gripe I have now is related to this one regarding the behavior at the beginning and end of <pre> blocks; maybe it is a dup: If you receive a message with multiple lines, reply to it, go to the second or third line of the quote and hit Enter to separate the lines and insert a comment, the message you send will have more blank lines than what you see in the editor. For example: > line 1 this added comment has no whitespace above it > line 2 in the editor will get sent as > line 1 this added comment has no whitespace above it > line 2 Oh, and the number of times you hit Enter doesn't always correspond to the number of blank lines you get, either. fwiw, I vote it should render the HTML as plaintext (and do whatever line wrapping it's going to do) FIRST, before you start editing, and then act like a normal plaintext editor.

Maybe we can take another approach here, fixing some of the editing problems at the cost of losing some of our wysiwyg wrapping. The reason the quotes are wrapped in a pre is to keep them being wrapped as long-short-long-short, e.g. having the last word of each quoted line wrapped on its own line in the message that's sent. (That's the sign of a really brain-dead mailer, and affects the readability of almost every message containing quoted text, and I continue to maintain that we shouldn't do it.) If we stopped wrapping the quotes in a pre, and made the serializer (probably on setting a particular flag) recognize quoted lines and not apply wrapping to them, that would solve the editing problems without having to write special code in the editor. (The special serializer code would be very simple.) The cost would be that the quoted lines, as they appeared in the reply window, would in fact be wrapped long-short-long-short, because the layout engine wouldn't have this special knowledge that lines starting with ">" are quotes and should be treated specially. A user who sized his window slightly larger than 80 columns would never even see this; it would only be an annoyance to someone using a window size fairly close to 72 columns (i.e. 72 columns plus the max word length of the quoted text). I'm not sure what our default window size is, but I suspect it's quite a bit wider than that. Another issue is that original lines which start with > will also be subject to this treatment, and would not be wrapped. I could make the requisite changes to the serializer and to the editor's InsertAsPlaintextQuotation routines quickly (a day) if people think this might be worth trying. Joe wouldn't have to change anything in the edit rules. I don't know what the chances are of such a change getting approved at this late date. It's somewhat risky in that it probably won't get much testing; OTOH, it's not clear that plaintext mail has ever gotten much testing, so perhaps that isn't considered a problem. Note: this ONLY addresses issues with quoted plaintext, not the various other caret-in-plaintext issues that people have mentioned. Let's hear from the people who use plaintext every day. Does this sound like an improvement, is it worth the tradeoff, and if it works, are there people who care enough to test it and push it through the approval process?

I think I would appreciate any kind of fix here, akkana's suggestion sounds like it might workforme. I'd be happy to test this, and push through the approval process if possible. Is it worth a day of work, now, at this point in time? I see this bug every day and would love to get it fixed. I could probably live without the fix, I have done so for a long time already. We should probably land this on the trunk with a pref and make a case to bring it over on the branch. Adding chofmann.

here are my thoughts. I like the idea of landing on the trunk. I like the idea of being able to flip prefs to control between the current behavior and the altunative that might be developed so we could back out quickly if last minute stoppers are found. We need to get someone up to speed on exactly what Joe would do here so we could make adjustments in the 6.1 endgame when Joe is planned to be on sabatical. Lets think about all the places where this might go wrong, and develop a plan to test and reduce risk everywhere we can.

Okay, I'm taking this bug from jfrancis, and will implement what I suggested, controlled by a pref so that it can be quickly reverted if something goes wrong (and commented as temporary -- I don't think we want both behaviors to stick around forever, do we?)

Okay, I think I have it working now. It turned out to be more difficult than predicted: in InsertAsQuotation, mail compose requires that the editor return one node which spans the entire insertion, which it then uses to do its insert caret before/after/selected code. (This would probably be better if rewritten to use a Range rather than assuming that the insertion is all inside one node, but that would require someone familiar with mail compose.) So I rewrote the editor part to wrap the inserted plaintext quotation inside a span instead of the pre which is currently used, and I only turn off wrapping in the serializer when we're inside a span and the line starts with ">". This is all on a (temporary) pref, editor.quotesPreformatted. False (the default) will turn on the new behavior, true goes back to wrapping quotes in a pre. I have only done minimal testing on it. Adding the plaintext mail gurus Daniel and Ben to the cc list. I don't think this should cause any problems format=flowed, since it doesn't change any logic regarding when to append spaces or not, but it does change whether we go into the "smart wrapping" code, so if space-appending happens in the smart wrapping code and not in the other code path, this could cause quotes not to be flowed. But they should have gone through that code path anyway, due to being wrapped in a pre, so this shouldn't change anything.

manager reviewed the need for the fix and agrees, approval for checkin to the trunk and branch after fix has received an r= and sr=, adding nsBranch keyword

back to 9.3

Bug 67391 is a related bug.

Prospective testers: I haven't heard any comments. If anyone wants it on the branch, plaintext mail users need to test pronto. Otherwise, maybe we should remove the nsBranch keyword so as not to confuse things?

Bug 58960 is also related to this bug.

I have not tested the patch so I don't know if it works but I have a couple of questions from reading it. It doesn't handle nested spans, is that ok? The new code doesn't accept empty strings in nsPlainTextSerializer::Write. I don't think it ever gets an empty string, but it might be best to at least add an assertion. + else if (type == eHTMLTag_span && mInSpan) { + mInSpan = PR_FALSE; + } + Not necessary to test if mInSpan is true. It should be, and if it's not we're just setting it to the same value it already has.

the patch as akkana attached it works fine for me so far, current linux trunk build.

Re Daniel's comments; Right, it doesn't handle nested spans. I thought about adding that, but in plaintext it should never happen. Perhaps I should change it to a count, though, mSpanLevel or something, so that it can't cause problems in converting from real html in the rare case where the user might have hand-added nested spans. I'll do that -- shouldn't hurt performance any and it makes it a little safer. I'll also add the suggested null check.

Nobody's been pushing for this to be in the branch; changing milestone accordingly. Do we want to try it on the trunk? If anyone wants it, please test and review the latest patch.

I applied the patch(id=41642) and added pref("editor.quotesPreformatted", false) in modules/libpref/src/init/editor.js. bug 67391 and bug 58960 seems to be fixed by the patch. I hope it be reviewed and checked in as soon as possible!

I'd love to do some testing on this patch, as it is one of my Most Hated problems in Compose. Will add results later today, if I get around doing it.

The fix for this bug affects bug 86476, which already has a patch in bug 83918. *********** 123 456 *********** If you copy the above two lines and "paste as quotation", the result is: *********** >+ > 123 > 456 > *********** '+' is the caret position.

r=mcafee on latest patch. I have only tested the old patch, but that worked fine for me. ping me if you need more testing, I'd say let's get this into the trunk and test it there.

Shaver, Simon, Kin, could we get a super-review on this?

Whoops, the patch still shows some debugging printfs that I had added to nsHTMLDataTransfer.cpp and nsEditorUtils.cpp. They've been removed in my version and would not be checked in. I can attach another patch if anyone wants to verify that.

Comments: + && Substring(aString, 0, 1) == NS_LITERAL_STRING(">"))) { This is a very expensive way to test one character in a string (it makes a temporary). Can you use [] or something instead? + NS_WITH_SERVICE(nsIPref, prefs, kPrefServiceCID, &rv); Current vogue is nsCOMPtr<nsIPref> = doGetService(kPRefServiceCID, &rv); Otherwise, sr=sfraser

I would love to use [] instead of Substring, but it isn't defined on the current string API. I spent a long time in the (already obsolete) string API docs, and Substring was the best way I found to check the first character of a string. Checking with scc now ...

Scott says to use: aString.First() == PRUnichar('>') (after checking for non-nullness, which we're already doing by checking length), so I've done so. Re prefs: I've replaced the NS_WITH_SERVICE line with: nsCOMPtr<nsIPref> prefs = do_GetService(kPrefServiceCID, &rv);I really wish the xpcom folks would make up their mind about blessed syntax (NS_WITH_SERVICE was supposed to be the permanent syntax we were all supposed to use instead of the previous syntax).

Okay, the fix is in the trunk. Go play! I'm going to keep this bug open because I'm not really happy with this fix, but I had a brainstorm -- if this works to fix the caret problems, we can probably get around the non-WYSIWYGness this introduces by putting a moz-pre style node on the <span> tag. I'll try that and see if it helps, and meanwhile, please post reports here on whether things work better now.

Now (win32 2001071604), if I reply to a message, delete part of the quoted text, and add some response text under the quoted text, the caret tends to jump up a line when I start typing. Or if the caret is on a blank line (again, under the quoted text) and I hit backspace to get rid of the extra line, the caret goes up two lines instead of just one.

I can't reproduce it reliably, but I happened across a set of keystrokes that makes all the text in my compose window disappear! That is, it behaves like the text is still there if you cursor around, but you can't see it. As if it's white on white even if you try to select it. I couldn't get it to come back, I had to kill the window.

I can reproduce what Eric commented when replying. Sometimes when replying, I remove a few >'s (I always reply _below_ the msg), and sometimes also the signature before writing. If I do that, and then start writing, the cursor behaves strange. One time, the caret looked like I wrote below the quoted text, but when I sent, the reply ended up inside the quoted text (so it looked like my reply was part of the previous message!) Hope this helps.

Recent builds wrap quoted text according to "Wrap plain text messages at" preference. Maybe the cause is the patch for this bug.

Closing this because bug 92331 has been opened on the current problem of the display wrapping. If there are other problems, please file them separately.

Really closing this time.

I've opened bug 93536 regarding the problem I described on July 16. Never did reproduce the disappearing text, though. :)

Marking verified based on the last few comments, remaining issue was logged separately.