Closed Bug 816073 Opened 12 years ago Closed 8 years ago

Wrongly spelled words not underlined if modifying other words in a sequence

Categories

(Core :: Spelling checker, defect)

15 Branch
defect
Not set
normal

Tracking

()

RESOLVED DUPLICATE of bug 1154791

People

(Reporter: virgil.dicu, Unassigned)

References

Details

(Keywords: regression)

Attachments

(1 file)

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
OS: Linux → All
Hardware: x86 → All
Aryeh, can you look into this, please?  Thanks!
Assignee: nobody → ayg
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.
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
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
Blocks: 590640
Thanks!
Status: NEW → ASSIGNED
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.
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?
Flags: needinfo?(ehsan)
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...
Flags: needinfo?(ehsan)
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.
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
Attachment #732856 - Flags: review?(ehsan)
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 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?
Attachment #732856 - Flags: feedback?(daniel)
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.
Attachment #732856 - Flags: review?(ehsan) → review+
Is there anyone else that can provide feedback? Maybe kaze?
Flags: needinfo?(ehsan)
(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... :/
Flags: needinfo?(ehsan)
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.
(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!
(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.
(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.
(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.
(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...
(I don't expect to work on this more anytime soon, unless Ehsan wants me to.)
Assignee: ayg → nobody
Status: ASSIGNED → NEW
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.
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → DUPLICATE
Attachment #732856 - Flags: feedback?(daniel)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: