Closed Bug 992944 Opened 11 years ago Closed 9 years ago

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

Categories

(Core :: Spelling checker, defect)

x86_64
Linux
defect
Not set
normal

Tracking

()

RESOLVED DUPLICATE of bug 1200533

People

(Reporter: jhorak, Assigned: jhorak)

References

Details

Attachments

(2 obsolete files)

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
Attached patch spell-check-locale.patch (obsolete) (deleted) — Splinter Review
Please have a look.
Assignee: nobody → jhorak
Attachment #8402690 - Flags: review?(ehsan)
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 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!
Attachment #8402690 - Flags: review?(ehsan)
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.
Attached patch spell-check-locale-w.patch (obsolete) (deleted) — Splinter Review
Attaching diff with -w.
Attachment #8403125 - Flags: feedback?(ehsan)
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!)
Attachment #8403125 - Flags: feedback?(ehsan) → review+
Attachment #8402690 - Flags: review+
Attachment #8403125 - Attachment is obsolete: true
Keywords: checkin-needed
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 ?
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.
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.)
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.
Hmm, ok, I guess that makes sense at least. Now, can you please confirm that bug 992118 did not fix the underlying issue here?
Problem is still present, I don't know how to fix the tests that they make some sense.
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.
Attachment #8402690 - Attachment is obsolete: true
Flags: needinfo?(jhorak)
Attachment #8402690 - Flags: review-
Status: NEW → RESOLVED
Closed: 9 years ago
Flags: needinfo?(jhorak)
Resolution: --- → DUPLICATE
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: