1456988 - Make dictionaries compatible by default. (Thunderbird 60 made my native language dictionary incompatible.)

Status: RESOLVED FIXED

(Toolkit :: Add-ons Manager, defect)

Description

[v1tesse]

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:59.0) Gecko/20100101 Firefox/59.0
Build ID: 20180323154952

Steps to reproduce:

Automatically updated to TB 60 beta


Actual results:

The Romanian spelling dictionary became incompatible


Expected results:

The Romanian dictionary should have kept working.

I say that because in Firefox, for example, I'm using the exact same dictionary. When FF switched to webextensions, I remember it saying something like "this is going to be incompatible with the new standard etc.", but after updating to Quantum, it was still there, even though it hasn't been updated since 2013 (https://addons.mozilla.org/en-US/firefox/addon/romanian-spellchecking-diction/).

In an e-mail app, typing is the main thing you do so no matter what other updates or improvements you make, if the main thing isn't working properly then nothing else matters. I don't think I make many spelling mistakes, but once in a while it comes in very handy.

While doing various tests, I found the config editor setting to bypass this restriction (extensions.strictCompatibility - false), but for most people who aren't tech savvy, this isn't really an option and, if things happen like with Firefox, even this setting might not work after a while.

Thankfully I had the time to figure things out, but I think this is an oversight that needs to be addressed.

Thanks in advance.

Comment 1

[Magnus Melin [:mkmelin]]

Can we just auto update the Thunderbird dictionaries to be compatible with 60? 
And those that are restartless, <em:type>64</em:type>, to be compatible with latest?


Philipp, what do you think?

Comment 2

[Magnus Melin [:mkmelin]]

Update server side, if that wasn't clear.

Comment 3

[Philipp Kewisch [:Fallen][📱📆]]

It might be possible, I haven't done a such thing though. The problem is that we'd need to do this compatibility bump on every beta release, which can be a fair amount of work. If we can fix this client side to ignore strict compatibility that would be preferable. TheOne would know the implications.

Comment 4

[Andreas Wagner [:TheOne] [use NI]]

Yes, making dictionaries compatible by default sounds like the right approach. I am not aware of any implications, but that doesn't mean there aren't any.

Comment 5

[Wayne Mery (:wsmwk)]

We would need this soon. How do we make it happen?

Comment 6

[Mihamina Rakotomandimby]

I face the problem here with 60.0b6 (64-bit) when I add Français dictionnary.

Comment 7

[Wayne Mery (:wsmwk)]

https://discourse.mozilla.org/t/dictionaries-will-break-if-60-0-ships-soon/
https://discourse.mozilla.org/t/greek-spelling-or-spellchecking-dictionary/

Comment 8

[Wayne Mery (:wsmwk)]

Philipp, is there someone other than you that can handle this?

Comment 9

[Mihamina Rakotomandimby]

For information, I worked arround the problem by allowing Thunderbird to install plugins even they are not explicitely told to be compatible with 60. This is done by editing the "extensions.strictCompatibility" setting to "false". I temporarily set this for the dictionnary, and then got back to the default "true" value.

Comment 10

[Philipp Kewisch [:Fallen][📱📆]]

I'm certain there is someone else who can do this, then again I don't think I signed up for doing it. IIUC, making dictionaries compatible by default requires a m-c patch that would allow for this. This is an engineering change I'd rather have someone else handle. I don't know that code very well.

Manually bumping all the dictionaries on AMO seems like a lot of work given it will need QA and such, so the first option sounds more lucrative to me.

Comment 11

[Jorg K (CEST = GMT+2)]

Would it be too terrible to require dictionaries to go with maxVersion set to * like we do for internal testing add-ons?
https://dxr.mozilla.org/comm-central/rev/f0c765cb850d750741bf4c63acb062deb6b442d9/mail/test/mozmill/folder-tree-modes/test-extension/install.rdf#15

Comment 12

[Wayne Mery (:wsmwk)]

To clarify, I wasn't suggesting you (fallen) should take this on - quite the opposite.  

I am just searching for the person/solution so we can have this in place when Thunderbird 60.0 is released - because we aren't making significant progress thus far, we typically we don't get fast progress on m-c patches, and most most of the dictionaries I see haven't had updates in recent years. (and compat wasn't done for firefox 60 or earlier, so we might assume it's not important to them)

Eyeballing the list at https://addons.mozilla.org/en-US/thunderbird/tag/dictionary?sort=users it's hard to know many users might be affected (the stats include firefox users).  Assuming more Thunderbird than Firefox users would want a dictionary (I don't know that's true), my guess is at least half a million Thunderbird users are affected.

Comment 13

[Wayne Mery (:wsmwk)]

Taking a stab that this needs to go to  Toolkit, Add-ons Manager

Comment 14

[Kris Maglione [:kmag]]

Dictionaries are already default compatible in gecko 60.

Comment 15

[Magnus Melin [:mkmelin]]

They are? Mind you we're settting extensions.strictCompatibility true in Thunderbird since we're still supporting old style add-ons - but those now generally need an update to work.

Comment 16

[Kris Maglione [:kmag]]

Compat by default only applies if strictCompatibility is not enabled. I'm not sure what legacy extensions have to do with setting strictCompatibility, though. We stopped setting that by default years before there was any idea of WebExtensions.

Comment 17

[Magnus Melin [:mkmelin]]

This is a request that dictionaries would still be compatible by default even when extensions.strictCompatibility is set to true

Comment 18

[Jorg K (CEST = GMT+2)]

We set strictCompatibility to avoid that add-ons not "made fit" for Gecko 60 would do damage in TB. Since there have been a plethora of interface changes in M-C between 52 and 60, most add-ons would need some changes anyway.

Comment 19

[Magnus Melin [:mkmelin]]

Attachment: bug1456988_dict_compat.patch

Untested (and probably incomplete)

Comment 20

[Kris Maglione [:kmag]]

(In reply to Magnus Melin from comment #17)
> This is a request that dictionaries would still be compatible by default
> even when extensions.strictCompatibility is set to true

I might accept a patch to do that for 60, since that's an ESR. But in general, we're trying to remove these kinds of special cases, and we're going to be removing support for legacy dictionaries in favor of WebExtension dictionaries (which set a maxversion of "*" by default), anyway.

Comment 21

[v1tesse]

I admit I'm not very knowledgeable when it comes to the inner workings of these kinds of things, but why not solve this like Firefox did - make a separate dictionary category and leave that always compatible?
https://imgur.com/a/3oYawDP

Like I mentioned at the beginning, even when Firefox made the switch to only support web extensions, this is why dictionaries remained compatible and, because of the separate category, this permanent compatibility didn't affect other addons.


Also, again, pardon my lack of knowledge, but isn't a dictionary basically a text list which typed words get compared to?
I get that sometime in the future, leaving dictionaries as legacy extensions, even in a separate category, might somehow cause some compatibility issues with other WebExtension features, but this doesn't seem to be the case for Firefox. At least not yet.

Furthermore, I get that living in an internet age, everything changes and gets updated more and more frequently, so addons shouldn't be seen as exceptions to that, but languages tend to evolve at a comparatively much slower pace, so dictionaries are usually seen as these monolithic things that don't really get modified very often (except when dictionary companies try to be "hip" and add new slang words).

Anyway, TL:DR, that's my suggestion. Separate dictionaries into a totally different category from normal addons like Firefox has done. Even if this might be more technically difficult to implement, it could be a more long-term solution.

Comment 22

[Marco A.G.Pinto [:marcoagpinto]]

[11:11] <marcoagpinto> guys?!
[11:11] <marcoagpinto> what's up with spellers in TB 60?
[11:11] <marcoagpinto> a guy reported for a tab "dictionaries" to be added and keep legacy spellers
[11:12] <marcoagpinto> I really don't know how to convert my speller
[11:41] <darktrojan> I still have my dictionary in 61
[11:42] <darktrojan> hmm, it doesn't appear to *work* though
[11:43] <%wsmwk> marcoagpinto: just change the max version afaik
[11:43] <marcoagpinto> wsmwk: I always change the max version
[11:43] <marcoagpinto> :)
[11:43] <marcoagpinto> but TB doesn't accept *
[11:43] <marcoagpinto> :)
[11:43] <marcoagpinto> FF accepts *
[11:47] <%wsmwk> hmm, then maybe that's why we are broken.  you should write detailed comments in the bug report

Comment 23

[Magnus Melin [:mkmelin]]

Attachment: bug1456988_dict_compat.patch

For Thunderbird, I tested with a profile created with verstion 52, and installed a dictionary there - the swedish one - https://addons.mozilla.org/en-US/thunderbird/addon/g%C3%B6rans-hemmasnickrade-ordli/?src=ss  - which according to install.rdf has <em:maxVersion>50.0</em:maxVersion>

After that, install the latest Thunderbird version and start with that profile.

Without the patch, now that Thunderbird has strict compatibility turned on by default the dictionary was disabled. With the patch it's all ok.

I've been testing it with trunk. Since tb nightlies has strict compatibility off, also patched it locally to set extensions.strictCompatibility true in mail/app/profile/all-thunderbird.js

Comment 24

[Kris Maglione [:kmag]]

Comment on attachment 8989017 [details] [diff] [review]
bug1456988_dict_compat.patch

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

::: toolkit/mozapps/extensions/internal/XPIInstall.jsm
@@ +716,5 @@
>             " in install manifest");
>        continue;
>      }
> +    if (addon.type == "webextension-dictionary") {
> +      targetApp.maxVersion = "*";

I don't think this is necessary if you're also making the compat-checking changes.

@@ +2601,3 @@
>      let ignoreMaxVersion = false;
> +    // Ignore strict compatibility for dictionaries by default.
> +    let ignoreStrictCompat = (this.addon.type == "webextension-dictionary");

I don't think this is quite the right approach. I think you want something like:

  (!AddonManager.strictCompatibility || this.addon.type == "webextension-dictionary")

so that we just ignore the AddonManager strict compatibility setting for dictionaries, but still respect the add-on-specific one.

You'll also need to make a similar change here:

https://searchfox.org/mozilla-central/rev/e45edf9ebc1f8e89686cace12c60085af7a4cdc5/toolkit/mozapps/extensions/internal/XPIDatabase.jsm#487-490

Really, we should probably just make the strictCompatibility pref type-specific... but that's probably a different bug.

::: toolkit/mozapps/extensions/internal/XPIProvider.jsm
@@ +2575,5 @@
>      let addons = await Promise.all(
>        Array.from(XPIStates.sideLoadedAddons.keys(),
>                   id => this.getAddonByID(id)));
>  
> +    return addons.filter(addon => (addon && addon.seen === false &&

This seems unrelated.

Comment 25

[Jorg K (CEST = GMT+2)]

Comment on attachment 8989017 [details] [diff] [review]
bug1456988_dict_compat.patch

I'll try the final version.

Comment 26

[Magnus Melin [:mkmelin]]

Attachment: bug1456988_dict_compat.patch

This works too.

Comment 27

[Magnus Melin [:mkmelin]]

kmag: ping on the review

Comment 28

[Onno Ekker [:nONoNonO UTC+1]]

I think the condition in XPIDatabase.jsm can be optimized a bit...

    if (this.type in COMPATIBLE_BY_DEFAULT_TYPES &&
        !this.strictCompatibility &&
        (!AddonManager.strictCompatibility || this.type == "webextension-dictionary")) {

Comment 29

[Kris Maglione [:kmag]]

Comment on attachment 8989046 [details] [diff] [review]
bug1456988_dict_compat.patch

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

Sorry, I didn't notice this had been updated.

r=me with these changes.

::: toolkit/mozapps/extensions/internal/XPIDatabase.jsm
@@ +490,3 @@
>      if (this.type in COMPATIBLE_BY_DEFAULT_TYPES &&
> +        ((!AddonManager.strictCompatibility && !this.strictCompatibility) ||
> +         (!this.strictCompatibility && this.type == "webextension-dictionary"))) {

This can be written as:

  if (this.type in COMPATIBLE_BY_DEFAULT_TYPES &&
      !this.strictCompatibility &&
      (!AddonManager.strictCompatibility ||
       this.type == "webextension-dictionary"))

::: toolkit/mozapps/extensions/internal/XPIInstall.jsm
@@ +2598,4 @@
>      let ignoreMaxVersion = false;
> +    // Ignore strict compatibility for dictionaries by default.
> +    let ignoreStrictCompat = (!AddonManager.strictCompatibility ||
> +                              this.addon.type == "webextension-dictionary");

This can just be `= this.addon.type == "webextension-dictionary"`. The !strictCompatibility case gets caught on the next line.

Comment 30

[Wayne Mery (:wsmwk)]

Thanks for moving this along. Just in time to land for building on Thursday.

Comment 31

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/205d60219ab8
don't use strict compatibility for dictionaries. r=kmag

Comment 32

[Magnus Melin [:mkmelin]]

(In reply to Kris Maglione [:kmag] from comment #29)
> This can just be `= this.addon.type == "webextension-dictionary"`. The
> !strictCompatibility case gets caught on the next line.

Right, that was in my original patch. 

Thanx for the review! Landed with comments addressed, and a needed adjustment in test_strictcompatibility.js which a try push showed

Comment 33

[Raul Gurzau (:RaulG)]

https://hg.mozilla.org/mozilla-central/rev/205d60219ab8

Comment 34

[Magnus Melin [:mkmelin]]

Uplift for Thunderbird60.

Comment 35

[Jorg K (CEST = GMT+2)]

I've been waiting for you ;-) - It's on the list of bugs to uplift already.

Comment 36

[Jorg K (CEST = GMT+2)]

Magnus, that patch doesn't apply to mozilla-esr60 at all. All hunks failed. Why do you think it would apply after three M-C revisions? :-(

Comment 37

[Jorg K (CEST = GMT+2)]

Hunks go here:
https://dxr.mozilla.org/mozilla-esr60/source/toolkit/mozapps/extensions/internal/XPIProvider.jsm#5025
https://dxr.mozilla.org/mozilla-esr60/source/toolkit/mozapps/extensions/internal/XPIInstall.jsm#2691
and into test_strictcompatibility.js. I'll do it tomorrow.

Comment 38

[Jorg K (CEST = GMT+2)]

Attachment: Mozilla60 version

This is the mozilla60 version. Sadly test_strictcompatibility.js has changed completely. Kris and Magnus, can you please check the test changes.

I'm pretty sure about changing the last argument to 'true' in these
-  do_check_compat_status(true, [true, false, false, false, false, false, false], run_test_1);
+  do_check_compat_status(true, [true, false, false, false, false, false, true], run_test_1);

but I'm unsure about
-    Assert.ok(!a7.strictCompatibility);
+    Assert.ok(a7.strictCompatibility);

I'd like to avoid landing something and having to follow up. Also, I don't see a way to do a try run for mozilla60 stuff. (Besides, the original author should really worry about that and not dump it into the lap of the code manager for the release.)

Comment 39

[Jorg K (CEST = GMT+2)]

Attachment: Mozilla60 version

Checked the test changes with Kris on IRC. Reverting one hunk.

Comment 40

[Jorg K (CEST = GMT+2)]

https://hg.mozilla.org/releases/mozilla-esr60/rev/cefdbe8256166a07816452429c5a3e27eb503652 (THUNDERBIRD_60_VERBRANCH)

Comment 41

[Jorg K (CEST = GMT+2)]

Test failure:
TEST-UNEXPECTED-FAIL | xpcshell.ini:toolkit/mozapps/extensions/test/xpcshell/test_strictcompatibility.js | do_check_compat_status/< - [do_check_compat_status/< : 145] false == true

That's this line when testing the dictionary:
  Assert.equal(a7.isCompatible, aAddonCompat[6]);

In the five sub-tests run, we always pass 'true' as aAddonCompat[6], but the test shows, that the add-on isn't compatible as expected.

Sadly it's hard to tell which of the sub-tests fails since the info strings are not printed.

Check https://taskcluster-artifacts.net/CU3bygYbSpOwfKwb86Nsew/0/public/logs/live_backing.log and search for "false == true".

I've come to the end of my sheriffing duties, so I'll refer this back to the original author Magnus.

Comment 42

[Jorg K (CEST = GMT+2)]

"webextension-dictionary" gives zero hits on mozilla-esr60:
https://dxr.mozilla.org/mozilla-esr60/search?q=webextension-dictionary&redirect=false

So the patch just appears to be wrong for mozilla60 and therefore not working which the test detects :-(

Comment 43

[Jorg K (CEST = GMT+2)]

This
https://dxr.mozilla.org/mozilla-esr60/rev/dd52b41d2b775e5c7261ce52795268b7670635fc/toolkit/mozapps/extensions/nsBlocklistService.js#1243
const types = ["extension", "theme", "locale", "dictionary", "service"];

together with
https://dxr.mozilla.org/mozilla-esr60/search?q=addon.type+path%3Atoolkit%2Fmozapps%2Fextensions&redirect=false

makes me think that it could/should be
  this.addon.type == "dictionary"
  this.type == "dictionary"
instead if
  this.addon.type == "webextension-dictionary"
  this.type == "webextension-dictionary"
in mozilla60.

Kris?

Comment 44

[Kris Maglione [:kmag]]

Correct.

Comment 45

[Jorg K (CEST = GMT+2)]

https://hg.mozilla.org/releases/mozilla-esr60/rev/abae8941234801d8aa5b6cfd8cc49ef62bc48e62 (THUNDERBIRD_60_VERBRANCH)
Let's see how that goes.

Comment 46

[Matt]

Sancus,  can we do something from the ATN end to bump these min versions to be more than 5.  It really is poor when we are basically offering dictionaries that can not be installed.

Comment 47

[Andrei Hajdukewycz [:sancus]]

I don't think we can, no, there's nothing to bump because ATN does not have any concept of compatibility for dictionaries. I assume they rely on what is in the file, and so those files need to be updated. Or this should be fixed client side, which sounds like the correct solution. Thunderbird on ATN has a "*" version nowadays so it should be possible to upload dictionaries with maxVersion "*" in install.rdf.

Comment 48

[Marco A.G.Pinto [:marcoagpinto]]

(In reply to Andrei Hajdukewycz [:sancus] from comment #47)
> I don't think we can, no, there's nothing to bump because ATN does not have
> any concept of compatibility for dictionaries. I assume they rely on what is
> in the file, and so those files need to be updated. Or this should be fixed
> client side, which sounds like the correct solution. Thunderbird on ATN has
> a "*" version nowadays so it should be possible to upload dictionaries with
> maxVersion "*" in install.rdf.

@sancus:
Could you confirm the "*" version? It was added then removed, was it added again?

Does this mean that in my next GB speller update I can add "*" as the maximum version?

Thanks,

Comment 49

[Matt]

@Sancus
Who do I need to see about getting appropriate access to fix the things on ATN.  Just upload fixed dictionaries where necessary.  I am no addon author but there does not appear to be much skill required to edit the install RDF file and just replace all the min version numbers with *

Comment 50

[Andrei Hajdukewycz [:sancus]]

(In reply to Marco A.G.Pinto [:marcoagpinto] from comment #48)
> @sancus:
> Could you confirm the "*" version? It was added then removed, was it added
> again?
> 
> Does this mean that in my next GB speller update I can add "*" as the
> maximum version?

https://addons.thunderbird.net/en-US/thunderbird/pages/appversions/ * is there, it should work.(In reply to Matt from comment #49)

> @Sancus
> Who do I need to see about getting appropriate access to fix the things on
> ATN.  Just upload fixed dictionaries where necessary.  I am no addon author
> but there does not appear to be much skill required to edit the install RDF
> file and just replace all the min version numbers with *

Well, if you're volunteering to maintain dictionary compatibility, we can probably get you added as a developer to them... admins can upload versions for any add-on, but we can't give out admin for something like this. I'm not sure if there is a lower level of access that would allow it.

Maybe Philipp knows?

Comment 51

[Jorg K (CEST = GMT+2)]

(In reply to Matt from comment #49)
> ... and just replace all the min version numbers with *
Let's do the *max* version number, please.

Comment 52

[Philipp Kewisch [:Fallen][📱📆]]

afaik the lowest level would be staff reviewer, but that also give access to a bunch of other things. I'm not sure this is still relevant now that we have wx dicts though.

Comment 53

[Andrei Hajdukewycz [:sancus]]

(In reply to Philipp Kewisch [:Fallen] [:📆] from comment #52)

afaik the lowest level would be staff reviewer, but that also give access to a bunch of other things. I'm not sure this is still relevant now that we have wx dicts though.

Yeah it's not relevant anymore.