Last Comment Bug 816073 - Wrongly spelled words not underlined if modifying other words in a sequence
: Wrongly spelled words not underlined if modifying other words in a sequence
Status: RESOLVED DUPLICATE of bug 1154791
: regression
Product: Core
Classification: Components
Component: Spelling checker (show other bugs)
: 15 Branch
: All All
: -- normal (vote)
: ---
Assigned To: Nobody; OK to take it and work on it
:
Mentors:
Depends on:
Blocks: 590640
  Show dependency treegraph
 
Reported: 2012-11-28 06:31 PST by Virgil Dicu [:virgil] [QA]
Modified: 2015-12-26 15:29 PST (History)
8 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Not-very-good patch (3.90 KB, patch)
2013-04-03 08:11 PDT, :Aryeh Gregor (away until August 15)
ehsan: review+
ehsan: feedback? (daniel)
Details | Diff | Review

Description Virgil Dicu [:virgil] [QA] 2012-11-28 06:31:26 PST
STR:
Have spell checking enabled
1. Open http://www-archive.mozilla.org/editor/midasdemo/
2. Change font to Arial (couldn't reproduce with the default font)
3. Type "oner twor threer fourr fiverr sicer sevenr"
4. Right click on "fourr" and select a valid word.

Actual result: 
The first two misspelled words (oner twor) are not underlined.

Could also reproduce on mail.yahoo.com and gmail.com


Last good nightly: 2012-05-18
First bad nightly: 2012-05-19

Pushlog:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=e794cef56df6&tochange=642d1a36702f

Suspected: bug 755480
Comment 1 :Ehsan Akhgari (busy, don't ask for review please) 2012-11-28 11:22:25 PST
Aryeh, can you look into this, please?  Thanks!
Comment 2 :Aryeh Gregor (away until August 15) 2013-03-14 05:44:36 PDT
I don't think this is bug 755480.  Alice, would you be interested in finding a regression range?  I suspect this wouldn't be too hard to track down if I knew which changeset regressed it.
Comment 3 Alice0775 White 2013-03-14 05:57:35 PDT
Regression window(m-i)
Good:
http://hg.mozilla.org/integration/mozilla-inbound/rev/11780e80c8c3
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:15.0) Gecko/15.0 Firefox/15.0a1 ID:20120517220915
Bad:
http://hg.mozilla.org/integration/mozilla-inbound/rev/e8ebc8f1825e
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:15.0) Gecko/15.0 Firefox/15.0a1 ID:20120517232316
Pushlog:
http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=11780e80c8c3&tochange=e8ebc8f1825e
Comment 4 Alice0775 White 2013-03-14 12:17:39 PDT
Last Good: 5161427d7c4c
First Bad: e8ebc8f1825e

Triggered by:
e8ebc8f1825e	Aryeh Gregor — Bug 590640 part 7 - Preserve type-in state when performing block commands; r=ehsan
Comment 5 :Aryeh Gregor (away until August 15) 2013-03-15 03:14:05 PDT
Thanks!
Comment 6 :Aryeh Gregor (away until August 15) 2013-03-17 06:55:28 PDT
More minimal test-case:

data:text/html,<!DOCTYPE html>
<div contenteditable>
<font face=Arial>baz baz baz baz</font>
</div>

Right-click the third "baz" and pick a corrected version, e.g., "bass".  The first "baz" loses its underline.  The bug doesn't occur if you remove a "baz", correct a different "baz", remove the <font> tag, remove the face=Arial attribute on the <font> tag, or otherwise change the <font> tag to a non-formatting tag (e.g., <span>, even with style="font-family:arial").  It still occurs if you change <font face=Arial> to another formatting tag like <b> or <i>.

When the bug occurs, the text node is split into three: "baz baz ", "bass", " baz".  When the bug doesn't occur, the text node remains whole.

Questions:

1) Why does the text node ever get split?  It should be modified in-place.

2) If it does get split, why does that remove the squiggles from non-final words in the initial text node?  Perhaps we trigger a recheck only at the end of the previous node and start of the next node, and the recheck only does the current word and subsequent words.
Comment 7 :Aryeh Gregor (away until August 15) 2013-03-17 07:07:37 PDT
I spent a while poking around, but couldn't find what code gets run when the user selects a spelling correction.  I'm not totally sure it's even in extensions/spellcheck/.  Ehsan, do you know offhand?
Comment 8 :Ehsan Akhgari (busy, don't ask for review please) 2013-03-19 08:01:46 PDT
Yes.  This is the UI code which handles replacing the misspelled words: <http://mxr.mozilla.org/mozilla-central/source/toolkit/content/InlineSpellChecker.jsm#266> which calls into <http://mxr.mozilla.org/mozilla-central/source/extensions/spellcheck/src/mozInlineSpellChecker.cpp#784>.  I think the correct fix here is to figure out why the InsertText call there injects a new text node, and just make it stick the text inside an existing text node if possible...
Comment 9 :Aryeh Gregor (away until August 15) 2013-04-03 07:34:55 PDT
So I narrowed it down to nsHTMLEditRules::CreateStyleForInsertText.  It looks like type-in state is set somehow, so it wants to insert a style tag prior to inserting the text, but then it eventually doesn't bother inserting the tag.  I need to figure out why it's not inserting the tag after all, and move that check earlier so it doesn't split the text node either.
Comment 10 :Aryeh Gregor (away until August 15) 2013-04-03 08:11:33 PDT
Created attachment 732856 [details] [diff] [review]
Not-very-good patch

This fixes the problem, but only by duplicating logic in a fragile way.  It's also not self-evident to me that it's correct, and I wouldn't bet too much money that it doesn't cause regressions.  But I don't see any better way to fix the problem.  Any ideas?

The editing spec solves this by remembering the type-in state before doing the insert, inserting the text without regard to styles, then selecting the text that was just inserted and restoring the type-in state afterwards.  The restoration process invokes high-level algorithms that sit just under execCommand(), which set the style on the selection and handle any splitting needed themselves.  This works out to be far neater than we do, but obviously, there's no way I'm touching such a project with a ten-foot-pole.

Also, how should I test this?  There's some JS API I can use to pick a spelling suggestion, right?

Try: https://tbpl.mozilla.org/?tree=Try&rev=2a01680f0334
Comment 11 :Ehsan Akhgari (busy, don't ask for review please) 2013-04-03 08:22:19 PDT
For testing this, couldn't you just test that a new textnode is not inserted in the DOM?  That seems much less fragile than using the spellchecker API in the test.
Comment 12 :Ehsan Akhgari (busy, don't ask for review please) 2013-04-03 08:22:59 PDT
Comment on attachment 732856 [details] [diff] [review]
Not-very-good patch

Review of attachment 732856 [details] [diff] [review]:
-----------------------------------------------------------------

At first glance I don't see anything obviously wrong with this patch, but as Aryeh mentions it's probably not ideal.  Daniel, what do you think?
Comment 13 :Aryeh Gregor (away until August 15) 2013-04-03 08:36:14 PDT
Yeah, you're right -- all the spell correction is doing is calling insertText, so I'll just replicate the bug using only insertText.  Good idea.
Comment 14 Ian Neal 2013-07-02 05:34:57 PDT
Is there anyone else that can provide feedback? Maybe kaze?
Comment 15 :Ehsan Akhgari (busy, don't ask for review please) 2013-07-02 09:33:13 PDT
(In reply to Ian Neal from comment #14)
> Is there anyone else that can provide feedback? Maybe kaze?

I'd really like Daniel's feedback on this... :/
Comment 16 Daniel Glazman (:glazou) 2013-07-17 11:25:08 PDT
Two things:

1. I think nsHTMLEditRules::IsPropertySet is interesting enough to be promoted to
   a public method in nsIHTMLEditor

2. the change to the InserTText transaction seems to me a little more problematic; I have a test case in mind and I'll test ASAP. As I told ehsan earlier, I have tons of conf calls today and I'll be able to build this only very late tonight or tomorrow morning. I promise to report back here tomorrow evening (french time) at the latest.
Comment 17 :Ehsan Akhgari (busy, don't ask for review please) 2013-07-17 14:27:11 PDT
(In reply to Daniel Glazman (:glazou) from comment #16)
> Two things:
> 
> 1. I think nsHTMLEditRules::IsPropertySet is interesting enough to be
> promoted to
>    a public method in nsIHTMLEditor

Are you planning on using it in BlueGriffon?  I prefer not to inflate the API set unless there's a good reason.

> 2. the change to the InserTText transaction seems to me a little more
> problematic; I have a test case in mind and I'll test ASAP. As I told ehsan
> earlier, I have tons of conf calls today and I'll be able to build this only
> very late tonight or tomorrow morning. I promise to report back here
> tomorrow evening (french time) at the latest.

Sounds good, thanks a lot!
Comment 18 Daniel Glazman (:glazou) 2013-07-18 01:58:17 PDT
(In reply to :Ehsan Akhgari (needinfo? me!) from comment #17)

> > 2. the change to the InserTText transaction seems to me a little more
> > problematic; I have a test case in mind and I'll test ASAP. As I told ehsan
> > earlier, I have tons of conf calls today and I'll be able to build this only
> > very late tonight or tomorrow morning. I promise to report back here
> > tomorrow evening (french time) at the latest.
> 
> Sounds good, thanks a lot!

Ok, I tested the patch and I think it's mostly harmless, even in the weirdest cases I could imagine...

BUT after a look at the spellchecker's code, I think the patch is a wrong strategy. The problem comes from mozInlineSpellChecker::ReplaceWord that calls nsIHTMLEditor::InsertText and that is wrong because the complex machinery done by InsertText - in particular the type state management - should not apply in this case. We're not entering or typing text. We're programmatically replacing a selection by some characterData, most probably unrelated to the current type state... So I think a better strategy would be to replace the call to InsertText there by the creation and application of a single InsertTextTxn transaction for the text to insert, at the collapsed selection's position.
Comment 19 :Ehsan Akhgari (busy, don't ask for review please) 2013-07-19 14:58:18 PDT
(In reply to comment #18)
> (In reply to :Ehsan Akhgari (needinfo? me!) from comment #17)
> 
> > > 2. the change to the InserTText transaction seems to me a little more
> > > problematic; I have a test case in mind and I'll test ASAP. As I told ehsan
> > > earlier, I have tons of conf calls today and I'll be able to build this only
> > > very late tonight or tomorrow morning. I promise to report back here
> > > tomorrow evening (french time) at the latest.
> > 
> > Sounds good, thanks a lot!
> 
> Ok, I tested the patch and I think it's mostly harmless, even in the weirdest
> cases I could imagine...
> 
> BUT after a look at the spellchecker's code, I think the patch is a wrong
> strategy. The problem comes from mozInlineSpellChecker::ReplaceWord that calls
> nsIHTMLEditor::InsertText and that is wrong because the complex machinery done
> by InsertText - in particular the type state management - should not apply in
> this case. We're not entering or typing text. We're programmatically replacing
> a selection by some characterData, most probably unrelated to the current type
> state... So I think a better strategy would be to replace the call to
> InsertText there by the creation and application of a single InsertTextTxn
> transaction for the text to insert, at the collapsed selection's position.

Hmm, that would be a bit complicated since I really prefer the transaction APIs to not be accessed outside of editor/, so we should probably expose an API for that.
Comment 20 Daniel Glazman (:glazou) 2013-07-19 23:45:34 PDT
(In reply to :Ehsan Akhgari (needinfo? me!) from comment #19)

> Hmm, that would be a bit complicated since I really prefer the transaction
> APIs to not be accessed outside of editor/, so we should probably expose an
> API for that.

Well... A spellchecker making sense only inside an editor of some sort (document
editor, text field, text area, etc..), I don't see this as an issue at all.
After all, the spellchecker directly calls the editor that is its sole user.
I still think the proposed strategy in the current patch is not optimal.
Comment 21 :Ehsan Akhgari (busy, don't ask for review please) 2013-07-22 13:41:02 PDT
(In reply to Daniel Glazman (:glazou) from comment #20)
> (In reply to :Ehsan Akhgari (needinfo? me!) from comment #19)
> 
> > Hmm, that would be a bit complicated since I really prefer the transaction
> > APIs to not be accessed outside of editor/, so we should probably expose an
> > API for that.
> 
> Well... A spellchecker making sense only inside an editor of some sort
> (document
> editor, text field, text area, etc..), I don't see this as an issue at all.
> After all, the spellchecker directly calls the editor that is its sole user.

It feels a bit dirty to me, but it won't kill us. :-)

> I still think the proposed strategy in the current patch is not optimal.

OK, hopefully someone will find some time to write the patch!  This is on my very long todo list, but it's been a while since I managed to knock anything off of that list...
Comment 22 :Aryeh Gregor (away until August 15) 2014-11-29 09:20:30 PST
(I don't expect to work on this more anytime soon, unless Ehsan wants me to.)
Comment 23 Jorg K (GMT+2, PTO during summer, NI me) 2015-12-26 15:29:19 PST
I tried the minimal test case from comment #6 and the test case from comment #0.

Replacing a misspelled word with a suggestion maintains the spell check marks on the other words.

Also I read in comment #6 that this has to do with splitting a node. Well, that didn't treat the spell check selection correctly. This was corrected in bug 1154791. So I'm making this bug here a duplicate.

*** This bug has been marked as a duplicate of bug 1154791 ***

Note You need to log in before you can comment on or make changes to this bug.