No English spellcheck available in ja package since Firefox 62.0

RESOLVED FIXED in Firefox 66

Status

defect
RESOLVED FIXED
7 months ago
3 months ago

People

(Reporter: alice0775, Assigned: glandium)

Tracking

({in-triage, jp-critical, regression})

62 Branch
mozilla66
Unspecified
Windows 10
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox-esr60 unaffected, firefox62 wontfix, firefox63+ wontfix, firefox64 wontfix, firefox65 wontfix, firefox66 fixed)

Details

Attachments

(1 attachment)

(Reporter)

Description

7 months ago
[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?
Flags: needinfo?(kmaglione+bmo)
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.
Flags: needinfo?(kmaglione+bmo) → needinfo?(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?
Flags: needinfo?(l10n)
Flags: needinfo?(gandalf)
Flags: needinfo?(francesco.lodolo)
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.
Flags: needinfo?(francesco.lodolo)
Kris - seems like we're not registering bundled dictionaries?
Flags: needinfo?(l10n) → needinfo?(kmaglione+bmo)
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.
Flags: needinfo?(kmaglione+bmo)
Pike - do you know how we add `en` dictionaries to `ja-JP` builds? Is there something missing in packaging step?
Flags: needinfo?(l10n)
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).
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

7 months ago
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.
Component: Spelling checker → General
Flags: needinfo?(l10n)
Product: Core → Firefox Build System

Updated

6 months ago
Keywords: in-triage
Ted mentioned he would look at this in our team meeting's triage yesterday
Flags: needinfo?(ted)

Updated

6 months ago
See Also: → 1496765
(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.
Kim, is that still an issue we should track for 64? Thanks
Flags: needinfo?(kmoir)
See Also: → 1461569
: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.
Flags: needinfo?(kmoir) → needinfo?(francesco.lodolo)
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.
Flags: needinfo?(francesco.lodolo)

(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?

Flags: needinfo?(francesco.lodolo)

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.

(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.

Flags: needinfo?(francesco.lodolo)
(Assignee)

Comment 18

3 months ago

(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.

Flags: needinfo?(mh+mozilla)
(Assignee)

Comment 19

3 months ago

check that it works.

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

(Assignee)

Comment 21

3 months ago

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?

Flags: needinfo?(francesco.lodolo)

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

Flags: needinfo?(francesco.lodolo) → needinfo?(l10n)
(Assignee)

Comment 23

3 months ago

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.

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)

Flags: needinfo?(ted)
Flags: needinfo?(l10n)
(Assignee)

Updated

3 months ago
Assignee: nobody → mh+mozilla
(Assignee)

Comment 25

3 months ago

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.

Flags: needinfo?(l10n)
Flags: needinfo?(francesco.lodolo)

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"?

Flags: needinfo?(l10n)
Flags: needinfo?(francesco.lodolo)
(Assignee)

Comment 27

3 months ago

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.

Attachment #9037134 - Attachment description: Bug 1496995 - Fill list of dictionaries from both the app and the langpack during repack. → Bug 1496995 - Account for all dictionaries when updating built_in_addons.json during l10n repack.
(Assignee)

Comment 28

3 months ago

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

3 months ago
Pushed by mh@glandium.org:
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

3 months ago
bugherder
Status: NEW → RESOLVED
Last Resolved: 3 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla66

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.

Flags: in-testsuite+

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.

(Assignee)

Comment 33

3 months ago

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

(Reporter)

Updated

3 months ago
Blocks: 1523981
You need to log in before you can comment on or make changes to this bug.