1496995 - No English spellcheck available in ja package since Firefox 62.0

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[Alice0775 White]

[Tracking Requested - why for this release]: English spell checker feature is missing in ja localized build.

The problem does not happen on Firefox61.
The problem start since Firefox62.
about:preferences: "自動スペルチェック機能を使用する"(Check your spelling as you type) has checked by default on both Firefox61 and 62.

Steps To Reproduce:
1. Install Firefox62.0 release ja build
2. Start Firefox with new clean profile
3. Open data:text/html,<textarea spellcheck="true">
4. Right click on the text area
   --- Bug, no "スペルチェックを行う"(Check Spelling) and  "言語"(Languages) menu item
5. type "colooor" and enter
   --- Bug, no red under line

Actual Results:
No "スペルチェックを行う"(Check Spelling) and  "言語"(Languages) menu item appears.
No red under line.


Expected Results:
"スペルチェックを行う"(Check Spelling) and  "言語"(Languages) menu item should appear.
And "スペルチェックを行う"(Check Spelling) is checked.

Red under line should appear.


Regression window(l10n ja build):
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=4e9446f9e8f0a75c7ffe063f1dfb311cc90d56cf&tochange=6b9076ac236cb0f9f301bc601eac03f9ec4205df

Regressed by : Bug 1457321

@:kmag,
Your bunch of patch causes the regression, can you please look into this?

Comment 1

[Kris Maglione [:kmag]]

As far as I understand, this is expected. We don't bundle the en-US dictionary in localized builds because that would cause words in the target language to be marked as misspelled.

Comment 2

[Zibi Braniecki [:zbraniecki][:gandalf]]

Hmm, I'm not very familiar with ja-JP build and it's often special. Pike, Flod - do you know if we are supposed to bundle `en` dictionary in ja-JP?

Comment 3

[Francesco Lodolo [:flod]]

We don't bundle the en-US dictionary for any language at build time, as comment 1 explains. 

But Japanese is shipping the en-US dictionary on its own, and it was updated recently (after many years) in bug 1461569. I've double checked with what we ship in en-US (mozilla-central), and files look correct as far as I can tell.

zh-TW is also shipping en-US dictionary, updated in bug 1461571, and it has the same issue as Japanese.

For what it's worth, I see the dictionary files in the omni.ja in 62.0.3, but can confirm the behavior. Installing a dictionary from AMO makes the spelling option menu appear, so those files are not read for some reason, and I'm not exactly sure how to help in that case.

I haven't seen other bugs reported for locales that bundle their own dictionaries, and I've double check with a Polish build that a bundled dictionary works as expected.

Comment 4

[Zibi Braniecki [:zbraniecki][:gandalf]]

Kris - seems like we're not registering bundled dictionaries?

Comment 5

[Kris Maglione [:kmag]]

We only register dictionaries that are listed in built_in_addons.json. The build and repackager scripts take care of adding the correct entries. If the ja build is doing something weird, then its added dictionaries wouldn't be listed.

Comment 6

[Zibi Braniecki [:zbraniecki][:gandalf]]

Pike - do you know how we add `en` dictionaries to `ja-JP` builds? Is there something missing in packaging step?

Comment 7

[Francesco Lodolo [:flod]]

As I said, Polish works (tested yesterday with a clean profile), so it's not just l10n bundled dictionaries
https://hg.mozilla.org/l10n-central/pl/file/tip/extensions/spellcheck/hunspell

I tested ja on Mac, but Alice is on Windows and I also tested zh-TW, so it's not ja-jP-mac.

I'm skimming through this file
https://hg.mozilla.org/mozreview/gecko/file/default/python/mozbuild/mozpack/packager/l10n.py

And I wonder if we don't register it because en-US ≠ ja. If that's the case, it's still bug 1457321 (+ted).

Comment 8

[Pascal Chevrel:pascalc (PTO until April 26)]

Tracking for 63 but please note that we are in the last week of betas, I would take a patch for beta 14 if it lands on mozilla-central and gets verified on nightly before the go to build on October 11.

Comment 9

[Axel Hecht [:Pike]]

I can confirm flod's diagnosis. We're packaging the en-US dictionary, if available in a locale. It's in the omni.ja. The built_in_addons.json in browser/omni.ja has a `"dictionaries": {}`, though.

Seems the code in https://dxr.mozilla.org/mozilla-central/source/python/mozbuild/mozpack/packager/l10n.py#224-226 doesn't find that, though I'm unsure why.

The fix is likely in that file, so moving over to Firefox Build.

Comment 10

[Kim Moir [:kmoir] ET]

Ted mentioned he would look at this in our team meeting's triage yesterday

Comment 11

[Pascal Chevrel:pascalc (PTO until April 26)]

(In reply to Pascal Chevrel:pascalc from comment #8)
> Tracking for 63 but please note that we are in the last week of betas, I
> would take a patch for beta 14 if it lands on mozilla-central and gets
> verified on nightly before the go to build on October 11.

Beta 14 is shipped, wontfix for 63.

Comment 12

[Pascal Chevrel:pascalc (PTO until April 26)]

Kim, is that still an issue we should track for 64? Thanks

Comment 13

[Kim Moir [:kmoir] ET]

:flod is this something we actually need to fix?  I talked to Ted and there isn't a clear path forward for addressing this issue.

Comment 14

[Francesco Lodolo [:flod]]

I think we should at least investigate to the point of figuring out what broke, since this is effectively a regression. If the fix is going to be too costly, we can regroup and decide.

I tried before to drop the en-US spellchecker from a few locales (Japanese, zh-*), and I was met with strong opposition.

Comment 15

[Andrew Overholt [:overholt]]

(In reply to Francesco Lodolo [:flod] from comment #14)

I tried before to drop the en-US spellchecker from a few locales (Japanese,
zh-*), and I was met with strong opposition.

Jumping in here without context, did we actually do this in bug 1461569, though?

Comment 16

[Jared Wein [:jaws] (please needinfo? me)]

Mike, could you take a look at this? It looks to be a problem with our packaging and we haven't had a response from Ted yet.

Comment 17

[Francesco Lodolo [:flod]]

(In reply to Francesco Lodolo [:flod] from comment #14)

I tried before to drop the en-US spellchecker from a few locales (Japanese,
zh-*), and I was met with strong opposition.

Jumping in here without context, did we actually do this in bug 1461569, though?

That bug clearly says that we keep the en-US dictionary, and that's still in tree.

Comment 18

[Mike Hommey [:glandium]]

(In reply to Axel Hecht [:Pike] from comment #9)

I can confirm flod's diagnosis. We're packaging the en-US dictionary, if
available in a locale. It's in the omni.ja. The built_in_addons.json in
browser/omni.ja has a "dictionaries": {}, though.

Seems the code in
https://dxr.mozilla.org/mozilla-central/source/python/mozbuild/mozpack/
packager/l10n.py#224-226 doesn't find that, though I'm unsure why.

Presumably because the en-us dictionary does not come from the l10n addon that is being merged in, but from the source that is being repacked.

So it would seem to me that block should be moved out of the loop, and the formatter/copier itself should be iterated to fill the dictionaries dict.

Presumably, the following would work:

dictionaries = {
    os.path.splitext(os.path.basename(p))[0]: p
    for p, _ in copier.find('dictionaries/*.dic')
}

I can attach a patch, but I'll need someone to test it somehow and check that it works.

Comment 19

[Mike Hommey [:glandium]]

check that it works.

which it won't, because the copier is already going to be formatted, which means dictionaries/*.dic won't match.

Comment 20

[Mike Hommey [:glandium]]

Attachment: Bug 1496995 - Account for all dictionaries when updating built_in_addons.json during l10n repack.

Comment 21

[Mike Hommey [:glandium]]

Comment on attachment 9037134 [details]
Bug 1496995 - Account for all dictionaries when updating built_in_addons.json during l10n repack.

Flod, can you somehow test this?

Comment 22

[Francesco Lodolo [:flod]]

I'm not sure we can test build stuff easily. Redirecting to Pike, who at least would know for sure.

Comment 23

[Mike Hommey [:glandium]]

I'm sure it's possible to do try builds with l10n, but I don't know exactly which ones should be run to test this patch.

Comment 24

[Axel Hecht [:Pike]]

ja, ja-JP-mac, zh-TW are the locales shipping en-US, https://dxr.mozilla.org/l10n-central/search?q=ext%3Adic&redirect=false

lv, mk, vi ship dictionaries that are not their locale code, probably also good to send to try.

My pet way to do so is to ./mach try fuzzy --full, and to search for 'repackage-l10n-lv etc.

(dropping ni on ted on the way)

Comment 25

[Mike Hommey [:glandium]]

Can you check those builds:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=7512e3ac242a97c088a8e26681575b5100640d7a

Feel free to trigger more repacks if necessary.

I don't know if some of those should end up with no english dictionary.

Comment 26

[Axel Hecht [:Pike]]

unzip -l firefox-vi/omni.ja|grep dict etc confirms that this does the trick:

it: None as it should be

ja:
3074 01-01-2010 00:00 dictionaries/en-US.aff
581571 01-01-2010 00:00 dictionaries/en-US.dic

vi:
1281 01-01-2010 00:00 dictionaries/vi-x-KieuCu.[Chuan].aff
39846 01-01-2010 00:00 dictionaries/vi-x-KieuCu.[Chuan].dic
1281 01-01-2010 00:00 dictionaries/vi-x-KieuMoi.[KhongChuan].aff
39852 01-01-2010 00:00 dictionaries/vi-x-KieuMoi.[KhongChuan].dic

The length of the en-US one in ja also matches that in the l10n repo, not in en-US, as expected.

Note, I've done some printf-debugging, 'cause I didn't understand the patch.

The current code loop does iterate over dictionaries/en-US.dic and .aff, but it seems that the formatter already has that entry (from the en-US code?), and thus doesn't add the dictionary to the dic? Maybe it's just good enough to split the conditions into "add if it's not there" and independently of that, "if it's a dic, keep a note for later to add it to builtins"?

Comment 27

[Mike Hommey [:glandium]]

See https://phabricator.services.mozilla.com/D16785#inline-91359

The patch actually changed nothing wrt the top-level omnijar. Dictionaries were already where they are. The problem was the json in browser/omni.ja. And the patch makes the italian refer to a dictionary that is not there.

Comment 28

[Mike Hommey [:glandium]]

You can check the builds in:
https://treeherder.mozilla.org/#/jobs?repo=try&selectedJob=222409334&revision=ab4a97e52e21cde626b05aaebb1d4380e7cc5b23

They should still have the right dictionaries and have built_in_addons.json updated with the right list of dictionaries.

Comment 29

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f51c5057cc00
Account for all dictionaries when updating built_in_addons.json during l10n repack. r=nalexander

Comment 30

[Oana Pop-Rus]

https://hg.mozilla.org/mozilla-central/rev/f51c5057cc00

Comment 31

[Ryan VanderMeulen [:RyanVM]]

Given where we are in the cycle, seems better to me that this fix ride with 66 rather than doing a last-minute uplift to 65 during RC week.

Comment 32

[Kris Maglione [:kmag]]

This is entirely a packager change, which means that we can be 100% sure of its exact effects with very little effort. I think it's worth taking for 65 given its impact.

Comment 33

[Mike Hommey [:glandium]]

I'd tend to agree with kmag. OTOH, this has been broken since 62...