Closed Bug 756984 Opened 12 years ago Closed 9 years ago

Changing location in editor doesn't preserve the font when returning to end of text/line

Categories

(Core :: DOM: Editor, defect)

Product:
Core
Component:
DOM: Editor
Type:
defect
Priority:
Not set
Severity:
major

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla39
Tracking Flags:
Tracking Status
firefox39 --- fixed

People

(Reporter: standard8, Assigned: jorgk-bmo)

References

(Depends on 1 open bug, Blocks 2 open bugs)

Details

Attachments

(7 files, 13 obsolete files)

Two videos showing the problem.
199.53 KB, application/zip
Details
composition font also lost after insertion of image.
90.13 KB, application/zip
Details
Alternate test, take 2, dumping out more info
876 bytes, text/html
Details
A more comprehensive text
1.11 KB, text/html
Details
Patch to fix all three problems: Click, "end" key, left arrow from start of previous line (updated coding style)
3.71 KB, patch
roc
: review+
Details | Diff | Splinter Review
Here comes another manual test.
915 bytes, text/html
Details
Unified patch (code + test changes + four times revised new test)
10.97 KB, patch
ehsan.akhgari
: review+
Details | Diff | Splinter Review 
STR:

1) Open midas demo
2) Type three lines of text
3) Select a different font for the middle line
4) go to end of first line, type some text

=> Font is the same as the first line, because it is default font.

5) go to end of second line, type some text

=> Font is the original (default) font, not the font selected in step 3.

Alternate STR:

a) Open midas demo
b) Type a word
c) select a different font, type a different word
d) click somewhere after the end of line (e.g. not changing cursor position)
e) type some more text

=> Font is now back to the original/default font.
This is happening because moving the cursor ANYWHERE, then clicking back at the end of the typing line moves the input point past the hidden [/ font ] tag at the end of that line.  Thus the font reverts to default.

Alternate steps to repeat:

1.  be typing
2. move cursor somewhere above the typing point (e.g. to insert text within previous words)
3. move cursor back to end of last line
4. Result: font changes to variable width because the replacement of cursor at the end of the line has moved it outside the [/ font ] tag.
Testing with Mozilla/5.0 (Windows NT 5.1; rv:15.0) Gecko/15.0 Thunderbird/15.0a1  20120521030204

It seems this issue is much improved since the landing of bug 750569
I have not been able to reproduce the loss of font by moving the cursor around with left-click mouse.
I did however see some anomalies with extra br tags being applied, and one case of a missing ending font tag.
But these are a small price to pay for the basic fix.
I'll test extensively with various fonts and html tag insertions over the coming days. But so far, it looks very encouraging.
It seems this issue is much improved since the landing of bug 750569
s/b bug 590640 sorry about that.
(some of these results probably should go into bug 590640) but as they pertain to this bug, the issue where the cursor was positioned outside of the font tag has been fixed somewhere between the ESR and our current Beta TB13 (bug number?)

Testing steps
Type in:The quick brown fox
Go back to "brown" and correct it to "red"
reposition the cursor (with mouse) just after "fox"
Type in:" jumped."

Here is the resulting code generated:
ESR:
<font face="Arial">The quick red fox</font> jumped.

beta tb13
<font face="Arial">The quick red fox jumped.<br>
    </font>

current trunk
<font face="Arial">The quick <font face="Arial">red</font> fox
      jumped.<br>
    </font>

The ESR behavior is obviously wrong
TB13 is correct
Current trunk displays the correct font, but adds extra font tags in the process

I suppose this is a result of the fix in bug 590640 "remembering" the type in state.
(In reply to Mark Banner (:standard8) from comment #0)
> STR:
> 
> 1) Open midas demo
> 2) Type three lines of text
> 3) Select a different font for the middle line
> 4) go to end of first line, type some text
> 
> => Font is the same as the first line, because it is default font.
> 
> 5) go to end of second line, type some text
> 
> => Font is the original (default) font, not the font selected in step 3.

WFM on nightly 15.0a1 (2012-05-21).

> Alternate STR:
> 
> a) Open midas demo
> b) Type a word
> c) select a different font, type a different word
> d) click somewhere after the end of line (e.g. not changing cursor position)
> e) type some more text
> 
> => Font is now back to the original/default font.

Also WFM.

(In reply to maybe-the-one from comment #1)
> Alternate steps to repeat:
> 
> 1.  be typing
> 2. move cursor somewhere above the typing point (e.g. to insert text within
> previous words)
> 3. move cursor back to end of last line
> 4. Result: font changes to variable width because the replacement of cursor
> at the end of the line has moved it outside the [/ font ] tag.

Also WFM.

(In reply to Joe Sabash from comment #3)
> Testing steps
> Type in:The quick brown fox
> Go back to "brown" and correct it to "red"
> reposition the cursor (with mouse) just after "fox"
> Type in:" jumped."
> 
> . . .
> 
> current trunk
> <font face="Arial">The quick <font face="Arial">red</font> fox
>       jumped.<br>
>     </font>

Filed as bug 757371.  Thanks!  I'll work on fixing that right away.  Is there anything else here that needs to be fixed, or should we close this as a duplicate of bug 590640?
I know I'm probably going to get a lot of push back on this, but I really, really think that these editor fixes should be back-ported to the ESR release, not require ESR users to wait almost a full YEAR before seeing them...

Enterprises will benefit MUCH more than home users with these fixes, as they are very frustrating for users who use HTML email, and most enterprise users do want to use HTML, like it or not...
According to <https://wiki.mozilla.org/Release_Management/ESR_Landing_Process>, only security and major stability fixes will be approved for ESR landing, so I'm afraid these don't qualify.  You can set "approval-mozilla-esr10?" on the relevant patches, but I'd expect it to be -'d immediately.  These patches touch complicated and buggy code, and have significant regression potential.  The whole idea of ESR is that it's stable and won't break if you upgrade -- if we backport anything but the most critical bug fixes, it will no longer be stable.

Not my decision, though, so feel free to request it if you want to argue with the people whose decision it is.
I can still reproduce this, but I think my STR weren't complete.

The single line case I can sometimes reproduce by clicking at the end of the line (rather than using the keyboard). It seems to be more reproducible if there are spaces in the line.

More test cases as well, being a bit more explicit about my actions and using 2012-05-21 nightly on Mac:

1) Open midas demo
2) Type three lines of one-word text, e.g.

three
lines
text

3) double-click the middle line to highlight 
4) select a different font
5) press right-arrow to unselect and go to end of line.
6) Start typing

=> Incorrect font used.

Slightly different case:

1) Open midas demo
2) Type three lines of one-word text, e.g.

three
lines
text

3) double-click the middle line to highlight 
4) select a different font
5) Click at the end of the last line
6) Use the left-arrow five times to go past the "text" and onto the end of the second line.
7) Type some more text

=> Incorrect font used.
(In reply to Mark Banner (:standard8) from comment #7)
> I can still reproduce this, but I think my STR weren't complete.
> 
> The single line case I can sometimes reproduce by clicking at the end of the
> line (rather than using the keyboard). It seems to be more reproducible if
> there are spaces in the line.
> 
> More test cases as well, being a bit more explicit about my actions and
> using 2012-05-21 nightly on Mac:

Thanks!

> 1) Open midas demo
> 2) Type three lines of one-word text, e.g.
> 
> three
> lines
> text
> 
> 3) double-click the middle line to highlight 
> 4) select a different font
> 5) press right-arrow to unselect and go to end of line.
> 6) Start typing
> 
> => Incorrect font used.

This doesn't quite work for me (15.0a1 (2012-05-21) on Ubuntu 11.10).  When I press right-arrow in step 5, it goes to the next line, not the end of the line.  But if I press right-arrow then left-arrow, I can reproduce.  So confirmed.

> Slightly different case:
> 
> 1) Open midas demo
> 2) Type three lines of one-word text, e.g.
> 
> three
> lines
> text
> 
> 3) double-click the middle line to highlight 
> 4) select a different font
> 5) Click at the end of the last line
> 6) Use the left-arrow five times to go past the "text" and onto the end of
> the second line.
> 7) Type some more text
> 
> => Incorrect font used.

Confirmed.  Thanks!  We need to normalize the selection better here.
Blocks: 517098
I am not sure if the patches for this bug apply currently to 16a2 but for yesterday's 16a2 I still have a problem with the following test:

1) Compose a new mail and type several lines of text into the main body of the mail.
2) Highlight all of the new text in the compose window.
3) Select Format->Size and increase the size to extra large.
4) Now (realising you want to add some more text) click in the middle of the text area and type in new text, and edit some of the text already there.
5) Nothing seems wrong at this stage but if the mail is sent then sometimes it seems to be fine in the copy that is actually sent, as confirmed by what appears in the copy in the Sent folder, but sometimes the font size is broken into sections of different sizes despite it all looking uniform at the point the Send button is about to be clicked.  This seems to have been the case for some time.

If that pertains to a different bug please refer to the relevant bug number - thanks.
As a general rule, filing new bugs is better than posting in an existing bug if you're unsure whether it's the same bug.  If two bugs are duplicates, they can be marked as such, but a single bug can't be easily split into two.  So please file a new bug.  Thanks.
OK will do - though recent testing with Mozilla/5.0 (X11; Linux x86_64; rv:16.0) Gecko/20120813 Thunderbird/16.0a2 ID:20120813042011 gave no further recurrence - if I see a recurrence I will post a new bug report for the later version with full details.
Blocks: 823523
This bug is still current at version 31.2.0 and 33 beta.

Given that it's been carried over from bug 250539 created in 2004, it might be a good idea to one day do something about it ;-)
Here are two cases to reproduce the problem.

CASE 1:
=======

Step 1:
Set the following options:
Options: Display, Formatting, Default Font: Script
Options: Composition, HTMl, Font: Batang

Step 2:
Reply to an e-mail written in Arial. Start typing in Batang.
Click into the quoted text. Font changes to Arial.

Step 3:
Click after the text written in Batang. Font says Arial but we write in Script (default font).

Result:
After returning to the end of the line, the font is lost, the composition continues in the default font and the in the HTML the text is outside the <font></font> tags. The UI shows incorrect information.

Message saved/sent has this HTML:
    <blockquote cite="mid:5452F2B1.2070100@gmail.com" type="cite"><font
        face="Arial">We have done little to address this trend.</font></blockquote>
    <font face="Batang">This is batang. </font>This is script<br>
See video 1 enclosed.

CASE 2:
=======

Step 1:
Set the following options:
Options: Display, Formatting, Default Font: Batang
Options: Composition, HTMl, Font: Batang

Step 2:
Reply to an e-mail written in Arial. Start typing in Batang.
Click into the quoted text. Font changes to Arial.

Step 3:
Click after the text written in Batang. Font says Arial but we write in Batang (default font).

Result:
After returning to the end of the line, the font is lost, the composition continues in the default font and the in the HTML the text is outside the <font></font> tags. The UI shows incorrect information.

Message saved/sent has this HTML:
    <blockquote cite="mid:5452F2B1.2070100@gmail.com" type="cite"><font
        face="Arial"><br>
        We have done little to address this trend.</font></blockquote>
    <font face="Batang">This is batang. </font>We continue to write in
    batang, or so it seems.<br>
See video 2 enclosed.
Composition font is also lost after inserting an image in a new message.
Watch the enclosed video. I am using these settings:
Options: Display, Formatting, Default Font: Arial.
Options: Composition, HTML, Font: Fixed width.
Thanks Jorg...

I see now why I never encountered this bug. I can see how anyone who encounters it would find it frustrating, but...

I never do anything fancy with respect to fonts in my emails, and I always recommend to others that they should never do anything fancy in HTML emails. I tell them that if they want someone to get something fancy, send them a binary attachment (PDF is what I recommend).

Since Outlook (as much as I hate it, it is the mail client with the largest install base) switched to using Word for its HTML rendering engine (in spite of the immediate and ongoing massive complaints and uproar), which is one of the worst HTML rendering engines there is, it just doesn't make sense to waste a lot of time making something look fancy when there is every probability that the vast majority of anyone you may send it to will NOT see it the way you intended.

All I ever do is sometimes use lists, change styles, etc, but only minor tweaks...

And I never edit quoted text either (that just seems wrong to me)...

Anyway, hopefully this one will now get squashed sooner rather than later now that iti is easily reproducible...
(In reply to Jorg K from comment #12)
> This bug is still current at version 31.2.0 and 33 beta.
> 
> Given that it's been carried over from bug 250539 created in 2004, it might
> be a good idea to one day do something about it ;-)

Unfortunately, we have no one actively working on the editor component, so basically all editor bugs on indefinitely on hold.  Occasionally one or two gets fixed here or there, but as things stand, I wouldn't count on it.
???

I thought the contrary was actually the case?

Unless I am sorely mistaken, or confusing components, the composer (editor) component just received some major changes, and regressions are actively being worked on.
(In reply to :Aryeh Gregor from comment #16)
> Unfortunately, we have no one actively working on the editor component, so
> basically all editor bugs on indefinitely on hold.  Occasionally one or two
> gets fixed here or there, but as things stand, I wouldn't count on it.

OK, when you say "editor" you mean "composer". The component that allows the user to more or less WYSIWYG enter text (with fonts, colour, etc.) and pictures and creates HTML which is sent out.

Charles beat me to it, but I thought this is being worked on, quoting:
https://mail.mozilla.org/pipermail/tb-planning/2014-October/003499.html
===
The Addressing bugs are due to some significant movement on getting the
Composer rewrite going. This is a very *good* thing.
It is painful when bugs like this show up, yes, but the composer has
been needing the rewrite for 10+ years, so I for one am glad it is
happening.
===

I've certainly experienced a few of the regressions (bug 1042561 and bug 1043310).
(In reply to Jorg K from comment #18)
> OK, when you say "editor" you mean "composer". The component that allows the
> user to more or less WYSIWYG enter text (with fonts, colour, etc.) and
> pictures and creates HTML which is sent out.

I mean the code under editor/ in the Gecko source tree, which handles the backend of editors for both Firefox and Thunderbird.  Thunderbird's composer uses the Gecko editor for composing mail, but also has its own code that it puts on top, so the post you link to may be referring to that.  This bug is in the editor itself, not the Thunderbird-specific code.  The two bugs you mentioned are Thunderbird-specific and not related to the editor (note "Product: Thunderbird", vs. this bug with "Product: Core").

That said, I actually haven't been paying much attention, and for all I know it could be someone has started working on editor again recently and I didn't notice.  It doesn't seem so based on glancing at the log, but I didn't look at the commits in detail.  If people are working on Thunderbird more, maybe they'll want to fix stuff in the editor too, since some high-profile Thunderbird bugs are really editor bugs.  So we can still hope!  :)
There's nobody actively working on it atm. The quote likely refers to the jsmime work, which has very little to do with the editor.
From Bug 250539

(In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!, PTO 11/3-11/21) from comment #196)
2010-08-25 07:18:58 PDT 
> It appears to me that this is an editor bug.  If someone can create a test
> case as an HTML file and file a bug in Core::Editor, I may try to sneak a
> fix in for Gecko 2.0.

(In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!, PTO 11/3-11/21) from comment #232)
2011-09-22 19:46:20 PDT 
> Everyone, please relax.  I'm working hard to work out a solution to this
> bug.  

What happened to that? The test case is here.

Is this a super-hard problem? To me at seems that it's just a simple special case: If cursor is moved to the very end of the text, pick up the font that was previously used.
Flags: needinfo?(ehsan.akhgari)
As Aryeh said, there's nobody who is currently working on the editor code base, unfortunately.  This is not a very hard problem, it is mostly an issue of having the human resources to work on this component.  But it is also not as easy as you suggest, because we don't remember the history of all of the fonts/etcs used during editing.

If someone wants to fix this, I think they should first look into where in the DOM we collapse the selection when you click at the end of the paragraph.  My suspicion is that place is outside of the element that has the respective style for the font set.  We should probably look into normalizing the selection in those cases to be inside the said element, and figure out the edge cases that we need to think about and also test the behavior of other browser engines in those cases, etc.
Flags: needinfo?(ehsan.akhgari)
Blocks: 802190
In nsEditor::InsertTextImpl we find this code:
if (mComposition) {
 ...
} else {
 if (node->IsNodeOfType(nsINode::eTEXT)) {
  ...
 } else {
  // we are inserting text into a non-text node.  first we have to create a
  // textnode (this also populates it with the text)
  node->AsElement()->List(stdout,0,EmptyCString());
  ...

Debugging shows that when the font is lost - after clicking elsewhere and then clicking again at the end of the line - "we are inserting text into a non-text node". Dumping out the 'node' at this point gives:
body@1197C400 text="#000000" bgcolor="#FFFFFF" _moz_dirty="" state=[40000020004] flags=[00104008] ranges:1 primaryframe=0BFDD498 refcount=62<
  tt@0DAA42E0 _moz_dirty="" state=[40000020000] flags=[00100000] primaryframe=0D3A5020 refcount=5<
    Text@0C58D650 flags=[02000008] primaryframe=0D3A5070 refcount=14<one>
  >
  br@0DCF3D00 _moz_dirty="" state=[40000020000] flags=[00100000] primaryframe=0BFDFA90 refcount=4<>
  br@0DCF3DC0 _moz_dirty="" state=[40000020000] flags=[00100000] primaryframe=0BFDF968 refcount=2<>
  div@0DCF3E80 _moz_dirty="" class="moz-cite-prefix" state=[40000020000] flags=[00100400] primaryframe=0BFDFD20 refcount=4<
    Text@0DCFFD80 flags=[03000008] primaryframe=0BFE0088 refcount=3<On 26/02/2015 20:59, Bugzilla@Mozilla wrote:>
    br@0DCF3EE0 _moz_dirty="" state=[40000020000] flags=[00100000] primaryframe=0BFDF8F0 refcount=2<>
  >
(Note: In my test I was answering a Bugzilla e-mail and my composition style was "tt". I had typed the word "one" before clicking elsewhere and then clicking again after "one". On the next keystroke the above dump was produced.)

This confirms what Ehsan said in comment #22: The key to this problem is what happens when clicking at the end of the line to continue typing. By the looks of it, the click does not identify the correct element where the user would like to continue typing. Instead a new node is created which is lacking the font information.

I could use some help to locate the code that translates the click into identifying the element. Somewhere in ns(HTML)EditorEventListener.cpp, I suppose.
Flags: needinfo?(ehsan)
I am having second thoughts. If I understood correctly, the idea is to continue adding to the pre-existing element at the end of the line instead of adding a new element which will lose the style or font. Is this really the correct approach?

I have three more questions in this context:
1) The paragraph is created when replying to an e-mail does have the correct style/font. It only gets lost after moving the caret around in the e-mail. How does Thunderbird communicate to the editor which style to use in the first place, in the case comment #23, the <tt>? Why can't this method be used for any subsequently created text elements? Currently further text elements are created with the "default" font, which is also configured in Thunderbird and must be communicated somehow to the editor.
2) Over in bug 1100966 (spell checker losing red underlines) another node is also created when clicking at the end of the line. Upon backspace, this node becomes empty and is merged (badly) with the preceding one, which in turn loses the underlines. Why didn't we consider to continue the existing node instead of creating a new one? In this case the merge would not happen.
3) Another problem with losing the style/font can be observed after inserting an image, see comment #14. I think the proposed approach of reusing a pre-existing text element can't be used here, since the preceding element is an image. So wouldn't it be easier to make sure the chosen style/font is observed when creating another text element after the image?
(In reply to Jorg K from comment #23)
> I could use some help to locate the code that translates the click into
> identifying the element. Somewhere in ns(HTML)EditorEventListener.cpp, I
> suppose.

nsFrame::HandlePress is called when you click on an element, and at least part of the selection normalization should probably happen inside there.

(In reply to Jorg K from comment #24)
> I am having second thoughts. If I understood correctly, the idea is to
> continue adding to the pre-existing element at the end of the line instead
> of adding a new element which will lose the style or font. Is this really
> the correct approach?

It's probably the easiest way to fix this bug.  There are probably other implementation strategies as well, but I can't think of an easier one.

> I have three more questions in this context:
> 1) The paragraph is created when replying to an e-mail does have the correct
> style/font. It only gets lost after moving the caret around in the e-mail.
> How does Thunderbird communicate to the editor which style to use in the
> first place, in the case comment #23, the <tt>?

I don't know how Thunderbird works, but this information is usually transferred to Gecko using document.execCommand calls.

> Why can't this method be
> used for any subsequently created text elements? 

I don't understand this question.

Note that the type-in state code is mostly used to handle sequences such as calling document.execCommand("bold") and starting to type.  It doesn't know how to handle anything more complicated (for example remembering the styles to be used for an arbitrary element at an arbitrary DOM position.)

> Currently further text
> elements are created with the "default" font, which is also configured in
> Thunderbird and must be communicated somehow to the editor.

Again, I don't know the Thunderbird specific parts here.

> 2) Over in bug 1100966 (spell checker losing red underlines) another node is
> also created when clicking at the end of the line. Upon backspace, this node
> becomes empty and is merged (badly) with the preceding one, which in turn
> loses the underlines. Why didn't we consider to continue the existing node
> instead of creating a new one? In this case the merge would not happen.

Because it's possible to fix that code more easily, and also we use the same joining code for other purposes such as when two nodes really need to be joined because the content in between them is being deleted.  Fixing this bug would make the steps to reproduce in that bug change, but these are separate issues.

> 3) Another problem with losing the style/font can be observed after
> inserting an image, see comment #14. I think the proposed approach of
> reusing a pre-existing text element can't be used here, since the preceding
> element is an image. So wouldn't it be easier to make sure the chosen
> style/font is observed when creating another text element after the image?

I _think_ to fix that part you need to get Thunderbird tell Gecko about how to format the new paragraph.  The fix suggested in comment 22 will not fix all of the issues with the type-in style being lost.
Flags: needinfo?(ehsan)
Thank you for your detailed answer which clarifies a lot of things. I will investigate the interaction between Thunderbird and the editor further.

Re. your last comment:
> I _think_ to fix that part you need to get Thunderbird tell Gecko about how
> to format the new paragraph. 

I tried with a <div contenteditable> in Firefox. The editor handles insertion of images by itself. So the question is: How could Thunderbird, the invoker of the editor, be notified, so it could in turn do whatever it does normally (I need to investigate what that is exactly) to communicate that the format required after the insertion? Can an callback be installed that the editor calls when new nodes are created in the DOM tree?

So far I get the following picture of the interaction between Thunderbird and the editor:
1) Thunderbird invokes the editor and communicates the initial conditions, font, size, etc.
2) The editor does all the editing. When new nodes are created in the DOM, no "special" style element is used, so to the Thunderbird user it looks like the style gets lost.
3) No callback to Thunderbird takes place while the editor is running.
4) Thunderbird may send "commands" to the editor, to change the font, bold, etc.
5) When the editing is done, Thunderbird receives a copy of the DOM (or the plain text or HTML) and stores it or sends it.
I have to correct point 3) a little. In an e-mail with different fonts, colours, bold, italics, clicking in the editor does communicate something back to Thunderbird so it can update it's controls; or maybe the editor just sets some "global" variables, that can be queried in the UI.
Sorry to trouble you again. I have some more questions to understand how the architecture hangs together. Let me summarise the questions from the previous posts here:

Where is the mouse click translated into identifying a node of the DOM tree?
Your answer was nsFrame::HandlePress. I looked through there in the debugger but didn't see anything connected to the editor. nsFrame::HandlePress handles clicks anywhere on the mail composition window. I would like to find the spot where the editor looks through the DOM tree to identify the node. I looked in nsHTMLEditorEventListener::MouseDown, but in there only the very last branch controlled by "else if (!isContextClick && buttonNumber == 0 && clickCount == 1)" is executed. I would imagine to find a traversal of the DOM tree to find the correct node.

How does Thunderbird communicate "composition font" or "default font" to the editor?
Your answer was that you didn't know. Fair enough. You suggested the document.execCommand could be used. Assuming that this is implemented in nsHTMLDocument::ExecCommand, I set a breakpoint which wasn't reached. Equally, then the UI controls for bold, italics, font, etc. are operated, the breakpoint wasn't reached. I'd like to know how this is passed to the editor. So I will ask in the Thunderbird group.

When the user writes an e-mail and clicks into a text, the UI controls are updated, that is the indicators for font, bold, italics, etc. reflect the properties of the text where the user clicked.
How is this done? How does the editor notify the UI, after identifying the node clicked and the finding the properties of the node. This is also important to know, because in cases where the font gets lost, the UI is not updated correctly.

And last question just repeated from comment #26. You said in comment #25:
> I _think_ to fix that part you need to get Thunderbird tell Gecko about how
> to format the new paragraph. 

I tried with a <div contenteditable> in Firefox. The editor handles insertion of images by itself. So the question is: How could Thunderbird, the invoker of the editor, be notified, so it could in turn do whatever it does normally (I need to investigate what that is exactly) to communicate that the format required after the insertion? Can an callback be installed that the editor calls when new nodes are created in the DOM tree?
Note: I've just noticed that when an image, math, table, link, etc. is inserted from the "Insert" menu, the composition style is NOT lost. It is only lost when an image is *pasted* in. That's an interesting observation, leading to the question how the system manages to maintain the font in these cases, whereas on paste the font gets lost.
Flags: needinfo?(ehsan)
(In reply to Jorg K from comment #26)
> Re. your last comment:
> > I _think_ to fix that part you need to get Thunderbird tell Gecko about how
> > to format the new paragraph. 
> 
> I tried with a <div contenteditable> in Firefox. The editor handles
> insertion of images by itself. So the question is: How could Thunderbird,
> the invoker of the editor, be notified, so it could in turn do whatever it
> does normally (I need to investigate what that is exactly) to communicate
> that the format required after the insertion?

If it works with a simple contenteditable element in Firefox, then Thunderbird is doing something that it should not be doing.  I have no idea what that might be though.  But at any rate, that is off topic for this bug.

> Can an callback be installed
> that the editor calls when new nodes are created in the DOM tree?

You can use a mutation observer to be notified when new nodes are injected into the tree, but like I said, that doesn't help if Thunderbird is doing something that it should not be doing.

> So far I get the following picture of the interaction between Thunderbird
> and the editor:
> 1) Thunderbird invokes the editor and communicates the initial conditions,
> font, size, etc.
> 2) The editor does all the editing. When new nodes are created in the DOM,
> no "special" style element is used, so to the Thunderbird user it looks like
> the style gets lost.
> 3) No callback to Thunderbird takes place while the editor is running.
> 4) Thunderbird may send "commands" to the editor, to change the font, bold,
> etc.
> 5) When the editing is done, Thunderbird receives a copy of the DOM (or the
> plain text or HTML) and stores it or sends it.

I'm not familiar enough with the Thunderbird side of things to be able to tell how it interacts with Gecko.  And I don't know anyone who does.  Your best bet may be reading the code.
Flags: needinfo?(ehsan)
(In reply to Jorg K from comment #28)
> Sorry to trouble you again. I have some more questions to understand how the
> architecture hangs together. Let me summarise the questions from the
> previous posts here:
> 
> Where is the mouse click translated into identifying a node of the DOM tree?
> Your answer was nsFrame::HandlePress. I looked through there in the debugger
> but didn't see anything connected to the editor.

Yes, that part has nothing to do with the editor.  In general, the editor is usually oblivious to selection changes, caret movement and such.

> nsFrame::HandlePress
> handles clicks anywhere on the mail composition window. I would like to find
> the spot where the editor looks through the DOM tree to identify the node. I
> looked in nsHTMLEditorEventListener::MouseDown, but in there only the very
> last branch controlled by "else if (!isContextClick && buttonNumber == 0 &&
> clickCount == 1)" is executed. I would imagine to find a traversal of the
> DOM tree to find the correct node.

I'm not exactly sure why you're looking at the editor code.  Nothing in comment 22 will probably need to be fixed in the editor code.

> How does Thunderbird communicate "composition font" or "default font" to the
> editor?
> Your answer was that you didn't know. Fair enough. You suggested the
> document.execCommand could be used. Assuming that this is implemented in
> nsHTMLDocument::ExecCommand, I set a breakpoint which wasn't reached.
> Equally, then the UI controls for bold, italics, font, etc. are operated,
> the breakpoint wasn't reached. I'd like to know how this is passed to the
> editor. So I will ask in the Thunderbird group.

I'd be curious to know, FWIW!

> When the user writes an e-mail and clicks into a text, the UI controls are
> updated, that is the indicators for font, bold, italics, etc. reflect the
> properties of the text where the user clicked.
> How is this done? How does the editor notify the UI, after identifying the
> node clicked and the finding the properties of the node. This is also
> important to know, because in cases where the font gets lost, the UI is not
> updated correctly.

Again, I know nothing about what Thunderbird does, but a normal web application which does this would probably use document.queryCommandEnabled and friends.  I think you should be looking at the Thunderbird code to see how it does things though, there is no point in guessing!
Thank you for your continued patience.
It is true that this bug is is about losing the font when clicking at the end of the line after clicking somewhere else. However, I'd like to consider other occasions where the font is lost, that is after inserting an image by paste.

Let's deal with this "off-topic" issue first: In Firefox, using <div contenteditable> you can paste in an image and then keep typing in the font that was used before the insertion. So as you concluded, TB must be doing something strange here.

As for the clicking at the end of the line: In Firefox, again with a <div contenteditable> I created two texts with different fonts, clicking in one, typing in one font, then clicking in the other one, then clicking at the end of the first one again does NOT lose the font.

So maybe this is not a Core/Editor bug, maybe it's solely a Thunderbird bug.

To answer your question why I was looking in the editor for the click processing: pure ignorance. Of course you can click onto a static document where no editor is involved and the click will be processed.

I will follow up on the other issues with the Thunderbird group
https://groups.google.com/forum/#!topic/mozilla.dev.apps.thunderbird/1xtQ_nruGcw
and keep you posted.
Jorg K and Ehsan Akhgari, thank you so much for working on this pesky problem! I'm sure that a lot of other Thunderbird users are grateful too.  This nuisance has been plaguing us for a very long time, and it would be *wonderful* to have it fixed!

Thanks again.

Gloria
@Ehsan from comment #30 ("I'd be curious to know, FWIW!"):

Turns out that the Thunderbird e-mail front end is one big JS/XUL application:
http://mxr.mozilla.org/comm-central/source/editor/ui/composer/content/editor.js

I haven't looked very much, but I can answer some questions. For example the "Bold button" is defined here:
http://mxr.mozilla.org/comm-central/source/editor/ui/composer/content/editorOverlay.xul#1138
There is an observer, so when the bold state changes, it gets noticed and the UI is updated.

I will now hunt through this code to see whether I can find the cause of the lost font.
Without a working font indicator it is impossible to tell what's going on. So fixing bug 1139524 first.

Further testing over in the other bug showed that by clicking at the end of the line sometimes the font in the next line is activated rather then the one of the line being clicked.
Depends on: 1139524
Just wanted to add another 'THANK YOU!!! to Jorg K for working on these editor bugs.

Too often these older bugs are filled with negative comments.
Just noticed: Not only the font gets lost, also the font size can get lost. For example when replying to a message in a small font, the size gets bigger when clicking at the end after correcting something in the same line.
Simple test using Midas: http://www-archive.mozilla.org/editor/midasdemo/

1) type two lines, like shown:
one
two

They are shown in Times.

2) select first line and make it arial, select second and make it courier

3) Click after the word "one" in the first line and type. Typing happens in Times. Wrong font.

You must reload the page before trying again.
Coming back to the suggestion from comment #25 and looking in nsFrame::HandlePress.

I traced it down into ns[Text]Frame::GetChildFrameContainingOffset with a call stack of:

nsFrame::GetChildFrameContainingOffset *or*
nsTextFrame::GetChildFrameContainingOffset
nsFrameSelection::GetFrameForNodeOffset
nsFrameSelection::BidiLevelFromClick
nsFrameSelection::HandleClick
nsFrame::HandlePress

It varies whether nsFrame::GetChildFrameContainingOffset or nsTextFrame::GetChildFrameContainingOffset is being called depending on whether you click on the text, but almost to the left of the text, or completely to the left of the text.

When the nsTextFrame::GetChildFrameContainingOffsetAny version is called, it offset is calculated correctly and the font is right. When the nsFrame::GetChildFrameContainingOffset is called, the font is wrong. In the latter case the caret still appears behind the text.

Try it with a wide letter, like "m"!

So in case a "Frame" being hit instead of a "TextFrame", perhaps the program should look whether there is a text right before the caret once placed.

Here some debug showing the two cases:
=== before BidiLevelFromClick
=== in nsTextFrame::GetChildFrameContainingOffset
nsTextFrame::GetChildFrameContainingOffset, offset=3
=== after BidiLevelFromClick

=== before BidiLevelFromClick
=== in nsFrame::GetChildFrameContainingOffset
=== after BidiLevelFromClick

Any further hints? Five minutes of your time can possibly save me five hours or five days of "reverse engineering".
Flags: needinfo?(ehsan)
c/left/right/g ;-)

It varies whether nsFrame::GetChildFrameContainingOffset or nsTextFrame::GetChildFrameContainingOffset is being called depending on whether you click on the text, but almost to the *right* of the text, or completely to the *right* of the text.
From these posts, it sure looks like people are working on this, but the metadat at the top of the bug page says, "Assigned To: 	Nobody; OK to take it and work on it "
I'm working on it, but I can mostly do the debugging. Now I need some help to actually fix the problem. If I can get that help, I can assign it to myself. Otherwise, it might wait another 10 years ;-((
Note: The predecessor bug 250539 is from 2004.
And jorg k, we REALLY REALLY REALLY REALLY appreciate it too!!
The problem with losing the style after inserting an image (see comment #14) has been moved to bug 1140617.
See Also: → 1140617
Oops, after a refresh of my source, behaviour of nsFrame::HandlePress has changed, now returns early:
http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsFrame.cpp#2816.

Behaviour hasn't changed though. Click "far" left, font changes, click left/on, font is correct.
Maybe better to look further up in call stack, for example PresShell::HandlePositionedEvent.
Jorg K - re Comment 36 (Just noticed: Not only the font gets lost, also the font size can get lost. For example when replying to a message in a small font, the size gets bigger when clicking at the end after correcting something in the same line.)

When I'm typing and correct something, the font size stays the same at the correction, but when I go to the end of the line to continue my typing, the font size is smaller, not larger.
>When I'm typing and correct something, the font size stays the same at the correction, but when I go to the end of the line to continue my typing, the font size is smaller, not larger."

That's because there is a hidden </ font> tag at the end of the line (but before the spot where you are typing).  When you move away from the line to do the correction, then click back to the right of the line in white space, the cursor has now been placed OUTSIDE the end font tag, so you have thus "lost" what font had previously been specified.
CORRECTION TO PREVIOUS (dang we need an edit capability)

That's because there is a hidden </ font> tag at the end of the line (but AFTER = to the right of, the spot where you are typing).
maybe-the-one - re comments 47 & 48: thanks for the clarifying explanation.
(In reply to Jorg K from comment #38)
> Coming back to the suggestion from comment #25 and looking in
> nsFrame::HandlePress.
> 
> I traced it down into ns[Text]Frame::GetChildFrameContainingOffset with a
> call stack of:
> 
> nsFrame::GetChildFrameContainingOffset *or*
> nsTextFrame::GetChildFrameContainingOffset
> nsFrameSelection::GetFrameForNodeOffset
> nsFrameSelection::BidiLevelFromClick
> nsFrameSelection::HandleClick
> nsFrame::HandlePress
> 
> It varies whether nsFrame::GetChildFrameContainingOffset or
> nsTextFrame::GetChildFrameContainingOffset is being called depending on
> whether you click on the text, but almost to the left of the text, or
> completely to the left of the text.
> 
> When the nsTextFrame::GetChildFrameContainingOffsetAny version is called, it
> offset is calculated correctly and the font is right. When the
> nsFrame::GetChildFrameContainingOffset is called, the font is wrong. In the
> latter case the caret still appears behind the text.

Yes, this is what I was explaining in comment 22.  The frame on which this function is called depends on the frame receiving the click event; if you click on the text it's going to be a textframe, otherwise it's going to be the parent frame (in the normal case as in the test case you describe above) or anything else that is under the mouse cursor.

> Try it with a wide letter, like "m"!
> 
> So in case a "Frame" being hit instead of a "TextFrame", perhaps the program
> should look whether there is a text right before the caret once placed.

No, we shouldn't change the way that we hit-test to find out which frame was clicked.

As I said in comment 22, we should probably look into normalizing the selection, so that if the line ends in an inline frame containing a text frame, we should put the selection at the end of the text frame as opposed to at the end of the inline frame terminating the line.

But before we can do that, we need to investigate the behavior of other engines in this situation (as in, we should see where they put the selection.)  You need to create a test case which lets you click somewhere and have it dump out the place of the selection (which you can get through window.getSelection()).  The simplest way to create such a test case is to set it up to print the selection after 10 seconds or so.  Then you should document the behavior of IE/Chrome/Opera/Safari on that test case.  If they all agree on putting the selection where I described above (IIRC some of the engines did that the last time I tested this years ago) then we can look into changing our code.  Beware that it will probably take a significant amount of work adjusting everywhere in our code where we depend on the existing behavior, same in our existing tests.
Flags: needinfo?(ehsan)
Sadly I don't understand some of what you wrote. Let me see what I understood.

You're saying you want to check how other rendering engines behave. I ran the test from comment #37 on Chrome and IE. Both continue text entry with the font present on the first line, so their behaviour is what I would expect.

Next you talk about "test cases" where you click and dump out the selection. In this case, we do a single click, so the selection will have one collapsed range. I don't know which property of the Selection or Range you want to inspect, maybe selRange.[start|end]Container.nodeName.

I've stripped down the so-called Midas demo and added some information about the clicking when carrying out the text case from comment #37. Result:

FF: BODY, continues in the wrong font.
Chrome: #text, continues with the correct font.
IE: #text if you used <enter> between the lines, or BODY if you used <shift><enter>, continues with correct font in both cases.

Let me know what to do next. And please, clear instructions ;-)
Flags: needinfo?(ehsan)
More results:
Safari (5.1.7, had it on some old machine): Same behaviour as Chrome.
Opera (27.0, fresh install): Same behaviour as Chrome.
Attached file Alternate test, much simpler (obsolete) — 
Here is a much simplified test "alternate.htm". Results:
FF: DIV, wrong font
IE: DIV, correct font
Chrome and Opera: #text, correct font.
(In reply to Jorg K from comment #51)
> Sadly I don't understand some of what you wrote. Let me see what I
> understood.
> 
> You're saying you want to check how other rendering engines behave. I ran
> the test from comment #37 on Chrome and IE. Both continue text entry with
> the font present on the first line, so their behaviour is what I would
> expect.

That's not what I asked.  I want to know where they put the selection.  Attachment 8575104 [details] kind of does that but I was hoping to get the offsets as well, but with the results in comment 54, it's clear that the exiting engines don't all agree on a single behavior, which means that my original idea may turn out to be incompatible with the Web.  :/

What versions of these browsers did you test?  What about Safari?

Note that whether or not other engines have this bug is not relevant.

> Next you talk about "test cases" where you click and dump out the selection.
> In this case, we do a single click, so the selection will have one collapsed
> range. I don't know which property of the Selection or Range you want to
> inspect, maybe selRange.[start|end]Container.nodeName.

Yeah, startContainer and endContainer, plus startOffset and endOffset.

> I've stripped down the so-called Midas demo and added some information about
> the clicking when carrying out the text case from comment #37. Result:
> 
> FF: BODY, continues in the wrong font.
> Chrome: #text, continues with the correct font.
> IE: #text if you used <enter> between the lines, or BODY if you used
> <shift><enter>, continues with correct font in both cases.
> 
> Let me know what to do next. And please, clear instructions ;-)

The first step is to read and understand how GetContentOffsetsFromPoint() works, specifically the call to GetSelectionClosestFrame(), which is the function that decides what frame is marked as selected ultimately when a given frame receives a click event.

For example in attachment 8575104 [details] if you click to the right of the first line, we get to GetSelectionClosestFrameForLine() selecting the BRFrame at the end of the first line.  Now, if we want to collapse the selection to the text frame, it means that we need to skip the BRFrame.  So you should modify that logic to ignore BRFrame's if there is anything else on the same line before them (i.e., if there is something that we haven't yet looked at through the line iterator.)  Then you should verify that the change actually makes us collapse the selection at the end of the text frame (so for example startContainer and endContainer should both be the text node and startOffset/endOffset should both be 10 depending on the length of the text frame).  Then you should post the change to the try server and see what existing tests fail, and debug them to figure out why.  There will be two classes of test failures, one for tests that are relying on the specific behavior we're changing here, so you should adjust those tests accordingly.  Another set of test failures will hopefully catch the edge cases that we have not thought very carefully about yet.  We should probably discuss the test failures once you get to that stage.

Beware that this will probably be a lengthy process and it requires a lot of debugging skills and also being comfortable finding your way across a large C++ code base.  Good luck!
Flags: needinfo?(ehsan)
Results:
FF: start=DIV(2) - end=DIV(2)
IE 10 and 11 (current): start=DIV(2) - end=DIV(2)
Chrome 41 (current): start=#text(10) - end=#text(10)
Opera 28 (current): start=#text(10) - end=#text(10)
Safari 5.1.7 (old): start=#text(10) - end=#text(10)
Please confirm that you are happy to change FF's behaviour to be like Chrome, Opera and Safari.

I read the last section of your comment (quote: "... Good luck!") as a confirmation, but before you were rather careful saying (1):
> If they all agree on putting the selection where I described above (...)
> then we can look into changing our code
and (2)
> it's clear that the exiting engines don't all agree on a single behavior, which 
> means that my original idea may turn out to be incompatible with the Web.  :/

Regarding the first statement: They don't all agree, FF and IE differ, but if FF moved into the Chrome camp, only IE would be left. And who knows what the new "Spartan" browser will do.

Regarding the second statement: I thought the original idea from comment #22 was to move the selection into the text:
> My suspicion is that place is outside of the element that has the respective style
> for the font set.  We should probably look into normalizing the selection in 
> those cases to be inside the said element
Also in comment #50 you said:
> As I said in comment 22, we should probably look into normalizing the selection,
> so that if the line ends in an inline frame containing a text frame, we should
> put the selection at the end of the text frame as opposed to at the end of the inline 
> frame terminating the line.

I'm not sure how that would be (quote) "incompatible with the Web".

Once I have your confirmation I will go ahead.

For me, this is the single most annoying bug in Thunderbird which after 10 years should finally be resolved.
Flags: needinfo?(ehsan)
From someone who is 1% knowledgeable on this stuff...
since the problem is that the cursor is returned, after clicking outside on the line being returned to, to a place outside the last (hidden) tag...which I have always seen to be </ font>...could you not simply detect if that tag existed and place the cursor just before it?

"For me, this is the single most annoying bug in Thunderbird which after 10 years should finally be resolved."  AMEN, and thank you so much for working on it.
Attachment #8575104 - Attachment is obsolete: true
Attachment #8574843 - Attachment is obsolete: true
(In reply to Jorg K from comment #57)
> Please confirm that you are happy to change FF's behaviour to be like
> Chrome, Opera and Safari.
> 
> I read the last section of your comment (quote: "... Good luck!") as a
> confirmation, but before you were rather careful saying (1):
> > If they all agree on putting the selection where I described above (...)
> > then we can look into changing our code
> and (2)
> > it's clear that the exiting engines don't all agree on a single behavior, which 
> > means that my original idea may turn out to be incompatible with the Web.  :/

I cannot predict how this change will pan out in the wild before we try it.  It may very well be that we try this approach and it turns out that it breaks existing Web pages in a way that would cause us to revert the fix.  It is only by trying this out that we can know for sure.

> Regarding the first statement: They don't all agree, FF and IE differ, but
> if FF moved into the Chrome camp, only IE would be left. And who knows what
> the new "Spartan" browser will do.

Well, what I meant was that the browsers do not have a consistent behavior here, so it could be that some websites are relying on Gecko's exact behavior here somehow.  But like I said it's impossible to predict.

> Regarding the second statement: I thought the original idea from comment #22
> was to move the selection into the text:
> > My suspicion is that place is outside of the element that has the respective style
> > for the font set.  We should probably look into normalizing the selection in 
> > those cases to be inside the said element
> Also in comment #50 you said:
> > As I said in comment 22, we should probably look into normalizing the selection,
> > so that if the line ends in an inline frame containing a text frame, we should
> > put the selection at the end of the text frame as opposed to at the end of the inline 
> > frame terminating the line.
> 
> I'm not sure how that would be (quote) "incompatible with the Web".
> 
> Once I have your confirmation I will go ahead.
> 
> For me, this is the single most annoying bug in Thunderbird which after 10
> years should finally be resolved.

This affects more than just Thunderbird, and concerns behavior that is visible to Web pages.  Web content can rely on the behavior of the engine in a lot of ways, for example by expecting that Firefox puts the selection somewhere specific when you go to the end of line.  This is one of the reasons why fixing this bug is very difficult.
Flags: needinfo?(ehsan)
The more conservative approach would be not to change the selection behaviour but to maintain/re-establish the correct "type in state" after the click. That is what IE does: The DIV is selected, but typing continues in the "correct" font, see comment #54. Would you prefer this approach?

One could argue that clicking 10cm or 4" away from the text to the right should not select the text. On the other hand, if no <br> follows, then you can click far to the right and the text is still selected. Also for text in <p></p> you can click far to the right and still select the text. Note that links are not followed, even though the text is selected. That's the last example.

Personally I would fire any web designer who creates websites for one browser alone and who creates special code for somewhat far-fetched cases of clicking a text.
It's not going past static FrameTarget GetSelectionClosestFrameForLine ()
http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsFrame.cpp#3540
... nor static FrameTarget GetSelectionClosestFrameForBlock nor static FrameTarget GetSelectionClosestFrame.

Also, very little processing in nsFrame::HandlePress. As I said before, leaving there at line 2819:
http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsFrame.cpp#2819

These functions fire when you hover over or click into the URL field (or the search box), but NOT the rendered HTML.

I'm not afraid of a large C++ code base, but I need a place to start. As I said, five minutes of your time can possibly save me five hours or five days of "reverse engineering".
Flags: needinfo?(ehsan)
I've done a little debugging.

On every mouse move event we get this:
 >>> Entering PresShell::HandleEvent, frame=06EA9488
PresShell::HandleEvent calling nsLayoutUtils::GetEventCoordinatesRelativeTo
PresShell::HandleEvent calling FindFrameTargetedByInputEvent
PresShell::HandleEvent assigning frame = target, frame=1DD7F010
=== about to call HandlePositionedEvent, frame=1DD7F010
=== in PresShell::HandlePositionedEvent, mCurrentEventContent/Frame=0D066380/1DD7F010
<<< Leaving PresShell::HandleEvent right after HandlePositionedEvent
(BTW, none of the frame numbers change when you move the cursor around. Unless you reload the page, they are always the same numbers).

Surely somewhere in there it works out what is under the cursor, since when the cursor is over a link, things happen (cursor changes, link target is displayed). Sadly, I haven't yet found that code.

If the button is clicked and released, we additionally get calls to HandlePress and HandleRelease
 >>> Entering PresShell::HandleEvent, frame=06EA9488
PresShell::HandleEvent calling nsLayoutUtils::GetEventCoordinatesRelativeTo
PresShell::HandleEvent calling FindFrameTargetedByInputEvent
PresShell::HandleEvent assigning frame = target, frame=1DD7F010
=== about to call HandlePositionedEvent, frame=1DD7F010
=== in PresShell::HandlePositionedEvent, mCurrentEventContent/Frame=0D066380/1DD7F010
=== in nsFrame::HandlePress
<<< Leaving PresShell::HandleEvent right after HandlePositionedEvent

 >>> Entering PresShell::HandleEvent, frame=06EA9488
PresShell::HandleEvent calling nsLayoutUtils::GetEventCoordinatesRelativeTo
PresShell::HandleEvent calling FindFrameTargetedByInputEvent
PresShell::HandleEvent assigning frame = target, frame=1DD7F010
=== about to call HandlePositionedEvent, frame=1DD7F010
=== in PresShell::HandlePositionedEvent, mCurrentEventContent/Frame=0D066380/1DD7F010
=== in nsFrame::HandleRelease
<<< Leaving PresShell::HandleEvent right after HandlePositionedEvent

Here is the stack from the HandlePress call:
nsFrame::HandlePress(<args>) Line 2776    C++
nsFrame::HandleEvent(<args>) Line 2562    C++
nsPresShellEventCB::HandleEvent(<args>) Line 507    C++
mozilla::EventTargetChainItem::HandleEventTargetChain(<args>) Line 346    C++
mozilla::EventDispatcher::Dispatch(<args>) Line 634    C++
PresShell::DispatchEventToDOM(<args>) Line 8168    C++
PresShell::HandleEventInternal(<args>) Line 8064    C++
PresShell::HandlePositionedEvent(<args>) Line 7896    C++
PresShell::HandleEvent(<args>) Line 7689    C++

As I said before, GetSelectionClosestFrameForLine is not called. The following statements I placed into the corresponding functions in nsFrame.cpp are not reached:
printf ("=== in GetSelectionClosestFrameForLine\n");
printf ("=== in GetSelectionClosestFrameForBlock\n");
printf ("=== in GetSelectionClosestFrame\n");
printf ("=== in nsIFrame::GetContentOffsetsFromPoint\n");

So the idea of discarding the BRFrame in GetSelectionClosestFrameForLine is perhaps not viable.

Further suggestions appreciated. I'd really like to find the code that identifies the element under the cursor. Also, is there a way to dump out/print these data structures? How do you "print" an nsIFrame or an nsBlockFrame?

Also, please answer the first question in comment #60 which suggested to not change the selected frame but to tweak the "type in state" like IE does. IE selects the DIV, but typing happens in the font of the preceding text element in the line. That may be an alternate way to fix the "lost font" problem that doesn't change existing selection behaviour.
Any idea why TypeInState::NotifySelectionChanged is never called when clicking around in a <div contenteditable>?
http://mxr.mozilla.org/mozilla-central/source/editor/libeditor/TypeInState.cpp#75

Is that not a listener to listen to the selection changing? Maybe that has something to do with our problem(s) (thinking also of bug 1140617).
(In reply to Jorg K from comment #60)
> The more conservative approach would be not to change the selection
> behaviour but to maintain/re-establish the correct "type in state" after the
> click. That is what IE does: The DIV is selected, but typing continues in
> the "correct" font, see comment #54. Would you prefer this approach?

No.  TypeInState can only remember the properties set while the selection has not changed, so it is never the right fix for bugs that occur when you move the selection around in the document.

> One could argue that clicking 10cm or 4" away from the text to the right
> should not select the text. On the other hand, if no <br> follows, then you
> can click far to the right and the text is still selected. Also for text in
> <p></p> you can click far to the right and still select the text. Note that
> links are not followed, even though the text is selected. That's the last
> example.

Sorry, I'm not really sure what you mean here.

> Personally I would fire any web designer who creates websites for one
> browser alone and who creates special code for somewhat far-fetched cases of
> clicking a text.

Or here.  :-)

(In reply to Jorg K from comment #61)
> It's not going past static FrameTarget GetSelectionClosestFrameForLine ()
> http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsFrame.cpp#3540
> ... nor static FrameTarget GetSelectionClosestFrameForBlock nor static
> FrameTarget GetSelectionClosestFrame.
> 
> Also, very little processing in nsFrame::HandlePress. As I said before,
> leaving there at line 2819:
> http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsFrame.cpp#2819

Hmm, I wasn't guessing in comment 55, I actually ran this under the debugger, so I'm pretty sure you're doing something wrong.  Not sure what exactly...

> These functions fire when you hover over or click into the URL field (or the
> search box), but NOT the rendered HTML.

Hmm, maybe that's because you're attached to the wrong process?  Does the active tab have an underline on its title?  If yes, you're in e10s mode where we run the rendered content in a separate process.  If this is what's happening, try either attaching the debugger to the content process, or open a new non-e10s window and load the test case there.

> I'm not afraid of a large C++ code base, but I need a place to start. As I
> said, five minutes of your time can possibly save me five hours or five days
> of "reverse engineering".

I'm trying to help, comment 55 should give you everything you need to get started.

(In reply to Jorg K from comment #62)
> As I said before, GetSelectionClosestFrameForLine is not called. The
> following statements I placed into the corresponding functions in
> nsFrame.cpp are not reached:
> printf ("=== in GetSelectionClosestFrameForLine\n");
> printf ("=== in GetSelectionClosestFrameForBlock\n");
> printf ("=== in GetSelectionClosestFrame\n");
> printf ("=== in nsIFrame::GetContentOffsetsFromPoint\n");

This cannot possibly be true, I guarantee that.  It's either you being attached to the wrong process or something else...

> Further suggestions appreciated. I'd really like to find the code that
> identifies the element under the cursor.

Again, nsIFrame::GetContentOffsetsFromPoint().

> Also, is there a way to dump
> out/print these data structures? How do you "print" an nsIFrame or an
> nsBlockFrame?

Yes!  You can call nsIFrame::DumpFrameTree() on any frame in the frame tree to get a full dump of the frame tree (you can look up the specific frame you have by looking for its address in the frametree dump.)  If you're under gdb or lldb, there is a helper command called ft, which dumps the frametree, otherwise you can just call that function under the debugger as |myFrame->DumpFrameTree()|.

Also if you have a debug build, you can use the Layout Debugger under the Tools menu which gives you a GUI which dumps things such as the frame tree, the content (DOM) tree, etc.
Flags: needinfo?(ehsan)
You are correct, I was in "e10s" mode and there was another process "plugin-container". Now I switched that off. I had noticed that my debugging had become strange, but couldn't figure out why. So thank you!

Before considering changing the selection behaviour, I'd like to repeat my question, but I don't want to infuriate you ;-)

The problem in this bug and in the other bug 1140617 is, that a line is continued with a new element.

In this bug, someone clicks on a "DIV", since they click to the right of the text. The new text is inserted into a new element and loses the font. In the other bug, an image is inserted. After that the new text is inserted into a new element and loses the font. In both cases we could look in the inline frame and see whether there is a text element immediately before, whose attributes should be used.

Is this an option? If we did that, we would *not* have to change the selection behaviour and wear all the possible consequences. Changing the selection will fix this bug but not the other bug 1140617.

If this is not a viable solution, I can look further into changing the selection behaviour. My debugging works now:

Debug when clicking on the text:
>>> Entering nsFrame::HandlePress
=== in nsIFrame::GetContentOffsetsFromPoint
=== in GetSelectionClosestFrame
=== in nsFrame::GetSelectionClosestFrameForBlock
>>> Entering nsFrameSelection::HandleClick
=== before BidiLevelFromClick
>>> Endering nsTextFrame::GetChildFrameContainingOffset
=== nsTextFrame::GetChildFrameContainingOffset, offset=10
<<< Leaving nsTextFrame::GetChildFrameContainingOffset
=== after BidiLevelFromClick
*** TypeInState::NotifySelectionChanged, container=05EF8900, offset=10 <-- the text of length 10
<<< Leaving nsFrameSelection::HandleClick after TakeFocus
<<< Leaving nsFrame::HandlePress

When clicking to the right of the text:
>>> Entering nsFrame::HandlePress
=== in nsIFrame::GetContentOffsetsFromPoint
=== in GetSelectionClosestFrame
=== in nsFrame::GetSelectionClosestFrameForBlock
=== in nsFrame::GetSelectionClosestFrameForLine
=== in GetSelectionClosestFrame
=== in nsFrame::GetSelectionClosestFrameForBlock
>>> Entering nsFrameSelection::HandleClick
=== before BidiLevelFromClick
=== after BidiLevelFromClick
*** TypeInState::NotifySelectionChanged, container=066CF4C8, offset=2 <-- the DIV
<<< Leaving nsFrameSelection::HandleClick after TakeFocus
<<< Leaving nsFrame::HandlePress
Flags: needinfo?(ehsan)
I don't think it's reasonable to start traversing the DOM tree every time that we want to perform an editing operation to find the right styles/properties to use.  It's a lot of unnecessary work.  It should be a lot easier to normalize the selection in a sane way to prevent it from going into places where it would cause bugs like this, such as on a BR node when you click at the end of the line.

If this approach proves to be impossible because of Web compatibility we can always re-evaluate.
Flags: needinfo?(ehsan)
Skipping brFrames when determining the closest frame in a line. Can only skip if the brFrame isn't the only selectable non-empty frame in the line. Otherwise one couldn't click in that line at all.

Need to be careful since ranges can become empty during editing.
Attachment #8576888 - Flags: review?(ehsan)
No longer depends on: 1139524
Blocks: 1142879
Comment on attachment 8576888 [details] [diff] [review]
three line change to fix a 10 y/o problem ;-)

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

Good start, but this is still far from being ready for review.  Did you push this to the try server?  Please include the treeherder URL so that we can look at the test failures.

Thanks!
Attachment #8576888 - Flags: review?(ehsan)
Sorry, you need to help me out here. Never done it before.
I will get the access rights as discussed on IRC.
Flags: needinfo?(ehsan)
(In reply to Archaeopteryx [:aryx] from comment #70)
> Pushed to Try:
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=bb676357b6c9

Canceled this as it misses the most important part of the tests, mochitests.  Repushed:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=8b34acaf40c3
Flags: needinfo?(ehsan)
When you have a minute, could you please look at the results or instruct me, how to interpret them.
Flags: needinfo?(ehsan)
The try results look good to me, so you just need to include an automated regression test (mochitest) and you can ask a reviewer to approve it to be included in Firefox.  You want to add a file patterned off something like this: <https://dxr.mozilla.org/mozilla-central/source/layout/generic/test/test_bug970363.html>  You could base it off one of the tests you attached to this bug, but instead of asking the user to click, you have to synthesize a click using a function from EventUtils.js <https://dxr.mozilla.org/mozilla-central/source/testing/mochitest/tests/SimpleTest/EventUtils.js>, possibly synthesizeMouse.  And instead of doing alert(), do something like is(myVariable, "#text", "selection should be in text node").  You have to add your test's name to the file layout/generic/test/mochitest.ini, and then you can run it with "./mach mochitest-plain layout/generic/test/test_bug756984.html".  It should fail before the patch is applied, and pass after the patch is applied.

Thanks a ton for working on this!  If you want feedback from me on if your test looks good before asking for review, please feel free to ask me via the "feedback" flag when uploading your patch.  I'm probably less busy than the reviewers.  :)
@Aryeh: Yor comment really surprises me given the following:

In comment #68 Ehsan said:
> Good start, but this is still *far* from being ready for review.

In comment #60 Ehsan said:
> This affects more than just Thunderbird, and concerns behavior that is visible to Web pages. 
> Web content can rely on the behavior of the engine in a lot of ways, for example by expecting
> that Firefox puts the selection somewhere specific when you go to the end of line.
> This is one of the reasons why fixing this bug is *very difficult*.

Also, we need to keep the sister bug 1140617 in mind. In bug 1140617 comment #8 Ehsan said:
> We still don't have a complete fix for bug 756984.  The stuff that you're working on is
> just the tip of the *iceberg*.  Can you please just *trust me* on this and wait for bug
> 756984 to be finished before worrying about this?  Thanks!

To me, it's just a three line change, and if you want, with a test case. Obviously none of the other tests checks the selection after the click, so I can certainly add such a test following your suggestions.

However, before I start, I want to make 100% sure that we're all on the same page. I have the impression that Ehsan has something grander in mind.
Flags: needinfo?(ayg)
Oops, I missed that part of comment #68 -- should have read more carefully.  I don't know what more there is to do, since it passed a try run.  But that's why Ehsan is in charge and not me.  ;)  In any event, you will need a test at the end of the day, so you could still go ahead and write it now.
Flags: needinfo?(ayg)
Sorry for the delay here, somehow I failed to note the needinfo flag!

As Aryeh said, the current try results look great!  And it seems like fixing this is going to be much easier than I thought after all.  :-)  The next step is to ensure that the selection is put in the exact same place through other means of moving to the end of a line.  For example, one such scenario that you need to investigate is if the caret is in the middle of the line, and you press End or Ctrl/Cmd+E.  Another example is when you have multiple lines, and you place the caret at the end of a longer line and use up/down to navigate to the end of a shorter line.  Same thing with using PageUp/PageDown in the same situation but with more lines of text obviously.  Some of these code paths can end up going through the same code and some may already do the right thing but you need to test each one of them to make sure.  The main entry point for all of these is nsFrameSelection::MoveCaret, where we end up calling nsIFrame::PeekOffset <https://dxr.mozilla.org/mozilla-central/source/layout/generic/nsSelection.cpp#969> which then gets us through the same machinery as you've seen before.

Another thing that would be useful to test (but I'm pretty sure that it would work correctly based on the code changes) is what happens in both of these cases when you have several inline elements nested underneath each other, such as:

<span><b><i><font>foo bar on the same line</font></i></b></span>

You should ensure that the selection is again put at the end of the text frame in this case, so that when the user starts to type again, the new text would have the correct styles imposed by all of those inline elements.

Once you've verified and fixed all of these cases, the next step would be to write a unit test as Aryeh mentioned that examines this behavior and ensures that we don't end up regressing it in the future.  In addition to simulating a mouse click, you'd need to use synthesizeKey, for example, synthesizeKey("VK_UP") for simulating the "Up" key, and so on.

And thanks again for working on this!
Flags: needinfo?(ehsan)
For the record, from black-box testing of WebKit a few years ago, it looked like it normalized the selection after every change.  Even if you called .addRange(), it copied the range and then stuck the selection endpoints inside a nearby text node if available, etc.  I think it's taking things too far to change script-specified selections, but the right way to do this is probably to have some sort of helper method in Selection like NormalizePoint(nsINode*, int32_t) and call it before every user-initiated selection change.  We might want to disallow other types of user-created selections from occurring in the future, although my brain is too rusty to supply any.

Do we want to allow a selection like <b>foo</b>{}<i>bar</i>, with the selection collapsed in between the <b> and <i>?  IIRC, WebKit in my testing forced this to be <b>foo[]</b><i>bar</i> (always making it on the previous text node).  A ten-second test in WordPad suggests this is the right thing to do.

I don't think any of this has to be in the scope of the current bug, though.
(In reply to :Aryeh Gregor from comment #77)
> For the record, from black-box testing of WebKit a few years ago, it looked
> like it normalized the selection after every change.  Even if you called
> .addRange(), it copied the range and then stuck the selection endpoints
> inside a nearby text node if available, etc.  I think it's taking things too
> far to change script-specified selections

Yeah, I think that's a bit too aggressive, and probably would cause authors to have to do insane hacks if they actually want to put the selection where the engine doesn't think is appropriate.

> but the right way to do this is
> probably to have some sort of helper method in Selection like
> NormalizePoint(nsINode*, int32_t) and call it before every user-initiated
> selection change.  We might want to disallow other types of user-created
> selections from occurring in the future, although my brain is too rusty to
> supply any.

Well, most of the code handling selection changes actually lives outside Selection.  ;-)  Depending on where we need to modify the behavior, having this kind of helper may or may not help...  See Jorg's patch for example, I think the way he modifies the existing logic is cleaner than going with the current logic and then fixing it up elsewhere.

> Do we want to allow a selection like <b>foo</b>{}<i>bar</i>, with the
> selection collapsed in between the <b> and <i>?  IIRC, WebKit in my testing
> forced this to be <b>foo[]</b><i>bar</i> (always making it on the previous
> text node).  A ten-second test in WordPad suggests this is the right thing
> to do.

Yes, that makes sense to me.  Follow-up bug perhaps?
BTW, Jorg, since your fix at least improves the situation where someone clicks at the end of the line, I'd be fine with landing it with a test case in this bug if you prefer to move the rest of the investigation and the fixes into another bug.  Whichever way you prefer is fine.  :-)
Thanks for the suggestions made in comment #78. I'm testing with

<span><b><i><font face=Courier new">foo bar on the same line</font></i></b></span><br>
<span><b><i><font face="Arial">foo bar on the same line</font></i></b></span><br>
<strong>foo bar on the same line</strong><br>
This is <s>strikethrough</s><br>
25 m<sup>2</sup><br>
H<sub>2</sub>SO<sub>4</sub><br>
and some <u>underline</u><br>
foobar

Moving around with the cursor already works with the current version of FF, including moving from a long line to a short one and getting to the end this way.

Pressing the "End" button is sadly NOT fixed by the patch. Clicking at the start of a line and then pressing "left arrow" to get to the end of the previous line is also NOT fixed by the patch.

So the next step is to fix the two mentioned cases: End button and "left arrow" at the start of the line. BTW, these two cases do work in Chrome and IE already.

I'd like to make sure that all positioning actions end up with the same correct result, clicking is only one of them.
Cool, so yeah, a fix to those two additional cases + the unit tests is probably all that we'd need here!
Here's a new patch. This fixes click beyond end of line *and* "end" key. Still open: Left arrow at the start of the subsequent line.
Assignee: nobody → mozilla
Attachment #8576888 - Attachment is obsolete: true
Can you please push this to the try server for me and tell me the exact steps to do it. I was reading
https://wiki.mozilla.org/ReleaseEngineering/TryServer#How_to_push_to_try and
http://trychooser.pub.build.mozilla.org/
I don't know which tests we need to run here and on which platforms.

I'm also struggling with Mercurial. This
https://developer.mozilla.org/en/docs/Mercurial_FAQ
apparently is out of date. I got to the "hg qnew", the patch is enclosed. I tried "hg commit" but got "abort: cannot commit over an applied mq patch". Hmm.
Attachment #8582014 - Attachment is obsolete: true
Flags: needinfo?(ehsan)
A good try line to use by default for editor changes is:
  try: -b do -p all -u all[x64] -t none
This will build on all platforms (to detect compile errors), and will run tests only on 64-bit Linux (to avoid wasting resources).  If there might be platform-specific test failures, remove the "[x64]", but I don't think you need to do that here.

For Mercurial -- if you're using mq (which you should), you don't ever want to do "hg commit".  The patch you've attached is exactly right.

Here are the try results for you: https://treeherder.mozilla.org/#/jobs?repo=try&revision=fd11f71e3daa
Okay, this caused some try failures.  One of them is an unexpected pass in richtext2, which is good!  It means you just have to update the test so it knows we're supposed to pass now.  In the case of richtext2, you want to edit the file editor/libeditor/tests/browserscope/lib/richtext2/currentStatus.js and remove the appropriate lines (this is different from how most tests need to be updated).  To check that it worked, run this (which may take a while and you have to keep the browser window focused):
  ./mach mochitest-plain editor/libeditor/tests/browserscope
If you're on Linux, you should be able to install the appropriate package for the xvfb-run command and use this instead so it runs without creating a window you have to keep in focus:
  xvfb-run ./mach mochitest-plain editor/libeditor/tests/browserscope

The other failure I see is  layout/generic/test/test_movement_by_characters.html, which you need to look at.  It could be your patch has a problem, or it could be the test needs to be updated.  If you're having trouble, please feel free to ask for help!
The failure in editor/libeditor/tests/test_selection_move_commands.xul also needs looking at.  Otherwise, looks good!
Looks like Aryeh answered your questions (thanks Aryeh!)

I realized that I forgot about another case that we need to test.  br frames are not the only reason for a line ending, we can also get line breaks at block boundaries, for example: <div>block 1<br><span>new line here</span><div>block2</div></div>  We need to ensure that the selection ends up on the text node when you put the selection at the end of "new line here" using any of the methods mentioned before.  Again, please note that some of those cases may already work properly and/or be fixed by your patch, but it's worth testing.
Flags: needinfo?(ehsan)
Comment on attachment 8582360 [details] [diff] [review]
Patch to fix all three problems: Click, "end" key, left arrow from start of previous line

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

Some feedback on your patch so far.

::: layout/generic/nsFrame.cpp
@@ +3555,3 @@
>    for (int32_t n = aLine->GetChildCount(); n;
>         --n, frame = frame->GetNextSibling()) {
> +    // Bug 756984: Skip brFrames. Can only skip if the line contains at least one selectable and non-empty frame before

Nit: you don't need to mention bug numbers in comments.  This information is available through hg/git blame.

@@ +3555,4 @@
>    for (int32_t n = aLine->GetChildCount(); n;
>         --n, frame = frame->GetNextSibling()) {
> +    // Bug 756984: Skip brFrames. Can only skip if the line contains at least one selectable and non-empty frame before
> +    if (!SelfIsSelectable(frame, aFlags) || frame->IsEmpty() || (canSkipBr && frame->GetType() == nsGkAtoms::brFrame))

Nit: please wrap lines at 80 characters.  For example, according to our coding style, this condition should be written as:

  if (!SelfIsSelectable(frame, aFlags) || frame->IsEmpty() ||
      (canSkipBr && frame->GetType() == nsGkAtoms::brFrame))

@@ +6801,5 @@
>          for (int32_t count = lineFrameCount; count;
>               --count, frame = frame->GetNextSibling()) {
>            if (!frame->IsGeneratedContentFrame()) {
> +            // Bug 756984: When jumping to the end of the line with the "end" key, skip over brFrames
> +            if (endOfLine && lineFrameCount > 1 && frame->GetType() == nsGkAtoms::brFrame) continue;

Nit: again, please adjust the whitespace here.  Also, please wrap if bodies in braces.

@@ +7090,5 @@
> +      nsIFrame *currentBlockFrame, *currentFirstFrame;
> +      nsRect usedRect;
> +      int32_t currentLine = nsFrame::GetLineNumber(traversedFrame, aScrollViewStop, &currentBlockFrame);
> +      nsAutoLineIterator it = currentBlockFrame->GetLineIterator();
> +      it->GetLine(currentLine, &currentFirstFrame, &lineFrameCount, usedRect);

There is already code which gets the line frame count earlier in this function.  Please refactor it out of the else block it currently lives in and just use lineFrameCount from that code here.  That already takes care of checking the return value of GetLine for errors too.  Also, I think you want to check atLineEdge instead of aJumpLines here.
I try not to do this very often anymore (useless comments in bugs), but...

I just wanted to say THANK YOU to Jorg, Ehsan and Aryeh (and anyone else involved) working on this bug!!

I must say, after the scare announcement from Mozilla about not directly funding Thunderbird anymore, I was worried, but it seems that there is much more movement on lots of older bugs (and new ones) lately, and it is extremely gratifying to see people stepping up.

I just wish I had some coding skills (and time to use them)...

So - thanks to all!!!
(In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from comment #88)
> Nit: you don't need to mention bug numbers in comments.  This information is
> available through hg/git blame.
There are many bug numbers in the code (including some that you entered for bug 596506), so this policy must have changed recently.

> Also, please wrap if bodies in braces.
Again, the policy seems to have changed here since there are many conditions of this style:
if (!range.content)
  return NS_ERROR_FAILURE;

> There is already code which gets the line frame count earlier in this
> function.  Please refactor it out of the else block it currently lives in
> and just use lineFrameCount from that code here.  That already takes care of
> checking the return value of GetLine for errors too.  Also, I think you want
> to check atLineEdge instead of aJumpLines here.
I disagree. I did what you suggest, but it didn't work. The
it->GetLine(thisLine, &firstFrame, &lineFrameCount, nonUsedRect);
ealier on gets the details for the previous line where the left arrow was pressed.
We then move on to the line before. The next
it->GetLine(currentLine, &currentFirstFrame, &lineFrameCount, usedRect);
is operating on the line we've just moved to, the one where we want to skip the brFrame.
The only thing which would be a bit cleaner is not to re-use the "it" variable but to declare another one.

I agree, I do need "atLineEdge".
Flags: needinfo?(ehsan)
(In reply to Jorg K from comment #90)
> (In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from
> comment #88)
> > Nit: you don't need to mention bug numbers in comments.  This information is
> > available through hg/git blame.
> There are many bug numbers in the code (including some that you entered for
> bug 596506), so this policy must have changed recently.

Well you're repeating it multiple times in the same file.  The bug number is really not adding any useful information in this case.

> > Also, please wrap if bodies in braces.
> Again, the policy seems to have changed here since there are many conditions
> of this style:
> if (!range.content)
>   return NS_ERROR_FAILURE;

We unfortunately don't consistently adhere to the coding style, but we should for new code that we're adding.

> > There is already code which gets the line frame count earlier in this
> > function.  Please refactor it out of the else block it currently lives in
> > and just use lineFrameCount from that code here.  That already takes care of
> > checking the return value of GetLine for errors too.  Also, I think you want
> > to check atLineEdge instead of aJumpLines here.
> I disagree. I did what you suggest, but it didn't work. The
> it->GetLine(thisLine, &firstFrame, &lineFrameCount, nonUsedRect);
> ealier on gets the details for the previous line where the left arrow was
> pressed.
> We then move on to the line before. The next
> it->GetLine(currentLine, &currentFirstFrame, &lineFrameCount, usedRect);
> is operating on the line we've just moved to, the one where we want to skip
> the brFrame.
> The only thing which would be a bit cleaner is not to re-use the "it"
> variable but to declare another one.

Oh yeah, good point.  So please make sure you check the return value of GetLine like above.
Flags: needinfo?(ehsan)
(In reply to :Aryeh Gregor from comment #84)
> A good try line to use by default for editor changes is:
>   try: -b do -p all -u all[x64] -t none

OK, but how do I send the patch to the try server? I have my "level 1" access rights and I believe SSH is set up correctly. I tried
hg push -f ssh://mozilla@jorgk.com@hg.mozilla.org/try/ *before* coming across the "hg qnew" command.
It returned: "No changes found" or words to that extent. I haven't tried after the "hg qnew".
So what is the exact sequence of commands? BTW, I'm on Windows 7. 30 seconds of your time save me three hours of (very frustrating) research (since I want to focus on the problem and not the infrastructure).

As for the test results: Mochitests 4, 5 and oth failed. I just had a very brief look:
oth says:
editor/libeditor/tests/test_selection_move_commands.xul | node after cmd_selectEndLine - got [object Text], expected [object HTMLBodyElement]
YES! Indeed, we want the text. I will need to change the test.
5 says:
layout/generic/test/test_movement_by_characters.html | Left movement broken in H <br>K - got [object Text], expected [object HTMLDivElement]
Same thing, we want the text.
4 says:
TEST-UNEXPECTED-PASS a few times, so let's how that's a change for the better. However, this stuff worries me (3x crashed, 1x time out)
965721 Intermittent test_bug409604.html, test_bug719533.html, test_richtext2.html, test_bug412567.html | application crashed [@ imgStatusTracker::RecordCancel()]
969526 Intermittent test_richtext.html,test_richtext2.html,test_bug436801.html | application crashed [@ KERNELBASE.dll + 0x89ae4] (ABORT: Should have mProgressTracker until we create mImage: 'mProgressTracker')
1129538 Intermittent test_draggableprop.html,test_richtext2.html | application crashed [@ mozalloc_abort(char const*)] after "ABORT: Should have given mProgressTracker to mImage: '!mProgressTracker', file /image/src/imgRequest.cpp, line 149"
1142900 Intermittent test_richtext2.html | application timed out after 330 seconds with no output

Any comments on these crashes and time-outs?

P.S.: Would you be able to let me know your time-zone so I know when not to expect feedback ;-)
(In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from comment #87)

> I realized that I forgot about another case that we need to test.  br frames
> are not the only reason for a line ending, we can also get line breaks at
> block boundaries, for example: <div>block 1<br><span>new line
> here</span><div>block2</div></div>

This works already.
My patch only tries to skip brFrames. In this example there is no brFrame at the end of "new line here".
In .hg/hgrc (or ~/.hgrc) set up the path alias:

mc-try = ssh://mozilla@jorgk.com@hg.mozilla.org/try

Then
 hg qgoto your-patch
 hg qnew -m "try: -b do -p all -u all[x64] -t none" try
 hg push -f -r tip mc-try && hg qpop && hg qdelete try

Or at least that what I do.
(In reply to Jorg K from comment #92)
> OK, but how do I send the patch to the try server? I have my "level 1"
> access rights and I believe SSH is set up correctly. I tried
> hg push -f ssh://mozilla@jorgk.com@hg.mozilla.org/try/ *before* coming
> across the "hg qnew" command.
> It returned: "No changes found" or words to that extent. I haven't tried
> after the "hg qnew".
> So what is the exact sequence of commands? BTW, I'm on Windows 7. 30 seconds
> of your time save me three hours of (very frustrating) research (since I
> want to focus on the problem and not the infrastructure).

If you have some changes you want to commit, and no patches currently applied:

  hg qnew mypatchname # This saves your changes to an mq patch
  hg qnew -m "try: -b do -p all -u all[x64] -t none" try # Make a second patch with the try line
  hg push -f mc-try # Make sure you have Magnus' line in .hg/hgrc
  hg qdelete try # Delete the empty try patch

Note: this will leave you with a patch in mq called "mypatchname" with your changes.  If you make any new changes to the patch and want to submit to try again, do the same, except instead of the first line, do "hg qref" to refresh the existing patch instead of making a new one.  To view your existing patches, try "hg qser -s"; to move around, you can use "hg qpop" and "hg qpush" and "hg qgoto"; for more help, try "hg help mq" or "hg help commandname".

If you see any weird changes that only appear on some platforms and don't look related to your changes, they're probably random (intermittent) failures that are unrelated to your changes.  You can usually spot changes that you really caused because they'll show up on all platforms, and look related to your changes.  If you're not sure, you can ask us, or ask on IRC for a quicker response.
  
> However, this stuff worries me (3x crashed, 1x time out)
> 965721 Intermittent test_bug409604.html, test_bug719533.html,
> test_richtext2.html, test_bug412567.html | application crashed [@
> imgStatusTracker::RecordCancel()]
> 969526 Intermittent
> test_richtext.html,test_richtext2.html,test_bug436801.html | application
> crashed [@ KERNELBASE.dll + 0x89ae4] (ABORT: Should have mProgressTracker
> until we create mImage: 'mProgressTracker')
> 1129538 Intermittent test_draggableprop.html,test_richtext2.html |
> application crashed [@ mozalloc_abort(char const*)] after "ABORT: Should
> have given mProgressTracker to mImage: '!mProgressTracker', file
> /image/src/imgRequest.cpp, line 149"
> 1142900 Intermittent test_richtext2.html | application timed out after 330
> seconds with no output
> 
> Any comments on these crashes and time-outs?

Don't worry about those.  These tests fail sometimes at random -- it's not connected to your changes.

> P.S.: Would you be able to let me know your time-zone so I know when not to
> expect feedback ;-)

I'm UTC+0200, and switching to UTC+0300 this Thursday night.  Last I was aware, Ehsan lives in eastern Canada and so should be UTC-0400.
Here's the updated code according to the review.
I will review the test failures next.
Attachment #8582360 - Attachment is obsolete: true
Attachment #8582633 - Flags: feedback?(ehsan)
These tests are a little mysterious. I wanted to look at
editor/libeditor/tests/test_selection_move_commands.xul first. Sadly
mach  mochitest-plain editor/libeditor/tests/test_selection_move_commands.xul
doesn't work? I've run single tests before, but they weren't XUL files.
So I ran
mach  mochitest-plain editor/libeditor/tests/ and got:

7043 INFO TEST-UNEXPECTED-PASS | editor/libeditor/tests/browserscope/test_richte
xt2.html | Browserscope richtext2 selection: S-Proposed-SM:e.f.lb_BR.BR-1_SC-1-d
M - 1 should equal 1
TEST-INFO | expected FAIL
7044 INFO TEST-UNEXPECTED-PASS | editor/libeditor/tests/browserscope/test_richte
xt2.html | Browserscope richtext2 selection: S-Proposed-SM:e.f.lb_BR.BR-1_SC-1-b
ody - 1 should equal 1
TEST-INFO | expected FAIL
7045 INFO TEST-UNEXPECTED-PASS | editor/libeditor/tests/browserscope/test_richte
xt2.html | Browserscope richtext2 selection: S-Proposed-SM:e.f.lb_BR.BR-1_SC-1-d
iv - 1 should equal 1
TEST-INFO | expected FAIL
7046 INFO TEST-UNEXPECTED-PASS | editor/libeditor/tests/browserscope/test_richte
xt2.html | Browserscope richtext2 selection: S-Proposed-SM:e.f.lb_BR.BR-1_SM-1-d
M - 1 should equal 1
TEST-INFO | expected FAIL
7047 INFO TEST-UNEXPECTED-PASS | editor/libeditor/tests/browserscope/test_richte
xt2.html | Browserscope richtext2 selection: S-Proposed-SM:e.f.lb_BR.BR-1_SM-1-b
ody - 1 should equal 1
TEST-INFO | expected FAIL
7048 INFO TEST-UNEXPECTED-PASS | editor/libeditor/tests/browserscope/test_richte
xt2.html | Browserscope richtext2 selection: S-Proposed-SM:e.f.lb_BR.BR-1_SM-1-d
iv - 1 should equal 1
TEST-INFO | expected FAIL
7049 INFO TEST-UNEXPECTED-FAIL | editor/libeditor/tests/test_bug1068979.html | S
hould have four SMP characters - got 𝐀𝐀, expected 𝐀𝐀𝐀𝐀
7050 INFO TEST-UNEXPECTED-FAIL | editor/libeditor/tests/test_bug1068979.html | T
hree complete characters should remain - got 𝐀, expected 𝐀𝐀𝐀
7051 INFO TEST-UNEXPECTED-FAIL | editor/libeditor/tests/test_bug1068979.html | T
wo complete characters should remain - got , expected 𝐀𝐀
7052 INFO TEST-UNEXPECTED-FAIL | editor/libeditor/tests/test_bug1068979.html | O
nly one complete SMP character should remain - got , expected 𝐀
7053 INFO TEST-UNEXPECTED-FAIL | editor/libeditor/tests/test_bug410986.html | Co
ntent should be pasted in plaintext format - got , expected green text
7054 INFO TEST-UNEXPECTED-FAIL | editor/libeditor/tests/test_bug410986.html | Ti
med out while polling clipboard for pasted data. - expected PASS
7055 INFO TEST-UNEXPECTED-FAIL | editor/libeditor/tests/test_bug410986.html | Fa
iled to copy the second item to the clipboard - expected PASS
7056 INFO TEST-UNEXPECTED-FAIL | editor/libeditor/tests/test_bug551704.html | Ti
med out while polling clipboard for pasted data. - expected PASS
SUITE-END | took 376s

So we got a few TEST-UNEXPECTED-PASS, we already new that.
No sight of test_selection_move_commands.xul, but there are other failures that don't appear in
https://treeherder.mozilla.org/#/jobs?repo=try&revision=fd11f71e3daa

Yes, I'm running the text on Windows 7, not on Linux, but still.
Flags: needinfo?(ayg)
Comment on attachment 8582686 [details] [diff] [review]
Easy fix for test due to new selection behaviour (layout/generic)

This fixes the failed test in layout/generic. More to come.
Attachment #8582686 - Attachment description: Easy fix for test due to new selection behaviour → Easy fix for test due to new selection behaviour (layout/generic)
Removed tests which now no longer fail due to changed selection behaviour.

I'd appreciate some help with the stuff from comment #97:
How do I run test_selection_move_commands.xul separately and what do I make of the additional unexpected failures?
(In reply to Jorg K from comment #97)
> These tests are a little mysterious. I wanted to look at
> editor/libeditor/tests/test_selection_move_commands.xul first. Sadly
> mach  mochitest-plain editor/libeditor/tests/test_selection_move_commands.xul
> doesn't work? I've run single tests before, but they weren't XUL files.

That test is part of the mochitest-chrome test suite, so you can use either mach mochitest-chrome, or even better, mach mochitest (which figures out which mochitest variant a test lives in.)

(The way you can know what test suite a test file belongs to is to look for its name in the .ini files in the same directory, for example, mochitest.ini and chrome.ini for mochitest-plain and mochitest-chrome, respectively.)
Comment on attachment 8582633 [details] [diff] [review]
Patch to fix all three problems: Click, "end" key, left arrow from start of previous line (updated coding style)

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

This looks fine to me, and in fact I think it's ready for roc to review!
Attachment #8582633 - Flags: feedback?(ehsan) → review?(roc)
Comment on attachment 8582701 [details] [diff] [review]
Easy fix for test due to new selection behaviour (richtext2)

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

Fantastic!  r=me.
Attachment #8582701 - Flags: review+
Attachment #8582686 - Flags: review+
Thanks, the "mach mochitest-chrome" worked, I will supply a fixed test tomorrow (now after 12 AM here). I assume I can just push a "unified" patch with the code and the updated tests to the try server.
This is the last of the updated tests.
Pushed to try server (thanks guys for the support!):
https://treeherder.mozilla.org/#/jobs?repo=try&revision=9782fa678cd1

Aryeh: Please help me review the results.

Actually, I followed what was suggested for the try:
try: -b do -p all -u all[x64] -t none

Just out of interest: Why build on all platforms and then just execute on Linux? I mean, why build it in the first place? Just to see that it compiles? If it already compiled once, and I only rerun tests, that doesn't make too much sense.

Further steps: If this try run goes well, the only thing missing is a test for the changes I made. Two scenarios are already covered by the existing tests which I had to change: the "moving around with the left arrow" and the "jumping to the end of the line with a key stroke". What's missing is the "clicking beyond the end of the line". The first try run in comment #71 was done with only the code to fix the clicking. No tests failed, so there wasn't a test for this, so it needs to be added now.
(In reply to Jorg K from comment #106)
> Pushed to try server (thanks guys for the support!):
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=9782fa678cd1

Treeherder isn't loading for me right now, but someone else who looked said it seemed fine.

> Just out of interest: Why build on all platforms and then just execute on
> Linux? I mean, why build it in the first place? Just to see that it
> compiles? If it already compiled once, and I only rerun tests, that doesn't
> make too much sense.

Yes, to see that it compiles.  Different platforms use different compilers, and maybe sometimes different compiler versions, and they treat different things as errors.  Compiling the code on all platforms is usually a good resources-safety tradeoff.  (Ideally all tests would be run on all platforms, but it overloads the try servers.)

> Further steps: If this try run goes well, the only thing missing is a test
> for the changes I made. Two scenarios are already covered by the existing
> tests which I had to change: the "moving around with the left arrow" and the
> "jumping to the end of the line with a key stroke". What's missing is the
> "clicking beyond the end of the line". The first try run in comment #71 was
> done with only the code to fix the clicking. No tests failed, so there
> wasn't a test for this, so it needs to be added now.

Sounds good!
Flags: needinfo?(ayg)
Is it worth running it on other systems as well since I found some - most likely unrelated - problems (see comment #97) when running it locally on Windows 7? Or are these known problems that always fail?
Try running the tests locally without your patches if you want to be sure (hg qpop -a will get rid of them if you're using mq).  If they fail even without your patches, don't worry about it.  In theory all tests should work on all supported platforms and configurations, but in practice some small fraction will break on some systems for various reasons, and in practice we only require that they work on the try servers.
Attached file Test case (obsolete) — 
Here is the test case. Forgive me if the JS is bad, I've never written any, just copied bits and pieces around.
Two questions:
Where in the mochitest.ini do I put my new test? I put it right at the front since I don't understand the syntax of "skip-if". Does that skip the next line? Perhaps you can suggest a line where it should go. Or say: After such and such.

In the test I use a <span>123</span> to get offset 3 when clicking behind it. Without the span, I get offset 4, which I find surprising, or clicking at the front gives offset 1 instead of 0. FF 36 does the same, so I guess it's right, but I'd like to understand why 4 and not 3, or 1 and not 0. Why does the span make a difference?

Anyway, the tests are passed. Without my changes, the first test fails as expected.

As I said in comment #106: There are already tests for hitting the "end" key and navigating with the arrow keys, so this test should be the only one we need additionally.
Attachment #8583259 - Flags: feedback?(ehsan)
(In reply to Jorg K from comment #110)
> Where in the mochitest.ini do I put my new test? I put it right at the front
> since I don't understand the syntax of "skip-if". Does that skip the next
> line? Perhaps you can suggest a line where it should go. Or say: After such
> and such.

Just add a line that says
  [test_bug756984.html]
(assuming that's the file's name).  Put it right before the next test line, like "[test_bug784410.html]" or whatever.  The "skip-if", "support-files", etc. lines apply to the preceding test file, so don't put it before one of those lines.  (This is confusing.  I looked it up in the online docs: https://ci.mozilla.org/job/mozilla-central-docs/Tree_Documentation/build/buildsystem/test_manifests.html)

> In the test I use a <span>123</span> to get offset 3 when clicking behind
> it. Without the span, I get offset 4, which I find surprising, or clicking
> at the front gives offset 1 instead of 0. FF 36 does the same, so I guess
> it's right, but I'd like to understand why 4 and not 3, or 1 and not 0. Why
> does the span make a difference?

You have a newline at the beginning of the div.  That's the first character in the text node (although it doesn't normally render).  If you did

  <div id='div1'>123<br>456<br></div>

all on one line, the offsets would be as you expected even without the span.

> Anyway, the tests are passed. Without my changes, the first test fails as
> expected.
> 
> As I said in comment #106: There are already tests for hitting the "end" key
> and navigating with the arrow keys, so this test should be the only one we
> need additionally.

Sounds great!
I'll revise the test and post a new one in a second.
Attached patch Test case (revised) (obsolete) — — Splinter Review 
If this is good to go, I'll submit another "unified" patch (code + all tests), if necessary.
Attachment #8583259 - Attachment is obsolete: true
Attachment #8583259 - Flags: feedback?(ehsan)
Attachment #8583354 - Flags: review?(ehsan)
Comment on attachment 8583354 [details] [diff] [review]
Test case (revised)

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

Looks like a great start!  Did you intend to test the rest of the cases in a separate patch?

::: layout/generic/test/test_bug756984.html
@@ +13,5 @@
> +<body>
> +<a target="_blank" href="https://bugzilla.mozilla.org/show_bug.cgi?id=756984">Mozilla Bug 756984</a>
> +<p id="display"></p>
> +<div id="content" style="display: none">
> +</div>

Since you mentioned you're new to writing tests, I'll add some bits about how this stuff works here.  I hope that helps!

The above is basically boilerplate that we include in most mochitests.  The only parts that are really required are the two script elements that load the SimpleTest and event synthesis APIs.

@@ +25,5 @@
> +
> +    /** Test for Bug 756984 **/
> +    /** We test that clicking beyond the end of a line terminated with <br> selects the preceding text, if any **/
> +
> +    SimpleTest.waitForExplicitFinish();

This is used to tell the test harness to not stop the test once the page load finishes (which is the default.)  This is required for all tests that can run past that point.  For example, this test is asynchronous because of the waitForFocus call below.

@@ +27,5 @@
> +    /** We test that clicking beyond the end of a line terminated with <br> selects the preceding text, if any **/
> +
> +    SimpleTest.waitForExplicitFinish();
> +
> +    SimpleTest.waitForFocus(function() {

This is required to ensure that the test window is raised to the top on the desktop, which is needed to ensure proper event delivery.

@@ +35,5 @@
> +      var sel = window.getSelection();
> +      var f = document.getElementById("div1");
> +      theDiv.focus();
> +      sel.collapse(theDiv, 0);
> +      synthesizeMouse(theDiv, 100, 2, {type: "mousedown"}, f.contentWindow);

The last argument of synthesizeMouse is the window object corresponding to the element that should receive the event.  If omitted, the default is the current window object.  Now, f is an HTMLDivElement, which doesn't have a contentWindow property, so f.contentWindow ends up as undefined.  The reason this works is that from the perspective of the callee function, omitted arguments are undefined, so that's what this function checks for: <https://dxr.mozilla.org/mozilla-central/source/testing/mochitest/tests/SimpleTest/EventUtils.js#814>.

So please just remove this last argument here and elsewhere in the test.

@@ +36,5 @@
> +      var f = document.getElementById("div1");
> +      theDiv.focus();
> +      sel.collapse(theDiv, 0);
> +      synthesizeMouse(theDiv, 100, 2, {type: "mousedown"}, f.contentWindow);
> +      synthesizeMouse(theDiv, 100, 2, {type: "mouseup"}, f.contentWindow);

Nit: You can just issue one synthesizeMouse as {type: "click"} and it will dispatch both of these events internally.

@@ +37,5 @@
> +      theDiv.focus();
> +      sel.collapse(theDiv, 0);
> +      synthesizeMouse(theDiv, 100, 2, {type: "mousedown"}, f.contentWindow);
> +      synthesizeMouse(theDiv, 100, 2, {type: "mouseup"}, f.contentWindow);
> +      var selObj = window.getSelection(); 

Nit: You already have |sel| above, no need to get it again, please remove this variable.

@@ +49,5 @@
> +      f = document.getElementById("div2");
> +      theDiv.focus();
> +      sel.collapse(theDiv, 0);
> +      synthesizeMouse(theDiv, 100, 2, {type: "mousedown"}, f.contentWindow);
> +      synthesizeMouse(theDiv, 100, 2, {type: "mouseup"}, f.contentWindow);

Hmm, doesn't this also click to the right of the first line?

@@ +53,5 @@
> +      synthesizeMouse(theDiv, 100, 2, {type: "mouseup"}, f.contentWindow);
> +      selObj = window.getSelection(); 
> +      selRange = selObj.getRangeAt(0);
> +      is(selRange.endContainer.nodeName, "DIV", "selection should be in DIV");
> +      is(selRange.endOffset, 0, "offset should be 0");

Not sure if I understand why this result would be desirable.  I think that because you're clicking at the end of the first line, this is Gecko putting the selection at the end of the first line, which is on the first br element.  Am I missing something?

@@ +55,5 @@
> +      selRange = selObj.getRangeAt(0);
> +      is(selRange.endContainer.nodeName, "DIV", "selection should be in DIV");
> +      is(selRange.endOffset, 0, "offset should be 0");
> +
> +      SimpleTest.finish();

This tells the test framework that the asynchronous test has been finished.  This should be the last thing that the test does.  If you have called waitForExplicitFinish in your test, it is an error to forget to call SimpleTest.finish().
Attachment #8583354 - Flags: review?(ehsan) → feedback+
Thank you very much for the review and all the additional explanation.
I hope I could address your questions in additional comments in the test.

Sadly switching from "mousedown/up" to "click" as you had suggested makes the test fail with:
failed | uncaught exception - NS_ERROR_FAILURE: Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsIDOMWindowUtils.sendMouseEvent] at chrome://specialpowers/content/specialpowersAPI.js:161

This is actually the only test I intended to submit ;-)

This is for the case where the user clicks beyond the end of the line.
There are two tests already which I had to adapt to the new selection behaviour which test the moving to the end
(editor/libeditor/tests/test_selection_move_commands.xul)
and moving from the subsequent line using left arrow
(layout/generic/test/test_movement_by_characters.html).

If you wanted to be really purist a test with a "div contenteditable" could be added where an empty line is first filled with some text, which is then removed so the range becomes empty. The code caters for this case.
Attachment #8582686 - Attachment is obsolete: true
Attachment #8582701 - Attachment is obsolete: true
Attachment #8582923 - Attachment is obsolete: true
Attachment #8582931 - Attachment is obsolete: true
Attachment #8583354 - Attachment is obsolete: true
Flags: needinfo?(ehsan)
(In reply to Jorg K from comment #115)
> Created attachment 8584346 [details] [diff] [review]
> Unified patch (code + test changes + twice revised new test)
> 
> Thank you very much for the review and all the additional explanation.
> I hope I could address your questions in additional comments in the test.
> 
> Sadly switching from "mousedown/up" to "click" as you had suggested makes
> the test fail with:
> failed | uncaught exception - NS_ERROR_FAILURE: Component returned failure
> code: 0x80004005 (NS_ERROR_FAILURE) [nsIDOMWindowUtils.sendMouseEvent] at
> chrome://specialpowers/content/specialpowersAPI.js:161

Err, sorry, I misspoke.  I should have said: "remove the type value" altogether (just pass an empty object `{}').  That should do the right thing: <https://dxr.mozilla.org/mozilla-central/source/testing/mochitest/tests/SimpleTest/EventUtils.js#272>

> This is actually the only test I intended to submit ;-)
> 
> This is for the case where the user clicks beyond the end of the line.
> There are two tests already which I had to adapt to the new selection
> behaviour which test the moving to the end
> (editor/libeditor/tests/test_selection_move_commands.xul)
> and moving from the subsequent line using left arrow
> (layout/generic/test/test_movement_by_characters.html).
> 
> If you wanted to be really purist a test with a "div contenteditable" could
> be added where an empty line is first filled with some text, which is then
> removed so the range becomes empty. The code caters for this case.

I think relying on the selection movement tests is sort of OK, but I really prefer to have much better testing on this, since this code is extremely fragile and I'm pretty sure that without extensive tests, we'll end up breaking it again in the future.  That being said, I guess it's not fair for me to make you do this.  :-)

But at the very least, we should have tests for clicking past the end of the line with the line terminating with a bunch of inline elements that have a text node inside them.  That is essentially what this bug is really about.  So please add some test along those lines.

Also, to reiterate, the second test in your test case is wrong...
Flags: needinfo?(ehsan)
Here comes the updated test. This time with some additional inline elements.

I'm not sure why the second test should be wrong, I'll add another attachment in a second to convince you ;-)
Attachment #8584346 - Attachment is obsolete: true
Attachment #8584608 - Flags: review?(ehsan)
Here some HTML to run a test by hand.

If clicking behind any of the eight lines in this test, "DIV" will be returned in the current version of FF.

With the new behaviour clicking behind lines 1-6 and 8, "#text" will be returned, but for the empty line 7, DIV is still returned.

If the code were wrong, this line couldn't be clicked at all and the caret would move to the previous or subsequent line. I know, because at first my code didn't cater for this case.

IMHO my test is correct, if you don't think so, please be more specific.

Changing the subject:
I'd like to see this landed one day very soon. I'd also request to land it on mozilla38, so TB 38 can pick it up. I don't want to mention again that this is fixing a long-standing TB bug that people have been pulling their hair out for over 10 years now.

If you really want more tests to be written, I can do that, but *afterwards* ;-)
Attachment #8584620 - Attachment description: Here come another manual test. → Here comes another manual test.
(In reply to Jorg K from comment #118)
> Created attachment 8584620 [details]
> Here comes another manual test.
> 
> Here some HTML to run a test by hand.
> 
> If clicking behind any of the eight lines in this test, "DIV" will be
> returned in the current version of FF.
> 
> With the new behaviour clicking behind lines 1-6 and 8, "#text" will be
> returned, but for the empty line 7, DIV is still returned.
> 
> If the code were wrong, this line couldn't be clicked at all and the caret
> would move to the previous or subsequent line. I know, because at first my
> code didn't cater for this case.
> 
> IMHO my test is correct, if you don't think so, please be more specific.

OK, I understand what you want to test now.  So your test is actually correct, it was the comments where you mentioned clicking past the *second* line that confused me.  :-)

> Changing the subject:
> I'd like to see this landed one day very soon. I'd also request to land it
> on mozilla38, so TB 38 can pick it up. I don't want to mention again that
> this is fixing a long-standing TB bug that people have been pulling their
> hair out for over 10 years now.

Sorry but this patch is not suitable for backporting into Aurora (which will soon become Beta) at this point, because it's quite risky in terms of the possibility to cause regressions.  It needs to land on trunk and ride the trains.  I understand that you'd like to see it fixed as soon as possible, but please be a bit more patient.  :-)

> If you really want more tests to be written, I can do that, but *afterwards*
> ;-)

OK, that's fair.  :-)
Comment on attachment 8584608 [details] [diff] [review]
Unified patch (code + test changes + three times revised new test)

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

Can you please revise the commit message to explain what the patch does?  Something like: "Bug 756984 - Collapse the selection on the last text node on the line, skipping br and inline frames when clicking past the end of line; r=roc,ehsan".

Thanks again!  Once you address these nits, this is ready to be checked in.

::: layout/generic/test/test_bug756984.html
@@ +43,5 @@
> +        is(selRange.endContainer.nodeName, "#text", "selection should be in text node");
> +        is(selRange.endOffset, 3, "offset should be 3");
> +      }
> +
> +      // click beyond the second line (100px to the left and 2px down), expect DIV.

Nit: please make this "first line".

@@ +47,5 @@
> +      // click beyond the second line (100px to the left and 2px down), expect DIV.
> +      // This is the previous behaviour which hasn't changed since the line is empty.
> +      // If the processing were wrong, the selection would end up in the preceing non-empty line.
> +      theDiv = document.getElementById("div4");
> +      sel = window.getSelection();

Nit: this is not needed.
Attachment #8584608 - Flags: review?(ehsan)
(In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from comment #119)
> (In reply to Jorg K from comment #118)
>> Changing the subject:
>> I'd like to see this landed one day very soon. I'd also request to land it
>> on mozilla38, so TB 38 can pick it up. I don't want to mention again that
>> this is fixing a long-standing TB bug that people have been pulling their
>> hair out for over 10 years now.

> Sorry but this patch is not suitable for backporting into Aurora (which will
> soon become Beta) at this point, because it's quite risky in terms of the
> possibility to cause regressions.  It needs to land on trunk and ride the
> trains.  I understand that you'd like to see it fixed as soon as possible,
> but please be a bit more patient.  :-)

Oh god, I hope I'm mis-reading this...

Are you seriously saying that this bugfix will NOT land in time for TB 38? Meaning, we will have to another full YEAR to see it fixed?

Please tell me it ain't so.

As for regressions - so you're saying it is OK for major regressions to happen for the entire lifecycle of TB 31 (see all of the Address auto-entry bugs that are only just now being fixed), but not for Firefox?
This should be good to go then ;-)
Attachment #8584608 - Attachment is obsolete: true
Attachment #8584639 - Flags: review?(ehsan)
(In reply to Charles from comment #121)

> Are you seriously saying that this bugfix will NOT land in time for TB 38?
> Meaning, we will have to another full YEAR to see it fixed?

Given that the version spacing is six weeks we get 6 * (45-38) = 42 weeks, that's 10 months ;-)
However, everyone is free to compile their own version, which is what I'll do.
(In reply to Jorg K from comment #123)
> (In reply to Charles from comment #121)
> 
> > Are you seriously saying that this bugfix will NOT land in time for TB 38?
> > Meaning, we will have to another full YEAR to see it fixed?
> 
> Given that the version spacing is six weeks we get 6 * (45-38) = 42 weeks,
> that's 10 months ;-)
> However, everyone is free to compile their own version, which is what I'll
> do.


Fine for us, but what about companies using TBird...

<sigh> I just don't get this kind of reluctance... even if there were regressions, they could easily be fixed...

Oh well, I guess beggars cannot be choosers...
Charles, this patch affects more than Thunderbird.  I'm not sure what the usual practices are with regards to stuff that are regression prone in Thunderbird, but in Firefox, we are usually very conservative, and prefer to give things more time to bake.
Comment on attachment 8584639 [details] [diff] [review]
Unified patch (code + test changes + four times revised new test)

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

Thanks, looks great!
Attachment #8584639 - Flags: review?(ehsan) → review+
Keywords: checkin-needed
(In reply to Charles from comment #124)

IMHO the problem is in the coupling between TB and FF. I think it's quite reasonable that the FF people are more conservative. Who would want to break a browser with a big market share.

TB on the other hand is client software. An nothing should stop client software to pick its foundation and additional patches. I've seen client software (KompoZer, or for example KDE with its so-called "build dependencies") where I couldn't believe my eyes how much they patch the underlying components.

This discussion, however, should be continued elsewhere.
(In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from comment #125)
> Charles, this patch affects more than Thunderbird.  I'm not sure what the
> usual practices are with regards to stuff that are regression prone in
> Thunderbird, but in Firefox, we are usually very conservative, and prefer to
> give things more time to bake.

Yes, but Firefox is primarily a web browser - just how bad could a regression in some code in the editor be?

That said, Jorg is correct that this isn't the right place for such a conversation...

So, all that is left is to say THANK YOU!!! to Jorg, Ehsan, and everyone else involved for finally getting this bug squashed!
I'm really sad that this patch will not land in TB 38. But I want to make a big thank to Jorg that is working on the correction of this long standing bug. Also thanks to Ehsan that worked this in the past.
(In reply to Charles from comment #128)
> (In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from
> comment #125)
> > Charles, this patch affects more than Thunderbird.  I'm not sure what the
> > usual practices are with regards to stuff that are regression prone in
> > Thunderbird, but in Firefox, we are usually very conservative, and prefer to
> > give things more time to bake.
> 
> Yes, but Firefox is primarily a web browser - just how bad could a
> regression in some code in the editor be?

Potentially very bad.  :-)  Any regression that can break lots of websites is very bad.
For those who want to see this in Thunderbird 38 -- I suggest talking to the Thunderbird people and asking them if they can cherry-pick the patch for Thunderbird without affecting Firefox.  If it's really a huge improvement for them, maybe they'll be willing to accept it despite lack of testing.  Perhaps file a bug against the Thunderbird product, or get on IRC/e-mail/etc. with the appropriate people.
https://hg.mozilla.org/mozilla-central/rev/bf414f68291c
Status: NEW → RESOLVED
Closed: 9 years ago
status-firefox39: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
Ummm... ok, but... who are these 'Thunderbird people', and how do we contact them?
Personally, I never understood why this problem involved browsers at all.  
It happens when COMPOSING email in THUNDERBIRD 
and manifests when reading email in THUNDERBIRD.
(In reply to maybe-the-one from comment #135)
> Personally, I never understood why this problem involved browsers at all.  

That you don't understand the facts doesn't change them ;-)

Let me explain: Thunderbird is client software that sits on top of Mozilla core software. The so-called "editor" that is used in Thunderbird when writing an e-mail messase is Mozilla core software. It is also used in Firefox: Please go to http://www-archive.mozilla.org/editor/midasdemo/ to check that even in Firefox you can "write".

The problem happens when losing the font goes unnoticed while writing. You will notice it when reading a reply that contains your original ill-formatted message.

The (In reply to Charles from comment #134)
> Ummm... ok, but... who are these 'Thunderbird people', and how do we contact them?

They are aware of the problem via this meta bug 1142879.
OK, thanks for explaining. I did not know that there was shared code.
And indeed, facts are facts even when one is ignorant of them.
Is there any way we can lobby for landing it in 38?
(In reply to maybe-the-one from comment #138)
> Is there any way we can lobby for landing it in 38?

Please move that discussion to bug 1142879 or elsewhere.  Thanks!
The progress on squashing this bug is excellent.  One thought is that currently TB v39 is in DAILY channel (where this fix has now presumably landed) and is due to move to EARLYBIRD week of March 31 which is tomorrow.  Presumably if users are on the Earlybird channel then they will get the fix once v39 moves to that channel in the next few days? Of course it would mean a longer delay for those running stable versions.  Anyway congratulations on this superb work.
Release channel is at 31.5 as of this post.  
Seems a long way to reach 39 assuming it moves +1 at a time
OK I guess the delay for one version is about two months if it does not get into v38?
No... if it doesn't get back-ported to 38, then it has to wait for version 45 - another, what, 10 months?
Blocks: 1159834
"No... if it doesn't get back-ported to 38, then it has to wait for version 45 - another, what, 10 months?"

I do not understand this given that above it says 
"RESOLVED FIXED "
and
"status-firefox39: 	fixed"

Why could it not go into 39,40,41,42,43,44?
38 will be the basis of the next Extended Service Release (ESR), followed by 45. If you use regular Firefox you'll get this fix in 39.
(In reply to maybe-the-one from comment #144)

> Why could it not go into 39,40,41,42,43,44?

I will go into all those versions. However, they will only ever be released as beta (and aurora) versions, since TB is on a 10 months release cycle (17, 24, 31, 38, 45. 7 * 6 weeks = 10 months) due to lack of staff and funding.
Oh, sorry, I should have read more context. Ignore my comments please.
So, if I am on Thunderbird 31, release channel, when will I get a version with the fix?
The decision was made to include this in TB38 (bug 1159834).
So it will be shipped with TB38 sometime between mid May and early June.
Depends on: 1237286
No longer depends on: 1237286
Depends on: 1237286
Depends on: 1263288
No longer blocks: 802190
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: