554806 - html editor deletes lines below return on certain emails composed in outlook 2003 [Affects TB 3.1 HTML composition]

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[George Hyman]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2.3pre) Gecko/20100324 Namoroka/3.6.3pre GTB6
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2.2pre) Gecko/20100324 Lanikai/3.1b2pre

If I do a  reply to an email written using outlook 200.  The attached email is an example.  To reproduce: reply to this email, position the cursor somewhere in the middle of the email at the extreme left, hit return to create a new line to start replying.

The rest of the email (i.e. below the return) is deleted.  This worked fine in TB 3.0.

Reproducible: Always

Steps to Reproduce:
1. open attached email file
2. position cursor at the left side at some convenient place in the email
3. hit return to open a new line
Actual Results:  
The text below the return disappears, should simply open a new line to allow writing the reply. 

Expected Results:  
Open a new line to allow adding comments/reply

Comment 1

[George Hyman]

Attachment: email that demonstrates the problem

Comment 2

[Joe Sabash [:JoeS1]]

Yes, I can confirm this.
We will need to confirm the regression, and establish a regression range to get any traction here.
The quoted printable makes it a bit harder to diagnose, but we really must start dealing with e-mail "as-it-is" in today's reality.
Tested in:
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2.2pre) Gecko/20100324 Lanikai/3.1b2pre ID:20100324032126

Comment 3

[Joe Sabash [:JoeS1]]

This looks to be a showstopper to me.
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1.9) Gecko/20100324 Shredder/3.0.5pre ID:20100324001346 works fine
Fails in:
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2.2pre) Gecko/20100324 Lanikai/3.1b2pre ID:20100324032126

That's pretty much a Gecko Core bug indication to me.
Tested in safe-mode also.

Replying to an html email by hitting enter within a blockquote is very much the norm IMO This basic functionality seems to be badly broken.

Comment 4

[Joe Sabash [:JoeS1]]

Request blocking

Comment 5

[Makoto Kato [:m_kato]]

George and Joe,  How about on 3.2a1pre?

1.9.2 tree has some problem to edit HTML.

Comment 6

[George Hyman]

Same behavior on Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.3a4pre) Gecko/20100324 Shredder/3.2a1pre.

Comment 7

[George Hyman]

A nit, correcting my typo - the mail in question came from outlook 2003 - last digit was lost.  Obviously the headers in the mail could tell you that as well :)

Comment 8

[Dan Mosedale (:dmosedale, :dmose)]

George, do you have any sort of feel of whether this happens on just this particular message, or some or most or all HTML messages from Outlook 2003?

Also, would you be willing to help us narrow down the regression range, as suggested by Joe?  Details on how to do that are at: http://quality.mozilla.org/documents-home/bugs-docs/bug-triaging-guidelines/finding-regression-windows

Comment 9

[George Hyman]

(In reply to comment #8)
> George, do you have any sort of feel of whether this happens on just this
> particular message, or some or most or all HTML messages from Outlook 2003?
> 
> Also, would you be willing to help us narrow down the regression range, as
> suggested by Joe?  Details on how to do that are at:
> http://quality.mozilla.org/documents-home/bugs-docs/bug-triaging-guidelines/finding-regression-windows

I don't have a good feel for whether it's all Outlook 2003 emails or just some, I do know that outlook 2007 emails don't seem to show the problem.  

I will work on the regression range, but it will be at least this evening or tomorrow (Friday 3/26) due to commitments to my "day job"

Comment 10

[Dan Mosedale (:dmosedale, :dmose)]

Thanks!

Comment 11

[George Hyman]

results so far:

Good (i.e. works OK): Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1.8) Gecko/20100221 Shredder/3.0.3pre -result:  ok  - from directory 2010-02-21-00-comm-1.9.1

Bad: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2.2pre) Gecko/20100221 Lanikai/3.1b1pre result: fail - from directory: /2010-02-21-03-
comm-1.9.2

These are the same day, the only difference is the 1.9.1 vs 1.9.2 

These are the earliest builds I could find at http://ftp.mozilla.org/pub/mozilla.org/thunderbird/nightly/  is there someplace else to look?

Comment 12

[Joe Sabash [:JoeS1]]

look at the year folders
http://ftp.mozilla.org/pub/mozilla.org/thunderbird/nightly/2010/

then back into 2009 if necessary, don't bother with build other than
comm-1.9.2

Sorry I can't give you a good starting point, maybe back up a month as a start.

Comment 13

[George Hyman]

OK, back to the 2009 folders.
The very first 1.9.2 - from 2009/12/2009-12-31-03-comm-1.9.2
version id: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2b6pre) Gecko/20100101 Lanikai/3.1a1pre 
Fails.

The 1.9.1 from the same day works fine. Version: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1.7) Gecko/20091231 Shredder/3.0.1pre 

It's now pretty clear (at least to me) that the problem was introduced in the creation of 1.9.2

Comment 14

[Joe Sabash [:JoeS1]]

A little confusion factor is that 3.1 switched Gecko versions in Dec
So 3.1 archives are called comm-central-trunk and used Gecko 1.9.3a1

So the regression range is:
OK in 20091115 nightly
Fails in:
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.3a1pre) Gecko/20091116 Shredder/3.1a1pre

Comment 15

[George Hyman]

(In reply to comment #14)
> A little confusion factor is that 3.1 switched Gecko versions in Dec
> So 3.1 archives are called comm-central-trunk and used Gecko 1.9.3a1
> 
> So the regression range is:
> OK in 20091115 nightly
> Fails in:
> Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.3a1pre) Gecko/20091116
> Shredder/3.1a1pre
Is this a narrow enough regression range, or is there more testing you need me to do?

Comment 16

[Ludovic Hirlimann [:Usul]]

http://hg.mozilla.org/comm-central/pushloghtml?startdate=2009-11-15&enddate=2009-11-16 

Is calendar only.

http://hg.mozilla.org/mozilla-central/pushloghtml?startdate=2009-11-15&enddate=2009-11-16

Nothing rings a bell. Dmose ?

Comment 17

[Joe Sabash [:JoeS1]]

Attempting to get a minimal test case for this.
This might be dependent on the problem mail being Content-Transfer-Encoding: quoted-printable if so it's tough to triage TB can't compose QP)

So I discovered a workaround (and maybe a clue)
Steps:
Open the test eml and hit reply (html mode)
Don't make any entries in reply
File>>Send later (message stored in outbox and QP stripped)
Open the saved message (from outbox) and reply.

Blockquote break on enter key now works normally.

I can't see a difference in the outbox saved message, except for the absence of quoted printable.

Comment 18

[George Hyman]

this WAR doesn't work, at least for me on Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2.2pre) Gecko/20100326 Lanikai/3.1b2pre - I get exactly the same behavior as before. I'm opening the email from my mailbox, not the example file, don't know if that matter.

Comment 19

[Joe Sabash [:JoeS1]]

(In reply to comment #18)
> this WAR doesn't work, at least for me on Mozilla/5.0 (Windows; U; Windows NT
> 5.1; en-US; rv:1.9.2.2pre) Gecko/20100326 Lanikai/3.1b2pre - I get exactly the
> same behavior as before. I'm opening the email from my mailbox, not the example
> file, don't know if that matter.

You are right, it only works on the saved eml file.

I hate the eml file concept...too many variables.

Comment 20

[Nomis101]

I can reproduce the STR from the initial post on Mac OS X (10.6), so I changed platform to All.
But I only can reproduce this with the attached eml file. I can't reproduce this with emails from "Microsoft Office Outlook 11" from my Inbox.

Comment 21

[George Hyman]

(In reply to comment #20)
> I can reproduce the STR from the initial post on Mac OS X (10.6), so I changed
> platform to All.
> But I only can reproduce this with the attached eml file. I can't reproduce
> this with emails from "Microsoft Office Outlook 11" from my Inbox.

would it help if I forwarded the relevant email(s) to someone so they have them in "native" form?

Comment 22

[Joe Sabash [:JoeS1]]

Ok Trying to isolate the specific area in the html that's causing the problem:
Removed the following tags (see attachment to test)
<o:smarttagtype namespaceuri="urn:schemas-microsoft-com:office:smarttags" name="Street">
<o:smarttagtype namespaceuri="urn:schemas-microsoft-com:office:smarttags" name="country-region">
<o:smarttagtype namespaceuri="urn:schemas-microsoft-com:office:smarttags" name="address">
<o:smarttagtype namespaceuri="urn:schemas-microsoft-com:office:smarttags" name="City">
<o:smarttagtype namespaceuri="urn:schemas-microsoft-com:office:smarttags" name="place">
<o:smarttagtype namespaceuri="urn:schemas-microsoft-com:office:smarttags" name="PersonName">

After removal "enter" seems to break the blockquote as usual.

Modified eml coming up.

Comment 23

[Joe Sabash [:JoeS1]]

Attachment: modified eml

Comment 24

[Joe Sabash [:JoeS1]]

This will require ongoing triage to determine how commonly this occurs.
So far it seems that the use of "smart tags" feature is required to trigger the bug (but not those tags alone) I'll work on a minimal testcase.
This might require setting up Microsoft Office Outlook 11, and some MS office stuff. So if anyone already has those apps set up, please feel free to add to the triage effort.

Comment 25

[George Hyman]

Attachment: another demonstration - from a different mailer

I've attached another email that demonstrates the problem, also from a microsoft mailer, but, I think, not from outlook 2003 (although I could be wrong).

Comment 26

[Joe Sabash [:JoeS1]]

(In reply to comment #25)
> Created an attachment (id=435710) [details]
> another demonstration - from a different mailer
> 
> I've attached another email that demonstrates the problem, also from a
> microsoft mailer, but, I think, not from outlook 2003 (although I could be
> wrong).

Keep them coming George, that example is much simpler.

Comment 27

[Joe Sabash [:JoeS1]]

(In reply to comment #16)
> http://hg.mozilla.org/comm-central/pushloghtml?startdate=2009-11-15&enddate=2009-11-16 
> 
> Is calendar only.
> 
> http://hg.mozilla.org/mozilla-central/pushloghtml?startdate=2009-11-15&enddate=2009-11-16
> 
> Nothing rings a bell. Dmose ?

Well, I vote for bug 527306
Mostly because of this comment, and it went into the proper branches
https://bugzilla.mozilla.org/show_bug.cgi?id=527306#c16

Comment 28

[George Hyman]

I've confirmed with the sender that the second example email is from outlook 2007.

Comment 29

[George Hyman]

Attachment: Another example, appears fairly simple, listed as exchange 6.5 mailer

Another example - also from a corporate source, (the Oakland A's) - I believe the mailer is exchange 6.5

Comment 30

[Dan Mosedale (:dmosedale, :dmose)]

roc, do you think your fix for bug 527306 could have caused this?  It certainly seems like the most plausible thing in the regression range...

Comment 31

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Seems unlikely since that patch was mostly just reverting part of a previous fix, but I guess it's possible. You could try backing it out.

Comment 32

[Nomis101]

Yes, it seems it is bug 527306. Today I've made a build from the current code and was still able to reproduce this with both attached mails. Than I've backed out the changes from bug 527306 (I've only backed out the changes for nsSelection.cpp, because the other files are only tests). And with this build the lines are not deleted anmore. I've set the cursor to the same position both times before hitting return, to make it reproducible.

Comment 33

[Joe Sabash [:JoeS1]]

Set blocking. 
Thanks to Nomis101
Much appreciated.

Comment 34

[George Hyman]

Is there a build available with the backout done that I could use to test/confirm?  When do we expect this change to make it into a generally available build?

Comment 35

[Joe Sabash [:JoeS1]]

Ping roc any hope for progress on this before TB3.1b2 (based on 1.9.2)
Build/relbranch date: Tuesday 27th April 

Workaround to break the blockquote:
Position the cursor on a blank line within the blockquote
Use Outdent instead of "enter"

The text below should be OK

Comment 36

[George Hyman]

Thanks - this WAR works just fine, removing a lot of the urgency, at least from my POV!

Comment 37

[Dan Mosedale (:dmosedale, :dmose)]

I just talked to roc, and he says that a testcase would be helpful to him here.  Adding Ludo and Gary to the CC in the hopes that one of you guys might be able to come up with one or find someone with the time who can...

Comment 38

[Joe Sabash [:JoeS1]]

https://bugzilla.mozilla.org/attachment.cgi?id=435710
Is fairly simple and 100% reproducible.
In that example, removing the first font tag:
<font size="2" face="Arial, sans-serif">
(the ending font tag for that is at the end of the body, with other font tags in between)
Anyhow removing that tag chases the problem away.
So...has something to do with nesting the font tags.

Comment 39

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I mean, a testcase that doesn't require Thunderbird to reproduce.

Comment 40

[Dan Mosedale (:dmosedale, :dmose)]

I suspect, but don't know, that if someone created a simple web page with a contentEditable element containing the HTML body of that message, this would have the same bug that we're seeing in Thunderbird.  http://blog.whatwg.org/the-road-to-html-5-contenteditable has some more info on how this works.

George or Joe, would one of you guys be willing to give that a shot?

Comment 41

[Joe Sabash [:JoeS1]]

Attachment: contentrditable body

The attachment does not show the same delete problem when edited here in.
One quirk though, the enter key does not break the blockquote (just adds a line within the blockquote)
So it looks like the enter key in mailnews must be a special case within our editor, that was broken by Roc's checkin.

Comment 42

[David :Bienvenu]

Attachment: minimal test case

This is the html I get when I save a draft that exhibits this problem. It's a fairly minimal test case, though I haven't tried it w/ content editable yet.

Comment 43

[David :Bienvenu]

Attachment: minimal message that exhibits the issue when replied to

Comment 44

[David :Bienvenu]

Attachment: special case editor handling

this fixes the issue in Thunderbird, but I don't know what effect it would have on the issue whose fix caused this regression, since I know absolutely nothing about layout or editor...

Comment 45

[Robert O'Callahan (:roc) (email my personal email if necessary)]

That patch would regress bug 527306.

What's the call stack that leads to selectFrames doing that flush that you need? Maybe we can put a flush higher up on that call stack.

The assertion you commented out no longer exists on trunk so that part is fine.

Comment 46

[David :Bienvenu]

Roc, thx for looking at this. We hit this code with a couple slightly different call stacks, when I press return:

>	gklayout.dll!nsTypedSelection::selectFrames(nsPresContext * aPresContext=0x1378e390, nsIRange * aRange=0x0b0b8a38, int aFlags=0x00000000)  Line 4411	C++
 	gklayout.dll!nsTypedSelection::Clear(nsPresContext * aPresContext=0x1378e390)  Line 3940	C++
 	gklayout.dll!nsTypedSelection::Collapse(nsINode * aParentNode=0x07387a18, int aOffset=0x00000001)  Line 5284	C++
 	gklayout.dll!nsTypedSelection::Collapse(nsIDOMNode * aParentNode=0x07387a3c, int aOffset=0x00000001)  Line 5264 + 0x15 bytes	C++
 	gklayout.dll!nsEditor::SplitNodeImpl(nsIDOMNode * aExistingRightNode=0x176b09a4, int aOffset=0x00000001, nsIDOMNode * aNewLeftNode=0x07387a3c, nsIDOMNode * aParent=0x114a91f8)  Line 3015 + 0x2a bytes	C++
 	gklayout.dll!SplitElementTxn::DoTransaction()  Line 109 + 0x36 bytes	C++
 	txmgr.dll!nsTransactionItem::DoTransaction()  Line 225 + 0x1c bytes	C++
 	txmgr.dll!nsTransactionManager::BeginTransaction(nsITransaction * aTransaction=0x12de9dc8)  Line 954 + 0x18 bytes	C++
 	txmgr.dll!nsTransactionManager::DoTransaction(nsITransaction * aTransaction=0x12de9dc8)  Line 119 + 0x14 bytes	C++
 	gklayout.dll!nsEditor::DoTransaction(nsITransaction * aTxn=0x12de9dc8)  Line 738 + 0x20 bytes	C++
 	gklayout.dll!nsEditor::SplitNode(nsIDOMNode * aNode=0x176b09a4, int aOffset=0x00000001, nsIDOMNode * * aNewLeftNode=0x003ae07c)  Line 1476 + 0x17 bytes	C++
 	gklayout.dll!nsEditor::SplitNodeDeep(nsIDOMNode * aNode=0x1526ef44, nsIDOMNode * aSplitPointParent=0x176b09a4, int aSplitPointOffset=0x00000001, int * outOffset=0x003ae218, int aNoEmptyContainers=0x00000001, nsCOMPtr<nsIDOMNode> * outLeftNode=0x003ae224, nsCOMPtr<nsIDOMNode> * outRightNode=0x003ae214)  Line 4210 + 0x36 bytes	C++
 	gklayout.dll!nsHTMLEditRules::SplitMailCites(nsISelection * aSelection=0x13798618, int aPlaintext=0x00000000, int * aHandled=0x003ae408)  Line 1837 + 0x44 bytes	C++
 	gklayout.dll!nsHTMLEditRules::WillInsertBreak(nsISelection * aSelection=0x13798618, int * aCancel=0x003ae44c, int * aHandled=0x003ae408)  Line 1610 + 0x14 bytes	C++
 	gklayout.dll!nsHTMLEditRules::WillDoAction(nsISelection * aSelection=0x13798618, nsRulesInfo * aInfo=0x003ae410, int * aCancel=0x003ae44c, int * aHandled=0x003ae408)  Line 664 + 0x14 bytes	C++
 	gklayout.dll!nsPlaintextEditor::InsertLineBreak()  Line 834 + 0x3a bytes	C++
 	gklayout.dll!nsPlaintextEditor::TypedText(const nsAString_internal & aString={...}, int aAction=0x00000002)  Line 445 + 0x18 bytes	C++

and similarly,

>	gklayout.dll!nsTypedSelection::selectFrames(nsPresContext * aPresContext=0x1378e390, nsIRange * aRange=0x15c814a8, int aFlags=0x00000001)  Line 4411	C++
 	gklayout.dll!nsTypedSelection::Collapse(nsINode * aParentNode=0x0cffbbf0, int aOffset=0x00000002)  Line 5319	C++
 	gklayout.dll!nsTypedSelection::Collapse(nsIDOMNode * aParentNode=0x0cffbc14, int aOffset=0x00000002)  Line 5264 + 0x15 bytes	C++
 	gklayout.dll!nsEditor::SplitNodeImpl(nsIDOMNode * aExistingRightNode=0x1526ef44, int aOffset=0x00000002, nsIDOMNode * aNewLeftNode=0x0cffbc14, nsIDOMNode * aParent=0x13799150)  Line 3015 + 0x2a bytes	C++

I can try moving the flush up to see if it still fixes the issue, but I won't have much of a clue about whether that still regresses the original issue.

Comment 47

[David :Bienvenu]

Attachment: moving the flush up one level also fixes the issue.

this also fixes it...

Comment 48

[Robert O'Callahan (:roc) (email my personal email if necessary)]

OK, to be safe, we need to move it up to editor, into nsEditor::SplitNodeImpl in this case.

Comment 49

[David :Bienvenu]

Attachment: move flush to editor

Thx roc - the attached patch (against trunk) worked. I'll go double check 1.9.2 as well.

Are you able to review this?

Comment 50

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Oh, fix the indentation in that patch.

Comment 51

[David :Bienvenu]

Attachment: fix indentation and move blank line

oops, fixed indentation and moved blank line to a more reasonable place...

Comment 52

[David :Bienvenu]

we're going to need moz-central 1.9.2 approval at some point, or land on our own rel-branch.

Comment 53

[Mark Banner (:standard8)]

As this is a now core bug, temporarily moving back to Mailnews core land so that we can remove the blocking flag (bug 533431), sorry for the bugspam.

Comment 54

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 445541 [details] [diff] [review]
fix indentation and move blank line

sr=dmose; thanks for making this happen!

Comment 55

[Dan Mosedale (:dmosedale, :dmose)]

I've just uploaded this patch to the Firefox trunk tryserver, so hopefully this will make it to http://tinderbox.mozilla.org/showbuilds.cgi?tree=MozillaTry before too long.

Comment 56

[Mark Banner (:standard8)]

Checked in to trunk:

http://hg.mozilla.org/mozilla-central/rev/41e6a1ad820d

Comment 57

[Mark Banner (:standard8)]

I've now landed this on a relbranch in mozilla-1.9.2 for Thunderbird 3.1 RC 1:

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/8922696d812c

I'll request approval in a mo for landing on default.

Comment 58

[Mark Banner (:standard8)]

Comment on attachment 445541 [details] [diff] [review]
fix indentation and move blank line

Requesting approval to land this on mozilla-1.9.2.

This fixes a regression in mozilla-1.9.2 (from mozilla-1.9.1) whereby bits of messages can get deleted by the editor when the user is inserting a blank line.

Trunk will already be testing this, and we're including it in TB 3.1rc1 (which will mean it'll get some more testing as well).

Comment 59

[LegNeato]

Comment on attachment 445541 [details] [diff] [review]
fix indentation and move blank line

a=LegNeato for 1.9.2.6. Please land this on mozilla-1.9.2 default.

Comment 60

[Mark Banner (:standard8)]

Checked in: http://hg.mozilla.org/releases/mozilla-1.9.2/rev/2c64404512a3