install dictionaries... option now missing from context menu

NEW
Unassigned

Status

()

Firefox
Menus
10 months ago
7 months ago

People

(Reporter: Brian S. Craigie, Unassigned)

Tracking

({regression})

52 Branch
regression
Points:
---

Firefox Tracking Flags

(firefox-esr45 unaffected, firefox52 wontfix, firefox-esr52 affected, firefox53+ fix-optional, firefox54+ wontfix, firefox55+ fix-optional, firefox56 fix-optional)

Details

Attachments

(2 attachments)

(Reporter)

Description

10 months ago
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
[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.
Blocks: 908570
Status: UNCONFIRMED → NEW
status-firefox52: --- → affected
status-firefox53: --- → affected
status-firefox54: --- → affected
status-firefox55: --- → affected
status-firefox-esr45: --- → unaffected
status-firefox-esr52: --- → affected
tracking-firefox53: --- → ?
Component: Untriaged → Menus
Ever confirmed: true
Flags: needinfo?(ehsan)

Comment 2

10 months ago
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...)
Flags: needinfo?(ehsan) → needinfo?(overholt)
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.
status-firefox53: affected → fix-optional
tracking-firefox53: ? → +
tracking-firefox54: --- → +
tracking-firefox55: --- → +
Keywords: regression
Masayuki, is this something you might take on?
status-firefox52: affected → wontfix
Flags: needinfo?(masayuki)
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).
Flags: needinfo?(masayuki)
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?
Flags: needinfo?(overholt) → needinfo?(ehsan)
Makoto-san could be interested in to this.

Comment 8

9 months ago
(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?
Flags: needinfo?(ehsan) → needinfo?(m_kato)
Maybe Dolske can help.
Flags: needinfo?(dolske)
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?
Flags: needinfo?(m_kato)
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.)
Flags: needinfo?(dolske)
Created attachment 8865482 [details]
Menu item in Firefox 50, German
Created attachment 8865567 [details] [diff] [review]
Patch v.1 (not right)
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"?
Flags: needinfo?(ehsan)
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

9 months ago
I agree that sounds like the desirable direction to move towards.
Flags: needinfo?(ehsan)
Tool late for 54 as 54 RC is released, mark 54 won't fix.
status-firefox54: affected → wontfix
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.
status-firefox55: affected → fix-optional
status-firefox56: --- → affected
Flags: needinfo?(past)
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.
Flags: needinfo?(past)
status-firefox56: affected → fix-optional
You need to log in before you can comment on or make changes to this bug.