Closed Bug 1424839 Opened 2 years ago Closed 2 years ago
Using right-click to correct misspelled word removes underlines from other misspelled words
59 bytes, text/x-review-board-request
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.
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
(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.
(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?
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  which doesn't go down the selection painting path if the frame isn't marked as selected way back here  (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.  https://searchfox.org/mozilla-central/rev/110706c3c09d457dc70293b213d7bccb4f6f5643/layout/generic/nsTextFrame.cpp#7021  https://searchfox.org/mozilla-central/rev/110706c3c09d457dc70293b213d7bccb4f6f5643/layout/generic/nsTextFrame.cpp#5228
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+
Pushed by firstname.lastname@example.org: https://hg.mozilla.org/integration/autoland/rev/8b6af5a37d64 Continue checking selections after a collapsed one r=mats
Thanks to all involved!
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.