1349745 - install dictionaries... option now missing from context menu

Status: RESOLVED WORKSFORME

(Firefox :: Menus, defect)

Description

[Brian S. Craigie]

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:52.0) Gecko/20100101 Firefox/52.0
Build ID: 20170316213829

Steps to reproduce:

Normally, to install a dictionary in Firefox for the first time, you right-mouse on a text box and choose install dictionaries...


Actual results:

The context menu item is missing


Expected results:

It should have offered "install dictionaries..." in the context menu

Comment 1

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

[Tracking Requested - why for this release]: Regression which hides the dictionary installation where it has the most context (context menu of text forms)

This is a regression from bug 908570.

To reproduce, use e.g. a German (de) build which doesn't have a dictionary included like the en-US one has.

Comment 2

[(no longer active)]

Andrew, can you please help find an owner?

(For some context, bug 908570 is a bug that I waited for feedback on for 4 years, and during that time lost all of what I used to know about it...)

Comment 3

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

Regression from 52. 

I think this is unlikely to get fixed in time for 53, but I would still take a patch if one magically appears.

Comment 4

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

Masayuki, is this something you might take on?

Comment 5

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Hmm, I don't have much time to work on this. If it's urgent, please look for another person. Otherwise, or there is no person, I'll take this into my queue (but lower priority, I still have serious regression at 52).

Comment 6

[Andrew Overholt [:overholt]]

We lived for years with bug 908570; could we back its changes out to avoid this regression which Masayuki (who would also have been my best guess for someone who could fix this) won't be able to get to soon?

Comment 7

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Makoto-san could be interested in to this.

Comment 8

[(no longer active)]

(In reply to Andrew Overholt [:overholt] from comment #6)
> We lived for years with bug 908570; could we back its changes out to avoid
> this regression which Masayuki (who would also have been my best guess for
> someone who could fix this) won't be able to get to soon?

This has been shipped in Firefox 52, so unfortunately backing out isn't really a good option any more.  Also we're talking about a shipped regression now.  :-(

Makoto-san, are you interested in taking this?

If not, is there nobody on the Firefox team who can help with this?

Comment 9

[Andrew Overholt [:overholt]]

Maybe Dolske can help.

Comment 10

[Makoto Kato [:m_kato]]

What right behavior?

Now, on 51.0, [Languages] - [Add Dictionaries...] menu appears when [Check Spelling] is turned on context menu of <input type="text">.  It is same as 55.0.

Top level's [Add Dictionaries...] menu disappears on contenteditable with spellcheck=false by bug 908570 (52.0).

Should there be [Add Dictionaries...] menu in top-level of context menu when [Check Spelling] is turned off?

Comment 11

[Justin Dolske [:Dolske]]

I think the request (comment 1) is that we're currently broken in that when the build has _no_ preinstalled dictionary installed, we don't show the submenu that allows installing a dictionary in the first place. (Actually, no, we used to show a top-level "Install Dictionaries..." menuitem, not the whole Languages submenu).

I guess we should fix that, although I sorta feel like even that UI was not very good.

I think the core fix is around the change to this line to before the change in attachment 8802771 [details] [diff] [review]:

http://searchfox.org/mozilla-central/rev/8b70b0a5038ef9472fe7c53e04a70c978bb06aed/browser/base/content/nsContextMenu.js#460

Previously it was unconditionally set to true, now it's conditional on showDictionaries. (Which really makes it unconditionally false, between the logic of the if/else here and how showDictionaries is set.)

Comment 12

[Justin Dolske [:Dolske]]

Attachment: Menu item in Firefox 50, German

Comment 13

[Justin Dolske [:Dolske]]

Attachment: Patch v.1 (not right)

Comment 14

[Justin Dolske [:Dolske]]

I think this is a step in the right direction, but it regresses attachment 790239 [details] from bug 905176. (And all the context menu tests are disabled in bug 1246296, so...). I don't understand the various states of things involved very well though, so I don't have a lot of confidence in it.

Basic idea here was to try and untangle things a bit so that there are 3 cases: (1) show the Languages submenu as normal, which includes Add Dictionary within (2) show only the top-level Add Dictionary menuitem, but not the Languages submenu and (3) show nothing.

I think the remaining bug is that case #2 is broken and needs to be stricter, as noted above. Seems like it wants to be "this.onEditableArea && mumble.noDictionariesInstalled"?

Comment 15

[Justin Dolske [:Dolske]]

Oh, for future reference: it's easy to test the no-dictionaries-installed case in a build by just deleting the two files in dist/FirefoxDebug.app/Contents/Resources/dictionaries/.

Comment 16

[(no longer active)]

I agree that sounds like the desirable direction to move towards.

Comment 17

[Gerry Chang [:gchang]]

Tool late for 54 as 54 RC is released, mark 54 won't fix.

Comment 18

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

Panos, any opinion here? Want to try to find someone to fix this for 56? If not, maybe it can be a good first bug if someone's willing to mentor.

Comment 19

[Panos Astithas (he/him) [:past] (please ni?)]

I discussed this with Justin and I think we should mark this as fix-optional for 56, too. I can't find anyone to work on this and it doesn't seem to be a huge problem so far, judging by the lack of duplicates and the number of localized builds it affects (about 1/3 of them ship without a dictionary).

I don't know that this is a good first bug, but it seems to be in a good place for someone somewhat experienced to pick up and finish.

Comment 20

[BugBot [:suhaib / :marco/ :calixte]]

Set release status flags based on info from the regressing bug 908570

Comment 21

[Ryan VanderMeulen [:RyanVM]]

This doesn't appear to be reproducible anymore. Feel free to reopen if there are still current steps to reproduce.