596333 - Automatic spell checking not always functional

Status: VERIFIED FIXED

(Core :: Spelling checker, defect)

Description

[Ryan Jones-Ward [:sciguyryan]]

In the most recently Minefield hourly the automatic spell checking feature does not always funtction. Sometimes you have to manually check in order for spelling errors to be identified.

This could be due to some of the recently editor changes that landed.

Comment 1

[Jim Jeffery not reading bug-mail 1/2/11]

regression range
20100913162558 ccaffbc6a970 bad 
20100913165011 68033b993ed7 OK

Comment 2

[(no longer active)]

Bug 240933 is the most likely cause here.

Comment 3

[(no longer active)]

Regression range: http://hg.mozilla.org/mozilla-central/pushloghtml?tochange=ccaffbc6a970&fromchange=68033b993ed7

I'm bisecting right now to figure out the offending changeset.

Comment 4

[(no longer active)]

The offending changeset: http://hg.mozilla.org/mozilla-central/rev/1f29b4fb184c

Comment 5

[(no longer active)]

It seems to me that the spell checker has been relying on a particular behavior from the editor when doing spell checking when typing.

What happens is that when you're typing, we don't spell check the word that the caret is on right now.  Therefore, (assuming that | represents the caret), when you're typing:

workd|

we won't spell check "workd".  When you press space, however:

workd |

the caret is no longer on the word, and we spell check the word immediately before the space.

The old behavior of the editor was that if you typed "workd ", it would inject five adjacent text nodes, each containing one of the letters.  Then, when this loop <http://mxr.mozilla.org/mozilla-central/source/extensions/spellcheck/src/mozInlineSpellWordUtil.cpp#562> was executed for the first time, checkBeforeOffset would be 0 (being at the start of the text node containing " "), so ContainsDOMWordSeparator would return false, so we'd end up looking at the previous text nodes, which would mean that mSoftText would end up being "workd " and we'd end up checking "workd" for misspellings.

With 1f29b4fb184c, we only have a single textnode, which means that in the above loop, checkBeforeOffset would be 5, and ContainsDOMWordSeparator would return true, which causes us not to look at the entire text node, so mSoftText would be " ", and we would just skip over "workd" when spell checking.

I have a patch to correct this behavior, which I'll attach to this bug soon.

Comment 6

[(no longer active)]

Attachment: Patch (v1)

This patch special cases the situation where ContainsDOMWordSeparator finds the separator in the same node, and tries to find the previous word separator (or just use 0 in case we're after the first entered word) and look at that word as well.

Comment 7

[(no longer active)]

When discussing this patch with Boris on IRC, he asked me what happens if you enter "workd" and then paste " x" (or "  x").  This got me thinking, and I think replacing checkBeforeOffset with firstOffsetInNode in the 2nd ContainsDOMWordSeparator call takes care of those paste cases.  I'll update the patch and the test shortly.

Comment 8

[(no longer active)]

Attachment: Patch (v2)

Based on comment 7.

Comment 9

[(no longer active)]

Attachment: Part 2

Boris brought a good point to my attention, that my analysis in comment 7 will not be valid for "   x", for example.  The only good way to handle such cases would be to make ContainsDOMWordSeparator greedy.  This patch does that.

Comment 10

[Boris Zbarsky [:bzbarsky]]

So if we enter that inner loop when i points to the last char of a run of whitespace that comes at the beginning of the text fragment, then we will exit the loop without changing i, right?  Is that desired?  Or do we want to set *aSeparatorOffset to 0 in that situation?

Comment 11

[(no longer active)]

Attachment: Part 2

(In reply to comment #10)
> So if we enter that inner loop when i points to the last char of a run of
> whitespace that comes at the beginning of the text fragment, then we will exit
> the loop without changing i, right?  Is that desired?  Or do we want to set
> *aSeparatorOffset to 0 in that situation?

I'm not sure if it really matters, because if the string begins with whitespace, then by definition there's nothing before it which we may be missing, but yeah, I guess for consistency, we'd want to set *aSeparatorOffset to 0 in that case.

Comment 12

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 475867 [details] [diff] [review]
Part 2

r=me

Comment 13

[(no longer active)]

http://hg.mozilla.org/mozilla-central/rev/dd8fa58848c0
http://hg.mozilla.org/mozilla-central/rev/85bcddd5576c

Comment 14

[u88484]

Seems to be working again. Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0b7pre) Gecko/20100917 Firefox/4.0b7pre

I'll wait a day or so or let someone else confirm before marking this verified.

Comment 15

[klonos]

latest x64 build dates 16-Sep-2010 05:30

http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/latest-trunk/

...so I cannot really check if this got solved. Can we please have an updated x64 build that includes the fix?

Thanx in advance.

Comment 16

[klonos]

...updated platform, since this happens for me in x64 Win7 as well.

Comment 17

[(no longer active)]

This is not x64 specific, it was a bug on all platforms and all architectures.

Comment 18

[klonos]

...I know, that's why I've set it to 'All' (it was set to 'x86' before). I still do not see a new win64 build available though (latest x64 build dates 16-Sep-2010 05:30 and still has the issue). Thus this is not fixed for win64.

Comment 19

[(no longer active)]

(In reply to comment #18)
> ...I know, that's why I've set it to 'All' (it was set to 'x86' before). I
> still do not see a new win64 build available though (latest x64 build dates
> 16-Sep-2010 05:30 and still has the issue). Thus this is not fixed for win64.

Please file a bug in mozilla.org::Release Engineering about the Win64 builds not being available.  It doesn't have anything to do with this bug.

Comment 20

[klonos]

...did so: Bug 599559. Thanx for pointing to the right direction.

Comment 21

[klonos]

...and it's fixed in win64 too ;)