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)
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
Assignee | ||
Comment 1•11 years ago
|
||
Please have a look.
Assignee: nobody → jhorak
Attachment #8402690 -
Flags: review?(ehsan)
Comment 2•11 years ago
|
||
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.
Updated•11 years ago
|
Comment 3•11 years ago
|
||
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)
Assignee | ||
Comment 4•11 years ago
|
||
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.
Assignee | ||
Comment 5•11 years ago
|
||
Attaching diff with -w.
Attachment #8403125 -
Flags: feedback?(ehsan)
Comment 6•11 years ago
|
||
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+
Assignee | ||
Updated•11 years ago
|
Attachment #8402690 -
Flags: review+
Assignee | ||
Updated•11 years ago
|
Attachment #8403125 -
Attachment is obsolete: true
Assignee | ||
Updated•11 years ago
|
Keywords: checkin-needed
Comment 7•11 years ago
|
||
Keywords: checkin-needed
Comment 8•11 years ago
|
||
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 ?
Assignee | ||
Comment 9•11 years ago
|
||
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•11 years ago
|
||
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.)
Assignee | ||
Comment 11•10 years ago
|
||
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•10 years ago
|
||
Hmm, ok, I guess that makes sense at least.
Now, can you please confirm that bug 992118 did not fix the underlying issue here?
Assignee | ||
Comment 13•10 years ago
|
||
Problem is still present, I don't know how to fix the tests that they make some sense.
Comment 14•9 years ago
|
||
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-
Updated•9 years ago
|
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.
Description
•