1424839 - Using right-click to correct misspelled word removes underlines from other misspelled words

Status: VERIFIED FIXED

(Core :: Spelling checker, defect)

Description

[Jorg K (CEST = GMT+2)]

Nightly 59.0a1 (2017-12-11) (64-bit):

STR:
Visit BMO and type into a comment box:

Thiss iss bad.

Correct "iss" using the right-click menu.

Unwanted side-effect. Underline removed from Thiss. After that, things go crazy: Double-clicking Thiss brings the underlines back, clicking elsewhere removes them again.

Most likely related to recent editor changes.

Alice, can you please find the regression.

Comment 1

[Alice0775 White]

Reproducible: not always

Steps to Reproduce:
1. Open data:text/html,<textarea autofocus>
2. Type as follows
   thiss iss bad
3. Right click on "iss" and choose correct spell.


Regression window:
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=0af5308b7c76e503d1bcb6e6f8fda176461e76ae&tochange=3b2fc3875b516ea82ff90a3c11294538b0aa2555

Regressed by: Bug 1408125

Comment 2

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away until 10/14)]

(In reply to Alice0775 White from comment #1)
> Reproducible: not always
> 
> Steps to Reproduce:
> 1. Open data:text/html,<textarea autofocus>
> 2. Type as follows
>    thiss iss bad
> 3. Right click on "iss" and choose correct spell.
> 
> 
> Regression window:
> https://hg.mozilla.org/integration/autoland/
> pushloghtml?fromchange=0af5308b7c76e503d1bcb6e6f8fda176461e76ae&tochange=3b2f
> c3875b516ea82ff90a3c11294538b0aa2555
> 
> Regressed by: Bug 1408125

Hmm, I built debug build with 0af5308b7c76 which is parent changeset of the first patch of bug 1408125. However, I see the odd underline behavior when clicking on the text node too. I see disappearing underlines when I click the text node. Otherwise, wen I click anonymous div element in the <textarea>, the underlines are back.

Comment 3

[Alice0775 White]

(In reply to Masayuki Nakano [:masayuki] (JST, +0900) from comment #2)
> 
> Hmm, I built debug build with 0af5308b7c76 which is parent changeset of the
> first patch of bug 1408125. However, I see the odd underline behavior when
> clicking on the text node too. I see disappearing underlines when I click
> the text node. Otherwise, wen I click anonymous div element in the
> <textarea>, the underlines are back.


Yes, I can reproduce the good build. it seems to be timing issue?


Correct range:
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=87305b25064ef513e0890696849bf0b06c48a586&tochange=b2db8b6d26d0828c6db93237e7050756ac0907be

Regressed by:b2db8b6d26d0	Adam Gashlin — Bug 1374338 - Search all ranges to avoid filtering r=mats


@:agashlin

Your patch seems to cause the regression.  Can you look in to this?

Comment 4

[Adam Gashlin (he/him) [:agashlin] (ex-moco)]

I suspect this is due to the "return false" inside the binary search, this should instead be "continue" to keep checking different selections. I'll test and hopefully fix this shortly.

Comment 5

[Jorg K (CEST = GMT+2)]

Great, thanks. And sorry for blaming this on editor changes.

It would be good to have a test, but I realise that spellcheck tests can be a pain to write (I wrote a few). If you feel so inclined, grep for SELECTION_SPELLCHECK in editor/libeditor/tests, editor/composer/test or elsewhere. Tests checking the spellcheck underlines use this. There are even tests which correct the spelling via the right-click menu, so it's doable.

Comment 6

[Adam Gashlin (he/him) [:agashlin] (ex-moco)]

Replacing the `return false` with `break` fixes the issue in my testing, this makes sense as we need to exit the binary search and then continue with the outer loop for the remaining selections.

Unfortunately it's going to be tricky to test this, the visible breakage is only graphical and ends up being exposed only due to an optimization [1] which doesn't go down the selection painting path if the frame isn't marked as selected way back here [2] (which ultimately calls IsNodeSelected).

I would have to expose the IsNodeSelected logic somehow in order to make this straightforwardly testable, I will look into how to do that but in the meantime I will submit the fix by itself.

[1] https://searchfox.org/mozilla-central/rev/110706c3c09d457dc70293b213d7bccb4f6f5643/layout/generic/nsTextFrame.cpp#7021
[2] https://searchfox.org/mozilla-central/rev/110706c3c09d457dc70293b213d7bccb4f6f5643/layout/generic/nsTextFrame.cpp#5228

Comment 7

[Adam Gashlin (he/him) [:agashlin] (ex-moco)]

Attachment: Bug 1424839 - Continue checking selections after a collapsed one

Review commit: https://reviewboard.mozilla.org/r/207712/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/207712/

Comment 8

[Mats Palmgren (inactive)]

Comment on attachment 8936970 [details]
Bug 1424839 - Continue checking selections after a collapsed one

https://reviewboard.mozilla.org/r/207712/#review213592

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/8b6af5a37d64
Continue checking selections after a collapsed one r=mats

Comment 10

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/8b6af5a37d64

Comment 11

[Jorg K (CEST = GMT+2)]

Thanks to all involved!