1200533 - Problems with dictionary selection: No dictionary set if no installed dictionary matches site language, content-language retrieved too late, various other problems

Status: RESOLVED FIXED

(Core :: Spelling checker, defect)

Description

[Jorg K (CEST = GMT+2)]

While working on bug 1073840, which aimed at establishing an "override" preference for dictionary choice, many problems were detected in the current implementation of the dictionary choice and fallback methods.

The most prominent ones are:
- Content preference "en-GB", "en-AU", etc. ignored on "en" site.
- No dictionary set if no installed dictionary matches site language.

Below is a full list:

https://dxr.mozilla.org/mozilla-central/source/editor/composer/nsEditorSpellCheck.cpp#616
("en-GB" on an "en" site like BMO gets tossed away)
https://dxr.mozilla.org/mozilla-central/source/editor/composer/nsEditorSpellCheck.cpp#764
(faulty error handling)
https://dxr.mozilla.org/mozilla-central/source/editor/composer/nsEditorSpellCheck.cpp#768
(this is wrong, that should be retrieved straight after getting the language of the
element/parent, if this isn't set)
https://dxr.mozilla.org/mozilla-central/source/editor/composer/nsEditorSpellCheck.cpp#834
(this is wrong, since rv controlls the flow of the fallback, but is used as the return 
status for a helper function)
https://dxr.mozilla.org/mozilla-central/source/editor/composer/nsEditorSpellCheck.cpp#853
(IMHO the status should be checked here)
https://dxr.mozilla.org/mozilla-central/source/editor/composer/nsEditorSpellCheck.cpp#859
(this is wrong, leading to no dictionary being selected)
https://dxr.mozilla.org/mozilla-central/source/editor/composer/nsEditorSpellCheck.cpp#871
(this is wrong, must not check rv again)
https://dxr.mozilla.org/mozilla-central/source/editor/composer/nsEditorSpellCheck.cpp#880
(this makes no sense, since en-US is not a useful fallback for most locales).

This bug aims at correcting these problems while maintaining the overall behaviour which is:

* If users go on a website which uses the lang attribute, we respect that.
* If users change their preferred dictionary on a website, they will get that language
  for that website in the future,
  and for all other cases where we are unsure of what language the website needs.

Note as an aside:
  This is not ideal because of two reasons:
  1. The user can't globally override the choice of the website author.
  2. They don't have a good UI for adjusting the default spell checking language.

Part of "unsure of what language the website needs" is some fallback processing, which uses a dictionary which the user previously set on another website and which at that point got stored in "spellchecker.dictionary". The fallback may also use the user's locale.

The clear order of priorities that will be established will be:
1) content preference
2) language set by the website, or any other dictionary that partly matches that,
   so if the website is "en-GB", a user who only has "en-US" will get that.
3) the value of "spellchecker.dictionary" which reflects a previous language choice of the user
   (on another site)
4) the user's locale
5) the content of the "LANG" environment variable (if set)
6) the first spell check dictionary installed.

Comment 1

[Jorg K (CEST = GMT+2)]

Attachment: Proposed solution (v1) - will be refreshed once bug 717433 lands

This is my proposed solution to fix the content preference behaviour and make the fallback behaviour work in all cases. It is also much simplified.

Print statements are still in the code for people who want to try and run it.

Comment 2

[Jorg K (CEST = GMT+2)]

Attachment: Some test cases where document/elements specify language.

Comment 3

[Jorg K (CEST = GMT+2)]

Attachment: Test case where document doesn't specify language

Comment 4

[Jorg K (CEST = GMT+2)]

Comment on attachment 8655408 [details] [diff] [review]
Proposed solution (v1) - will be refreshed once bug 717433 lands

The try run https://treeherder.mozilla.org/#/jobs?repo=try&revision=dc2cea6394e5 failed with:
INFO TEST-UNEXPECTED-FAIL | editor/composer/test/test_bug678842.html | unexpected lang en-US - got "en-US", expected "".

This is to be expected, since the purpose of this bug is to always supply a valid dictionary (so that for example en-US users don't have to change the dictionary for every single "foreign language" site they visit). Since the test machines (I assume) are build with locale en-US, this will be returned now instead of "".

I would like some feedback on whether it is considered useful to clean up the dictionary selection which has a few problems (as outlined in comment #0) and at the same time change the fallback behaviour slightly, as in the case of the test that now fails.

I'm asking Robert, since Ehsan expresses in his nickname that he doesn't want to be asked for reviews. Also, he is opposed to any change in this area, as can be read here:
https://groups.google.com/forum/#!topic/mozilla.dev.platform/Et02D8Mk2d0.

Changing the particular test to do something useful would cause considerable work, since (I assume) the tests all run on the en-US locale with no further dictionaries available. I meaningful test would be:
Load page once, get fallback en-US (since lang="testing-XXX" is specified in the test), set other dictionary, load page again, get other dictionary.

Other tests should be performed on cases as shown in the other attachments.

I'm happy to keep working on this, but I don't want to have my work go to waste should the whole concept be rejected in favour of "cementing in" the status quo.

Comment 5

[Jorg K (CEST = GMT+2)]

I just saw that bug 717433 (from 2012) reports the first part of what I try to fix here:
Content preference en-GB ignored on "en" site. Maybe time to fix it for the all the people in the Commonwealth who dislike the en-US spelling of "Recognize your neighbor by his color" ;-)

Comment 6

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

(In reply to Jorg K (GMT+2) from comment #4)
> I'm asking Robert, since Ehsan expresses in his nickname that he doesn't
> want to be asked for reviews. Also, he is opposed to any change in this
> area, as can be read here:
> https://groups.google.com/forum/#!topic/mozilla.dev.platform/Et02D8Mk2d0.

I can't override Ehsan's opinion here, sorry!

Comment 7

[Jorg K (CEST = GMT+2)]

Comment on attachment 8655408 [details] [diff] [review]
Proposed solution (v1) - will be refreshed once bug 717433 lands

Part of this patch has been accepted in bug 717433. The patch will be refreshed in due course.

Comment 8

[Jorg K (CEST = GMT+2)]

Just repeating the second part of the problem:
"No dictionary set if no installed dictionary matches site language".

Quoting from the mailing list:

I've installed a German localised version of Firefox and one dictionary, the German one. This represents the average non-English speaking user.
* On every new site I visit that supplies a "lang" setting, I have to explicitly select
  the German spell checking dictionary. No default is supplied.
* On every new site I visit that *does not* supply a "lang" setting, I automatically
  get the German dictionary selected (since there my last choice, which was saved in 
  spellchecker.dictionary, gets applied).

It is a) highly annoying having to set the dictionary over and over and b) very inconsistent, since the user doesn't generally analyse the HTML content of the site to understand the behaviour.

Comment 9

[Jorg K (CEST = GMT+2)]

From bug 717433 comment #16:
test_add_remove_dictionaries.xul shows how to install dictionaries in a test and helper_bug1170484.js used in test_bug1170484.html shows how to click on the spell checking context menu to change the language (helper_bug1170484.js shows how to pick a spelling suggestion from the context menu which should be similar.)

This is relevant should we go ahead with the cleanup and need to change the test as detailed in comment #4.

Comment 10

[Jorg K (CEST = GMT+2)]

Attachment: Proposed solution (v2) - refreshed since bug 717433 landed

This is the refreshed patch after the landing of bug 717433.

Attachment 8656164 [details] [diff] of this bug shows, how an "en-GB" dictionary can be added for testing.

Comment 11

[Jorg K (CEST = GMT+2)]

I'm changing the summary, since the first complaint has been fixed in bug 717433.

Remaining problems are:
- No dictionary set if no installed dictionary matches site language (details in comment #8).
- <meta http-equiv="Content-Language" content="en-US"> retrieved too late in the processing.
- "en-US" makes little sense as a fallback.
- error handling and flow control wrong in various places.
- logic overly complicated.

Ehsan, when you have some "bandwidth", would you consider accepting some or all of this. Of course I will write an appropriate test (as I've just done for bug 717433).

My offer stands: After the cleanup I will assume responsibility for this issue for one year ... and you'll never have to look at it again after all those years ;-)

Comment 12

[(no longer active)]

Redirecting to smaug (please see my dev-platform post of a few seconds ago.)

Comment 13

[Jorg K (CEST = GMT+2)]

Hi Olli, if you have some time, please look at my proposal.
The print statements are in place, so you can see this at work.
Sorry, this looks messy, since I'm making lots of little changes everywhere. Maybe it would be easiest to apply the patch and look at the result.

Once we agree, I will write a test. Attachment 8656164 [details] [diff] (of another bug) shows how to add another dictionary or more to conduct meaningful testing. Test cases are already attached.

Comment 14

[Olli Pettay [:smaug][[e-mail]]]

I'll try to look at this tomorrow.

Comment 15

[Jorg K (CEST = GMT+2)]

Thanks, no rush, I have plenty of other work. This has been broken for years, so one day more or less makes no difference ;-)

Comment 16

[Jorg K (CEST = GMT+2)]

Attachment: Proposed solution (v3) - refreshed since bug 717433 landed

We should cater for sites which specify the language in all lower case, for example "en-gb". This way, we can fix bug 1200186 here at the same time.

This is meant to be a "steel brush" clean-up after all ;-)

Comment 17

[Jorg K (CEST = GMT+2)]

Attachment: Some test cases where document/elements specify language, one more: en-gb

Added case where site specifies "en-gb".

Comment 18

[Jorg K (CEST = GMT+2)]

Attachment: Proposed solution (v4) - slowly converging towards perfection ;-)

Another improvement in the logic.

Instead of calling SetCurrentDictionary(), we check first, whether the dictionary is in our list. This prevents needless calls and also has the advantage, that the currently set dictionary, which is used as a fallback, is not changed.

Comment 19

[Olli Pettay [:smaug][[e-mail]]]

Could you explain the behavior you want to achieve with the patch (and I think we need some comment in the code too explaining the behavior, this is getting complicated.)

Nit, don't ever refer to http://www.w3.org/TR/html401. It is obsolete spec, at least in this case
And we try to follow HTML spec, not W3C's HTML5, so links to https://html.spec.whatwg.org/ are preferred.

Comment 20

[Jorg K (CEST = GMT+2)]

Oh, you haven't followed the discussion. Here is the desired behaviour, actually, it says so in comment #0, but I'll repeat it. If you call this complicated, you should look at the original code, which is full of ... try this, if we haven't tried it yet ...

The clear order of priorities that will be established will be:
1) content preference, so the language the user set for the site before.
2) language set by the website, or any other dictionary that partly matches that.
   Eg. if the website is "en-GB", a user who only has "en-US" will get that.
   If the website is generic "en", the user will get one of the "en-*" installed, (almost) at random.
   However, we prefer what is stored in "spellchecker.dictionary",
   so if the user chose en-AU before, they will get en-AU on a plain "en" site.
3) the value of "spellchecker.dictionary" which reflects a previous language choice of the user
   (on another site)
4) the user's locale
If by then still no possible dictionary is found, we will just leave the current one set. If that's also not set, there are two last (desperate) options:
5) the content of the "LANG" environment variable (if set)
6) the first spell check dictionary installed.

This is pretty much what is currently happening, however, the code is cleaned up. All the error handling is correct.

Also, the most obvious change is, that there will always be a dictionary set. The case described in comment #8 where a user repeatedly has to set dictionaries, will not occur any more.

So can you please try this using the test case in attachment 8656399 [details]. If you're happy with the way it works, please do a review. Once we have that, I will address the nits, for example remove the comment referring to the old spec, add the necessary tests (and modify the existing test for bug 678842 (see comment #4)).

Comment 21

[Jorg K (CEST = GMT+2)]

I forgot to repeat: While cleaning up, I'd like to make the compare done on the language provided by the site case insensitive, to fix bug 1200186 as well. Also, I want to eliminate useless calls to SetCurrentDictionary(). Previously it was called with "en" on a a generic "en" site, knowing in advance that this would fail! In short: A clean-up with a steel brush while maintaining most of the behaviour.

Comment 22

[Olli Pettay [:smaug][[e-mail]]]

(In reply to Jorg K (GMT+2) from comment #20)
> Oh, you haven't followed the discussion.
I have, but there has been so many variants of the explanation and different bugs fixing different issues ;)
And as you indicate in comment 21, the patch does more than what comment 0 is about.

Thanks for clarifying.

Comment 23

[Lalo Martins]

(In reply to Jorg K (GMT+2) from comment #20)
> 2) language set by the website, or any other dictionary that partly matches
> that.
>    Eg. if the website is "en-GB", a user who only has "en-US" will get that.
>    If the website is generic "en", the user will get one of the "en-*"
> installed, (almost) at random.

On a separate (sub-)topic, it could be argued that the locale should come into play here. So, e.g., if the site is "en" and the user has en-GB, en-US, en-AU (common case in Ubuntu apparently), but the locale is one of those three, it should be preferred.

But I don't know if you want to consider that in scope. Perfectly fine by me if you don't.

Comment 24

[Jorg K (CEST = GMT+2)]

OK. We could retrieve the locale at the start and then select the en-* that matches it.
I can do that. I would still give "spellchecker.dictionary" a higher priority, since this contains a dictionary that the user once set actively.

Right now, I don't want to change the patch while my reviewer is looking at it ;-)
Let's see what he says.

Comment 25

[Olli Pettay [:smaug][[e-mail]]]

Feel free to change the patch. I'll look at it more tomorrow.
(and the updated patch could remove printfs and contain the comments I asked and fix nits)

Comment 26

[Jorg K (CEST = GMT+2)]

Are you sure you want to look without the printf? They are quite good to see what it's doing. I was considering leaving them with some #if DEBUG. They could be useful in the future. There was only one nit so far, which was the comment.

Comment 27

[Olli Pettay [:smaug][[e-mail]]]

I want to look at patches as if they would be ready to be committed.

Comment 28

[Jorg K (CEST = GMT+2)]

One other purpose of this bug is to close the following bugs. They are all confused. My aim is to state the expected behaviour and close them. Here is a list:
bug 455235, bug 728069, bug 836230, bug 853970, bug 858666, bug 909040, bug 923356, bug 932925, bug 1200186 (not confused).

Comment 29

[Jorg K (CEST = GMT+2)]

(In reply to Olli Pettay [:smaug] from comment #27)
> I want to look at patches as if they would be ready to be committed.
And the debug? Remove or #ifdef DEBUG?

Comment 30

[Olli Pettay [:smaug][[e-mail]]]

if you can live without extra DEBUG, good, but
I guess this code is executed so rarely that DEBUG code is fine in this case and doesn't cause too much noise to the terminal.

Comment 31

[Jorg K (CEST = GMT+2)]

Attachment: Proposed solution (v5) - slowly converging towards perfection ;-)

Nits fixed: Not mentioning old HTML spec any more, also included large comment that describes what we're doing.

I wrapped the printf into #ifdef DEBUG_DICT since these carefully crafted print statements add value should we want to debug this in the future.

This can be reviewed so far, but I also need to change the test for bug 678842 (comment #4) and also add a proper test based on the attached test cases. Of course this could be done in a separate patch.

(I couldn't request Olli as reviewer, so I'm adding NI).

Comment 32

[Jorg K (CEST = GMT+2)]

Attachment: Proposed solution (v6) - slowly converging towards perfection ;-)

Improved some comments.

Comment 33

[Jorg K (CEST = GMT+2)]

Attachment: Modification of test for bug 678842

It's pretty obvious why this has to change.

Now, for lang="testing-XXX" (see bug678842_subframe.html) we default to the locale "en-US" instead of no dictionary.

So we change this to "en-GB" and test that this sticks upon loading the page again.

Limited try run here:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=ba784a63c41c
(sorry about the incorrect bug number in on of the patches, I will correct later)

If we all agree, I'll add the test for this bug next.

Comment 34

[Jorg K (CEST = GMT+2)]

Attachment: Modification of test for bug 678842 and bug 717433

Better to reset spellchecker.dictionary to avoid side-effects later.

Better try than sorry ;-)
https://treeherder.mozilla.org/#/jobs?repo=try&revision=b5cb352b83bd

Comment 35

[Jorg K (CEST = GMT+2)]

Try run green. Test for the bug will come soon.
Olli, how is the review coming along? ;-)
As I said, best to look at the patched result an run it.

Comment 36

[Jorg K (CEST = GMT+2)]

Attachment: Modification of test for bug 678842 and bug 717433 (v2)

Oops, forgot to declare some variables.

Comment 37

[Jorg K (CEST = GMT+2)]

Attachment: Modification of test for bug 678842 and bug 717433 (v3)

Added missing <body> tags

Comment 38

[Jorg K (CEST = GMT+2)]

Attachment: Test for the changed code (v1)

Here's the missing test.

New try run:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=42e2c96b8849

Comment 39

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8656977 [details] [diff] [review]
Proposed solution (v6) - slowly converging towards perfection ;-)

>+  /*
>+   * We try to derive the dictionary to use based on the following priorities:
>+   * 1) Content preference, so the language the user set for the site before.
>+   * 2) Language set by the website, or any other dictionary that partly matches that.
>+   *    Eg. if the website is "en-GB", a user who only has "en-US" will get that.
>+   *    If the website is generic "en", the user will get one of the "en-*" installed,
>+   *    (almost) at random.
>+   *    However, we prefer what is stored in "spellchecker.dictionary",
>+   *    so if the user chose "en-AU" before, they will get "en-AU" on a plain "en" site.
>+   * 3) The value of "spellchecker.dictionary" which reflects a previous language
>+   *    choice of the user (on another site).
>+   * 4) The user's locale
>+   * 5) Leave the current dictionary set.
Would  5) Use the currently set dictionary



>+  // Get the language from the element or its closest parent according to:
>+  // https://html.spec.whatwg.org/#attr-lang
>+  // This is used in SetCurrentDictionary.


We have UpdateDictionaryHolder on stack, so mUpdateDictionaryRunning is true when 
SetCurrentDictionary is called, and SetCurrentDictionary
only uses mPreferredLang if if (!mUpdateDictionaryRunning) {
But ok, the value is possibly used later.


>+  // Priority 2:
>+  // After checking the content preferences, we use the language of the document.
...language of the element or document.


>+  dictName.Assign(mPreferredLang);
>+#ifdef DEBUG_DICT
>+  printf ("***** Assigned from element/doc |%s|\n",
>+          NS_ConvertUTF16toUTF8(dictName).get());
>+#endif
> 
>-  // Then, try to use language computed from element
>-  if (!mPreferredLang.IsEmpty()) {
>-    dictName.Assign(mPreferredLang);
>-  }
>+  // Auxiliary status.
>+  nsresult rv2;
nsresult rv; and just reuse the same variable later.

>+  if (!dictName.IsEmpty()) {
>+    for (i = 0; i < dictCount; i++) {
>+      nsAutoString dictStr(dictList.ElementAt(i));
>+      if (dictName.Equals(dictStr, nsCaseInsensitiveStringComparator())) {
So we go through dictList here first time


>+      // Try dictionary.spellchecker preference, if it starts with langCode,
>+      // so we don't just get any random dictionary matching the language.
>+      if (!preferredDict.IsEmpty() &&
>+          nsStyleUtil::DashMatchCompare(preferredDict, langCode, comparator)) {
>+        // Try to set it, if we've got it.
>+        for (i = 0; i < dictCount; i++) {
>+          nsAutoString dictStr(dictList.ElementAt(i));
>+          if (dictStr.Equals(preferredDict)) {
and then here second time (is there are reason why we couldn't do case-insensitive comparison here?)


>+        for (i = 0; i < dictCount; i++) {
>           nsAutoString dictStr(dictList.ElementAt(i));
>-
>-          if (dictStr.Equals(dictName) ||
>-              dictStr.Equals(preferedDict) ||
>-              dictStr.Equals(langCode)) {
>-            // We have already tried it
>-            continue;
>-          }
>+          if (nsStyleUtil::DashMatchCompare(dictStr, langCode, comparator))
then here third time

>+  // Priority 3:
>+  // If the document didn't supply a dictionary or the setting failed,
>+  // try the user preference next.
>+  if (NS_FAILED (rv)) {
>+    if (!preferredDict.IsEmpty()) {
>+      dictName.Assign(preferredDict);
>+
>+      // Try to set it, if we've got it.
>+      for (i = 0; i < dictCount; i++) {
>+        nsAutoString dictStr(dictList.ElementAt(i));
>+        if (dictStr.Equals(dictName)) {
and here fourth time

>+      for (i = 0; i < dictCount; i++) {
>+        nsAutoString dictStr(dictList.ElementAt(i));
>+        if (dictStr.Equals(dictName)) {
and again here, fifth time.

This cries for some helper method.
If you need different kinds of Equals -checks, pass some enum value indicating what kind of comparison should be done.

Comment 40

[Olli Pettay [:smaug][[e-mail]]]

In other words, mostly just nits, but I could take a look at a new patch.

Comment 41

[Jorg K (CEST = GMT+2)]

FANTASTIC!!! Thanks!!
(In reply to Olli Pettay [:smaug] (reviewing overload. not reviewing for couple of days. If you want a review from me, please send email) from comment #39)

> Would  5) Use the currently set dictionary
Will fix comment.

> >+  // Priority 2:
> >+  // After checking the content preferences, we use the language of the document.
> ...language of the element or document.
Will fix comment.

> >+  dictName.Assign(mPreferredLang);
> >+#ifdef DEBUG_DICT
> >+  printf ("***** Assigned from element/doc |%s|\n",
> >+          NS_ConvertUTF16toUTF8(dictName).get());
> >+#endif
We're OK with the debug print?

> >+  // Auxiliary status.
> >+  nsresult rv2;
> nsresult rv; and just reuse the same variable later.
I don't understand. I need two statuses, rv, to drive the logic, rv2 for other things.

> and then here second time (is there are reason why we couldn't do
> case-insensitive comparison here?)
No need. preferredDict is from the preference. This we store when the user selects a dictionary. The casing will be right. We only need case-insensitive from what came from the document.

> and again here, fifth time.
> This cries for some helper method.
OK, I thought of a helper function. But the cases are quite different. I'll see what I can do.

Comment 42

[Olli Pettay [:smaug][[e-mail]]]

> > >+  // Auxiliary status.
> > >+  nsresult rv2;
> > nsresult rv; and just reuse the same variable later.
> I don't understand. I need two statuses, rv, to drive the logic, rv2 for
> other things.
Ah, I see. ok, fine to leave rv2.

Comment 43

[Jorg K (CEST = GMT+2)]

Do you want to review the tests separately? Or shall I merge all patches?

Comment 44

[Jorg K (CEST = GMT+2)]

Oh, you still haven't answered:
We're OK with the debug print?

Comment 45

[Olli Pettay [:smaug][[e-mail]]]

I'm just reviewing the other testing patch.

(In reply to Jorg K (GMT+2) from comment #41)
> > and then here second time (is there are reason why we couldn't do
> > case-insensitive comparison here?)
> No need. preferredDict is from the preference. This we store when the user
> selects a dictionary. The casing will be right. We only need
> case-insensitive from what came from the document.
I mostly just wondered whether we could do as few different kinds of comparisons as possible.
So, if case-insensitive comparison here doesn't break anything, it should be fine to use.
This is not in any way performance critical code.

Comment 46

[Olli Pettay [:smaug][[e-mail]]]

I'm ok with +#ifdef DEBUG_DICT and having printf inside it.

Comment 47

[Jorg K (CEST = GMT+2)]

OK. Looks like you're reviewing the patches separately. So I won't merge them.
I'll leave the debug and fix the nits/refactoring and we're done here. Hooray!
And we will have proper tests for once.

Comment 48

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8657292 [details] [diff] [review]
Test for the changed code (v1)

>+<textarea id="text1">root en-US</textarea>
>+<textarea id="text2" lang="en-GB">root en-US, but element en-GB</textarea>
>+<textarea id="text3" lang="en-gb">root en-US, but element en-gb (lower case)</textarea>
>+<textarea id="text4" lang="en-ZA">root en-US, but element en-ZA (which is not installed)</textarea>
>+<textarea id="text5" lang="en">root en-US, but element en</textarea>
>+<textarea id="text6" lang="ko">root en-US, but element ko (which is not installed)</textarea>
Might be a bit easir to read the other parts of the test if the id somehow indicated also which language the textarea has


>+<iframe id="content"></iframe>
>+
>+</div>
>+<pre id="test">
>+<script class="testbody" ttype="application/javascript">
>+
>+/** Test for Bug 1200533 **/
>+/** Visit the elements defined above and check the dictionary we got **/
>+SimpleTest.waitForExplicitFinish();
>+var content = document.getElementById('content');
>+
>+var tests = [
>+    // text area, value of spellchecker.dictionaty, result.
dictionary


mostly rs+ (rubberstamp)

Comment 49

[Jorg K (CEST = GMT+2)]

Attachment: Test for the changed code (v2)

Carrying over Olli Pettay's [:smaug] r/rs+.
Fixed nits.

Comment 50

[Jorg K (CEST = GMT+2)]

Attachment: Proposed solution (v7) - slowly converging towards perfection ;-)

Nits fixed. Refactored. Please review.

Another try run, better save than sorry:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=73d55858aec8

Comment 51

[Jorg K (CEST = GMT+2)]

Attachment: Proposed solution (v8) - slowly converging towards perfection ;-)

That didn't compile on Linux (due to a copy/paste error). Here we go again:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=f8b3db21b89d
(Note: The two underlying patches don't appear since they are still present on the try repository from the previous push:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=73d55858aec8)

Comment 52

[Jorg K (CEST = GMT+2)]

Attachment: Proposed solution (v9) - slowly converging towards perfection ;-)

Reformatted comments a bit, so everything complies with the "80 character" rule.
Also added some history. I know that Ehsan doesn't like bug numbers in the code, but I think it's important to understand how this has grown over time.

Comment 53

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8657528 [details] [diff] [review]
Proposed solution (v9) - slowly converging towards perfection ;-)

>+// Helper function that iterates over the list of dictionaries and sets the one
>+// that matches based on a given comparison type.
>+nsresult
>+nsEditorSpellCheck::TryDictionary(nsAutoString dictName,
>+                                  nsTArray<nsString>& dictList,
>+                                  enum dictCompare compareType)
arguments should be in aFoo form.

>+{
>+  nsresult rv = NS_ERROR_NOT_AVAILABLE;
>+
>+  nsDefaultStringComparator comparator;
Perhaps define also caseInsensitiveComparator
and pass that as a param in DICT_COMPARE_CASE_INSENSITIVE case.

>+  for (uint32_t i = 0; i < dictList.Length(); i++) {
>+    nsAutoString dictStr(dictList.ElementAt(i));
>+    bool equals = false;
>+    switch (compareType) {
>+      case DICT_NORMAL_COMPARE:
>+        equals = dictName.Equals(dictStr);
>+        break;
>+      case DICT_COMPARE_CASE_INSENSITIVE:
>+        equals = dictName.Equals(dictStr, nsCaseInsensitiveStringComparator());
>+        break;
>+      case DICT_COMPARE_DASHMATCH:
>+        equals = nsStyleUtil::DashMatchCompare(dictStr, dictName, comparator);
>+        break;
>+    }
>+    if (equals) {
>+      rv = SetCurrentDictionary(dictStr);
>+#ifdef DEBUG_DICT
>+      if (NS_SUCCEEDED(rv))
>+        printf("***** Set |%s|.\n", NS_ConvertUTF16toUTF8(dictStr).get());
>+#endif
>+      break;
Do we actually want to break here if SetCurrentDictionary failed? Especially in case of 
DICT_COMPARE_DASHMATCH.
Please fix or explain.

>+  /*
>+   * We try to derive the dictionary to use based on the following priorities:
>+   * 1) Content preference, so the language the user set for the site before.
>+   *    (Introduced in bug 678842 and corrected in bug 717433.)
>+   * 2) Language set by the website, or any other dictionary that partly
>+   *    matches that. (Introduced in bug 338427.)
>+   *    Eg. if the website is "en-GB", a user who only has "en-US" will get
>+   *    that. If the website is generic "en", the user will get one of the
>+   *    "en-*" installed, (almost) at random.
>+   *    However, we prefer what is stored in "spellchecker.dictionary",
>+   *    so if the user chose "en-AU" before, they will get "en-AU" on a plain
>+   *    "en" site. (Introduced in bug 682564.)
>+   * 3) The value of "spellchecker.dictionary" which reflects a previous
>+   *    language choice of the user (on another site).
>+   *    (This was the original behaviour before the aforementionted bugs
aforementioned
>+  if (NS_FAILED (rv)) {
extra spec after D
Same also elsewhere.


>+  nsresult TryDictionary(nsAutoString dictName, nsTArray<nsString>& dictList,
>+                         enum dictCompare compareType);
aFoo for arguments

Comment 54

[Jorg K (CEST = GMT+2)]

Attachment: Proposed solution (v10) - slowly converging towards perfection - Now perfect ;-)

Thanks a lot!

Carrying over Olli Pettay's [:smaug] r+.
Fixed nits.

As for the questions:
Re. the break:
Since we set a *valid* dictionary, the call should never fail.
Before the logic was: Try setting this, failed, try another one.
Now we're smarter. We already know the available dictionaries, so we only set it if we have it.

I didn't do the "perhaps" part you suggested, I didn't want another argument to the function. However, I cleaned up the comparator business some more.

Comment 55

[Jorg K (CEST = GMT+2)]

Dear Sheriff,

this bug has three patches:
- Modification of test for bug 678842 and bug 717433 (v3)
- Test for the changed code (v2)
- Proposed solution (v10) - slowly converging towards perfection - Now perfect ;-)

These patches can be applied in any order, but they must be landed as a set.
On the first patch "Modification of test for ..." I forgot to put r=smaug.

Would you be able to add this before committing it to the code base. Thank you.

Comment 56

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/74e6798a38a4
https://hg.mozilla.org/integration/mozilla-inbound/rev/b3ad852a044a
https://hg.mozilla.org/integration/mozilla-inbound/rev/9f3ca77e2d7b

Comment 57

[Carsten Book [:Tomcat]]

(In reply to Jorg K (GMT+2) from comment #55)
> Dear Sheriff,
> 
> this bug has three patches:
> - Modification of test for bug 678842 and bug 717433 (v3)
> - Test for the changed code (v2)
> - Proposed solution (v10) - slowly converging towards perfection - Now
> perfect ;-)
> 
> These patches can be applied in any order, but they must be landed as a set.
> On the first patch "Modification of test for ..." I forgot to put r=smaug.
> 
> Would you be able to add this before committing it to the code base. Thank
> you.

Hey Jork, thats no problem. also thanks for this great instructions!

Comment 58

[Carsten Book [:Tomcat]]

Jork, sorry had to back this out for test failures like https://treeherder.mozilla.org/logviewer.html#?job_id=13770342&repo=mozilla-inbound

Comment 59

[Pulsebot]

Backout:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2bb231870f2d

Comment 60

[Jorg K (CEST = GMT+2)]

Sorry, yes, this patch changes the way the dictionary is selected in a spellcheck situation.
I only fixed the mochitests, but obviously there are more tests affected.

The failures from comment #58 were in:
editor/reftests/338427-2.html and
editor/reftests/338427-3.html

I'm doing another try run and then there will be another part to be landed:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=fe69473b6989
Let's see whether there are more.

Comment 61

[Jorg K (CEST = GMT+2)]

Attachment: Modification of test for bug 338427

Olli, can you please rs+ this.

What the old test tried to achieve is not possible any more:
Have a misspelled word and suppress its spell check with an invalid language.

So I changed it to three good words, which will be spelled OK in en-US,
which is now the default (from the locale), if no dictionary matches the element's language.

I'm trying it here again:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=7fb81f0462aa

Comment 62

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8658241 [details] [diff] [review]
Modification of test for bug 338427

rs+...but this kind of hints that this is possibly the case where we may need to switch back to the old behavior.
Not having any spellchecking for unknown language makes totally sense
(and so does having the default spellchecking).

So, be prepared to back this out and change the patch once we've got some feedback about the new behavior. Maybe people don't like it, or maybe they do.

Comment 63

[Jorg K (CEST = GMT+2)]

(In reply to Olli Pettay [:smaug] from comment #62)
> So, be prepared to back this out and change the patch once we've got some
> feedback about the new behavior. Maybe people don't like it, or maybe they
> do.

Thanks for the review.

The test cases run while spellchecker.dictionary is unset. This will almost never happen in real life. Also, at least test case 1 here, the one that didn't fail, was ill-conceived. I added a comment, so the next guy along can understand. The other test for bug 338427 (test_bug338427.html) doesn't actually do any meaningful testing. The bug was: Set the language from the element, but no test case in any of the tests for bug 338427 does that. It's hard, since you need more dictionaries. Only my fine test here (attachment 8657339 [details] [diff] [review]) does the right testing.

The idea was to clean up the behaviour that no one could understand. That's why we have 10+ confused bugs in the area, see meta-bug 1073827. Show me the first person who liked the old behaviour. ;-)
Let me know if you can explain this line to any user:
https://dxr.mozilla.org/mozilla-central/source/editor/composer/nsEditorSpellCheck.cpp#852
(quick, the code will go as soon as this is landed).

Average users don't inspect the source code of the website. They don't know why in come cases no dictionary was selected with the old behaviour, in some cases one was selected. This is outlined in comment #8.

Anyway, enough ranting. I appreciate your help and I think we've done a great job which removes a problem people always complained about: the erratic spellchecker.

Comment 64

[Jorg K (CEST = GMT+2)]

Dear Sheriff,

this time I got it right. There are now four patches instead of three:
- Modification of test for bug 678842 and bug 717433 (v3)
- Test for the changed code (v2)
- Proposed solution (v10) - slowly converging towards perfection - Now perfect ;-)
- Modification of test for bug 338427

Apply in any order. If you're the same sheriff as before, you've already seen patches 1 to 3, only 4 is new, it fixes the test failures.

On the first patch "Modification of test for ..." I forgot to put r=smaug.
Would you be able to add this before committing it to the code base. Thank you.

Comment 65

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d1d9d161eacd
https://hg.mozilla.org/integration/mozilla-inbound/rev/631002821280
https://hg.mozilla.org/integration/mozilla-inbound/rev/08cd638ca00d
https://hg.mozilla.org/integration/mozilla-inbound/rev/fefcbd37135b

Comment 66

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/d1d9d161eacd
https://hg.mozilla.org/mozilla-central/rev/631002821280
https://hg.mozilla.org/mozilla-central/rev/08cd638ca00d
https://hg.mozilla.org/mozilla-central/rev/fefcbd37135b