Closed Bug 1726532 Opened 4 months ago Closed 2 months ago

Spell check corrects spelling but does not remove original spelling error

Categories

(Core :: Spelling checker, defect, P1)

Desktop
Windows 10
defect

Tracking

()

RESOLVED FIXED
93 Branch
Tracking Status
thunderbird_esr91 --- unaffected
firefox-esr78 --- unaffected
firefox-esr91 --- unaffected
firefox91 --- unaffected
firefox92 --- wontfix
firefox93 --- fixed

People

(Reporter: d2ogilvi, Assigned: masayuki)

References

(Blocks 1 open bug, Regression)

Details

(Keywords: regression)

Attachments

(3 files)

If I have the string "A nice picxnic in the park!", and do a spell check and press "replace all" for the "picxnic" spell error, the line is corrected to "A nice picnicpicxnic in the park!'... the old spelling error is not removed.

This is a regression of bug 1721801

Or more likely from bug 1726080.

Severity: -- → S3
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: needinfo?(masayuki)
Keywords: regression
Priority: -- → P1
Regressed by: 1726080
Duplicate of this bug: 1726572

We clearly need some automated tests in this area.

Is this really a regression of bug 1726080? I bet that it's wrong. See the patch, I didn't change any business logic. I guess that this is a regression of bug 1718924.

(In reply to Wayne Mery (:wsmwk) from comment #4)

We clearly need some automated tests in this area.

Please help to write spellchecker tests in m-c. Firefox does not have complicated features for it. I really love tb developers to contribute writing in tests into m-c.

See bug 1612329 for writing the tests. I usually add tests for comm-central specific API, but it's impossible to do it only by me!

Okay, it seems that selection management in TextServicesDocument is broken (and perhaps, it's been already broken before the refactoring in edge cases).

Assignee: nobody → masayuki
Status: NEW → ASSIGNED
Component: Message Compose Window → Spelling checker
Flags: needinfo?(masayuki)
Product: Thunderbird → Core
Version: Thunderbird 93 → unspecified

Really odd. I cannot write a mochitest which run nsIEditorSpellCheck.ReplaceWord() in <textarea>. It seems that even if there are spellcheck selection ranges, TextServicesDocument does not store text as expected. So, the loop in mozSpellChecker::Replace() cannot find wrong word. Why??

Ah, okay, I can write a test with contenteditable, but I hit infinite loop with commenting out a lot of assertions...

This is a regression point:
https://searchfox.org/mozilla-central/diff/886c96059ef6408618040356017028621bc574b9/editor/spellchecker/TextServicesDocument.cpp#1662

But ReplaceWord() does not work as expected with my testcase. If a word which should be replaced is start of a text node, it's not replaced... Keep investigating...

Confirmed that this is a regression of bug 1718924.

Regressed by: 1718924
No longer regressed by: 1726080

And only fixing the regression of bug 1718924 is not enough. Due to another regression of bug 1311934, misspelled words immediately after <br> element isn't replaced correctly.

Regressed by: 1311934

This is a mistake at replacing the argument value aOffset to
aOffsetInTextInBlock.
https://searchfox.org/mozilla-central/diff/886c96059ef6408618040356017028621bc574b9/editor/spellchecker/TextServicesDocument.cpp#1662

And also make mozSpellChecker::Replace() stop replacing if
TextServicesDocument fails to set selection or insert text because if
TextServicesDocument succeeded to insert text, but failed to delete text,
the loop becomes an infinite loop.

TextServicesDocument::LastSelectedBlock() may return NS_OK with
UINT32_MAX for both offset and length of the block. The assertions causes
the new test not runnable in debug build. So, we should make the situation
as an error case if they are required.

Ideally, we should fix the odd behavior, but it's not safe for uplift.
Therefore, this patch just adds the wallpapers.

Depends on D123282

This is a regression of bug 1311934. The traditional
TextServicesDocument::IsBlockNode() was wrong name, the users of the method
intent to check whether a word is not split by the found sibling. Therefore,
the method returned true for <br> element. However, of course,
HTMLEditUtils::IsBlockElement() is not so because of an inline element.

For making the callers clearer, this patch adds <br> element check into
each place. Without this patch, the new test fails to replace a misspelled
word immediately after <br> element.

Depends on D123283

Works now for me on TB DAILY. Thanks! :)

Flags: qe-verify+

I tried to reproduce this issue on Firefox Nightly (93.0a1 from 19 Aug 2021) on Win10, but I was not able to reproduce it. What I did was to open a google translate page and selected the text entered there in the search text and from right click did a Spell check.
Can you please confirm the issue was reproduced on Firefox build? And if so can you please provide detailed steps?

Thank you.

Flags: needinfo?(d2ogilvi)
Flags: needinfo?(d2ogilvi) → needinfo?(masayuki)

Wrong comment. Still need info.

Status: RESOLVED → VERIFIED
Flags: qe-verify+
Status: VERIFIED → RESOLVED
Closed: 3 months ago2 months ago
Flags: qe-verify+

No, I couldn't reproduce this bug with user operation in Firefox, and perhaps, it's not reproducible in content process at least since this is a bug only when calling an API which can run only in chrome process. Therefore, I added the new test to reproduce this bug in Firefox.

Flags: needinfo?(masayuki)

Thanks Masayuki. Removing qe+ flag, as we can not do anymore testing on this.

Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.