Closed Bug 1581774 Opened 4 years ago Closed 4 years ago

Spellcheck doesn't check message body any more when started in the subject

Categories

(Core :: DOM: Editor, defect, P2)

Product:
Core
Component:
DOM: Editor
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla71
Tracking Flags:
Tracking Status
firefox-esr60 --- unaffected
firefox-esr68 --- wontfix
firefox67 --- wontfix
firefox68 --- wontfix
firefox69 --- wontfix
firefox70 --- wontfix
firefox71 --- fixed

People

(Reporter: khushil324, Assigned: jorgk-bmo)

References

(Regression)

Details

(Keywords: regression)

Attachments

(2 files, 4 obsolete files)

Bug 1581774 - revert part of rev 5e2fad03c885 from bug 1507543 for Thunderbirds since it breaks the spellcheck.
47 bytes, text/x-phabricator-request
Details | Review
1581774-fix-spell.patch
1.81 KB, patch
jorgk-bmo
: review+
Details | Diff | Splinter Review

This error is shown when editor part is not selected and you click on the Spelling button to check the spelling:
JavaScript error: chrome://messenger/content/messengercompose/EdSpellCheck.js, line 210: NS_ERROR_NOT_INITIALIZED: Component returned failure code: 0xc1f30001 (NS_ERROR_NOT_INITIALIZED) [nsIEditorSpellCheck.GetNextMisspelledWord]
https://bugzilla.mozilla.org/show_bug.cgi?id=1580715#c13

What does "editor part is not selected" mean. Not clicked into the message body? Then the buttons shouldn't be available.

Yes, Not clicked into the message body.

Assignee: nobody → khushil324
Attachment #9093529 - Flags: review?(mkmelin+mozilla)
Status: NEW → ASSIGNED

Is the spell button still enabled if the focus is on the subject field or body is plain text?

Flags: needinfo?(khushil324)
Comment on attachment 9093529 [details] [diff] [review]
Bug-1581774_spelling-button-disabled.patch

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

You can spell check in plain text mode too
Attachment #9093529 - Flags: review?(mkmelin+mozilla) → review-

I thought we're removing "observes" in bug 1581305, so why add more?

That bug only removes the <observes> element, not yet the attributes. We'll get to the attribute too, but it's easier to do one thing at ta time. The attributes are easier to convert since then you know what element you need to do what on, which is not the case with the element.

When selecting the subject field or body is plain text, GetCurrentDictionary is giving null and it is creating the problem: https://searchfox.org/comm-central/source/mail/components/compose/content/dialogs/EdSpellCheck.js#78
Does anyone have any idea how gSpellChecker is working? Do we need to change anything here: https://searchfox.org/comm-central/source/mail/components/compose/content/dialogs/EdSpellCheck.js#50

Flags: needinfo?(khushil324)

I know a whole lot about that spellchecking stuff.

In TB 68, when you click the button while in the subject, you get:
NS_ERROR_NOT_INITIALIZED: Component returned failure code: 0xc1f30001 (NS_ERROR_NOT_INITIALIZED) [nsIEditorSpellCheck.GetNextMisspelledWord]
It works when you start in a plaintext body.

Did moving the files from editor to compose break spellcheck? Like it doesn't spellcheck the subject any more. I have a compile going, so I can't try it.

I checked when I was moving the editor stuff and it was showing the error there also.
When selecting the subject, gSpellChecker.GetCurrentDictionary() is giving null string and it is creating the problem.

You didn't answer the question. Does inline spellchecking in the subject still work? Then gSpellChecker.GetCurrentDictionary() shouldn't fail.

No, inline spellchecking in the subject is not working on the trunk. Showing the same error: NS_ERROR_NOT_INITIALIZED: Component returned failure code: 0xc1f30001 (NS_ERROR_NOT_INITIALIZED) [nsIEditorSpellCheck.GetNextMisspelledWord]

Inline spellchecking in the subject is working. Type thiss iss aa misspellllling and you'll see that everything is underlined.

(In reply to Jorg K (GMT+2) from comment #1)

What does "editor part is not selected" mean. Not clicked into the message body? Then the buttons shouldn't be available.

Haha, I was wrong. If you're in the subject, you still want to change the language for the inline spellcheck. That's why I wanted to split the button into "language choice" and "actual spellcheck". But that was knocked back by the conservative crowd back in bug 368915, 258 comments and totally frustrating. Just check attachment 8625069 [details] :-(

So since in the subject and elsewhere, there is no spellchecker initialised, this code in EdSpellCheck.js

  try {
    curLang = gSpellChecker.GetCurrentDictionary();
  } catch (ex) {
    curLang = "";
  }

  InitLanguageMenu(curLang);

So I'd just take the easy way out here: if (!curLang) return; Or a try/catch around the gSpellChecker.GetNextMisspelledWord();.

You need to experiment a bit. I think in bug 368915 there was some discussion about not launching the panel at all, but then the user click would just be ignored.

Looking at bug 1580715 comment #13: What happened to the "onClose is not defined" issue. I don't see that one in TB 68.

Attachment #9093529 - Attachment is obsolete: true
Attachment #9094552 - Flags: review?(mkmelin+mozilla)

How about a comment? What about my suggestion from comment #14: if (!curLang) return; ?

(In reply to Jorg K (GMT+2) from comment #14)

Looking at bug 1580715 comment #13: What happened to the "onClose is not defined" issue. I don't see that one in TB 68.

This got fixed during the bug 1580715 for the trunk. We no longer need onClose function.

I have implemented try/catch around the gSpellChecker.GetNextMisspelledWord(); as we need this at two places. if (!curLang) return; will resolve the problem in spellCheckStarted function only.

This error will also pop when you click on recheck button. So it needs a try and catch there also.

I also liked the suggestion of two different buttons: "language choice" and "actual spellcheck". Waiting for Magnus's comment on it.

Comment on attachment 9094552 [details] [diff] [review]
Bug-1581774_spelling-check-on-subject.patch

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

Let's avoid the try/catch when we can. 
I'll let Jörg handle the review since he dug around a lot in this area.
Attachment #9094552 - Flags: review?(mkmelin+mozilla)

That's right, I've dug around this area an awful lot.

How about adding the if (!curLang) and remembering that we weren't really initialised in a global. Than use that instead of try/catch.

(In reply to Jorg K (GMT+2) from comment #19)

That's right, I've dug around this area an awful lot.

How about adding the if (!curLang) and remembering that we weren't really initialised in a global. Than use that instead of try/catch.

Sure.

I think we need to hide stop button when we don't have the current dictionary. What do you think?

Attachment #9094552 - Attachment is obsolete: true
Attachment #9094564 - Flags: review?(jorgk)
Comment on attachment 9094564 [details] [diff] [review]
Bug-1581774_spelling-button-disabled.patch

> I think we need to hide stop button when we don't have the current dictionary. What do you think?

I've never seen the "Stop" button, but yes, if you start the spelling on the body, it shows up briefly. I assume you can "stop" the spellcheck on larger e-mails?

The patch works somewhat, but if you click "Check Word", "Replace", "Replace All" or "Add Word", you still get the error. At least your check needs to go into NextWord() where you had your try/catch before.

I also noticed that clicking "Edit" shows "undefined" as word to add, that's not pretty. Maybe we can fix this while we're here.
Attachment #9094564 - Flags: review?(jorgk)

(In reply to Jorg K (GMT+2) from comment #22)

I've never seen the "Stop" button, but yes, if you start the spelling on the
body, it shows up briefly. I assume you can "stop" the spellcheck on larger
e-mails?

I am asking about when we don't have the current dictionary and spelling check is not happening because of it. For that case, I guess there should only be cancel button.

Attachment #9094564 - Attachment is obsolete: true
Attachment #9094860 - Flags: review?(jorgk)

Thanks, this looks promising. I'll try it in a few hours (I usually try to take the afternoon off).

OK, I've looked at this now in more detail. Try this without your patch:

Start a new message. Type a misspelled word. Try the spellcheck. Works, English (United States), complains about the error, etc.

Now, click the subject and then start the spellcheck. Dialog comes up, language is empty. That's the first issue. Now select English, then click "Recheck Text". Aha, the misspelled word is now shown, so it knows to spellcheck the body.

Now repeat in TB 60. And you won't believe it, when you start the spell check, the language isn't lost and the first bad word shows up. To TB 68/trunk is just broken :-( - Time to ask Alice to find the regression.

Alice, here are the STR:

Make sure you have at least one dictionary installed. The en-US Daily builds will have am English dictionary.

Start a new message, type a misspelled word into the body. Click onto the subject, then click onto the spelling button:

TB 60.9: Good: The spellcheck dialog comes up, English is selected, and the first misspelled word is shown.

TB 68.1: Bad: The spellcheck dialog comes up, the language selection is empty and the misspelled word isn't shown.

Flags: needinfo?(alice0775)
Comment on attachment 9094860 [details] [diff] [review]
Bug-1581774_spelling-check-on-subject.patch

Thanks for the effort, but we're on the wrong track here, not your fault, I gave plenty of wrong advice :-( - We just need to trigger a spellcheck of the body, like we always did, see previous comment. Alice will tell us when that broke, so it will be easier to fix.
Attachment #9094860 - Flags: review?(jorgk)

Thank you so much, Alice, for you invaluable support!

That gives us something to work with. Khushil, can you try to focus the body element before the spellcheck starts. I don't have time right now, but I can assist you some other day.

Keywords: regression
Regressed by: 1507543
Summary: console shows error when editor part is not selected and you click on the Spelling button to check the spelling → Spellcheck doesn't check message body any more when started in the subject
Assignee: khushil324 → jorgk
Component: Message Compose Window → Editor
Product: Thunderbird → Core
Comment on attachment 9094860 [details] [diff] [review]
Bug-1581774_spelling-check-on-subject.patch

Sorry, I have to take over here.
Attachment #9094860 - Attachment is obsolete: true

We start the spellcheck from various parts of the UI, so we need to restore this line
rootContent = htmlEditor->GetActiveEditingHost();

Patch as in Phab with additional comment added. Carrying over r=m_kato.

Attachment #9095420 - Flags: review+

Dear sheriff, please land the attached patch the "old fashioned" way. Reviewer is m_kato:
Bug 1581774 - revert part of rev 5e2fad03c885 from bug 1507543 for Thunderbirds since it breaks the spellcheck. r=m_kato
Note that the attached patch has an additional comment.

I can't land it myself using Lando due to bug 1576146.

Keywords: checkin-needed

Pushed by nerli@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/7b1c8131090e
revert part of rev 5e2fad03c885 from bug 1507543 for Thunderbirds since it breaks the spellcheck. r=m_kato

Keywords: checkin-needed

Makoto-san, can you please let me know which sort of test you had in mind and I'll file a follow-up bug. It could be a test in Thunderbird or maybe even a Gecko reftest where we set the bit on the editor, like here:
https://searchfox.org/mozilla-central/rev/f43ae7e1c43a4a940b658381157a6ea6c5a185c1/editor/libeditor/tests/test_bug1330796.html#89

Priority: -- → P2

https://hg.mozilla.org/mozilla-central/rev/7b1c8131090e

Status: ASSIGNED → RESOLVED
Closed: 4 years ago
status-firefox71: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla71

Ryan, this only affects Thunderbird, so I'll deal with it on our branch.

Liz, as I said in comment #38, this is only for TB. Just mark FF 70 and FF 68 as WONTFIX. We've got TB 68 covered, see comment #39.

Has Regression Range: --- → yes
You need to log in before you can comment on or make changes to this bug.