992944 - Spell checker skips current locale when pref spellchecker.dictionary or document content invalid

Status: RESOLVED DUPLICATE of bug 1200533

(Core :: Spelling checker, defect)

Description

[Jan Horak [:jhorak]]

When guessed dictionary from document content or spellchecker.dictionary perf does not exists the spellchecker is not trying to get dictionary from current locale.

This is hitting Fedora users because we use system dictionaries and there's no 'en' dictionary, only 'en-country/region'. In the end the default dictionary is 'en_AG' which makes users quite unsatisfied.

Downstream bug: https://bugzilla.redhat.com/show_bug.cgi?id=439598

Comment 1

[Jan Horak [:jhorak]]

Attachment: spell-check-locale.patch

Please have a look.

Comment 2

[(no longer active)]

Thanks for your patch, Jan!

I'm definitely open to change our behavior here but there has been a few recent bugs on issues similar to this, so I think it might make sense to take a few moments to reconcile things.  The bugs that I'm talking about are: bug 991253, bug 992118 and bug 991278.  CCing Landry.

About this patch, can you please explain the issue in more detail?  Is this about people having bogus spellchecker.dictionary prefs or something?  Specifically I'm not sure if I understand the second paragraph of comment 0 completely.

It would be nice if we came up with a plan of how we want to change this code first.

Comment 3

[(no longer active)]

Comment on attachment 8402690 [details] [diff] [review]
spell-check-locale.patch

Also can you please attach a diff -w patch for review?  I think it's mostly moving code around so it would make it easier for me to check the changes to the logic.  Thanks!

Comment 4

[Jan Horak [:jhorak]]

Thanks for the response.

The mPreferredLang usually contains 'en' string:
http://mxr.mozilla.org/mozilla-central/source/editor/composer/src/nsEditorSpellCheck.cpp#748

When it is not found the first dictionary from dictionary list is picked:
http://mxr.mozilla.org/mozilla-central/source/editor/composer/src/nsEditorSpellCheck.cpp#856

Problem is that 'en' is never found when you have en-US.dic, en-GB.dic, en-CA.dic, en-whatever.dic installed and the spell checker picks first from dictionary list which is most likely the one you don't want to. What you usually want is dictionary based on current locale, not the first one from the list of installed dictionaries.

This bug is relevant only for Linux distros where Firefox/Thunderbird use system dictionaries, where you can find symlink /usr/lib64/firefox/dictionaries -> /usr/share/myspell.

I hope I made it clear, you can check downstream bug what people are complain about.

Comment 5

[Jan Horak [:jhorak]]

Attachment: spell-check-locale-w.patch

Attaching diff with -w.

Comment 6

[(no longer active)]

Comment on attachment 8403125 [details] [diff] [review]
spell-check-locale-w.patch

I understand now, this makes sense to me.  Thanks!

(Obviously you want to check in the other version of the patch of course!)

Comment 7

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5a50c5065173

Comment 8

[Carsten Book [:Tomcat]]

seems this caused a lot of mochitest failures like 
https://tbpl.mozilla.org/php/getParsedLog.php?id=37557373&tree=Mozilla-Inbound
https://tbpl.mozilla.org/php/getParsedLog.php?id=37557137&tree=Mozilla-Inbound
and
https://tbpl.mozilla.org/php/getParsedLog.php?id=37557181&tree=Mozilla-Inbound

and had to backout this change

jan, would it be possible to run a try run next time before you push this change ?

Comment 9

[Jan Horak [:jhorak]]

I finally have try server access, so I'll run it next time. 

I've got problems with tests, especially ref test. Problem is that when you're trying to set lang or dictionary to testing-XX, it fall back to en-US because it is default locale and this dictionary is found. Ideally I need more dictionaries installed that I can switch between them.

Comment 10

[(no longer active)]

Jan, you shouldn't need to install new dictionaries when running those reftests.  In fact, the tests which use testing-XX are trying to test the case when there is a website which uses a language for which you don't have a dictionary installed.  Perhaps your patch breaks the fallback path for those cases somehow.  Have you tried capturing one of these failing tests in gdb?

(Please feel free to ping me on IRC if you needed help debugging this.)

Comment 11

[Jan Horak [:jhorak]]

Yes, it breaks these tests because they expect no dictionary when switching to testing-XX and expecting all English words as misspell but my patch falls back to en-US.

Comment 12

[(no longer active)]

Hmm, ok, I guess that makes sense at least.

Now, can you please confirm that bug 992118 did not fix the underlying issue here?

Comment 13

[Jan Horak [:jhorak]]

Problem is still present, I don't know how to fix the tests that they make some sense.

Comment 14

[Jorg K (CEST = GMT+2)]

Comment on attachment 8402690 [details] [diff] [review]
spell-check-locale.patch

Patch obsolete after rework of nsEditorSpellCheck.cpp in bug 1200533.

Is this still an issue in FF 43? Bug 1200533 should have fixed this. If fixed, please close bug.