Automatically select language for spell check based on user input

Bug #303269 reported by era
30
This bug affects 6 people
Affects Status Importance Assigned to Milestone
Mozilla Firefox
Fix Released
Wishlist
firefox (Ubuntu)
Confirmed
Undecided
Unassigned

Bug Description

Binary package hint: firefox

As suggested in comments to http://brainstorm.ubuntu.com/idea/14655/

In an edit field, the language used for the spelling checker could be selected automatically based on identifying the language used for text written so far. Systems for doing this exist, and have tolerable accuracy for a single sentence of input.

Granted, this should probably be routed upstream.

Revision history for this message
In , Syskin (syskin) wrote :

Are there plans to add this? I thought it was obvious but I just discovered it's not there yet.

Would be a horrible waste of a perfectly good html attribute, if there was a FF version with spellcheck but not knowing how to use it.

blocking-firefox2?

Revision history for this message
In , Mike Connor (mconnor) wrote :

At this point, I wouldn't block on this, but it'd be nice to have for those bilingual people out there.

Revision history for this message
In , Syskin (syskin) wrote :

Can someone tell me if there is some spellcheck "initialization" code (hopefully in js?) which selects the dictionary?

I'd like to try to write a loop that iterates parents (in DOM) until it finds the "lang" attribute (or reaches top-level and falls back to last dictionary used).

Revision history for this message
In , Syskin (syskin) wrote :

OK dictionary is selected here: http://lxr.mozilla.org/mozilla1.8/source/editor/composer/src/nsEditorSpellCheck.cpp#189

But it is my understanding that this nsEditorSpellCheck::InitSpellChecker() is called only once per browser instance? Would be really sucky, because this would mean that current dictionary is a per-browser setting and I can't even compose two messages in two languages at the same time?? (with different dictionaries in different windows/tabs/textareas).

:/

Is this correct?

Revision history for this message
In , Ria-klaassen (ria-klaassen) wrote :

Additionally there could be a check for the top 5 most used words in a certain language in the page, and also the URL could point to a language.
I could not find <html lang= in any page with a comment box or forum that I tried.
If it can't be implemented hard-coded, hopefully some kind and brainy person will make an extension for this when 2.0 comes out.

Revision history for this message
In , Ria-klaassen (ria-klaassen) wrote :

*** Bug 344760 has been marked as a duplicate of this bug. ***

Revision history for this message
In , Zeniko (zeniko) wrote :

(In reply to comment #4)
> But it is my understanding that this nsEditorSpellCheck::InitSpellChecker() is
> called only once per browser instance?

nsEditorSpellCheck is no service and you get one instance per input/textarea element for which spell checking is possible. InitSpellChecker is thus called once per element and would thus be the appropriate place for figuring out the page's default language.

You should be able to start searching from aEditor->GetRootElement. Make sure to consider HTML's lang attribute and xml:lang (whichever is appropriate) and as a bonus it would be nice to also select not completely specified languages (e.g. select es-ES for lang="es").

(In reply to comment #2)
> but it'd be nice to have for those bilingual people out there.

Of which there are an awful lot on the English dominated Internet of a polyglot world. :)

Revision history for this message
In , Pkasting (pkasting) wrote :

(In reply to comment #7)
> nsEditorSpellCheck is no service and you get one instance per input/textarea
> element for which spell checking is possible. InitSpellChecker is thus called
> once per element and would thus be the appropriate place for figuring out the
> page's default language.
>
> You should be able to start searching from aEditor->GetRootElement. Make sure
> to consider HTML's lang attribute and xml:lang (whichever is appropriate) and
> as a bonus it would be nice to also select not completely specified languages
> (e.g. select es-ES for lang="es").

Definitely take a look at how the spellcheck attribute support is implemented as well, since this is another case where the editor needs to do the right thing based on page content. (Although that's much more complex than this, in that it needs to support all kinds of bizarre dynamic behavior and overriding.)

Revision history for this message
In , Fred (eldmannen) wrote :

Or it could use the domain suffix.

Example, www.example.se it would use Swedish dictionary, if I goto www.example.it it would use Italian dictionary.

Revision history for this message
In , Teoli2003 (teoli2003) wrote :

(In reply to comment #9)
> Or it could use the domain suffix.
>
> Example, www.example.se it would use Swedish dictionary, if I goto
> www.example.it it would use Italian dictionary.
>

Not at all. There are plenty of countries speaking several official languages (Canada for example) and there are also no reason that a specific site to use an official language of its suffix. Finally, how .com, .org and other non-country related suffixes should be handled?

Revision history for this message
In , Syskin (syskin) wrote :

Finding the language by recursively going up in DOM is the easy part. Having Firefox able to use different languages in different textboxes at the same time doesn't seem to work, though.

Revision history for this message
In , Jeppe-ioslo (jeppe-ioslo) wrote :

I'll give you my 2c.

Whatever language you end up with by looking at the lang variable etc, there should be a button/dropdown somewhere logical where the user can override the setting. After all, not all webmasters know what they are doing..

It should also be easy to install dictionaries for new languages.

Revision history for this message
In , Dao (dao) wrote :

(In reply to comment #7)
> Make sure
> to consider HTML's lang attribute and xml:lang (whichever is appropriate) and
> as a bonus it would be nice to also select not completely specified languages
> (e.g. select es-ES for lang="es").

(In reply to comment #9)
> Or it could use the domain suffix.
>
> Example, www.example.se it would use Swedish dictionary, if I goto
> www.example.it it would use Italian dictionary.

(In reply to comment #12)
> Whatever language you end up with by looking at the lang variable etc, there
> should be a button/dropdown somewhere logical where the user can override the
> setting. After all, not all webmasters know what they are doing..

I've implemented these three points in this extension (my first one):
https://addons.mozilla.org/firefox/3414/

The problem is, I don't know how to change the language for given form fields. Hence you have to reload the page whenever the language is changed manually or automatically (at least when listening to DOMContentLoaded, which fires after the spellcheckers are instantiated). Maybe somebody can help me out here.

Revision history for this message
In , Jo-hermans (jo-hermans) wrote :

*** Bug 357976 has been marked as a duplicate of this bug. ***

Revision history for this message
In , Sciguyryan (sciguyryan) wrote :

*** Bug 358145 has been marked as a duplicate of this bug. ***

Revision history for this message
In , Arnaud Renevier (arenevier) wrote :

(In reply to comment #7)

>
> Make sure
> to consider HTML's lang attribute and xml:lang (whichever is appropriate)

What should be done in case those two attributes are set on the same tags, and are conflicting ?
Ok, this should not happen, but this could happen, and surely already happens on some bad coded pages.

Is one of those attributes to be picked randomly, or are they to be discarded ?
If so, should search for a lang attribute be continued in parent tags or no ?

Revision history for this message
In , Dao (dao) wrote :

(In reply to comment #16)
> What should be done in case those two attributes are set on the same tags, and
> are conflicting ?

When sent as text/html, xml:lang shouldn't receive attention. When sent as XML, lang is obsolete and should at least have a lower priority, if it's not ignored at all.

Revision history for this message
In , Ria-klaassen (ria-klaassen) wrote :

*** Bug 359709 has been marked as a duplicate of this bug. ***

Revision history for this message
In , Manuel Vazquez Acosta (mva-led) wrote :

When I issued bug 359709 I had something else in mind, this bug, however, may be enhanced with those ideas too.

Here they are (more detailed. My fault not to did so earlier):

We could write code to automatically detect the language. I mean, in the absence of lang and xml:lang attributes - no other information is used besides the text itself.

There are several techniques that can be used - a simple one: count the letters frequencies in the text. There are lots of paper on this topic. You can search Google Scholar for "Automatic Language Detection" and see our options.

This feature can be used in Composer alone. Composer can automatically (with the user's consent) add the lang or xml:lang as appropriated. This is one of the reason I'd rather reopen bug 359709 so we can keep browser-specific features separated from editor-wide features. Should we?

Revision history for this message
In , Arnaud Renevier (arenevier) wrote :

Created an attachment (id=244939)
incomplete fix

Hi,
I tried to tinker a bit with this bug and came with a partial solution.

In nsEditorSpellCheck::InitSpellChecker(), I go from the editor dom element and goes up to the parents node until I find a lang, or xml:lang attribute. If there are none, same behaviour as before is applied : it tries to get language from pref.
Then, there is a loop that looks the names of all directories installed. If there is one that matches exactly the language + subcode requested, get that one. Otherwise, gets the first one that matches the language (first part of full identifier).

My patch still has a few problems.
* At first, this is my first attempt with C++ inside firefox, and my understanding of all that xpcom stuff is far far from perfect. So there are probably problems because of that.
* to choose if I am in a html or xhtml context, I find the content-type of the document. If it is "text/html", then I assume we're in html. I don't check if we're in application/xhtml+xml because if we insert xhtml inside another xml document (svg or xul for example), content-type is not application/xhtml+xml. So I assume if content-type is not "text/html", then we're in xhtml context. I think that that function is only called from within a html or xhtml editable tag, but may be there is a more robust way to get the context.
* To match the required language against installed dictionaries, I use the same method that is used to match locals with chrome.manifest files. So, I just copy-pasted code because I didn't find a way to avoid duplication.
* That behaviour could be improved to :
- match the shortest code ie : prefer "en" instead of "en-US" if "en-GB" is requested and not available.
- try to match more than the first part of the code ie : prefer "en-GB" instead of "en-US" if "en-GB-oed" is requested and not available
* If "lang" attribute is modified dynamically, another check might be done, and language of spellchecker could be changed if needed
* Last, but may be the more difficult problem : when lang requested is not available, or when lang attribute exists and is empty, spellchecker is set by preferences instead of being disabled. That is because :
- I did not succeed in disabling spellchecker.
- Even if I succeed, if user still wants to enable spellchecker with a language she chooses, nsEditorSpellCheck::InitSpellChecker() would be called again, and spellchecker would be disabled again.

Even if my solution has many flaws, I still post it because may be it will be useful if someone wants to investigates more. Also, if someone wants to point me directions to improve my patch, I can work on it more.

Revision history for this message
In , Ria-klaassen (ria-klaassen) wrote :

*** Bug 360664 has been marked as a duplicate of this bug. ***

Revision history for this message
In , Tbertels+bugzilla (tbertels+bugzilla) wrote :

(In reply to comment #5)
> I could not find <html lang= in any page with a comment box or forum that I
> tried.
Indeed.
So, I was thinking about comparing the spellchecks between the installed dictionaries to find out in which language we're writing.
If dictionary A finds 3 errors when dictionary B finds 1 error, then we would suppose that the dictionary B is the good one for this text.
I don't think it would take much words to find out and this would also work for Thunderbird, which is a plus.

Revision history for this message
In , Dao (dao) wrote :

(In reply to comment #22)
> If dictionary A finds 3 errors when dictionary B finds 1 error, then we would
> suppose that the dictionary B is the good one for this text.
> I don't think it would take much words to find out and this would also work for
> Thunderbird, which is a plus.

See the last updates here:
http://en.design-noir.de/mozilla/dictionary-switcher/ (Firefox)
http://en.design-noir.de/mozilla/dictionary-switcher-tb/ (Thunderbird)

The Firefox version is a bit adventuresome, because it's not easy with unlimited textboxes floating around. The Thunderbird version is pretty straightforward though -- I could even write a patch for Thunderbird based on that.

Revision history for this message
In , Dao (dao) wrote :

(In reply to comment #23)
> The Thunderbird version is pretty straightforward though -- I could even
> write a patch for Thunderbird based on that.

-> bug 367312

Revision history for this message
In , Brettw-gmail (brettw-gmail) wrote :

*** Bug 355487 has been marked as a duplicate of this bug. ***

Revision history for this message
In , Ria-klaassen (ria-klaassen) wrote :

*** Bug 368320 has been marked as a duplicate of this bug. ***

Revision history for this message
In , Jo-hermans (jo-hermans) wrote :

*** Bug 443971 has been marked as a duplicate of this bug. ***

Revision history for this message
era (era) wrote :

Binary package hint: firefox

As suggested in comments to http://brainstorm.ubuntu.com/idea/14655/

In an edit field, the language used for the spelling checker could be selected automatically based on identifying the language used for text written so far. Systems for doing this exist, and have tolerable accuracy for a single sentence of input.

Granted, this should probably be routed upstream.

Revision history for this message
era (era) wrote :

Upstream bug https://bugzilla.mozilla.org/show_bug.cgi?id=448959 is vaguely related, although not quite the same idea.

Revision history for this message
era (era) wrote :

Actually https://bugzilla.mozilla.org/show_bug.cgi?id=338427 is closer to the original Brainstorm idea, and also incorporates the language identification concept in comments.

Changed in firefox:
status: Unknown → Confirmed
Revision history for this message
In , Petr Písař (petr-pisar) wrote :

Just FYI, Firefox (and Seamonkey too) has language detection code implemented already.

If you do context click on any element and language has been somehow derived, you will get "Properties" as the last item in the context menu. Selecting this item, a window pops up and you get current language in "Other properties" section of the window.

Revision history for this message
In , S-marshall (s-marshall) wrote :

Created an attachment (id=365640)
Test case

Here's a testcase that demonstrates how this behaviour should work.

(Note that bilingual systems probably don't often use lang tag at present. However maybe they would do if browsers took some notice of it!)

Particularly for language teaching, this would be pretty helpful.

Revision history for this message
John Vivirito (gnomefreak) wrote :

Marking confrimed for now since upstream bug is windows only we need a confirmation of that bug in linux as well

affects: firefox (Ubuntu) → firefox-3.0 (Ubuntu)
Changed in firefox-3.0 (Ubuntu):
status: New → Confirmed
Revision history for this message
In , Mardeg (mardeg) wrote :

*** Bug 498189 has been marked as a duplicate of this bug. ***

Revision history for this message
In , antistress (antistress) wrote :

if simple as-you-type dictionary detection is likely to take time to implement, a quick workaround could be to allow the user to set multiple dictionaries at the same time until the real fix be released.

I'm french and i'd be happy to set both en and fr on Firefox since i only use these languages

If you want to try, Midori webbrowser on Linux already allows that

Revision history for this message
In , Xtc4uall (xtc4uall) wrote :

*** Bug 535049 has been marked as a duplicate of this bug. ***

Revision history for this message
Dov Grobgeld (dov-grobgeld) wrote :

I trivial simplification is to recognize languages for various character sets. E.g. if the user has English and Russian installed and starts typing in Cyrillic letters, then it is obvious that the Russian dictionary should be used.

Revision history for this message
In , Altaveron (altaveron) wrote :

Can't use spell checking in text with 2 or more languages. Please fix this bug either in the Firefox and the Thunderbird.

Revision history for this message
In , Mounir-lamouri (mounir-lamouri) wrote :

(In reply to comment #7)
> (In reply to comment #4)
> > But it is my understanding that this nsEditorSpellCheck::InitSpellChecker() is
> > called only once per browser instance?
>
> nsEditorSpellCheck is no service and you get one instance per input/textarea
> element for which spell checking is possible. InitSpellChecker is thus called
> once per element and would thus be the appropriate place for figuring out the
> page's default language.
>
> You should be able to start searching from aEditor->GetRootElement. Make sure
> to consider HTML's lang attribute and xml:lang (whichever is appropriate) and
> as a bonus it would be nice to also select not completely specified languages
> (e.g. select es-ES for lang="es").
>
> (In reply to comment #2)
> > but it'd be nice to have for those bilingual people out there.
>
> Of which there are an awful lot on the English dominated Internet of a polyglot
> world. :)

Unfortunately, even if it's not too hard to check the lang that has to be used during the call to nsEditorSpellCheck::InitSpellChecker() (for each editor's instance), the problem is quite hard because there is only one instance of nsISpellChecker (implemented by extensions/spellcheck/) which is used to set the dictionary. In other words, changing the dictionary for one editor will propagate the change for all the editors. That's exactly the behavior you have when you change the language with right click -> Languages -> <language>. And that's exactly what we don't want for @lang. Actually, we may don't want that when changing the language manually too.

On top of my head, I would say the best way to fix this bug would be to set the desired dictionary when the editor got the focus so it could be possible to set the dictionary to the language corresponding to @lang and still be able to revert it when using a field with another @lang or with the default one.

Revision history for this message
In , Mounir-lamouri (mounir-lamouri) wrote :

Created attachment 461208
Patch v1

Ehsan, could you review the editor changes?
Who would you recommend for spellchecker changes review?

This patch updates the spellchecker's dictionary each time an editor is focused, taking in account @lang. For the moment, @lang has to correspond exactly to the name of one of your dictionary (like 'en-US' or 'fr-FR'). Checking for the first part of the lang code will be a follow-up.

Unfortunately this patch comes with one regression: if the user change the dictionary for one field (using right click -> languages -> <dictionary>) this will be forgotten as soon as he/she focus another field. I think the current behavior is really annoying (this is saved as long as you don't 'revert' it). If that can be done in a follow-up, I would prefer to have this saved to be used by default for the entire document and not the entire session.

By the way, my previous comment had some mistakes. There isn't one instance of nsISpellChecker but on for each editor. Actually, I think that's the spellcheck engine service but I will not bet on it...

Changed in firefox:
status: Confirmed → In Progress
Changed in firefox:
importance: Unknown → Wishlist
papukaija (papukaija)
affects: firefox-3.0 (Ubuntu) → firefox (Ubuntu)
53 comments hidden view all 133 comments
Revision history for this message
In , Ehsan-mozilla (ehsan-mozilla) wrote :

Comment on attachment 551030
patch v2.5

>diff -r 61bb2bb510c9 editor/libeditor/html/tests/test_bug484181.html
> function runTest() {
> gMisspeltWords = ["haz", "cheezburger"];
>+ var edit = document.getElementById("edit");
>+ edit.focus();
>+ SimpleTest.executeSoon(function() {
> is(isSpellingCheckOk(), true, "All misspellings before editing are accounted for.");
>
>- var edit = document.getElementById("edit");
> append(" becaz I'm a lolcat!");
> SimpleTest.executeSoon(function() {
> gMisspeltWords.push("becaz");
> gMisspeltWords.push("lolcat");
> is(isSpellingCheckOk(), true, "All misspellings after typing are accounted for.");
>
> SimpleTest.finish();
> });
>+ });
> }

Nit: could you please adjust the indentation here?

>diff -r 61bb2bb510c9 extensions/spellcheck/src/mozInlineSpellChecker.cpp
>+ if (NS_FAILED(rv))
>+ continue;

Why is this change needed?

>diff -r 61bb2bb510c9 layout/reftests/editor/338427-2.html
>--- /dev/null Thu Jan 01 00:00:00 1970 +0000
>+++ b/layout/reftests/editor/338427-2.html Thu Aug 04 23:34:57 2011 +0200
>@@ -0,0 +1,16 @@
>+<!DOCTYPE html>
>+<html class="reftest-wait">
>+<script>
>+function init() {
>+ var editor = document.getElementById('editor');
>+ editor.addEventListener("focus", function() {
>+ editor.blur();
>+ document.documentElement.className = '';
>+ }, false);
>+ editor.focus();

As I said before, this is wrong.

Also, it's probably a good idea for you to run the reftests manually to verify that these tests path. You can do this with this command, for example:

make -C /path/to/objdir reftest TEST_PATH=layout/reftests/editor/reftest.list

>diff -r 61bb2bb510c9 toolkit/content/InlineSpellChecker.jsm
>- if (! spellchecker.CheckCurrentWord(this.mMisspelling))
>- return 0; // word seems not misspelled after all (?)
>+ try {
>+ if (! spellchecker.CheckCurrentWord(this.mMisspelling))
>+ return 0; // word seems not misspelled after all (?)
>+ } catch(e) {
>+ return 0;
>+ }

Why is this change needed?

>- var curlang = spellchecker.GetCurrentDictionary();
>+ var curlang = "";
>+ try {
>+ curlang = spellchecker.GetCurrentDictionary();
>+ } catch(e) {}

Also, why is this one needed?

Revision history for this message
In , Arnaud Renevier (arenevier) wrote :

I run the test on my system before submitting a patch either here, or on try server. Tests can succeed on my machine but fail in other environments. For example, as I told in comment 78, this can be caused by different font settings.

About, when the blur is made inside double timeout, updateCurrentDictionary is not called.

> Why is this change needed?

because when editor has an unkown language, there is no language set, and CheckWords returns a failure error.

Thanks for review and commiting the patch :)

Revision history for this message
In , Ehsan-mozilla (ehsan-mozilla) wrote :

(In reply to comment #89)
> I run the test on my system before submitting a patch either here, or on try
> server. Tests can succeed on my machine but fail in other environments. For
> example, as I told in comment 78, this can be caused by different font
> settings.

Hmm, I was talking about this failure http://tinderbox.mozilla.org/showlog.cgi?log=Try/1312534392.1312536112.31421.gz in comment 78. But I just remembered that Android can't handle focus correctly yet. So, can you just mark the test as needs-focus in the reftest manifest? That should make Android skip the test.

> About, when the blur is made inside double timeout, updateCurrentDictionary is
> not called.

Why?

> > Why is this change needed?
>
> because when editor has an unkown language, there is no language set, and
> CheckWords returns a failure error.

Then can we just not attempt to do a spell check in that case?

Revision history for this message
In , Arnaud Renevier (arenevier) wrote :
Download full text (3.8 KiB)

Created attachment 552033
patch v2.6

> But I just remembered that Android can't handle focus
> correctly yet. So, can you just mark the test as needs-focus in the reftest
> manifest? That should make Android skip the test.

What do you mean with: can't handle focus correctly ?
Because, if I understand correctly, needs-focus just check if content if out-of-process
http://hg.mozilla.org/mozilla-central/file/a0e3c589c8fa/layout/tools/reftest/reftest.js#l859
Is this the cause focus fails on Android, or should the directive be skip-if(Android) instead ?

I've investigated more about the CheckWords call

> >- var curlang = spellchecker.GetCurrentDictionary();
> >+ var curlang = "";
> >+ try {
> >+ curlang = spellchecker.GetCurrentDictionary();
> >+ } catch(e) {}
>
> Also, why is this one needed?

this code is executed when selecting a dictionary in context menu. This can
happen even when no dictionary is active. So this check is strictly needed.

> >- if (! spellchecker.CheckCurrentWord(this.mMisspelling))
> >- return 0; // word seems not misspelled after all (?)
> >+ try {
> >+ if (! spellchecker.CheckCurrentWord(this.mMisspelling))
> >+ return 0; // word seems not misspelled after all (?)
> >+ } catch(e) {
> >+ return 0;
> >+ }
>
> Why is this change needed?
>

I think it's less needed, because this.mOverMisspelling should not be true if
there is no misspelled word. But then, spellchecker.CheckCurrentWord is as much
unneeded as the try/catch around it.

> Then can we just not attempt to do a spell check in that case?

Good idea. In attached patch, I return early from
mozInlineSpellChecker::ResumeCheck if there is not dictionary available.

> >diff -r 61bb2bb510c9 extensions/spellcheck/src/mozInlineSpellChecker.cpp
> >+ if (NS_FAILED(rv))
> >+ continue;
>
> Why is this change needed?

Then, it should be less needed. But isn't it better to check for functions
results. (This call may return with NS_ERROR_OUT_OF_MEMORY for example)

Also, this iteration fixes indentation nit.

> > About, when the blur is made inside double timeout, updateCurrentDictionary is
> > not called.
>
> Why?

I don't known exactly why.
But with something like that *in a reftest*, the focus listener is not
triggered, event if wrapping the setTimeout inside another setTimeout.

    var editor = document.getElementById('editor');
    editor.addEventListener("focus", function() {
        // not reached in reftest
    }, false);
        window.setTimeout(function() {
            editor.blur();
        }, 0);

And we need to trigger the focus listener because that's when UpdateCurrentDictionary is called. Otherwise, test seem to pass, but for the wrong reason: because no dictionary has been set.

> Because spellchecking doesn't happen on focus, we post an event to the event
> loop and start the spellchecking when that event is processed.

So, what I tried to do was using setTimeout inside the focus handler

    var editor = document.getElementById('editor');
    editor.addEventListener("focus", function() {
        window.setTimeout(function() {
            editor.addEventListener("blur", function() {
       ...

Read more...

Changed in firefox:
status: In Progress → Confirmed
Revision history for this message
In , Release-a (release-a) wrote :

Try run for 49167a17b8af is complete.
Detailed breakdown of the results available here:
    http://tbpl.mozilla.org/?tree=Try&rev=49167a17b8af
Results (out of 223 total builds):
    success: 202
    warnings: 18
    failure: 3
Builds available at http://<email address hidden>

Revision history for this message
In , Arnaud Renevier (arenevier) wrote :

Created attachment 552655
patch v2.7

I fixed the reftest with kaze and volkmar help. We came to a better solution:
make the test and the reference exactly similar except the reference will have spellcheck=false. Then, there's no need anymore to blur the editor.

I also modified another thing since last patch:

> > Then can we just not attempt to do a spell check in that case?
>
> Good idea. In attached patch, I return early from
> mozInlineSpellChecker::ResumeCheck if there is not dictionary available.
>

Actually, I also need to cleanup previously underline words, by calling RemoveRange. Otherwise, a word may stay underlined, even if editor has changed @lang to a unknown one.
That case is coved by test-3

I've tested attached patch on tryserver, and it looks like it's correct:
http://tbpl.mozilla.org/?tree=Try&rev=3adb6f0528ac

Revision history for this message
In , Ehsan-mozilla (ehsan-mozilla) wrote :

Comment on attachment 552655
patch v2.7

Review of attachment 552655:
-----------------------------------------------------------------

This looks great! Thanks a lot for your hard work on this. :-)

Revision history for this message
In , Ehsan-mozilla (ehsan-mozilla) wrote :

Landed on mozilla-inbound.

Revision history for this message
In , Khuey (khuey) wrote :
Revision history for this message
In , Ms2ger (ms2ger) wrote :

> SetCurrentDictionary(NS_LITERAL_STRING("").get());

Can we fix that signature, pretty please?

Revision history for this message
In , Arnaud Renevier (arenevier) wrote :

Oups. Look like there's a serious bug in this patch :(
When user manually sets a non default dictionary, it's dismissed as soon as editor is blured and focused again (because we go the UpdateCurrentDictionary path again).

Revision history for this message
In , Arnaud Renevier (arenevier) wrote :

Created attachment 553025
part2: do not override user choice

quick wip patch. I'll try to add some tests tomorrow.

Revision history for this message
In , Arnaud Renevier (arenevier) wrote :

Created attachment 553143
part2: do not override manually set dictionary

patch to fix issue described in comment 98.
This works by setting a boolean member mDictWasSetManually to PR_TRUE when SetCurrentDictionary is called, but not from UpdateCurrentDictionary
Once that member is true, UpdateCurrentDictionary (and SaveDefaultDictionary) will do nothing.
This patch also fixes a typo in a variable name (s/currentDictonary/currentDictionary/)

Revision history for this message
In , Arnaud Renevier (arenevier) wrote :

Created attachment 553145
fix Get/SetCurrentDictionary signature

(In reply to Ms2ger from comment #97)
> > SetCurrentDictionary(NS_LITERAL_STRING("").get());
>
> Can we fix that signature, pretty please?

Here is a patch changing SetCurrentDictionary and GetCurrentDictionary to use nsAString& instead of PRUnichar*

Revision history for this message
In , Arnaud Renevier (arenevier) wrote :

I've tested both patches on try server, and they look correct
http://tbpl.mozilla.org/?tree=Try&rev=8300b6949f82

Revision history for this message
In , Ms2ger (ms2ger) wrote :

Comment on attachment 553145
fix Get/SetCurrentDictionary signature

Some nits:

>--- a/editor/composer/src/nsEditorSpellCheck.cpp Mon Aug 15 09:56:38 2011 +0200
>+++ b/editor/composer/src/nsEditorSpellCheck.cpp Mon Aug 15 10:26:10 2011 +0200
>+nsEditorSpellCheck::GetCurrentDictionary(nsAString& aDictionary)
> {
>+ nsresult rv = mSpellChecker->GetCurrentDictionary(aDictionary);
> return rv;

"return mSpellChecker->GetCurrentDictionary(aDictionary);"

> nsEditorSpellCheck::SaveDefaultDictionary()
> {
> if (!mDictWasSetManually) {
> return NS_OK;
> }
>- PRUnichar *dictName = nsnull;
>- nsresult rv = GetCurrentDictionary(&dictName);
>
>- if (NS_SUCCEEDED(rv) && dictName && *dictName) {
>- rv = Preferences::SetString("spellchecker.dictionary", dictName);
>- }
>+ nsAutoString dictName;
>+ nsresult rv = GetCurrentDictionary(dictName);
>+ NS_ENSURE_SUCCESS(rv, rv);
>
>- if (dictName) {
>- nsMemory::Free(dictName);
>- }
>-
>+ rv = Preferences::SetString("spellchecker.dictionary", dictName);
> return rv;

Same comment, and I think you're changing behaviour here if dictName is the empty string (can that happen?)

>@@ -501,65 +485,65 @@ nsEditorSpellCheck::UpdateCurrentDiction
>- SetCurrentDictionary(NS_LITERAL_STRING("").get());
>+ SetCurrentDictionary(NS_LITERAL_STRING(""));

EmptyString()

>--- a/extensions/spellcheck/src/mozInlineSpellChecker.cpp Mon Aug 15 09:56:38 2011 +0200
>+++ b/extensions/spellcheck/src/mozInlineSpellChecker.cpp Mon Aug 15 10:26:10 2011 +0200
>@@ -1755,36 +1755,27 @@ nsresult mozInlineSpellChecker::KeyPress
> }
>
> NS_IMETHODIMP mozInlineSpellChecker::UpdateCurrentDictionary()
> {
>+ previousDictionary.Assign(NS_LITERAL_STRING(""));

"previousDictionary.Truncate();"

>+ currentDictionary.Assign(NS_LITERAL_STRING(""));

And here

But overall, this looks much better!

Changed in firefox:
status: Confirmed → Fix Released
Revision history for this message
In , Arnaud Renevier (arenevier) wrote :

Created attachment 553150
part3: fix Get/SetCurrentDictionary signature

patch for the Get/SetCurrentDictionary signature, resolving issues raised my Ms2ger.

> Same comment, and I think you're changing behaviour here if dictName is the
> empty string (can that happen?)

If SetCurrentDictionary has been called with empty string, GetCurrentDictionary will return an error.

Revision history for this message
In , Ehsan-mozilla (ehsan-mozilla) wrote :

Comment on attachment 553143
part2: do not override manually set dictionary

>+ PRBool mUpdateDictionaryRunning;
>+ PRBool mDictWasSetManually;

Please use PRPackedBool.

>+class UpdateDictionnaryHolder {
>+ private:
>+ nsEditorSpellCheck* mSpellCheck;
>+ public:
>+ UpdateDictionnaryHolder(nsEditorSpellCheck* esc): mSpellCheck(esc) {
>+ if (mSpellCheck) {
>+ mSpellCheck->BeginUpdateDictionary();
>+ }
>+ }
>+ ~UpdateDictionnaryHolder() {
>+ if (mSpellCheck) {
>+ mSpellCheck->EndUpdateDictionary();
>+ }
>+ }
> };

Also, please move this class to the cpp file.

Revision history for this message
In , Ehsan-mozilla (ehsan-mozilla) wrote :

Comment on attachment 553150
part3: fix Get/SetCurrentDictionary signature

This really belongs to another bug... but r=me nevertheless!

Revision history for this message
In , Arnaud Renevier (arenevier) wrote :

(In reply to Ehsan Akhgari [:ehsan] from comment #105)
> Comment on attachment 553143
> part2: do not override manually set dictionary
>
> >+ PRBool mUpdateDictionaryRunning;
> >+ PRBool mDictWasSetManually;
>
> Please use PRPackedBool.
>
> >+class UpdateDictionnaryHolder {
> >+ private:
> >+ nsEditorSpellCheck* mSpellCheck;
> >+ public:
> >+ UpdateDictionnaryHolder(nsEditorSpellCheck* esc): mSpellCheck(esc) {
> >+ if (mSpellCheck) {
> >+ mSpellCheck->BeginUpdateDictionary();
> >+ }
> >+ }
> >+ ~UpdateDictionnaryHolder() {
> >+ if (mSpellCheck) {
> >+ mSpellCheck->EndUpdateDictionary();
> >+ }
> >+ }
> > };
>
> Also, please move this class to the cpp file.

Patch fixes those 2 stuffs.

Revision history for this message
In , Ehsan-mozilla (ehsan-mozilla) wrote :
Revision history for this message
In , Neil-httl (neil-httl) wrote :

Comment on attachment 552655
patch v2.7

>+ PRUnichar *currentDictionary = nsnull;
>+ rv = mSpellCheck->GetCurrentDictionary(&currentDictionary);
Fortunately you fixed this leak in attachment 553150!

Revision history for this message
In , L. David Baron (dbaron) wrote :

(In reply to Boris Zbarsky (:bz) from comment #61)
> David, what do you think of the above bits about making the style system's
> concept of "language" take into account xml:lang?

We should do that. It's bug 234485.

Revision history for this message
In , Purkat (purkat) wrote :

(In reply to arno renevier from comment #98)
> Oups. Look like there's a serious bug in this patch :(
> When user manually sets a non default dictionary, it's dismissed as soon as
> editor is blured and focused again (because we go the
> UpdateCurrentDictionary path again).

Should this bug be fixed in latest Nightly (9.0a1 (2011-08-17))? I'm still seeing it.

Revision history for this message
In , Eshepherd (eshepherd) wrote :

Is this in Nightly or Aurora? It's flagged as Gecko 8, but someone documented it as Gecko 9. I'd like to confirm which it is.

Revision history for this message
In , Arnaud Renevier (arenevier) wrote :

(In reply to Eric Shepherd [:sheppy] from comment #112)
> Is this in Nightly or Aurora? It's flagged as Gecko 8, but someone
> documented it as Gecko 9. I'd like to confirm which it is.

It's currently in Aurora, but backout from gecko8 is in process. See bug 684467.

Revision history for this message
In , Eshepherd (eshepherd) wrote :

This got written up here:

https://developer.mozilla.org/en/HTML/Controlling_spell_checking_in_HTML_forms#Controlling_the_spellchecker_language

And it's listed on Firefox 9 for developers. Firefox 9? ZOMG!

Revision history for this message
In , Yann Dìnendal (yannbreliere) wrote :

Bugzilla should set a lang="en" to take advantage of this.

Revision history for this message
In , Acelists (acelists) wrote :

Could you please file the bug (there is a bugzilla Product) and reference this bug there? Thanks.

Revision history for this message
In , Yann Dìnendal (yannbreliere) wrote :
Revision history for this message
In , Bugzzilla (bugzzilla) wrote :

In which Firefox version is this supposed to be fixed? It still does not work in FF 11.

Revision history for this message
In , Bzbarsky (bzbarsky) wrote :

Michael, this was fixed as of Firefox 8.

If you have a testcase that shows it not working in Firefox 11, please file a bug and cc me?

Revision history for this message
In , Bugzzilla (bugzzilla) wrote :

I've checked it again, and the behaviour is not really consistent. Sometimes it changes the language according to the lang attribute, and sometimes it does not. Is it possible that FF does not really select the language, but the dictionary that was last selected manually on the same page? And sometimes spellchecking is automatically turned off, and I have to turn it back on manually. This is very odd.

Revision history for this message
In , Ehsan-mozilla (ehsan-mozilla) wrote :

If you have steps to reproduce, please file a new bug and we'll discuss it there.

Revision history for this message
In , Subscrg (subscrg) wrote :

Still FF 31.0 spell checker doesn't respect lang attributes in textareas:

<!DOCTYPE html>
<html lang="en">
<head>
 <meta http-equiv="Content-Type" content="text/html; charset=UTF-8">
 <title>Textareas with different lang attributes</title>
</head>
<body>
 <form>
  <textarea lang="it" rows="5" cols="40" spellcheck="true">Questa casella di testo contiene del testo in italiano</textarea>
  <br>
  <textarea lang="en" rows="5" cols="40" spellcheck="true">This textarea contains english text</textarea>
 </form>
</body>
</html>

Whit this code the spell checker will be set to English language on both the textareas, even the one that is clearly marked as lang=it; if I right click on the Italian textarea and choose Italian as language, BOTH the textareas will turn to Italian language!

Even if you remove the lang attribute on the html tag (which in my opinion shouldn't be necessary, since it correctly states that the text of the page is in English), the behaviour is the very same.

Compare this behaviour with what IE10 does: it correctly uses a different dictionary for each textarea; Chrome doesn't support more that one dictionary ad a time, so it's impossible it could work correctly.

Revision history for this message
In , Bzbarsky (bzbarsky) wrote :

This bug is about the page language only. Please file a separate bug on using different spellchecking dictionaries for different parts of the page.

Revision history for this message
In , Subscrg (subscrg) wrote :

From what I read in the first post of this bug, it refers EXACTLY to the fact that the browser should respect BOTH the default page language AND the lang attribute specified in each textarea...

And that is exactly the bug I was talking about!

Revision history for this message
In , Bzbarsky (bzbarsky) wrote :

Yes, but this bug was then morphed into just following the general-page-specified language, and fixed in that form. Which is why followup work should happen in a separate bug.

Revision history for this message
In , Subscrg (subscrg) wrote :

So this turned into fixing a different bug? :)

Ok, I'll post another bug report with the very same test case!

Revision history for this message
In , Erling (erlingwl) wrote :
Displaying first 40 and last 40 comments. View all 133 comments or add a comment.
This report contains Public information  
Everyone can see this information.

Other bug subscribers

Remote bug watches

Bug watches keep track of this bug in other bug trackers.