1100966 - Inline spell checker loses red underlines after a backspace is used

Status: RESOLVED FIXED

(Core :: DOM: Editor, defect)

Description

[Jorg K (CEST = GMT+2)]

Attachment: spelling mistakes get cleared.mp4

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:33.0) Gecko/20100101 Firefox/33.0
Build ID: 20141106120505

Steps to reproduce:

Inline spell checker loses red underlines after a period of time.

Watch the attached video.

(I'm using v 34 beta, but the bug has been there for a long time and can also be observed on the daily version of 16th Nov. 2014 for example).

Comment 1

[[e-mail]]

Here are my comments regarding this that I put in bug 917027

For sure Jork K!  TB is by far the most awesome email client I've ever used but this has got to be the single most annoying aspect of the WHOLE program.  Instead of typing away on a long email without breaking my thought AND THEN going back to check if there are any spellings flagged; I instead have to stop as soon as one is flagged and correct it.  If I don't, then the "flagging" goes away and then I don't remember which words I had misspelled.  You would think something like this would be elementary but apparently not because I've been dealing with this for years.  I'm being literal.

Comment 2

[Jorg K (CEST = GMT+2)]

Please be aware of Mozilla's triage system. An "unconfirmed" bug with no votes is basically invisible to the developers.

Can someone in QA please more the bug to status "NEW". It takes two minutes to reproduce:
Write an e-mail with inline spell-checking turned on, make some mistake, make more mistakes, keep typing until at some stage the red underlines disappear at random. Any version of TB will do.

Comment 3

[Jorg K (CEST = GMT+2)]

The red underline disappears when the "backspace" key is used. This is also visible on the video.
A workaround is to save the draft message, that makes the red underlines come back.

The bug is reproducible on Windows (using Windows 7). The following versions show the bug:
24.8.1, 31.2, 34 (beta), daily 16th Nov. 2014.

On Linux (using Mint 17) this bug is NOT present. I tried Version 24.4 and 31.2.

Comment 5

[[e-mail]]

I never realized that it's the backspace that "breaks" it.  That is good info.  Makes it a little easier to manage the issue. Thanks Jorg K

Comment 7

[Jorg K (CEST = GMT+2)]

If we take a look at bug 922648 where something similar is described, people over in the other bug observe that "Select all, Delete" gives an empty windows where the problem no longer occurs.

I CANNOT confirm this. Even after "Select all, delete", the problem still happens. For me, "Select all, delete" gets rid of my default composition font "fixed width" and places me into "variable width". Once in variable width, it's harder to produce the problem. I need to click back to a misspelled word, correct it, click forward at the end of the text and eventually press backspace.

Comment 8

[Jorg K (CEST = GMT+2)]

With this knowledge I went back to Linux. This time I tried a bit harder and, lo and behold, the bug CAN be reproduced in Linux!! So please ignore the part of comment #3 that states the opposite.

Comment 9

[Jorg K (CEST = GMT+2)]

Attachment: losing underlines in FF.zip

The problem can be very easily demonstrated in Firefox alone. Here using the current version at the time, 33.1.1. HTML is included in the archive.

Comment 10

[Jorg K (CEST = GMT+2)]

For the record: This can be reproduced in Firefox version 4.0

Comment 11

[Jorg K (CEST = GMT+2)]

Making this reproducible in FF was a suggestion of Ehsan Akhgari. He also wrote to me in Nov. 2014:
===
Try to debug this.  Set breakpoints on <http://mxr.mozilla.org/mozilla-central/source/extensions/spellcheck/src/mozInlineSpellChecker.cpp#1297> (which is the function that schedules a spell check to happen some time later) and <http://mxr.mozilla.org/mozilla-central/source/extensions/spellcheck/src/mozInlineSpellChecker.cpp#1602> (which is the function that resumes checking a range in the document) to see where things go wrong.  My suspicion would be that we call CleanupRangesInSelection and then either return early or get confused as to what range to check etc, therefore leaving a portion of the document without any spelling error ranges.  (Note that we represent misspelled regions using ranges in the spell checking selection, which is later on used to draw the wiggly underlines in our text rendering code for text frames corresponding to those ranges.)
===

I watched the process in the debugger, but sadly a lot of things happen on each keystroke or backspace. I'll dedicate more time to it in 2015.

Comment 12

[Jorg K (CEST = GMT+2)]

I'm not having a lot of joy debugging this. As advised, I set a breakpoint inside ResumeCheck
http://mxr.mozilla.org/mozilla-central/source/extensions/spellcheck/src/mozInlineSpellChecker.cpp#1605
especially at:
http://mxr.mozilla.org/mozilla-central/source/extensions/spellcheck/src/mozInlineSpellChecker.cpp#1667
where DoSpellCheck is called.

Strangely for every keystroke, that is, for every character newly entered or removed (with backspace) the DoSpellCheck function is called three times.

Any hint would be appreciated.

Comment 13

[Andrew Overholt [:overholt]]

Hi Jorg, just so you're not waiting without knowing, Ehsan's not around until 2 weeks from today, Feb 23.

Comment 14

[Jorg K (CEST = GMT+2)]

Thanks, his "name" in Bugzilla currently reads: [Away: 1/29-2/20] ;-)

Comment 15

[(no longer active)]

(In reply to Jorg K from comment #12)
> I'm not having a lot of joy debugging this. As advised, I set a breakpoint
> inside ResumeCheck
> http://mxr.mozilla.org/mozilla-central/source/extensions/spellcheck/src/
> mozInlineSpellChecker.cpp#1605
> especially at:
> http://mxr.mozilla.org/mozilla-central/source/extensions/spellcheck/src/
> mozInlineSpellChecker.cpp#1667
> where DoSpellCheck is called.
> 
> Strangely for every keystroke, that is, for every character newly entered or
> removed (with backspace) the DoSpellCheck function is called three times.

Well, what happens in each of those invocations?  Do we return early from DoSpellCheck for some reason?  Do we get to the main while loop inside the function where we look at the words?  And does something different happen in each one of those three invocations?

Comment 16

[Jorg K (CEST = GMT+2)]

Looks like debugging event driven stuff in the debugger gets confusing results. So I made another attempt. I added debug prints to the code, the log is enclosed. No funny extra invocation, one invocation per click/keystroke.
 
We start off with a very simple case (also enclosed):
=====
correct onee, threee and fivee, position after sixx, type space,a,backspace
onee twoo threee fourr fivee sixx
====

Then a sequence of four clicks and six other keystrokes reproduces the problem:
c1: click into "onee" (the one on the second line)
 1: backspace to correct
c2: click into "threee"
 2: backspace to correct
c3: click into "fivee"
 3: backspace to correct
c4: click after "sixx"
 4: type space
 5: type a
 6: backspace.
On the last backspace the spellcheck exists early on the
if (aStatus->mRange->Collapsed())

Further hints needed.

Comment 17

[Jorg K (CEST = GMT+2)]

Attachment: Simple HTML to reproduce the bug

Comment 18

[Jorg K (CEST = GMT+2)]

Attachment: Debug output with comments

Comment 19

[(no longer active)]

(In reply to Jorg K from comment #16)
> Looks like debugging event driven stuff in the debugger gets confusing
> results. So I made another attempt. I added debug prints to the code, the
> log is enclosed. No funny extra invocation, one invocation per
> click/keystroke.
>  
> We start off with a very simple case (also enclosed):
> =====
> correct onee, threee and fivee, position after sixx, type space,a,backspace
> onee twoo threee fourr fivee sixx
> ====
> 
> Then a sequence of four clicks and six other keystrokes reproduces the
> problem:
> c1: click into "onee" (the one on the second line)
>  1: backspace to correct
> c2: click into "threee"
>  2: backspace to correct
> c3: click into "fivee"
>  3: backspace to correct
> c4: click after "sixx"
>  4: type space
>  5: type a
>  6: backspace.

FWIW I can't reproduce using these instructions.  Perhaps that's because I'm positioning the caret in the wrong place?  ("one|e" vs "onee|", for example.)  But anyhow...

> On the last backspace the spellcheck exists early on the
> if (aStatus->mRange->Collapsed())
> 
> Further hints needed.

That's OK, see the comments before the start of the function to get an idea of how this works.  Basically we maintain the current word that is being typed (in this case "a") in mRange, and also in mNoCheckRange, and once you press space, we clear mNoCheckRange which triggers the last word to be typed to be spell checked (this happens so that we don't constantly mark the word that you're in the middle of typing as misspelled before you finish typing it.)  So when you press backspace in step 6, mRange is empty, and we bail out early because there is no work to do.  But that should not clear the existing misspelled ranges.

Thinking more about this, what's probably happening is that we are removing the existing words from the spell check selection.  Can you set a breakpoint on mozInlineSpellChecker::RemoveRange and see if it gets called after pressing backspace in step 6, and if so, why?

Comment 20

[Jorg K (CEST = GMT+2)]

I carried out more debugging as suggested. What can be seen below is that after the backspace is pressed, all ranges that refer to the second line of the text are being removed. The first line has four spelling errors (0 to 3), so the remaining three spelling mistakes in the second line are removed.

The question is, why are they removed. Obviously they need to fulfill the condition "collapsed":
nsRange *checkRange = aSelection->GetRangeAt(index);
if (checkRange) {
 if (checkRange->Collapsed()) {
  printf("-- calling RemoveRange with loop index %d\n", index);
  RemoveRange(aSelection, checkRange);

Looks like pressing backspace is "collapsing" more ranges than desired.

Debug output:

=== Entering mozInlineSpellChecker::DoSpellCheck
-- originalRangeCount: 7
-- SetEnd,SetPosition: done
-- before while loop
->Got word "a"
=== Leaving mozInlineSpellChecker::DoSpellCheck

hit backspace

=== Entering mozInlineSpellChecker::CleanupRangesInSelection
-- count: 7
-- calling RemoveRange with loop index 4
=== Entering mozInlineSpellChecker::RemoveRange
-- decremented mNumWordsInSpellSelection, current value 6
=== Leaving mozInlineSpellChecker::RemoveRange with error code 0
-- calling RemoveRange with loop index 4
=== Entering mozInlineSpellChecker::RemoveRange
-- decremented mNumWordsInSpellSelection, current value 5
=== Leaving mozInlineSpellChecker::RemoveRange with error code 0
-- calling RemoveRange with loop index 4
=== Entering mozInlineSpellChecker::RemoveRange
-- decremented mNumWordsInSpellSelection, current value 4
=== Leaving mozInlineSpellChecker::RemoveRange with error code 0
=== Leaving mozInlineSpellChecker::CleanupRangesInSelection

=== Entering mozInlineSpellChecker::DoSpellCheck
=== Leaving mozInlineSpellChecker::DoSpellCheck early due to aStatus->mRange->Collapsed()

Comment 21

[(no longer active)]

OK, I finally managed to reproduce and debugged this a bit, and found out where the bug is.  What I'm going to describe next requires some knowledge of DOM ranges, please first take a look at <https://dom.spec.whatwg.org/#range>.  Also, note that you can reproduce simpler by starting by putting the cursor after "sixx".

The bug happens in nsEditor::JoinNodesImpl.  Here is what happens.  When you press the cursor at the end of the 2nd line and start to type, we inject a text node adjacent to the original text node that contains the misspelled ranges.  Then, when you press backspace, we note that there are two adjacent text nodes and attempt to join the two text nodes.  So far, so good.

Right now we have a "left" node roughly containing "one twoo three fourr five sixx", and a "right" node containing " a".  JoinNodesImpl's caller (https://dxr.mozilla.org/mozilla-central/source/editor/libeditor/JoinNodeTxn.cpp#67) passes the right node as the node to keep and the left node as the node to delete.  The join basically works by appending the text content of the two nodes together, deleting the "node to delete" and set the text data on the "node to keep" to the appended text content.

However, we have some spell check ranges in the left node, which we are deleting.  According to the DOM Range spec <https://dom.spec.whatwg.org/#concept-node-remove>, the remove operation sets all of those ranges to point to (parent node, index of the removed node).  This is what causes those misspelling ranges to be collapsed.  Then, the spell checker sees the collapsed ranges, and correctly gets rid of them.

Now, there is code in nsEditor::JoinNodesImpl to save the position of the current selection (see selStartNode) and restore it when the join operation is finished.  There are two problems with that code, however.  One that it completely ignores the spell check selection, and also that it completely ignores multi-range selections, as indicated here: <https://dxr.mozilla.org/mozilla-central/source/editor/libeditor/nsEditor.cpp#2781>!

A bit of back story on selections, we have 9 different kinds of selections <https://dxr.mozilla.org/mozilla-central/source/dom/base/nsISelectionController.idl#22>.  SELECTION_NORMAL is the normal selection that you see when selecting text on the screen.  There are other selection types, and SELECTION_SPELLCHECK is one of them.  (You can in fact test to see that the same bug happens with all other selection types, for example use the Highlight All option of the findbar to create a highlight on "one" or something, and follow the steps to see that when the bug occurs, the highlight selection also disappears.)  Also, note that each selection can have multiple ranges inside it (that's what I meant above by multi-range selections).  So this bug basically affects all selections except for the first range of SELECTION_NORMAL!

The proper fix for this bug is to replace the code that deals with selStartNode etc and restructure it to maintain an array of the following tuple (selection, startnode, startoffset, endnode, endoffset).  Then before we start the merge, we need to iterate over all selection types, and also on each range in each one of those selections, and record these tuples in an array, then when the merge is done we should iterate over the saved array, and fix up each range that we have recorded on the correct selection.

The fix is actually easier than what may appear on the first look, given that the existing logic for one range in one selection exists, and you can copy the exact same logic.  Please let me know if you're interested in writing a fix, and sorry that this description ended up being a bit lengthy.

Comment 22

[Jorg K (CEST = GMT+2)]

(In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from comment #21)

> Please let me know if you're interested in
> writing a fix, and sorry that this description ended up being a bit lengthy.

Well, complicated things need a wordy explanation.

As for who provides the fix: We should go for best value for the buck. Since so far my skills are limited to typing "./mach build extensions/spellcheck/src/" after adding some debug and stopping here and there with the debugger (sometimes even with completely wrong results, see my comment #12) it would take me a long time to fix. If you can do it in hour or two, please do it.

<rant>
Let me add this as background: I'm really interested in helping the (currently unfunded and understaffed) Thunderbird team. I feel, since this is a core problem, the core team which is funded by MoCo might want to take care of it.

Although the bug looks like my personal "hobby" and few people are using "<div contenteditable>", Thunderbird users are suffering from this bug daily. Since other things are/were wrong with spell checking in Thunderbird, lots of bugs regarding spell checking have become very confused, mixing various issues. So I tried to condense this last severely annoying problem into this bug.
</rant>

Comment 23

[Jorg K (CEST = GMT+2)]

s/MoCo/MoFo/

Comment 24

[[e-mail]]

I'm going to pipe in here to say thanks Jorg K for pressing the issue on this and you are right on with comment 22.
I've been watching the progress on this bug because I'm really hoping this finally gets fixed one day!  I'm baffled as to how there isn't a vast mob of TB users who complain about this daily.  I guess people just don't take the time or maybe they move on to a different email client.  Any way, I'm starting to rant on myself....like I said, thanks for staying on top of this and pressing this issue.  I believe this bug has been going on for years (yes, years) and it seams like no one is ever concerned about resolving it.  You the man!

Comment 26

[(no longer active)]

(In reply to Jorg K from comment #22)
> (In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from
> comment #21)
> 
> > Please let me know if you're interested in
> > writing a fix, and sorry that this description ended up being a bit lengthy.
> 
> Well, complicated things need a wordy explanation.
> 
> As for who provides the fix: We should go for best value for the buck. Since
> so far my skills are limited to typing "./mach build
> extensions/spellcheck/src/" after adding some debug and stopping here and
> there with the debugger (sometimes even with completely wrong results, see
> my comment #12) it would take me a long time to fix. If you can do it in
> hour or two, please do it.

It will probably take longer than that given the fact that I need to write tests and whatnot, but I'll try to get to it.

> <rant>
> Let me add this as background: I'm really interested in helping the
> (currently unfunded and understaffed) Thunderbird team. I feel, since this
> is a core problem, the core team which is funded by MoCo might want to take
> care of it.

Your facts are wrong.  :-)  We have no paid staff who is actively working on the editor/ code.  As the module owner, I try to make occasional fixes in my spare time whenever I can, but that races against all of the other things I want to do in my spare time!

> Although the bug looks like my personal "hobby" and few people are using
> "<div contenteditable>", Thunderbird users are suffering from this bug
> daily.

Please note that there is nothing Thunderbird specific about this bug.  This also happens in Firefox when writing emails in webmail software, blog posts, etc.

> Since other things are/were wrong with spell checking in Thunderbird,
> lots of bugs regarding spell checking have become very confused, mixing
> various issues. So I tried to condense this last severely annoying problem
> into this bug.
> </rant>

Thanks for pushing on this and specially for coming up with steps to reproduce.  I had occasionally seen this issue myself as well, but never managed to reproduce and investigate until now!

Comment 27

[Jorg K (CEST = GMT+2)]

(In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from comment #26)

> Your facts are wrong.  :-)  We have no paid staff who is actively working on
> the editor/ code.  As the module owner, I try to make occasional fixes in my
> spare time whenever I can, but that races against all of the other things I
> want to do in my spare time!

Not having insight into the internal workings of Mozilla, I'm very happy to stand corrected.
So you are saying that there are modules in Mozilla/Firefox that have an owner who is not allowed to work on the module in their paid time? So Mozilla abandoned responsibility for some of the code and handed it off to individuals donating their spare time? I don't expect "active development", but I would expect fixes to extremely annoying and long-standing bugs like this one (and bug 756984).

> Please note that there is nothing Thunderbird specific about this bug.  This
> also happens in Firefox when writing emails in webmail software, blog posts,
> etc.

That's what I thought. But I've never seen it happen on any such occasion. Surely it can't be reproduced in this "blog post" or any other web form I've tried.

Comment 28

[(no longer active)]

(In reply to Jorg K from comment #27)
> (In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from
> comment #26)
> 
> > Your facts are wrong.  :-)  We have no paid staff who is actively working on
> > the editor/ code.  As the module owner, I try to make occasional fixes in my
> > spare time whenever I can, but that races against all of the other things I
> > want to do in my spare time!
> 
> Not having insight into the internal workings of Mozilla, I'm very happy to
> stand corrected.
> So you are saying that there are modules in Mozilla/Firefox that have an
> owner who is not allowed to work on the module in their paid time?

I didn't say I'm not allowed to work on this.  But I have other responsibilities and the list of things that I want to work on is always longer than the list of things that I have time to work on.  It's a matter of prioritization.

> So
> Mozilla abandoned responsibility for some of the code and handed it off to
> individuals donating their spare time?

You're putting words into my mouth.  :/  Mozilla is not a magical machine which produces bug fixes, it's a community of paid and volunteer individuals who do their best to fix as many issues as they can.  And there is no rule that says module owners are paid staff, I was for example a module owner before I became an employee.

> I don't expect "active development",
> but I would expect fixes to extremely annoying and long-standing bugs like
> this one (and bug 756984).

The reality of any large and complicated codebase (Thunderbird included) is that there are parts of the cold which are older and buggier than others, and there is a finite amount of human power working on the codebase, and sometimes some components receive less attention than others because of a host of reasons.  That all being said, I have tried to help both here and on bug 756984, so I don't understand why you're complaining to me.

I'd really like to urge you to keep the discussion technical, if you're not satisfied with the rate of bug fixes/etc, Bugzilla is not the right venue to discuss that.  Thanks!

> > Please note that there is nothing Thunderbird specific about this bug.  This
> > also happens in Firefox when writing emails in webmail software, blog posts,
> > etc.
> 
> That's what I thought. But I've never seen it happen on any such occasion.
> Surely it can't be reproduced in this "blog post" or any other web form I've
> tried.

Bugzilla uses plaintext fields (<textarea>) which have a cleaner underlying DOM structure which is why you won't see this bug there.  I have seen this bug personally in gmail.  And I loaded and debugged the test case in Firefox.  :-)

Comment 29

[(no longer active)]

(Needinfoing myself as I still plan to take a look at this.)

Comment 30

[Jorg K (CEST = GMT+2)]

(In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from comment #28)

> That all being said, I have tried to help both here and on
> bug 756984, so I don't understand why you're complaining to me.

I'm very appreciative of your help! I wasn't complaining to you, I was only complaining about the status of things in general. Let's face it, those who complain ultimately make the world better ;-)

> I'd really like to urge you to keep the discussion technical, if you're not
> satisfied with the rate of bug fixes/etc, Bugzilla is not the right venue to
> discuss that.  Thanks!

Indeed. I will take another look at bug 756984 as priorities permit. Just allow me one last comment (since you touched on the subject): This other editor bug 756984 (carried over from bug 250539) is 10+ years old, so you'd have to be very patient to be "satisfied with the rate of bug fixes".

Comment 31

[(no longer active)]

(In reply to Jorg K from comment #30)
> Indeed. I will take another look at bug 756984 as priorities permit. Just
> allow me one last comment (since you touched on the subject): This other
> editor bug 756984 (carried over from bug 250539) is 10+ years old, so you'd
> have to be very patient to be "satisfied with the rate of bug fixes".

I have never met an engineer who decides which bugs to fix based on when they were reported.  ;-)

Comment 32

[Paul Rako]

Hi all, thanks for the great work. I have the red spell-check underlines disappear my Thunderbird 31.5.0, and in the BlueGriffon editor 1.7.2. This is on a Win 7 box, but I also had the same problems in Win XP with earlier versions, even Firefox text boxes. Its been about 5 years of disappearing red underlines, not to say it is the same bug. It does happen with a backspace, and seems to most often just wipe the red underlines from the same paragraph.

I cannot reproduce this behavior on in my Firefox 28.0, so I suspect it is fixed in that release.

In Thunderbird I just ask for a dialog box spell check before send, but I recently found that I can just do a "Save" and all the red underlines re-appear. For BlueGriffon, I either enter "Source" mode and go back to "Wysiwyg" mode, or doing a save will also bring the red underlines back.

It is completely reproducible in BlueGriffon,it does have to be multi-line text before all the red underlines go poof on a backspace.

In Thunderbird, I can have red underlines on one line and a backspace does nothing. With a <enter> (does "carriage return" show my age?) it will work OK, no effect on backspace. With a backspace across the <enter> all the red underlines go away except for the last word on what used to be the first line, but what is now in the middle of the single line. I can have a a paragraph of underlined words, and if I hit <Delete> and pull a LF/CR up from below it does nothing unless the line below also had text on in. Then it erases all the red underlines on the paragraphs except for the last one on the former paragraph and any underlines on the text that the <Delete> pulled up to join that paragraph. A backspace across a CR/LF does the same. If it is just a bank line, nothing happens. 

So to summarize:
Firefox 28.0 can't reproduce
Thunderbird 31.5.0 backspace or delete across a CR/LF with text on the line.
BlueGriffon, backspace but only muli-line text.

So while this might be a bug in the Gekko core, it does look like the Firefox folks have done a work-around or these are different revs of Gekko at work here.

Comment 33

[Jorg K (CEST = GMT+2)]

(In reply to Paul Rako from comment #32)

> Firefox 28.0 can't reproduce

This is a Gecko core problem, reproducible also in Firefox, but not with plaintext fields (<textarea>). Download attachment "Simple HTML to reproduce the bug" and refer to comment #16.

Comment 34

[(no longer active)]

Attachment: Remember all ranges for all selections when joining nodes in the editor transactions

This patch fixes some symptoms, the most common of which misspelling
ranges disappearing when performing some editor operations.

Comment 35

[(no longer active)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=3fc95c909ab2

Comment 36

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

Comment on attachment 8576000 [details] [diff] [review]
Remember all ranges for all selections when joining nodes in the editor transactions

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

::: editor/libeditor/nsEditor.cpp
@@ +2804,2 @@
>      selection->Collapse(aNodeToKeep, AssertedCast<int32_t>(firstNodeLength));
> +  } else if (!savedRanges.IsEmpty()) {

Seems to me that if GetShouldTxnSetSelection() returns true we should still restore all saved ranges other than the ones belonging to the normal selection. Right?

@@ +2828,5 @@
>  
> +      // Adjust selection if needed.
> +      if (bNeedToAdjust) {
> +        range.mSelection->Collapse(range.mStartNode, range.mStartOffset);
> +        range.mSelection->Extend(range.mEndNode, range.mEndOffset);

Does this actually work? Doesn't this mean that if a selection should have multiple ranges in it after the join operation, we'll wipe out all but the last range that needed adjustment?

It's probably simpler to not do this bNeedToAdjust thing, and always reconstitute all selections from their ranges.

Comment 38

[(no longer active)]

Ah you're right, this only fixes things for the last range that needs adjustment, sorry for not testing things more.  :/

Comment 39

[(no longer active)]

Attachment: Remember all ranges for all selections when joining nodes in the editor transactions

This patch fixes some symptoms, the most common of which misspelling
ranges disappearing when performing some editor operations.

Comment 40

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

Comment on attachment 8576777 [details] [diff] [review]
Remember all ranges for all selections when joining nodes in the editor transactions

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

Excellent

Comment 41

[(no longer active)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/64ae62dab70b

Comment 42

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/64ae62dab70b

Comment 43

[Jorg K (CEST = GMT+2)]

Can we *PLEASE* have this for mozilla38. This is a very annoying problem for Thunderbird users and it would be great to ship it with TB 38. Thanking you in advance ;-)

Comment 44

[(no longer active)]

(In reply to Jorg K from comment #43)
> Can we *PLEASE* have this for mozilla38. This is a very annoying problem for
> Thunderbird users and it would be great to ship it with TB 38. Thanking you
> in advance ;-)

I don't feel comfortable taking this fix on Aurora, since we only have two weeks of testing left and this is prone to potential regressions.

Comment 45

[Sylvestre Ledru [:Sylvestre]]

If Ehsan is not comfortable with an uplift to 38, I am not either (especially for an ESR release)...

Comment 46

[Jorg K (CEST = GMT+2)]

Attachment: test case showing that the problem persists

You will hate this as much as I do, but the problem persists.
I noticed it in Thunderbird, where the text you enter typically has a font assigned.

Please open the attachment in Firefox. You will see:
thiss is stilll not fixed
Position behind the "stilll" and backspace to delete the third "l".
Then type space. The "thiss" loses its red underline.
It doesn't matter whether it's <tt> or some other font.

Comment 47

[[e-mail]]

I also have still seen this issue in TB 31.6.0 but have not been able to get an exact dublicateable (is that a word?) scenario.  The first time I saw the issue was still persisting was when I had a misspelled word followed by a bunch of text and then another misspelled word.  I backspaced to correct the 2nd misspelled word and after correcting it, I noticed that the first misspelled word lost it's red underline.  I had a suspicion that it had to do with misspelled words being in different sentences but haven't had time to do any testing.  I've seen in other situations as well but results don't seem to be consistent.  Haven't found an underlying cause yet but the problem is for sure still there.

Comment 48

[Jorg K (CEST = GMT+2)]

As you can see in comment #42, the first fix was landed on mozilla39, so of course the behaviour of TB31.x is unchanged. As you can see in comment #45, TB38 will not be fixed. So we'll have to wait until TB45 in 2016.

Comment 49

[(no longer active)]

Please file a new bug.

Comment 50

[Jorg K (CEST = GMT+2)]

New bug 1154791.