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

RESOLVED FIXED in mozilla1.9.3a5

Status

()

Core
Layout
--
major
RESOLVED FIXED
8 years ago
8 years ago

People

(Reporter: George Hyman, Assigned: Bienvenu)

Tracking

({regression})

unspecified
mozilla1.9.3a5
x86
All
regression
Points:
---

Firefox Tracking Flags

(status1.9.2 .7-fixed)

Details

(Whiteboard: [tb31needed][rc1/final fixed])

Attachments

(10 attachments, 1 obsolete attachment)

(Reporter)

Description

8 years ago
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
(Reporter)

Comment 1

8 years ago
Created attachment 434742 [details]
email that demonstrates the problem

Comment 2

8 years ago
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
Status: UNCONFIRMED → NEW
Ever confirmed: true

Comment 3

8 years ago
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.
Keywords: regression

Comment 4

8 years ago
Request blocking
blocking-thunderbird3.1: --- → ?
George and Joe,  How about on 3.2a1pre?

1.9.2 tree has some problem to edit HTML.
(Reporter)

Comment 6

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

Comment 7

8 years ago
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 :)
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
blocking-thunderbird3.1: ? → rc1+
Keywords: regressionwindow-wanted
(Reporter)

Comment 9

8 years ago
(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"
Thanks!
(Reporter)

Comment 11

8 years ago
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

8 years ago
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.
(Reporter)

Comment 13

8 years ago
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

8 years ago
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
(Reporter)

Comment 15

8 years ago
(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 17

8 years ago
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.
(Reporter)

Comment 18

8 years ago
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

8 years ago
(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

8 years ago
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.
OS: Windows XP → All
(Reporter)

Comment 21

8 years ago
(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

8 years ago
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

8 years ago
Created attachment 435350 [details]
modified eml

Comment 24

8 years ago
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.
(Reporter)

Comment 25

8 years ago
Created attachment 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).

Comment 26

8 years ago
(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.
(Reporter)

Comment 28

8 years ago
I've confirmed with the sender that the second example email is from outlook 2007.
(Reporter)

Comment 29

8 years ago
Created attachment 436541 [details]
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

Updated

8 years ago
Attachment #436541 - Attachment mime type: message/rfc822 → text/plan

Updated

8 years ago
Attachment #436541 - Attachment mime type: text/plan → text/plain
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...
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

8 years ago
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

8 years ago
Set blocking. 
Thanks to Nomis101
Much appreciated.
Blocks: 527306
Component: Message Compose Window → Layout
Product: Thunderbird → Core
QA Contact: message-compose → layout
Target Milestone: --- → mozilla1.9.3
Assignee: nobody → roc
blocking2.0: --- → ?
(Reporter)

Comment 34

8 years ago
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

8 years ago
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
Summary: html editor deletes lines below return on certain emails composed in outlook 2003 → html editor deletes lines below return on certain emails composed in outlook 2003 [Affects TB 3.1 HTML composition]
(Reporter)

Comment 36

8 years ago
Thanks - this WAR works just fine, removing a lot of the urgency, at least from my POV!
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

8 years ago
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.
I mean, a testcase that doesn't require Thunderbird to reproduce.

Updated

8 years ago
Attachment #435710 - Attachment mime type: message/rfc822 → text/plain
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

8 years ago
Created attachment 443853 [details]
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.

Updated

8 years ago
Whiteboard: [needs specific test case]
(Assignee)

Comment 42

8 years ago
Created attachment 445195 [details] [diff] [review]
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.
(Assignee)

Comment 43

8 years ago
Created attachment 445196 [details]
minimal message that exhibits the issue when replied to
(Assignee)

Comment 44

8 years ago
Created attachment 445226 [details] [diff] [review]
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...
Attachment #445226 - Flags: feedback?(roc)
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.
(Assignee)

Comment 46

8 years ago
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.
(Assignee)

Comment 47

8 years ago
Created attachment 445386 [details] [diff] [review]
moving the flush up one level also fixes the issue.

this also fixes it...
OK, to be safe, we need to move it up to editor, into nsEditor::SplitNodeImpl in this case.
(Assignee)

Comment 49

8 years ago
Created attachment 445481 [details] [diff] [review]
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?
Attachment #445481 - Flags: review?(roc)
Oh, fix the indentation in that patch.
(Assignee)

Updated

8 years ago
Attachment #445481 - Flags: superreview?(dmose)
(Assignee)

Comment 51

8 years ago
Created attachment 445541 [details] [diff] [review]
fix indentation and move blank line

oops, fixed indentation and moved blank line to a more reasonable place...
Assignee: roc → bienvenu
Attachment #445481 - Attachment is obsolete: true
Attachment #445541 - Flags: superreview?(dmose)
Attachment #445541 - Flags: review+
Attachment #445481 - Flags: superreview?(dmose)
(Assignee)

Comment 52

8 years ago
we're going to need moz-central 1.9.2 approval at some point, or land on our own rel-branch.
Whiteboard: [needs specific test case] → [has patch, needs sr dmose]
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.
Component: Layout → Composition
Product: Core → MailNews Core
QA Contact: layout → composition
Target Milestone: mozilla1.9.3 → ---
blocking-thunderbird3.1: rc1+ → ---
Component: Composition → Layout
Product: MailNews Core → Core
QA Contact: composition → layout
Whiteboard: [has patch, needs sr dmose] → [tb31needs][has patch, needs sr dmose]
Comment on attachment 445541 [details] [diff] [review]
fix indentation and move blank line

sr=dmose; thanks for making this happen!
Attachment #445541 - Flags: superreview?(dmose) → superreview+

Updated

8 years ago
Whiteboard: [tb31needs][has patch, needs sr dmose] → [tb31needs][needs checkin on trunk; approval/check 1.9.2]
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.
Checked in to trunk:

http://hg.mozilla.org/mozilla-central/rev/41e6a1ad820d
Status: NEW → RESOLVED
blocking2.0: ? → ---
Last Resolved: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.3a5
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.
Whiteboard: [tb31needs][needs checkin on trunk; approval/check 1.9.2] → [tb31needs][rc1/final fixed][needs approval 1.9.2.5 for TB 3.1.1]
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).
Attachment #445541 - Flags: approval1.9.2.5?

Comment 59

8 years ago
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.
Attachment #445541 - Flags: approval1.9.2.5? → approval1.9.2.6+
Checked in: http://hg.mozilla.org/releases/mozilla-1.9.2/rev/2c64404512a3
status1.9.2: --- → .6-fixed
Whiteboard: [tb31needs][rc1/final fixed][needs approval 1.9.2.5 for TB 3.1.1] → [tb31needed][rc1/final fixed]
You need to log in before you can comment on or make changes to this bug.