Spell check dialog shows no misspelled word and no suggestions

RESOLVED FIXED in mozilla9

Status

()

Core
Spelling checker
RESOLVED FIXED
6 years ago
4 years ago

People

(Reporter: Paenglab, Assigned: Bienvenu)

Tracking

({regression})

Trunk
mozilla9
x86
All
regression
Points:
---

Firefox Tracking Flags

(firefox9-)

Details

(Whiteboard: [tbtrack9])

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

6 years ago
On writing a message the misspelled words are correctly underlined. When checking with F7 the spell check window opens but no misspelled word nor a suggestion is shown.

When check before sending is enabled, the spell check window opens with the same behavior but the buttons on bottom right are now 'Stop' and 'Close' instead of 'Stop' and 'Send'.

I have no error in the error console found which is related to this bug.

Last good: Mozilla/5.0 (Windows NT 6.1; rv:9.0a1) Gecko/20110914 Thunderbird/9.0a
First bad: Mozilla/5.0 (Windows NT 6.1; rv:9.0a1) Gecko/20110915 Thunderbird/9.0a

This bug is no dupe of bug 686878 because the other is about dictionary installation problems.
(Reporter)

Updated

6 years ago
Keywords: regression
(Reporter)

Comment 1

6 years ago
I tried it also under Linux with the same result.
OS: Windows 7 → All
Nothing obvious in c-c changes:

http://hg.mozilla.org/comm-central/pushloghtml?fromchange=77a6a1df6eb3&tochange=dc0820370836

m-c changes are:

http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=cfd1e3533f0f&tochange=5faaa652594f

Therefore I'd bet on a regression from bug 591780.
Blocks: 591780
Component: Message Compose Window → Spelling checker
Product: Thunderbird → Core
QA Contact: message-compose → spelling-checker
I think this needs tracking - I'm not sure if the UI for this dialog is in Firefox, but even if it isn't something has broken which could affect add-ons (and does affect other apps).
tracking-firefox9: --- → ?
Summary: Spell check shows no misspelled word and no suggestions → Spell check dialog shows no misspelled word and no suggestions
Whiteboard: [tbtrack9]

Comment 4

6 years ago
When looking through the code, I found three bugs, but I don't know if they are related to this.

http://mxr.mozilla.org/comm-central/search?string=spellChecker.dictionary+%3D&find=&findi=&filter=^[^\0]*%24&hitlimit=&tree=comm-central
I see three places in the code using mozISpellCheckingEngine.dictionary=x, which I would assume to be wrong since bug 338427, and even more wrong with the combination of bug 338427 and bug 591780. The correct way to set the dictionary would be nsIInlineSpellChecker.spellChecker.SetCurrentDictionary(x)

Setting the current language directly on mozISpellCheckingEngine would cause strange changes in the current language whenever an editor with inline spell checking gains or looses focus.

Updated

6 years ago
Duplicate of this bug: 687420
Mark, can you try fixing the stuff in comment 4?  FWIW, this UI is not in Firefox, and this looks like a comm-central bug to me...
Bienvenu: see comment 6 please.
(Assignee)

Comment 8

6 years ago
yeah, this is pretty broken on the trunk. I'll try to fix this before we go to Aurora.
(Assignee)

Comment 9

6 years ago
Issue seems to be that mozSpellChecker has no converter, which breaks spell checking. SetCurrentDictionary used to 

mozSpellChecker::SetCurrentDictionary(const nsAString &aDictionary)
{

...

  nsXPIDLString language;
  
  nsCOMPtr<mozISpellI18NManager> serv(do_GetService("@mozilla.org/spellchecker/i18nmanager;1", &res));
  if(serv && NS_SUCCEEDED(res)){
    res = serv->GetUtil(language.get(),getter_AddRefs(mConverter));

But it no longer does that. The patch in Bug 591780 removed the code, which breaks our spell checking.
(Assignee)

Comment 10

6 years ago
Jesper, any idea why that code was removed?

I did make the changes locally you suggested about Get/SetCurrentDictionary, which I suspect are necessary, but not sufficient.

Comment 11

6 years ago
(In reply to David :Bienvenu from comment #10)
> Jesper, any idea why that code was removed?

No, I don't remember having any intension of removing that.
Yes, indeed that seems to be a bug which should be fixed.  Sorry for not catching that in the review process.  :/
(Assignee)

Comment 13

6 years ago
Created attachment 561921 [details] [diff] [review]
fix moz-central part of issue

I'll attach a patch for the get/setcurrentdictionary changes as well.
Assignee: nobody → dbienvenu
Attachment #561921 - Flags: review?(ehsan)
(Assignee)

Comment 14

6 years ago
Created attachment 561924 [details] [diff] [review]
use get/setcurrentdictionary
Attachment #561924 - Flags: review?(mbanner)
Comment on attachment 561921 [details] [diff] [review]
fix moz-central part of issue

>+      return serv->GetUtil(language.get(),getter_AddRefs(mConverter));

Nit: space after comma.

r=me.
Attachment #561921 - Flags: review?(ehsan) → review+

Comment 16

6 years ago
Comment on attachment 561924 [details] [diff] [review]
use get/setcurrentdictionary

That does not make sense to me. You are trying to call a method from nsIEditorSpellCheck on an object implementing mozISpellCheckingEngine.
(Assignee)

Comment 17

6 years ago
(In reply to Jesper Kristensen from comment #16)
> Comment on attachment 561924 [details] [diff] [review] [diff] [details] [review]
> use get/setcurrentdictionary
> 
> That does not make sense to me. You are trying to call a method from
> nsIEditorSpellCheck on an object implementing mozISpellCheckingEngine.

Ah, I misunderstood your previous comment, then.
(Assignee)

Updated

6 years ago
Attachment #561924 - Attachment is obsolete: true
Attachment #561924 - Flags: review?(mbanner)

Comment 18

6 years ago
Created attachment 562135 [details] [diff] [review]
fix setting dictionary

This is what I meant. I tested it, and it doesn't fix this bug. It however fixes another bug: When more than one dictionary is installed, you sometimes cannot switch between them for inline spell check. Maybe I should file that as a separate bug.
(Assignee)

Comment 19

6 years ago
thx, Jesper, yes, that should be covered in a new bug.
So, David, is your m-c patch enough here?  We should land that patch before the migration day (Tuesday).
(Assignee)

Comment 21

6 years ago
(In reply to Ehsan Akhgari [:ehsan] from comment #20)
> So, David, is your m-c patch enough here?  We should land that patch before
> the migration day (Tuesday).

Yeah, I've been trying to get set up with mozilla-inbound so I can push there. Which I have just now done:

http://hg.mozilla.org/integration/mozilla-inbound/rev/7226c37145cd

Comment 22

6 years ago
Comment on attachment 562135 [details] [diff] [review]
fix setting dictionary

(In reply to David :Bienvenu from comment #19)
filed bug 688860
Attachment #562135 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/7226c37145cd
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla9
Comment on attachment 561921 [details] [diff] [review]
fix moz-central part of issue

(In reply to Ehsan Akhgari [:ehsan] from comment #15)
> Comment on attachment 561921 [details] [diff] [review] [diff] [details] [review]
> fix moz-central part of issue
> 
> >+      return serv->GetUtil(language.get(),getter_AddRefs(mConverter));
> 
> Nit: space after comma.

You forget to address this.

Also, I don't understand what the language local is for. Surely language.get() is just null?
(Assignee)

Comment 25

6 years ago
(In reply to Ms2ger from comment #24)

> 
> You forget to address this.
Oh, sorry, I did fix it locally, but then we I decided to use moz-inbound, I forgot to carry over that change.
> 
> Also, I don't understand what the language local is for. Surely
> language.get() is just null?

for both this, and the lack of space after the comma, I'm just restoring code that was removed before, inadvertently, from what Jesper said. In essence, backing out a small part of his patch. I have no idea why the code was the way it was before, but yeah, you're right, it would be the empty string (not null, I don't think).

Comment 26

6 years ago
Generally the spell checker interfaces contain a lot of methods, attributes and parameters "for future use". See for example the many methods in mozHunspell.cpp which return NS_ERROR_NOT_IMPLEMENTED. The first parameter to GetUtil is such an example. The value is never used, so you can pass in whatever you want. It would probably also be fine to just call GetUtil once in the constructor instead of every time the current dictionary is changed.

Updated

6 years ago
tracking-firefox9: ? → -
You need to log in before you can comment on or make changes to this bug.