Last Comment Bug 746993 - Editor selection caret acts strangely when pressing enter after a <br> node.
: Editor selection caret acts strangely when pressing enter after a <br> node.
Status: RESOLVED FIXED
: regression
Product: Core
Classification: Components
Component: Editor (show other bugs)
: Trunk
: x86 All
: -- normal (vote)
: mozilla14
Assigned To: Mike Conley (:mconley) - (Needinfo me!)
:
Mentors:
Depends on: 680164
Blocks: 743632
  Show dependency treegraph
 
Reported: 2012-04-19 08:02 PDT by Mike Conley (:mconley) - (Needinfo me!)
Modified: 2012-05-02 15:28 PDT (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
+
fixed


Attachments
Fix Patch v1 (1.74 KB, patch)
2012-04-20 08:34 PDT, Mike Conley (:mconley) - (Needinfo me!)
ehsan: review+
Details | Diff | Splinter Review
Test case v1 (4.09 KB, patch)
2012-04-20 08:35 PDT, Mike Conley (:mconley) - (Needinfo me!)
no flags Details | Diff | Splinter Review
Test case v2 (4.09 KB, patch)
2012-04-20 08:41 PDT, Mike Conley (:mconley) - (Needinfo me!)
ehsan: review+
Details | Diff | Splinter Review

Description Mike Conley (:mconley) - (Needinfo me!) 2012-04-19 08:02:17 PDT
Just noticed this on nightly.

STR:

1)  Set up an account to reply above the quote
2)  Reply to a message using the HTML composer
3)  Type some characters, press enter, type some characters, press enter

What happens?

The caret doesn't appear to move when pressing enter.  However, the text appears to go onto the next line.

What's expected?

The caret should be placed where the text will be inserted.


I thought this might be fallout from the Filelink insertion stuff I did in nsMsgCompose.cpp, but this problem only appears on Daily, and not EarlyBird.

Currently bisecting.
Comment 1 Mike Conley (:mconley) - (Needinfo me!) 2012-04-19 09:31:14 PDT
Ehsan and I are pretty sure this is an editor bug, since we were able to reproduce this in Firefox nightly.
Comment 2 Mike Conley (:mconley) - (Needinfo me!) 2012-04-19 09:33:34 PDT
STR on Firefox:

1)  Go to http://www-archive.mozilla.org/editor/midasdemo/
2)  Click on "View HTML source"
3)  Insert the following HTML:

<html xmlns="http://www.w3.org/1999/xhtml"><head><meta http-equiv="Content-Type" content="text/html;charset=ISO-8859-1" /></head><body text="#000000" bgcolor="#FFFFFF"><br /><div class="moz-cite-prefix">On 19/04/2012 9:07 AM, Mike Conley wrote:<br /></div><blockquote type="cite" cite="mid:4F900E01.1030101@mozilla.com">This is a buggy situation.
<br />
<br />

</blockquote><br /><br /></body></html>

4)  Uncheck "View HTML source"
5)  Type some text at the beginning of the text box, press enter, more typing, press enter.

Problem is as described in https://bugzilla.mozilla.org/show_bug.cgi?id=746993#c0
Comment 3 Mike Conley (:mconley) - (Needinfo me!) 2012-04-19 09:59:03 PDT
mozregression gave me this range:

http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=7702bca6b64d&tochange=e636439e342f
Comment 4 Mike Conley (:mconley) - (Needinfo me!) 2012-04-19 12:32:07 PDT
Ehsan helped me debug this, and I've got a patch that seems to fix the issue.

Patch + test coming up next.
Comment 5 Mike Conley (:mconley) - (Needinfo me!) 2012-04-20 08:34:07 PDT
Created attachment 616983 [details] [diff] [review]
Fix Patch v1
Comment 6 Mike Conley (:mconley) - (Needinfo me!) 2012-04-20 08:35:03 PDT
Created attachment 616984 [details] [diff] [review]
Test case v1
Comment 7 Mike Conley (:mconley) - (Needinfo me!) 2012-04-20 08:36:16 PDT
Try build is baking on https://tbpl.mozilla.org/?tree=Try&rev=95206221a65a
Comment 8 Mike Conley (:mconley) - (Needinfo me!) 2012-04-20 08:41:24 PDT
Created attachment 616985 [details] [diff] [review]
Test case v2

Fixing the whitespace in Makefile.in
Comment 9 :Ehsan Akhgari 2012-04-20 08:52:23 PDT
Comment on attachment 616983 [details] [diff] [review]
Fix Patch v1

Review of attachment 616983 [details] [diff] [review]:
-----------------------------------------------------------------

::: editor/libeditor/html/nsHTMLEditRules.cpp
@@ +7683,5 @@
>    PRInt32 selOffset;
>    res = mHTMLEditor->GetStartNodeAndOffset(aSelection, getter_AddRefs(selNode), &selOffset);
>    NS_ENSURE_SUCCESS(res, res);
> +
> +  // are we after a <br>?  If so we want to stick to whatever is after <br>.

Please add a comment as to why we need to do this before checking for block siblings.
Comment 10 :Ehsan Akhgari 2012-04-20 08:55:27 PDT
Please land as a single changeset.  Note that this doesn't need approval.
Comment 11 Mike Conley (:mconley) - (Needinfo me!) 2012-04-20 10:03:22 PDT
Committed to mozilla-inbound as https://hg.mozilla.org/integration/mozilla-inbound/rev/7dc2085a8cc6
Comment 12 Phil Ringnalda (:philor) 2012-04-20 20:06:49 PDT
https://hg.mozilla.org/mozilla-central/rev/7dc2085a8cc6
Comment 13 :Ms2ger (⌚ UTC+1/+2) 2012-04-21 03:30:29 PDT
Comment on attachment 616985 [details] [diff] [review]
Test case v2

Review of attachment 616985 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/base/tests/test_reftests_with_caret.html
@@ +130,5 @@
>    tests.push([ 'bug613433-2.html' , 'bug613433-ref.html' ]);   // bug 681332
>    tests.push([ 'bug613433-3.html' , 'bug613433-ref.html' ]);   // bug 681332
>    tests.push([ 'bug613807-1.html' , 'bug613807-1-ref.html' ]); // bug 680574
>    tests.push([ 'bug634406-1.html' , 'bug634406-1-ref.html' ]); // bug 681146
> +  tests.push([ 'bug746993-1.html' , 'bug746993-1-ref.html' ]); // bug 746993

Why doesn't this test pass on Windows, or, if it does, why isn't it run there?
Comment 14 Mike Conley (:mconley) - (Needinfo me!) 2012-04-21 12:47:03 PDT
(In reply to Ms2ger from comment #13)
> Comment on attachment 616985 [details] [diff] [review]
> Test case v2
> 
> Review of attachment 616985 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: layout/base/tests/test_reftests_with_caret.html
> @@ +130,5 @@
> >    tests.push([ 'bug613433-2.html' , 'bug613433-ref.html' ]);   // bug 681332
> >    tests.push([ 'bug613433-3.html' , 'bug613433-ref.html' ]);   // bug 681332
> >    tests.push([ 'bug613807-1.html' , 'bug613807-1-ref.html' ]); // bug 680574
> >    tests.push([ 'bug634406-1.html' , 'bug634406-1-ref.html' ]); // bug 681146
> > +  tests.push([ 'bug746993-1.html' , 'bug746993-1-ref.html' ]); // bug 746993
> 
> Why doesn't this test pass on Windows, or, if it does, why isn't it run
> there?

Just following Ehsan's direction on that one.  Ehsan?
Comment 15 :Ehsan Akhgari 2012-04-22 12:49:00 PDT
These tests can be very unreliable on Windows, for reasons which are unclear to me as I've never had the time to investigate.  Therefore, every time somebody wants to add a test, I ask them to add them to this section.  Once we determine the root cause of the problem, we can fix them all up wholesale.  This is bug 680164.

Note You need to log in before you can comment on or make changes to this bug.