Closed Bug 69638 Opened 23 years ago Closed 23 years ago

Caret/delete does not behave right in plaintext compose

Categories

(MailNews Core :: Composition, defect, P3)

defect

Tracking

(Not tracked)

VERIFIED FIXED
mozilla1.0

People

(Reporter: mcafee, Assigned: akkzilla)

Details

(Whiteboard: [plaintext] Partial fix is in, better fix on the way)

Attachments

(2 files)

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?
OS: Linux → All
this might be a bug for Joe...
no idea
snarf
Assignee: ducarroz → jfrancis
Priority: -- → P3
Target Milestone: --- → mozilla0.9.1
test
test
Status: NEW → ASSIGNED
Summary: Cursor/delete does not behave right in plaintext compose → Caret/delete does not behave right in plaintext compose
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.
Assignee: jfrancis → ducarroz
Status: ASSIGNED → NEW
reassign to varada
Assignee: ducarroz → 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.
Keywords: nsbeta1
Target Milestone: mozilla0.9.1 → ---
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-.
Keywords: nsbeta1nsbeta1-
Target Milestone: --- → Future
I've been told I should reassign this back to the editor team.  So I'm removing
my previous markings and reassigning to jfrancis.
Assignee: varada → jfrancis
Keywords: nsbeta1-nsbeta1
Target Milestone: Future → ---
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.
Target Milestone: --- → mozilla1.0
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.
Status: NEW → ASSIGNED
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 
Keywords: correctness
Whiteboard: [plaintext]
Target Milestone: mozilla1.0 → mozilla0.9.2
Target Milestone: mozilla0.9.2 → mozilla0.9.3
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.
Target Milestone: mozilla0.9.3 → mozilla1.0
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?)
Assignee: jfrancis → akkana
Status: ASSIGNED → NEW
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.
Status: NEW → ASSIGNED
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
Keywords: nsBranch
back to 9.3
Target Milestone: mozilla1.0 → mozilla0.9.3
Bug 67391 is a related bug.
Whiteboard: [plaintext] → [plaintext] FIX IN HAND, NEEDS TESTING
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.
Keywords: nsBranch
Target Milestone: mozilla0.9.3 → mozilla1.0
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?
Whiteboard: [plaintext] FIX IN HAND, NEEDS TESTING → [plaintext] FIX IN HAND, NEEDS SR
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).
Whiteboard: [plaintext] FIX IN HAND, NEEDS SR → [plaintext] FIX IN HAND
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.
Whiteboard: [plaintext] FIX IN HAND → [plaintext] Partial fix is in, better fix on the way
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.
Hardware: PC → All
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.
Status: ASSIGNED → RESOLVED
Closed: 23 years ago
Resolution: --- → FIXED
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.
Status: RESOLVED → VERIFIED
Product: MailNews → Core
Product: Core → MailNews Core
You need to log in before you can comment on or make changes to this bug.