1154791 - Inline spell checker loses red underlines after a backspace is used - take two

Status: RESOLVED FIXED

(Core :: DOM: Editor, defect)

Description

[Jorg K (CEST = GMT+2)]

Attachment: test case showing the problem

Continuation of bug 1100966:

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.
Having a font element is essential for the bug.

This affects Thunderbird, since the core editor is used to type messages.

Comment 1

[Jorg K (CEST = GMT+2)]

Need info for you kind attention - as per bug 1100966 comment #29 ;-)

Comment 2

[(no longer active)]

Sorry I have no time to devote to this right now.  It would be great if you can help debug this if you have the tme.  The same debugging techniques as the previous bug can help you here as well.

Comment 3

[Jorg K (CEST = GMT+2)]

OK, after the 23rd of April. Debugging is no fun on a small travel laptop.

Comment 4

[Jorg K (CEST = GMT+2)]

Attachment: mozInlineSpellChecker-with-debug-april-2015.zip, if you want to see where the debug is printed

I remembered that this is debugged with printing out stuff, since debugging is impossible in the debugger in this event driven stuff. I still had the code.

Here is the debug:

When loading the page:

[3816] WARNING: NS_ENSURE_TRUE(rootContent) failed: file d:\mozilla-source\mozilla-central\editor\composer\nsEditorSpellCheck.cpp, line 697
info: 0, warning: 0, error: 2
Done
=== Entering mozInlineSpellChecker::CleanupRangesInSelection
-- count: 0
=== Leaving mozInlineSpellChecker::CleanupRangesInSelection
=== Entering mozInlineSpellChecker::DoSpellCheck
-- originalRangeCount: 0
-- SetEnd,SetPosition: done
-- before while loop
->Got word "thiss"
->Got word "is"
->Got word "stilll"
->Got word "not"
->Got word "fixed"
=== Leaving mozInlineSpellChecker::DoSpellCheck

when clicking behind "stilll", two red underlines appear

=== Entering mozInlineSpellChecker::CleanupRangesInSelection
-- count: 0
=== Leaving mozInlineSpellChecker::CleanupRangesInSelection
=== Entering mozInlineSpellChecker::DoSpellCheck
-- originalRangeCount: 0
-- SetEnd,SetPosition: done
-- before while loop
->Got word "thiss"
->Got word "is"
->Got word "stilll"
->Got word "not"
->Got word "fixed"
=== Leaving mozInlineSpellChecker::DoSpellCheck

when deleting the third "l" with a backspace

=== Entering mozInlineSpellChecker::CleanupRangesInSelection
-- count: 2
=== Leaving mozInlineSpellChecker::CleanupRangesInSelection
=== Entering mozInlineSpellChecker::DoSpellCheck
-- originalRangeCount: 2
-- SetEnd,SetPosition: done
-- before while loop
->Got word "is"
-- for (...) RemoveRange: done
->Got word "still"
=== Entering mozInlineSpellChecker::RemoveRange
-- decremented mNumWordsInSpellSelection, current value 1
=== Leaving mozInlineSpellChecker::RemoveRange with error code 0
-- for (...) RemoveRange: done
=== Leaving mozInlineSpellChecker::DoSpellCheck

when typing space, red underline under "thiss" is lost

=== Entering mozInlineSpellChecker::CleanupRangesInSelection
-- count: 1
-- calling RemoveRange with loop index 0
=== Entering mozInlineSpellChecker::RemoveRange
-- decremented mNumWordsInSpellSelection, current value 0
=== Leaving mozInlineSpellChecker::RemoveRange with error code 0
=== Leaving mozInlineSpellChecker::CleanupRangesInSelection
=== Entering mozInlineSpellChecker::DoSpellCheck
-- originalRangeCount: 0
-- SetEnd,SetPosition: done
-- before while loop
->Got word "is"
->Got word "still"
=== Leaving mozInlineSpellChecker::DoSpellCheck

Later on

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

My comment:
After the space key is pressed, a call to CleanupRangesInSelection is happening. In there, the remaining valid range corresponding to "thiss" is erroneously removed:
-- count: 1
-- calling RemoveRange with loop index 0
=== Entering mozInlineSpellChecker::RemoveRange
-- decremented mNumWordsInSpellSelection, current value 0

which comes from:
if (checkRange->Collapsed()) {
  printf("-- calling RemoveRange with loop index %d\n", index);
  RemoveRange(aSelection, checkRange);

and
printf("=== Entering mozInlineSpellChecker::RemoveRange\n");
aSpellCheckSelection->RemoveRange(*aRange, rv);
if (!rv.Failed() && mNumWordsInSpellSelection) {
  mNumWordsInSpellSelection--;
  printf("-- decremented mNumWordsInSpellSelection, current value %d\n", mNumWordsInSpellSelection);

I wonder which collapsed range it wants to remove, since one got removed before, and why it hits the still valid and non-collapsed range containing the "thiss".

Obviously some processing corrupted the internals.

Comment 5

[(no longer active)]

You're looking for the wrong thing, you should look for another place where the editor code is ripping out the selections from underneath the spell checker, just like the other bug.  If I had to make a wild guess, I would suspect nsEditor::SplitNodeImpl, since it has the same broken logic, and we are splitting a text node here.  You should be able to do exactly what I did in my patch there for this function and see if that fixes this issue.

Comment 6

[Jorg K (CEST = GMT+2)]

Will do, thanks.

Comment 7

[Jorg K (CEST = GMT+2)]

Attachment: Proposed patch, tests still missing

Comment 8

[Jorg K (CEST = GMT+2)]

Comment on attachment 8593304 [details]
mozInlineSpellChecker-with-debug-april-2015.zip, if you want to see where the debug is printed

I did a cut & paste job, it fixes the problem, if there are no objections, I'll submit it to the try server.

Comment 9

[(no longer active)]

Comment on attachment 8593513 [details] [diff] [review]
Proposed patch, tests still missing

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

This is the right idea for a fix, but I don't think it's a good patch since it duplicates a lot of code that we have in JoinNodesImpl.  Ideally you'd add two helper functions, for example SaveAllSelectionRanges, and RestoreAllSelectionRanges which both of these functions would call before and after modifying the DOM, but I haven't looked closely to see if the logic can be extracted very cleanly.  At the very least, I suspect that refactoring the first half into SaveAllSelectionRanges should be possible.  Can you please see if you can refactor things this way?  Thanks!

Comment 10

[Jorg K (CEST = GMT+2)]

The stuff you want to factor out is too different:

In the join part you have this extra code:

      // If selection endpoint is between the nodes, remember it as being
      // in the one that is going away instead.  This simplifies later selection
      // adjustment logic at end of this method.
      if (range.mStartNode) {
        if (range.mStartNode == parent &&
            joinOffset < range.mStartOffset &&
            range.mStartOffset <= keepOffset) {
          range.mStartNode = aNodeToJoin;
          range.mStartOffset = firstNodeLength;
        }
        if (range.mEndNode == parent &&
            joinOffset < range.mEndOffset &&
            range.mEndOffset <= keepOffset) {
          range.mEndNode = aNodeToJoin;
          range.mEndOffset = firstNodeLength;
        }
      }

The only thing in common are about eight lines. Not worth the effort.

The second part is also too different to factor out the common stuff:

Join:

    if (range.mStartNode == aNodeToJoin) {
      range.mStartNode = aNodeToKeep;
    } else if (range.mStartNode == aNodeToKeep) {
      range.mStartOffset += firstNodeLength;
    }

Split:

    if (range.mStartNode == &aExistingRightNode) {
      if (range.mStartOffset < aOffset) {
        range.mStartNode = &aNewLeftNode;
      } else {
        range.mStartOffset -= aOffset;
      }
    }

Why make it extra complicated for the sake of saving a few common lines?

OK, I did what you said, I looked, and my result is that it's now worth doing.

I will go ahead with another copy&paste job and adapt your test from bug 1100966 to this new case.

Comment 11

[Jorg K (CEST = GMT+2)]

Attachment: Proposed patch, complete with test case

https://treeherder.mozilla.org/#/jobs?repo=try&revision=6e599d277f32

Too bad that also includes the fix to bug 1140105, but that won't make a difference.

I haven't worked out how to submit a single patch, they told me to use:
mc-try = ssh://[e-mail]@hg.mozilla.org/try
hg push -f -r tip mc-try

Comment 12

[(no longer active)]

(In reply to Jorg K (at the beach until 22nd April) from comment #10)
> The stuff you want to factor out is too different:

OK, fair.  Can you please set the review flag once this passes tests to avoid round-trips?  My review queue is super long these days.  :(

Thanks!

Comment 13

[Jorg K (CEST = GMT+2)]

Comment on attachment 8593539 [details] [diff] [review]
Proposed patch, complete with test case

Passed the Mochitests (no idea why M2 always fails).

Comment 14

[(no longer active)]

Comment on attachment 8593539 [details] [diff] [review]
Proposed patch, complete with test case

The M2 failures are almost definitely caused by the patch.  :-)

Comment 15

[Jorg K (CEST = GMT+2)]

Attachment: Proposed patch, complete with test case (conflict with bug 1140105 removed)

As I said in comment #11: This try run included the patch to bug 1140105 as well.
The try run to this bug (see bug 1140105 comment #28)
https://treeherder.mozilla.org/#/jobs?repo=try&revision=018f3fb1f578
most likely caused the M2 failures.

So I pulled the other patch out and submitted this change again:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=8820dca180fe

Comment 16

[Jorg K (CEST = GMT+2)]

Comment on attachment 8593758 [details] [diff] [review]
Proposed patch, complete with test case (conflict with bug 1140105 removed)

Here is the *clean* try run with *only* the patch from this current bug:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=8820dca180fe

The failure in M2 was caused by the accidentally included patch for bug 1140105 (where it had slipped the review). Refer to the other bug 1140105 for details.

Comment 17

[(no longer active)]

Comment on attachment 8593758 [details] [diff] [review]
Proposed patch, complete with test case (conflict with bug 1140105 removed)

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

Looks great, but minusing mainly for the comment about the test.

::: editor/libeditor/nsEditor.cpp
@@ +2609,5 @@
>  nsEditor::SplitNodeImpl(nsIContent& aExistingRightNode,
>                          int32_t aOffset,
>                          nsIContent& aNewLeftNode)
>  {
> +

Nit: please remove the unneeded blank line.

@@ +2627,5 @@
> +    for (uint32_t j = 0; j < range.mSelection->RangeCount(); ++j) {
> +      nsRefPtr<nsRange> r = range.mSelection->GetRangeAt(j);
> +      if (!r->IsPositioned()) {
> +        continue;
> +      }

You didn't need to copy this condition.  This is what GetStartNodeAndOffset and GetEndNodeAndOffset checked, and those functions were used in JoinNodesImpl.  They are not used here, so you're changing the semantics by this check (although I think it's harmless since these ranges should always be positioned.)

I think it's probably better to convert both this check and the similar one in JoinNodesImpl to |MOZ_ASSERT(r->IsPositioned());|.

::: editor/libeditor/tests/test_bug1154791.html
@@ +37,5 @@
> +      synthesizeKey(" ", {});
> +
> +      var sel = getSpellCheckSelection();
> +      is(sel.rangeCount, 1, "We should have one misspelled word");
> +      is(sel.getRangeAt(0), "thiss", "Correct misspelled word");

Please use a test case with two misspelled words, otherwise this test wouldn't test whether we can restore multiple ranges successfully.  My original patch for the previous bug was buggy and didn't handle this case correctly, and I didn't realize that since my test was originally examining only one misspelled range.

Comment 18

[Jorg K (CEST = GMT+2)]

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

> > +    for (uint32_t j = 0; j < range.mSelection->RangeCount(); ++j) {
> > +      nsRefPtr<nsRange> r = range.mSelection->GetRangeAt(j);
> > +      if (!r->IsPositioned()) {
> > +        continue;
> > +      }
> 
> You didn't need to copy this condition.  This is what GetStartNodeAndOffset
> and GetEndNodeAndOffset checked, and those functions were used in
> JoinNodesImpl.  They are not used here, so you're changing the semantics by
> this check (although I think it's harmless since these ranges should always
> be positioned.)
> 
> I think it's probably better to convert both this check and the similar one
> in JoinNodesImpl to |MOZ_ASSERT(r->IsPositioned());|.

Your instructions are not clear:
First you said "no need to copy" they you said "convert *both* this check and the similar one ...".
Sadly, a contradiction. Please choose one:
1) remove here and convert the other
1a) remove here and leave the other one as you originally had it
2) leave here and convert both
3) Other: please specify.

Comment 19

[(no longer active)]

I said you _didn't_ need to copy.  :-)  Please add the assertions in both places as I asked in comment 17.

Comment 20

[Jorg K (CEST = GMT+2)]

Attachment: Proposed patch, complete with test case (hopefully final version)

Here is the next and hopefully final round:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=ce581612c902
sadly with a few failures, but I guess that has nothing to do with us. All I changed in the code from the previous run was the MOZ_ASSERT(r->IsPositioned());

Frankly, English is not my native tongue despite living 16 years in Australia. "You didn't need to copy" can mean two things: "You didn't need to copy, but it's good that you did" or "You didn't need to copy, and please undo it". Looks like you meant the former (despite saying "you're changing the semantics").

Have a good weekend!

Comment 21

[(no longer active)]

(In reply to Jorg K (at the beach until 22nd April) from comment #20)
> Here is the next and hopefully final round:
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=ce581612c902
> sadly with a few failures, but I guess that has nothing to do with us. All I
> changed in the code from the previous run was the
> MOZ_ASSERT(r->IsPositioned());

None of them are related to your push, you're good!  :-)

> Frankly, English is not my native tongue despite living 16 years in
> Australia. "You didn't need to copy" can mean two things: "You didn't need
> to copy, but it's good that you did" or "You didn't need to copy, and please
> undo it". Looks like you meant the former (despite saying "you're changing
> the semantics").

Haha, it's not my native language either.  What I meant to say was "copying this part was not necessary for this fix, since the original code didn't call IsPositioned()."  Sorry for the confusion!

> Have a good weekend!

You too!

Comment 22

[(no longer active)]

Comment on attachment 8594206 [details] [diff] [review]
Proposed patch, complete with test case (hopefully final version)

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

Looks great, thanks a lot for your patch!

Comment 23

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/76d12a766103

Comment 24

[Carsten Book [:Tomcat]]

sorry had to back this out for test failures like https://treeherder.mozilla.org/logviewer.html#?job_id=9107041&repo=mozilla-inbound

Comment 25

[Pulsebot]

Backout:
https://hg.mozilla.org/integration/mozilla-inbound/rev/45d726bed683

Comment 26

[Jorg K (CEST = GMT+2)]

What's going on here??
01:23:28 INFO - 1693 INFO TEST-UNEXPECTED-FAIL | editor/libeditor/tests/test_bug1154791.html | Correct misspelled word - got thiss, expected thiss
01:23:28 INFO - 1694 INFO TEST-UNEXPECTED-FAIL | editor/libeditor/tests/test_bug1154791.html | Correct misspelled word - got onee, expected onee

The test says:
is(sel.getRangeAt(0), "thiss", "Correct misspelled word");
is(sel.getRangeAt(1), "onee", "Correct misspelled word");

We got what we expected and the test fails? How good is that?

Never failed on the try run:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=ce581612c902

Comment 27

[Jorg K (CEST = GMT+2)]

@Aryeh: Ehsan seems to be very busy, can you tell me what's going on here, please.

Comment 28

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

I don't know without debugging the test, which might be hard if you can't reproduce locally.  It could be the strings aren't the same but they look the same in the log output, like maybe there's trailing whitespace (although it looks like there's not).

It's also theoretically possible that it's not actually caused by your commit, because test runs on inbound are coalesced, i.e., the test suite is run once for many pushes.  The sherriffs do their best to back out the correct push, but mistakes do occasionally happen.  It doesn't seem likely that anyone else changed the spellchecker at the same time, though.

Unfortunately, I can't help much more than that without putting time into it, and I work only when on break from other things, which I no longer am.  Next break starts July 27, so if you haven't figured it out by then I could try to take a look.

Comment 29

[Jorg K (CEST = GMT+2)]

Attachment: Proposed patch, complete with test case (updated)

Carrying forward Ehsan's review+

Solved the riddle why the test failed:
Test was:
is(sel.getRangeAt(0), "thiss", "Correct misspelled word");
is(sel.getRangeAt(1), "onee", "Correct misspelled word");

After bug 949614 landed, this must now be:
is(String(sel.getRangeAt(0)), "thiss", "Correct misspelled word");
is(String(sel.getRangeAt(1)), "onee", "Correct misspelled word");

Fixed up the test and it's ready to go again.

Comment 30

[Jorg K (CEST = GMT+2)]

Works now: https://treeherder.mozilla.org/#/jobs?repo=try&revision=0dcaa065a2ad

Comment 31

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/85187d5c5219

Comment 32

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/85187d5c5219

Comment 33

[(no longer active)]

(In reply to Jorg K from comment #29)
> Solved the riddle why the test failed:
> Test was:
> is(sel.getRangeAt(0), "thiss", "Correct misspelled word");
> is(sel.getRangeAt(1), "onee", "Correct misspelled word");
> 
> After bug 949614 landed, this must now be:
> is(String(sel.getRangeAt(0)), "thiss", "Correct misspelled word");
> is(String(sel.getRangeAt(1)), "onee", "Correct misspelled word");

We recently switched the mochitest is() function from doing comparisons with == to doing them with ===.  In JS, comparing using == can convert one side of the comparison to another type, but === compares more strictly and returns false if the two sides are not the same type.  getRangeAt returns a Range object which is convertible to string, so before that change your patch was green on try, but you got unlucky and your patch landed after that change.

That should explain the failure and why converting the Range object to a String directly fixed it