Closed Bug 1424839 Opened 2 years ago Closed 2 years ago

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

Categories

(Core :: Spelling checker, defect)

defect
Not set

Tracking

()

VERIFIED FIXED
mozilla59
Tracking Status
firefox-esr52 --- unaffected
firefox57 --- unaffected
firefox58 --- unaffected
firefox59 --- fixed

People

(Reporter: jorgk-bmo, Assigned: agashlin)

References

Details

(Keywords: regression)

Attachments

(1 file)

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.
Flags: needinfo?(masayuki)
Flags: needinfo?(alice0775)
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
Blocks: 1408125
Flags: needinfo?(alice0775)
(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.
Flags: needinfo?(masayuki)
(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?
Blocks: 1374338
No longer blocks: 1408125
Flags: needinfo?(agashlin)
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.
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.
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
Flags: needinfo?(agashlin)
Assignee: nobody → agashlin
Status: NEW → ASSIGNED
Attachment #8936970 - Flags: review?(mats)
Comment on attachment 8936970 [details]
Bug 1424839 - Continue checking selections after a collapsed one

https://reviewboard.mozilla.org/r/207712/#review213592
Attachment #8936970 - Flags: review?(mats) → review+
Keywords: checkin-needed
Pushed by aciure@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/8b6af5a37d64
Continue checking selections after a collapsed one r=mats
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/8b6af5a37d64
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
Thanks to all involved!
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.