The default bug view has changed. See this FAQ.

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

RESOLVED FIXED in Firefox 40

Status

()

Core
Editor
RESOLVED FIXED
2 years ago
a year ago

People

(Reporter: Jorg K (GMT+1), Assigned: Jorg K (GMT+1))

Tracking

Trunk
mozilla40
x86_64
All
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox40 fixed)

Details

Attachments

(2 attachments, 5 obsolete attachments)

(Assignee)

Description

2 years ago
Created attachment 8592891 [details]
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.
(Assignee)

Comment 1

2 years ago
Need info for you kind attention - as per bug 1100966 comment #29 ;-)
Flags: needinfo?(ehsan)
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.
Flags: needinfo?(ehsan)
(Assignee)

Comment 3

2 years ago
OK, after the 23rd of April. Debugging is no fun on a small travel laptop.
(Assignee)

Comment 4

2 years ago
Created attachment 8593304 [details]
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.
Flags: needinfo?(ehsan)
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.
Flags: needinfo?(ehsan)
(Assignee)

Comment 6

2 years ago
Will do, thanks.
(Assignee)

Comment 7

2 years ago
Created attachment 8593513 [details] [diff] [review]
Proposed patch, tests still missing
Attachment #8593513 - Flags: feedback?(ehsan)
(Assignee)

Comment 8

2 years ago
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.
Attachment #8593304 - Attachment is obsolete: true
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!
Attachment #8593513 - Flags: feedback?(ehsan) → feedback-
(Assignee)

Comment 10

2 years ago
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.
Flags: needinfo?(ehsan)
(Assignee)

Comment 11

2 years ago
Created attachment 8593539 [details] [diff] [review]
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://mozilla@jorgk.com@hg.mozilla.org/try
hg push -f -r tip mc-try
Attachment #8593513 - Attachment is obsolete: true
Attachment #8593539 - Flags: review?(ehsan)
(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!
Flags: needinfo?(ehsan)

Updated

2 years ago
Attachment #8593539 - Flags: review?(ehsan)
(Assignee)

Comment 13

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

Passed the Mochitests (no idea why M2 always fails).
Attachment #8593539 - Flags: review?(ehsan)
Comment on attachment 8593539 [details] [diff] [review]
Proposed patch, complete with test case

The M2 failures are almost definitely caused by the patch.  :-)
Attachment #8593539 - Flags: review?(ehsan)
(Assignee)

Comment 15

2 years ago
Created attachment 8593758 [details] [diff] [review]
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
Attachment #8593539 - Attachment is obsolete: true
(Assignee)

Comment 16

2 years ago
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.
Attachment #8593758 - Flags: review?(ehsan)
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.
Attachment #8593758 - Flags: review?(ehsan) → review-
(Assignee)

Comment 18

2 years ago
(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.
Flags: needinfo?(ehsan)
I said you _didn't_ need to copy.  :-)  Please add the assertions in both places as I asked in comment 17.
Flags: needinfo?(ehsan)
(Assignee)

Comment 20

2 years ago
Created attachment 8594206 [details] [diff] [review]
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!
Attachment #8593758 - Attachment is obsolete: true
Attachment #8594206 - Flags: review?(ehsan)
(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 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!
Attachment #8594206 - Flags: review?(ehsan) → review+

Updated

2 years ago
Keywords: checkin-needed
https://hg.mozilla.org/integration/mozilla-inbound/rev/76d12a766103
Assignee: nobody → mozilla
Keywords: checkin-needed
sorry had to back this out for test failures like https://treeherder.mozilla.org/logviewer.html#?job_id=9107041&repo=mozilla-inbound
Flags: needinfo?(mozilla)

Comment 25

2 years ago
Backout:
https://hg.mozilla.org/integration/mozilla-inbound/rev/45d726bed683
(Assignee)

Comment 26

2 years ago
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
Flags: needinfo?(mozilla) → needinfo?(ehsan)
(Assignee)

Comment 27

2 years ago
@Aryeh: Ehsan seems to be very busy, can you tell me what's going on here, please.
Flags: needinfo?(ayg)
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.
Flags: needinfo?(ayg)
(Assignee)

Comment 29

2 years ago
Created attachment 8596083 [details] [diff] [review]
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.
Attachment #8594206 - Attachment is obsolete: true
Flags: needinfo?(ehsan)
Attachment #8596083 - Flags: review+
(Assignee)

Updated

2 years ago
Keywords: checkin-needed
(Assignee)

Comment 30

2 years ago
Works now: https://treeherder.mozilla.org/#/jobs?repo=try&revision=0dcaa065a2ad
https://hg.mozilla.org/integration/mozilla-inbound/rev/85187d5c5219
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/85187d5c5219
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox40: affected → fixed
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
(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
Depends on: 1158920

Updated

2 years ago
Blocks: 1159815
(Assignee)

Updated

2 years ago
Duplicate of this bug: 670209

Updated

2 years ago
Blocks: 1177785
(Assignee)

Updated

a year ago
Duplicate of this bug: 816073
You need to log in before you can comment on or make changes to this bug.