Closed Bug 1410214 Opened 2 years ago Closed 2 years ago

Implement support for WebExtensions Dictionaries

Categories

(Toolkit :: Add-ons Manager, enhancement, P5)

enhancement

Tracking

()

VERIFIED FIXED
mozilla61
Tracking Status
firefox-esr60 64+ verified
firefox57 --- wontfix
firefox61 --- fixed

People

(Reporter: jorgev, Assigned: kmag)

References

Details

(Keywords: dev-doc-needed)

Attachments

(4 files)

In order to move away from legacy packaging and legacy manifests, we should follow the lead of bug 1395363 and also move dictionaries to WebExtensions packaging.

Dictionaries are much simpler. They just have the manifest and a `dictionaries` folder with two dictionary files (`en-US.aff` and `en-US.dic` would be their names for US English).

They don't have a chrome.manifest or other manifest flags, so we just need something in the new manifest that identifies them as dictionaries. It could just be a `dictionary` flag, or if we want to be consistent with language packs it could be a `dictionary_id` that corresponds to the locale code.

Most dictionaries on AMO can be found here, if examples are needed:
https://addons.mozilla.org/firefox/language-tools/

The corresponding issue for AMO support is here:
https://github.com/mozilla/addons-server/issues/6739
Thoughts:

Would we want to support multi-dictionary packages? 
Would we want to make sure that in the future we can put dictionaries in langpacks?
> Would we want to support multi-dictionary packages? 

We don't support those now, right? Assuming that, I think it's best to keep them separate. That way they can be handled independently on AMO and the Add-ons Manager. We moved away from multi-XPI installers because they were too rare and wasteful to maintain.

> Would we want to make sure that in the future we can put dictionaries in langpacks?

I think those should also be separate for the same reasons.
I'm not advocating for changing how we package, but I think we should design the manifest.json structure so that we can be more flexible in the future without having to alter the schema
Severity: normal → enhancement
Priority: -- → P5
From a quick glance, it looks like the naming of the files and having them in a directory is a constraint of mozISpellCheckingEngine.  I propose that we use all the regular shared manifest properties (name, version, author, etc) and add a new property "dictionary_dir" (the underscore convention is already used throughout manifest.json...) that specifies where (inside the xpi) the "langcode.eff" and "langcode.dic" files are.

(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #1)
> Would we want to support multi-dictionary packages? 

It looks like this might "just work" if you put files for two different languages into the dictionaries directory?

> Would we want to make sure that in the future we can put dictionaries in
> langpacks?

I think this format would naturally support that.  For now an xpi would be invalid if it includes both "langpack_id" and "dictionary_dir" properties, but that could be changed later.  I think the biggest challenge if we want to do that would be with the UX -- we have separate categories in about:addons for Dictionaries and Language Packs.  If I have something that has both and I disable it in one of those categories, that would also disable it in the other category which is not obvious or intuitive.  We could group them together in about:addons but that would be a chunk of front-end work.  Anyway, that's all for some time in the future, not right now...
That sounds good to me!

> It looks like this might "just work" if you put files for two different languages into the dictionaries directory?

In that case, would there be any manifest.json field with the list of language codes for which the dictionaries are bundled in the package?
(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #5)
> > It looks like this might "just work" if you put files for two different languages into the dictionaries directory?
> 
> In that case, would there be any manifest.json field with the list of
> language codes for which the dictionaries are bundled in the package?

That does seem like a useful thing to have, for organizing on AMO if nothing else, but I'm not sure what we would actually do with that information when installing a dictionary.  It looks like mozISpellCheckingEngine already derives the language code from the basename of the .aff and .dic files when scanning the provided directory [1].  If we put it into a manifest property too then its just something we have to keep in sync between the add-ons manager and mozISpellCheckingEngine :(

[1] http://searchfox.org/mozilla-central/rev/7e090b227f7a0ec44d4ded604823d48823158c51/extensions/spellcheck/hunspell/glue/mozHunspell.cpp#446-469
Duplicate of this bug: 1452330
I'd rather we list the languages and the dictionary files that belong to them explicitly in the manifest, even if only so that we don't have to do directory scans to load them. If that requires changing mozISpellCheckEngine, so be it.
Blocks: 857456
Assignee: nobody → kmaglione+bmo
Comment on attachment 8971095 [details]
Bug 1410214: Implement support for WebExtension-style dictionary add-ons.

https://reviewboard.mozilla.org/r/239888/#review245568

Looks great! Thanks Kris!

A few things I'd like to see in the followups:

 - unpacking adds to complexity. Would be nice to get spellchecking to be able to handle packaged extensions
 - langpack+dictionaries combo. Now that both langpacks and dictionaries are webextensions and can handle multiple locales, it would be nice to finalize it by ability to get both in one.
Attachment #8971095 - Flags: review?(gandalf) → review+
Comment on attachment 8971099 [details]
Bug 1410214: Part 2 - Add a stub Hunspell FileMgr that allows it to read URLs.

https://reviewboard.mozilla.org/r/239892/#review245600

::: extensions/spellcheck/hunspell/glue/mozHunspellFileMgr.h:1
(Diff revision 2)
> +#ifndef mozHunspellFileMgr_h

Why don't you add license block here?

::: extensions/spellcheck/hunspell/glue/mozHunspellFileMgr.h:11
(Diff revision 2)
> +#include "mozilla/Result.h"
> +#include "mozilla/ResultExtensions.h"
> +#include "nsIInputStream.h"
> +#include "nsReadLine.h"
> +
> +class FileMgr final

Hmm, I don't like this name and namespace, but we need to keep this for existing code of Hunspell, sigh.

::: extensions/spellcheck/hunspell/glue/mozHunspellFileMgr.h:14
(Diff revision 2)
> +#include "nsReadLine.h"
> +
> +class FileMgr final
> +{
> +public:
> +  FileMgr(const char* filename, const char* key = nullptr);

nit: use "a" prefix for arguments and please explain what the arguments mean with comment.

::: extensions/spellcheck/hunspell/glue/mozHunspellFileMgr.h:17
(Diff revision 2)
> +  bool getline(std::string&);
> +  int getlinenum() const { return mLineNum; }

Sigh, those method names are different from out coding styles, but they need to be so...

Anyway, could you add explanation what they do?

::: extensions/spellcheck/hunspell/glue/mozHunspellFileMgr.cpp:1
(Diff revision 2)
> +#include "mozHunspellFileMgr.h"

Why don't you add license block here?

::: extensions/spellcheck/hunspell/glue/mozHunspellFileMgr.cpp:13
(Diff revision 2)
> +
> +using namespace mozilla;
> +
> +FileMgr::FileMgr(const char* filename, const char* key)
> +{
> +  Unused << NS_WARN_IF(Open(nsDependentCString(filename)).isErr());

Don't use NS_WARN_IF() as this style. Please use NS_WARNING_ASSERTION() instead. E.g.,

DebugOnly<Result<Ok, nsresult>> ret = Open(...);
NS_WARNING_ASSERTION(!ret.IsErr(),
  "...");

::: extensions/spellcheck/hunspell/glue/mozHunspellFileMgr.cpp:17
(Diff revision 2)
> +FileMgr::Open(const nsACString& aPath)
> +{
> +  nsCOMPtr<nsIURI> uri;

Shouldn't we check if current context is enough safe for reading a local file?

::: extensions/spellcheck/hunspell/glue/mozHunspellFileMgr.cpp:41
(Diff revision 2)
> +FileMgr::ReadLine()
> +{
> +  if (!mStream) {

Shouldn't we need to do security check?

::: extensions/spellcheck/hunspell/glue/mozHunspellFileMgr.cpp:47
(Diff revision 2)
> +{
> +  if (!mStream) {
> +    return Err(NS_ERROR_NOT_INITIALIZED);
> +  }
> +
> +  nsCString line;

I'm not sure if this can be nsAutoCString. If not, how about to make ReadLine() take an out argument? Although, I don't know if usual length of each line is less than 64 characters.

::: extensions/spellcheck/hunspell/glue/mozHunspellFileMgr.cpp:61
(Diff revision 2)
> +  auto res = ReadLine();
> +  if (res.isErr()) {
> +    return false;
> +  }
> +
> +  nsCString line = res.unwrap();
> +  result.assign(line.BeginReading(), line.Length());

If we use nsAutoCString here and each line is enough short for nsAutoCString, we don't need to allocate memory space in the heap. That must make this faster.
Attachment #8971099 - Flags: review?(masayuki) → review-
Comment on attachment 8971100 [details]
Bug 1410214: Part 3 - Support packed WebExtension dictionaries.

https://reviewboard.mozilla.org/r/239894/#review245608

Assuming that this won't break dictionary files in unicode path.

::: extensions/spellcheck/hunspell/glue/mozHunspell.h:70
(Diff revision 2)
> +#include "nsIURI.h"
>  #include "nsIObserver.h"

For keeping A-Z order, nsIURI.h should be below nsIObserver.h.

::: extensions/spellcheck/hunspell/glue/mozHunspell.cpp:77
(Diff revision 2)
>  #include "mozInlineSpellChecker.h"
>  #include "mozilla/Services.h"
>  #include <stdlib.h>
>  #include "nsIPrefService.h"
>  #include "nsIPrefBranch.h"
> +#include "nsNetUtil.h"

Hmm, but this file's includes are messy...

::: extensions/spellcheck/hunspell/glue/mozHunspell.cpp:168
(Diff revision 2)
> -#ifdef XP_WIN
> +  nsresult rv = affFile->GetSpec(affFileName);
> -  nsAutoString affFileNameU;
> -  nsresult rv = affFile->GetPath(affFileNameU);
>    NS_ENSURE_SUCCESS(rv, rv);

Hmm, I hope that nsIURI supports unicode file path completely.
Attachment #8971100 - Flags: review?(masayuki) → review+
Comment on attachment 8971100 [details]
Bug 1410214: Part 3 - Support packed WebExtension dictionaries.

Kimura-san, if you think that using nsIURI is not enough for handling dictionaries in unicode path, mark this as r-.
Attachment #8971100 - Flags: review?(VYV03354)
Comment on attachment 8971099 [details]
Bug 1410214: Part 2 - Add a stub Hunspell FileMgr that allows it to read URLs.

https://reviewboard.mozilla.org/r/239892/#review245600

> Shouldn't we check if current context is enough safe for reading a local file?

I can't think of any checks that would be necessary.

> Shouldn't we need to do security check?

No. Open2() does security checks, but these are system principal loads, anyway.
(In reply to Masayuki Nakano [:masayuki] (JST, +0900) from comment #16)
> Assuming that this won't break dictionary files in unicode path.

It won't. If there are dictionary files with files we can't read using file: or jar: URIs, then we won't be able to load most of the profile or omni jar, anyway. But I really want to get rid of loading dictionaries from plain files altogether. They should really all be loaded from omni.ja or extension XPIs.
Comment on attachment 8971099 [details]
Bug 1410214: Part 2 - Add a stub Hunspell FileMgr that allows it to read URLs.

https://reviewboard.mozilla.org/r/239892/#review245620

Okay, thanks!
Attachment #8971099 - Flags: review?(masayuki) → review+
Comment on attachment 8971099 [details]
Bug 1410214: Part 2 - Add a stub Hunspell FileMgr that allows it to read URLs.

https://reviewboard.mozilla.org/r/239892/#review245700

::: extensions/spellcheck/hunspell/glue/mozHunspellFileMgr.cpp:31
(Diff revision 3)
> +  nsCOMPtr<nsIURI> uri;
> +
> +  nsresult rv = NS_NewURI(getter_AddRefs(uri), aPath);
> +  if (NS_FAILED(rv)) {
> +    nsCOMPtr<nsIFile> file;
> +    MOZ_TRY(NS_NewNativeLocalFile(aPath, false, getter_AddRefs(file)));

How can this work with Unicode paths on Windows?
Attachment #8971099 - Flags: review-
(In reply to Kris Maglione [:kmag] (long backlog; ping on IRC if you're blocked) from comment #19)
> (In reply to Masayuki Nakano [:masayuki] (JST, +0900) from comment #16)
> > Assuming that this won't break dictionary files in unicode path.
> 
> It won't. If there are dictionary files with files we can't read using file:
> or jar: URIs, then we won't be able to load most of the profile or omni jar,
> anyway. But I really want to get rid of loading dictionaries from plain
> files altogether. They should really all be loaded from omni.ja or extension
> XPIs.

Most other loads from profile or omni jar deal with Unicode paths correctly. Even if some of them do not, we should fix them instead of introducing yet another breakage.
See Also: → 1420827
Please add an automated test to make sure that Unicode paths will not break dictionaries.
Comment on attachment 8971095 [details]
Bug 1410214: Implement support for WebExtension-style dictionary add-ons.

https://reviewboard.mozilla.org/r/239888/#review245840

This is a nit but the only thing I don't like about this is the `.dic` path in the manifest from which `.aff` is quietly derived.  How about just including the path with no extension and derive both files from that?

::: extensions/spellcheck/idl/mozISpellCheckingEngine.idl:96
(Diff revision 2)
> +   */
> +  void addDictionary(in AString lang, in nsIFile dir);
> +
> +  /**
> +   * Remove a dictionary with the given language code and path. If the path does
> +   * not match that of the current entry with the given languate code, it is not

typo

::: toolkit/components/extensions/Extension.jsm:66
(Diff revision 2)
>    this, "processScript",
>    () => Cc["@mozilla.org/webextensions/extension-process-script;1"]
>            .getService().wrappedJSObject);
>  
>  XPCOMUtils.defineLazyGetter(
> +  this, "OSPath", () => {

Please add a comment explaining that this is about manipulating paths that appear in the manifest which always use / as a separator, so directly importing the Unix backend is deliberate.

::: toolkit/components/extensions/Extension.jsm:732
(Diff revision 2)
>  
>  
>        this.startupData = {chromeEntries, langpackId, l10nRegistrySources, languages};
> +    } else if (this.type == "dictionary") {
> +      let dictionaries = {};
> +      for (let [lang, path] of Object.entries(manifest.dictionaries)) {

I don't know that much about locale codes, is the regexp in the manifest sufficient to validate them or is there additional validation that could/should happen here?

::: toolkit/components/extensions/Extension.jsm:1889
(Diff revision 2)
> +      // Make sure that any dictionary we provided is no longer used.
> +      if (spellCheck.dictionary in this.dictionaries) {
> +        spellCheck.dictionary = spellCheck.dictionary;
> +      }

I don't understand this
Comment on attachment 8971100 [details]
Bug 1410214: Part 3 - Support packed WebExtension dictionaries.

https://reviewboard.mozilla.org/r/239894/#review245854
Attachment #8971100 - Flags: review?(aswan) → review+
Comment on attachment 8971099 [details]
Bug 1410214: Part 2 - Add a stub Hunspell FileMgr that allows it to read URLs.

https://reviewboard.mozilla.org/r/239892/#review245700

> How can this work with Unicode paths on Windows?

I don't know if it works with Unicode paths on Windows, but it doesn't matter, because we don't actually use it. After part 3, we always load dictionaries using URLs.
(In reply to Masatoshi Kimura [:emk] from comment #25)
> (In reply to Kris Maglione [:kmag] (long backlog; ping on IRC if you're
> blocked) from comment #19)
> > (In reply to Masayuki Nakano [:masayuki] (JST, +0900) from comment #16)
> > > Assuming that this won't break dictionary files in unicode path.
> > 
> > It won't. If there are dictionary files with files we can't read using file:
> > or jar: URIs, then we won't be able to load most of the profile or omni jar,
> > anyway. But I really want to get rid of loading dictionaries from plain
> > files altogether. They should really all be loaded from omni.ja or extension
> > XPIs.
> 
> Most other loads from profile or omni jar deal with Unicode paths correctly.
> Even if some of them do not, we should fix them instead of introducing yet
> another breakage.

My point here is that most of what we load from the profile or omni jar use file: or jar: URLs the same way we do here. If they work there, they work here. If they don't work there, we have bigger problems.
Comment on attachment 8971095 [details]
Bug 1410214: Implement support for WebExtension-style dictionary add-ons.

https://reviewboard.mozilla.org/r/239888/#review245840

> I don't know that much about locale codes, is the regexp in the manifest sufficient to validate them or is there additional validation that could/should happen here?

The schema does the same validation for these properties as it does for langpack add-ons, so it's probably good enough. The dictionary service doesn't actually care much what the dictionaries are called, and in old-style dictionary add-ons, they could be named... pretty much anything.

> I don't understand this

Setting the dictionary property causes hunspell to reload the dictionary with that name. If the current dictionary was provided by the extension, we need to force it to reload at this point so that we either fall back to a builtin dictionary or to no dictionary at all.
Comment on attachment 8971095 [details]
Bug 1410214: Implement support for WebExtension-style dictionary add-ons.

https://reviewboard.mozilla.org/r/239888/#review245840

> Setting the dictionary property causes hunspell to reload the dictionary with that name. If the current dictionary was provided by the extension, we need to force it to reload at this point so that we either fall back to a builtin dictionary or to no dictionary at all.

I see, can you add a comment explaining that the setter has that side effect then?
Comment on attachment 8971095 [details]
Bug 1410214: Implement support for WebExtension-style dictionary add-ons.

https://reviewboard.mozilla.org/r/239888/#review245948
Attachment #8971095 - Flags: review?(aswan) → review+
Comment on attachment 8971099 [details]
Bug 1410214: Part 2 - Add a stub Hunspell FileMgr that allows it to read URLs.

https://reviewboard.mozilla.org/r/239892/#review245958

::: extensions/spellcheck/hunspell/glue/mozHunspellFileMgr.cpp:31
(Diff revision 3)
> +  nsCOMPtr<nsIURI> uri;
> +
> +  nsresult rv = NS_NewURI(getter_AddRefs(uri), aPath);
> +  if (NS_FAILED(rv)) {
> +    nsCOMPtr<nsIFile> file;
> +    MOZ_TRY(NS_NewNativeLocalFile(aPath, false, getter_AddRefs(file)));

Why is this necessary if it is never used? Please remove it because it blocks bug 1420827.
Attachment #8971099 - Flags: review-
(In reply to Masatoshi Kimura [:emk] from comment #34)
> Why is this necessary if it is never used? Please remove it because it
> blocks bug 1420827.

It was necessary as a stepping stone between parts 2 and 3. After that, I left it for compatibility with the stock Hunspell API, which deals with native paths. But that's not really necessary, so I can remove it in part 3 if you prefer.
(In reply to Kris Maglione [:kmag] (long backlog; ping on IRC if you're blocked) from comment #35)
> It was necessary as a stepping stone between parts 2 and 3. After that, I
> left it for compatibility with the stock Hunspell API, which deals with
> native paths. But that's not really necessary, so I can remove it in part 3
> if you prefer.

OK, please remove it in part 3.
Blocks: 1457321
https://hg.mozilla.org/integration/mozilla-inbound/rev/2f1e8f0e6b5316f9119a7111100af50c4b7025af
Bug 1410214: Part 1 - Implement support for WebExtension-style dictionary add-ons. r=aswan,gandalf

https://hg.mozilla.org/integration/mozilla-inbound/rev/3dd497b3e3f15b06e3e786d00addcd5ef85b97cb
Bug 1410214: Part 2 - Add a stub Hunspell FileMgr that allows it to read URLs. r=masayuki

https://hg.mozilla.org/integration/mozilla-inbound/rev/30448613bb72e41e386baf2e5e88cabd5ff720a8
Bug 1410214: Part 3 - Support packed WebExtension dictionaries. r=aswan,masayuki
Is manual testing required on this bug? If yes, please provide some STR and the proper extension(if required) or set the “qe-verify -“ flag.

Thanks!
Flags: needinfo?(kmaglione+bmo)
Flags: needinfo?(kmaglione+bmo) → qe-verify-
[Tracking Requested - why for this release]: Including this in 60 allows us to proceed with migrating dictionaries on addons.mozilla.org without stranding ESR users until 68; it also allows us to remove support for install.rdf from Firefox; it also simplifies things for multilingual Firefox going forward.
Comment on attachment 8971095 [details]
Bug 1410214: Implement support for WebExtension-style dictionary add-ons.

[ESR Uplift Approval Request]

If this is not a sec:{high,crit} bug, please state case for ESR consideration: AMO currently has a bunch of dictionaries add-ons that use "old-style" packaging (ie, they use install.rdf).  The AMO folks have a script ready to migrate all these dictionaries to new packaging (ie, manifest.json) but they haven't yet run it since it would prevent ESR users from installing new dictionaries from AMO.  However, we hope to remove install.rdf support entirely (bug 857456) during the 65 cycle which requires migrating dictionaries first.  If we get this patch into ESR 60.4, that will minimize the impact on ESR users.

User impact if declined: As described above, ESR users viewing a dictionary on AMO will not be able to install the dictionary.  (users of existing dictionaries should not be affected)

Fix Landed on Version: 61

Risk to taking this patch: Medium

Why is the change risky/not risky? (and alternatives if risky): The patch touches a fair chunk of code but it has been well-tested both in automation and in release/beta/nightly.

String or UUID changes made by this patch: none
Attachment #8971095 - Flags: approval-mozilla-esr60?
We'd also need to take bug 1460600, right? Which I can't see going over well with downstream distros in the middle of an ESR cycle.
Flags: needinfo?(aswan)
I don't think so, the uplift request here is just for part 1 and over in bug 1460600, Kris says that was needed due to the second patch on this bug (which we don't need to uplift).  But forwarding the ni to Kris to check my work there...
Flags: needinfo?(aswan) → needinfo?(kmaglione+bmo)
We don't need bug 1460600. It's only relevant with the second patch, but even with it, it's not really necessary. It would just be up to any third parties who build with that flag to stop using it.
Flags: needinfo?(kmaglione+bmo)
I am a bit concerned about uplifting this in the middle of our larger ESR cycle. 

If we did do manual QA on this for ESR 60.4.0, what would that testing look like? Any reason not to try it? What might break here for ESR users?   What will the migration or impact look like to users or ESR admins?
Flags: needinfo?(kmaglione+bmo)
Flags: needinfo?(ddurst)
(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #45)
> I am a bit concerned about uplifting this in the middle of our larger ESR
> cycle. 
> 
> If we did do manual QA on this for ESR 60.4.0, what would that testing look
> like?

I'm not sure. There's not much to test, really, except that the WebExtension dictionaries work.

> What might break here for ESR users?

Nothing. This patch doesn't affect anything except the ability to use WebExtension dictionaries, which are otherwise not supported on ESR. Without it, though, ESR users will be unable to install dictionaries from AMO, since AMO is removing support for old-style dictionaries.

> What will the migration or impact look like to users or ESR admins?

Neither users nor admins will have to do any migration at all until the next ESR release when support for legacy dictionary types is dropped.
Flags: needinfo?(kmaglione+bmo)
This looks like an okay exception to our general rule about uplifting only stability and security fixes. It will allow for compatibility with these new WebExtension dictionaries.
It still seems worth testing a couple of Webextension style dictionaries. It doesn't have to be extensive but as Kris says, they should work. That can be after uplift, to confirm that they do work on ESR with these patches applied, and we aren't missing anything that may not have made it into the esr branch since 60. 

David, or Jorge, can you suggest a popular extensions (or two)  Thanks!
Flags: qe-verify-
Flags: qe-verify+
Flags: needinfo?(jorge)
Comment on attachment 8971095 [details]
Bug 1410214: Implement support for WebExtension-style dictionary add-ons.

Taking this patch for ESR60 to ensure webextension compatibility.
Attachment #8971095 - Flags: approval-mozilla-esr60? → approval-mozilla-esr60+
Can you also suggest wording for a release note for ESR? Thanks.
(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #50)
> Can you also suggest wording for a release note for ESR? Thanks.

"Added support for WebExtension-style spelling dictionary format, to allow ongoing compatibility with dictionaries hosted on addons.mozilla.org."
This is a file that was auto-converted to WebExtensions on the AMO dev server and I signed on the production server. I did a quick test on Nightly and it works.
Flags: needinfo?(jorge)
Awesome, thanks for the converted example Jorge!  Krupa would you mind testing with that on ESR once the patch lands on the ESR60 branch? And, maybe testing with an older dictionary on the previous ESR to make sure that an update doesn't do anything to a previously installed dictionary. Hope that makes sense.
Flags: needinfo?(kraj)
This is going to need a rebased patch for ESR60.
Flags: needinfo?(kmaglione+bmo)
Vlad needinfo'ed here will help with the testing of this change.
Flags: needinfo?(kraj) → needinfo?(vlad.jiman)
Thank you for the test extension, using it along with others from previous test scenarios (arabic, korean, etc)  I've successfully ran the dictionaries suite on FF 60.3.1 ESR, however I have a question: After installing the test extension via temporary addon ( from about:debugging), it does not show up in the context menu when attempting to use the Spell Checking - under the Languages menu, is this an expected behavior on ESR? while installing the extension from AMO or from file this issue is not reproducible. 
No other issues have been found during testing of the dictionaries on ESR.
Flags: needinfo?(vlad.jiman) → needinfo?(kmaglione+bmo)
Status: RESOLVED → VERIFIED
Flags: qe-verify+

This page documents how to create dictionaries:
https://developer.mozilla.org/en-US/docs/Mozilla/Creating_a_spell_check_dictionary_add-on

I'm not sure if it should be updated in place or migrated somewhere else as part of the MDN emphasis on web standards instead of Firefox-specific things, but we should have documentation for dictionary authors somewhere. In any case, the page above is out of date after the changes from this bug.

Keywords: dev-doc-needed
Flags: needinfo?(kmaglione+bmo)
Flags: needinfo?(ddurst)
You need to log in before you can comment on or make changes to this bug.