816281 - "Avant-Garde" Not Spellchecked Properly

Status: RESOLVED FIXED

(Core :: Spelling checker, defect)

Description

[draco18s]

User Agent: Mozilla/5.0 (Windows NT 6.0; WOW64; rv:16.0) Gecko/20100101 Firefox/16.0
Build ID: 20121024073032

Steps to reproduce:

Type Avant-Gaurd
R-click to correct the spelling


Actual results:

Avant-Gaurd was corrected to Avaunt-Gaurd.  Which Firefox indicates is also spelled incorrectly.  R-clicking again, it suggests Avaunt-Gaurd.  Which is also incorrect, and is underlined.  Which corrects to Avaunt-Guard.  Which is also not the correct spelling.


Expected results:

It should correctly spellcheck to "Avant-Garde"

Comment 1

[Mihaela Velimiroviciu (:mihaelav)]

Mozilla/5.0 (X11; Linux i686; rv:20.0) Gecko/20121204 Firefox/20.0

Confirming spell-checking issue.

Comment 2

[(no longer active)]

See <https://developer.mozilla.org/en-US/docs/Adding_a_new_word_to_the_en-US_dictionary> for how to fix this bug.

Comment 3

[draco18s]

(In reply to Ehsan Akhgari [:ehsan] from comment #2)
> See
> <https://developer.mozilla.org/en-US/docs/Adding_a_new_word_to_the_en-
> US_dictionary> for how to fix this bug.

I know how to add new words to the dictionary, that wasn't actually the problem.  The problem is that it suggested a spelling and then said that that new spelling was ALSO WRONG.  The fact that the suggestions were incorrectly spelled was a side note.

Comment 4

[(no longer active)]

Well that's because we don't have avant-garde in our en-US dictionary, which makes it impossible for the spell checker to show the correct suggestion.

Comment 5

[draco18s]

(In reply to Ehsan Akhgari [:ehsan] from comment #4)
> Well that's because we don't have avant-garde in our en-US dictionary, which
> makes it impossible for the spell checker to show the correct suggestion.

Did you actually read the initial report?

Avant-Gaurd -> Avaunt-Gaurd -> Avaunt-Gaurd -> Avaunt-Guard

Each of the first three are underlined in red and using the spellcheck feature it suggests the next one over.  If "Avaunt-Gaurd" is a suggested spelling, meaning that it's in the dictionary, why is the spellchecker saying it's wrong?

Also, if "avant-garde" isn't in the dictionary, because it's not en-US, then why are "Avaunt-Gaurd", "Avaunt-Gaurd", and "Avaunt-Guard" in the dictionary (and SPELLED WRONG)?

Comment 6

[(no longer active)]

(In reply to comment #5)
> (In reply to Ehsan Akhgari [:ehsan] from comment #4)
> > Well that's because we don't have avant-garde in our en-US dictionary, which
> > makes it impossible for the spell checker to show the correct suggestion.
> 
> Did you actually read the initial report?
> 
> Avant-Gaurd -> Avaunt-Gaurd -> Avaunt-Gaurd -> Avaunt-Guard
> 
> Each of the first three are underlined in red and using the spellcheck feature
> it suggests the next one over.  If "Avaunt-Gaurd" is a suggested spelling,
> meaning that it's in the dictionary, why is the spellchecker saying it's wrong?
> 
> Also, if "avant-garde" isn't in the dictionary, because it's not en-US, then
> why are "Avaunt-Gaurd", "Avaunt-Gaurd", and "Avaunt-Guard" in the dictionary
> (and SPELLED WRONG)?

None of these words are in the dictionary.  This is just the spell checker getting mislead because of the hyphen in this word.  Please see <http://mxr.mozilla.org/mozilla-central/source/extensions/spellcheck/locales/en-US/hunspell/en-US.dic> for the dictionary source that I'm talking about.

Comment 7

[draco18s]

(In reply to Ehsan Akhgari [:ehsan] from comment #6)
> This is just the spell checker
> getting mislead because of the hyphen

Sounds like a bug!

Comment 8

[Joshua Gaines]

I would like to take a stab at the this bug.

Comment 9

[(no longer active)]

Great, thanks!

Comment 10

[Joshua Gaines]

Ok I have come up with a fix for this bug. It was simple, I just added "Avant-garde" and "avant-garde" to the US-EN.dic file. Now when you type the incorrect spelling "avant-gaurd", then "avant-garde" is now suggested as a correction. I am attaching two patch files. Patch1.txt is where I altered the extensions\spellcheck\locales\en-US\hunspell\dictionary-sourceshunspell-en_US-20081205.dic to contain the new words. Patch2.txt is just the diff of the upstream dictionary file where the lower lever dictionaries are merged together into one main dictionary.

Now, I am just waiting on how to proceed since this is my first bug fix. Do I need to create some sort of automated test for this fix? I have obvisouly built firefox with my changes and manually tested it.

Comment 11

[Joshua Gaines]

Attachment: Patch1

This patch is just the diff of where I added the word to the lower level hunspell-en_US-20081205.dic found in \extensions\spellcheck\locales\en-US\hunspell\dictionary-sources

Comment 12

[Joshua Gaines]

Attachment: Patch2

This patch is just a diff of the final dictionary file after the make merges the lower dictionary files together.

Comment 13

[(no longer active)]

Comment on attachment 706935 [details] [diff] [review]
Patch1

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

Thanks a lot for your patch, Joshua!

Please make a patch which contains all of the changes to the repository, not one patch per file.  See <https://developer.mozilla.org/en-US/docs/Creating_a_patch> for an explanation on how to do that.

Also, although we usually require automated tests alongside patches, I don't think there's much point in an automated test for this, so you don't need to add one here.

Waiting to see your next patch! :-)

::: extensions/spellcheck/locales/en-US/hunspell/en-US.dic
@@ +1265,5 @@
>  Autumn/M
>  Av/M
>  Ava/M
>  Avalon
> +Avant-garde

You shouldn't need to add the upper-case version.

Comment 14

[Joshua Gaines]

Attachment: FullPatch

This is a full diff of the altered directory.

Comment 15

[(no longer active)]

Comment on attachment 709372 [details] [diff] [review]
FullPatch

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

::: extensions/spellcheck/locales/en-US/hunspell/en-US.dic
@@ +1265,5 @@
>  Autumn/M
>  Av/M
>  Ava/M
>  Avalon
> +Avant-garde

As I said before, you don't need the upper-case version.

Comment 16

[Joshua Gaines]

Attachment: Capitalized form of avant-garde removed

Lets try this one more time

Comment 17

[(no longer active)]

Comment on attachment 709465 [details] [diff] [review]
Capitalized form of avant-garde removed

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

::: extensions/spellcheck/locales/en-US/hunspell/en-US.dic
@@ +1265,5 @@
>  Autumn/M
>  Av/M
>  Ava/M
>  Avalon
> +Avant-garde

Still here!  Am I missing something?

Comment 18

[Joshua Gaines]

Attachment: Capitialized form of Avant-garde removed

Okay I apologize for my previous patch submissions. Turns out that removing the word from the lower stream dictionaries does not remove it from the upper level merged dictionary. I was thinking that each time I would build the upper level dictionary was reconstructed from the lower level dictionaries. Anyways, I hope this is the correct patch needed.

Comment 19

[(no longer active)]

Comment on attachment 715459 [details] [diff] [review]
Capitialized form of Avant-garde removed

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

::: extensions/spellcheck/locales/en-US/hunspell/en-US.dic
@@ +1,1 @@
> +57444

Hmm, this count is wrong.  I'd expect attempting to load this dictionary to crash, etc.  :(

I suggest that you start over on a clean tree, and don't use the previous changes made to the dictionaries...  Sorry that this proved to be this much of a hassle.  :-)

Comment 20

[Joshua Gaines]

Attachment: Clean Tree Patch

Hopefully I got it write this time. I pulled a clean tree and added only the lower case word to the dictionary. Sorry I didn't knock this simple fix out quickly.

Comment 21

[(no longer active)]

Comment on attachment 718415 [details] [diff] [review]
Clean Tree Patch

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

The en-US.dic file seems to be missing from this patch...

Comment 22

[Joshua Gaines]

Attachment: Fixed? Final Patch?

So I think I am going for the record of patch attempts for a trivial fix. I am very sorry if I cause any inconvenience. I forgot to merge the dictionaries and move the new en-US.dic to the top directory. I am hoping this I have go it correct this time.

Comment 23

[(no longer active)]

Comment on attachment 718776 [details] [diff] [review]
Fixed? Final Patch?

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

Looks great, thanks a lot!  And no worries about the number of iterations, it was astonishing to see you stick around this whole time and persevere until everything is right!  Thanks for doing that!  :-)

Comment 24

[(no longer active)]

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