1129859 - Wrong dictionary chosen when lang attribute is en-US on some Linux distros

Status: RESOLVED FIXED

(Core :: Spelling checker, defect)

Description

[Jan Horak [:jhorak]]

Some Linux distros using '_' as separator in dictionary name between language and region. When page specifies region in lang attribute like lang="en-US" then it fails to find correct dictionary because system dictionary name is actually named "en_US".

Comment 1

[Jan Horak [:jhorak]]

Attachment: Fix for Linux dicts v1

Please have a look.

Comment 2

[Jan Horak [:jhorak]]

Try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=b94218ccadf5

Comment 3

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 8559725 [details] [diff] [review]
Fix for Linux dicts v1

Review of attachment 8559725 [details] [diff] [review]:
-----------------------------------------------------------------

::: editor/composer/nsEditorSpellCheck.cpp
@@ +794,5 @@
> +    if (NS_FAILED(rv)) {
> +      int32_t dashIdx = dictName.FindChar('-');
> +      if (dashIdx != -1) {
> +        nsString dictNameUnderscored(dictName);
> +        dictNameUnderscored.SetCharAt('_', dashIdx);

This doesn't seem like the right way to fix this bug:

-- This is platform-specific so shouldn't really show up in cross-platform code.
-- SetCurrentDictionary (and maybe other code) looks for a '-' and this will break that.
-- This gets stored in page-specific prefs that could potentially be shared between platforms.

Seems to me there should be a lower-level place where we can replace - with _, when actually loading a dictionary.

But I don't really understand this bug. Can you explain in more detail?

Comment 4

[Jan Horak [:jhorak]]

Problem is that you are using '-' separator, while Fedora or RHEL is using '_' separator in dictionary names. They (Fedora) use it to match witch locale and changing it would break others apps like LibreOffice, so we can't change dictionary names in Fedora. So in Fedora's Firefox fallback code is called when page has something like lang='en-US' in source and so first of dictionary in dictionary list is picked, which is usually wrong because user have usually a lot of English dictionaries for various regions installed by default.

Changing it in lower level (ie. replace '_' with '-' in dictionary name) breaks some test, like test with base_utf8 dictionary.

Comment 5

[Jan Horak [:jhorak]]

With this change: https://hg.mozilla.org/try/rev/8a0da54a68e7
and modified tests (ie. replacing all '_' character in test dictionary' filenames  by '-') looks fine. 
Corresponding try run:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=8a0da54a68e7
Issue in Linux is solve by this. Could you please give me a feedback to this change? Thank you.

Comment 6

[Robert O'Callahan (:roc) (email my personal email if necessary)]

> Problem is that you are using '-' separator, while Fedora or RHEL is using '_' separator in dictionary
> names.

FWIW the BCP47 spec says '-': http://www.ietf.org/rfc/bcp/bcp47.txt

(In reply to Jan Horak from comment #5)
> With this change: https://hg.mozilla.org/try/rev/8a0da54a68e7
> and modified tests (ie. replacing all '_' character in test dictionary'
> filenames  by '-') looks fine. 
> Corresponding try run:
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=8a0da54a68e7
> Issue in Linux is solve by this. Could you please give me a feedback to this
> change? Thank you.

This looks good to me!

Comment 7

[Jan Horak [:jhorak]]

Attachment: Fix for Linux dicts v2

Attaching fix for this bug, replace all '_' by '-' in dictionary names. Due to this change all '_' characters in dictionary filenames used for testing has been replaced by '-'. I also removed some code in nsEditorSpellcheck.cpp which is no longer necessary.

Try run:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=47ca377de5ab

Comment 8

[Jan Horak [:jhorak]]

Thanks for the review.

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/226ad2adf361

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/226ad2adf361