1162823 - en-us spelling checker should not mark non Latin words as misspelled.

Status: RESOLVED FIXED

(Core :: Spelling checker, defect)

Description

[Alice0775 White]

[Tracking Requested - why for this release]: regression

Reported on http://forums.mozillazine.org/viewtopic.php?p=14147555#p14147555

Steps To Reproduce:
1. installed en-US nightly https://ftp.mozilla.org/pub/mozilla.org ... a-central/
2. type any non en-us languages into textarea or copy みなさん、こんにちは and paste into textarea.

Actual Results::
spelling checker mark as misspelled words

Expected Results:
it should not mark all non Latin words (korean, japanese, arab, thai, ...) as misspelled.

Regression window:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=74e3f0993a06&tochange=d1b2c31a46d0

Regressed by: Bug 1137544

Comment 1

[Kevin Atkinson]

The latest Hunspell en-us dictionary is now in UTF-8.

If this causes a problem you can change it back to ISO-8859-1 but you would also need to remove the ICONV lines.

If this is required to be a persistent change than fixing the upgrade script to patch the affix file would be the best bet, an alternative would be for my scripts to just not replace the affix file but that has the potential to create problems down the road.

Ehsan: Do you have any time to figure this out?

Comment 2

[Kevin Atkinson]

Attachment: en-US-iso88591.diff

Here is a patch to revert the en-US dictionary to use iso8859-1 instead of UTF-8.

You can use it to see if it solves the current problem.

Comment 3

[Ekanan Ketunuti]

Comment on attachment 8603119 [details] [diff] [review]
en-US-iso88591.diff

All of files in dictionary-sources are not going to build. So this patch actually doesn't fix the problem. :)
https://mxr.mozilla.org/mozilla-central/source/extensions/spellcheck/locales/Makefile.in#5

Ehsan, also we should re-enable spell check on these changes too, yes?
https://hg.mozilla.org/mozilla-central/rev/f0935eccab87

Comment 4

[(no longer active)]

Oops, I'll fix this.  I can't believe I mistakenly broke a test that caught this, but we should have better tests for this...

Comment 5

[(no longer active)]

Attachment: Do not treat non-Latin words as misspelled

Comment 6

[(no longer active)]

Ekanan, I would appreciate if you could please help review this.  :-)

In case you're not familiar with the reftest framework, it is documented here: <https://developer.mozilla.org/en-US/docs/Creating_reftest-based_unit_tests>.  Essentially it loads two pages and compares the rendering to make sure that they are equal.  The reftests that I have added have one text box with non-Latin chars, focused, to ensure that we do spell checking on it, and another one with spellcheck="false".  If the spell checker marks the non-Latin chars as misspelled, the rendering won't be equal because of the squiggly red underlines...  Hope it all makes sense!

Comment 7

[(no longer active)]

(In reply to Ekanan Ketunuti from comment #3)
> Ehsan, also we should re-enable spell check on these changes too, yes?
> https://hg.mozilla.org/mozilla-central/rev/f0935eccab87

I did that, but note that one of the test fixes in that commit is unrelated to this issue, so I left that one in.

Comment 8

[Kevin Atkinson]

Ehsan:  Just a heads up I noticed you took my advice and changed the affix file to ISO-8859-1.  That's fine for now but it could create complications down the road if any non-ascii words make it into the wordlist.  On my side that is not going to happen as I convert words like café to cafe but it might happen on your side.

Comment 9

[Ekanan Ketunuti]

Comment on attachment 8603344 [details] [diff] [review]
Do not treat non-Latin words as misspelled

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

LGTM

Comment 10

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a9a4f21d0daf

Comment 11

[Phil Ringnalda (:philor)]

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

Comment 12

[(no longer active)]

(In reply to Kevin Atkinson from comment #8)
> Ehsan:  Just a heads up I noticed you took my advice and changed the affix
> file to ISO-8859-1.  That's fine for now but it could create complications
> down the road if any non-ascii words make it into the wordlist.  On my side
> that is not going to happen as I convert words like café to cafe but it
> might happen on your side.

Yeah, that's true.  I'll try to figure out what causes this bug when the en-US is marked as UTF-8, and will try to fix it.

Comment 13

[Liz Henry (:lizzard) (relman/hg->git project)]

Ehsan do you want to file a new bug for that issue, or reopen this one? 

It seems to me the regression is fixed and comment 8 is more about a potential problem. As this stands I don't think we need to track it for 40 or 41.

Comment 14

[(no longer active)]

(In reply to Liz Henry (:lizzard) from comment #13)
> Ehsan do you want to file a new bug for that issue, or reopen this one? 

Filed bug 1162823.

> It seems to me the regression is fixed and comment 8 is more about a
> potential problem.

Indeed.

> As this stands I don't think we need to track it for 40
> or 41.

Hmm, I don't agree with this part.  We should still track this for 40, to get it on the radar if the patch ends up being backed out for some reason down the line, because this would be a pretty bad regression to ship accidentally.

Comment 15

[Liz Henry (:lizzard) (relman/hg->git project)]

OK. Thanks Ehsan. That's a good point!