Closed Bug 717433 Opened 13 years ago Closed 9 years ago

Chosen dictionary intermittently resets from en-GB to en-US after session restart

Categories

(Core :: Spelling checker, defect)

Product:
Core
Component:
Spelling checker
Platform:
x86
Windows 7
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla43
Tracking Flags:
Tracking Status
firefox43 --- fixed

People

(Reporter: emorley, Assigned: jorgk-bmo)

References

(Blocks 1 open bug)

Details

Attachments

(3 files, 3 obsolete files)

about:support
(deleted), text/plain
Details
Proposed solution
(deleted), patch
roc
: review+
Details | Diff | Splinter Review
Test for the patch (v2)
(deleted), patch
roc
: review+
Details | Diff | Splinter Review
Attached file about:support (deleted) —
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:12.0a1) Gecko/20120111 Firefox/12.0a1 See attachment for about:support dump. Installed dictionaries: Only en-GB (British English Dictionary addon v1.19.1) I haven't been able to get it to occur consistently, but STR along the lines of: 1) In a gmail app-tab start new draft email 2) Type a word that has a different spelling in the US eg neighbour 3) Spot that the word is marked as misspelt, check context menu - find it is set to US English 4) Set back to UK English 5) Finish email + send, return to gmail inbox 6) Carry on browsing, restart session at some point. Expected: Once set to en-GB, preference is remembered even after Nightly restart. Actual: ~10-20% of the time the pref is reset back to en-US after session restart.
I'm seeing the same thing.
Ed, did you get a chance to debug this?
I will look into this.
Sorry for no rely so far - I was having great trouble identifying when I'd successfully repeated the STR, since the change only seemed to become apparent on Firefox restart. Inspection of prefs.js after app exit, showed the pref had changed from en-GB to en-US - so I decided to annotate parts of mozHunspell::SetDictionary in a local debug build to be able to see in the console when I'd successfully recreated the STR, but then struggled to get it to do so in my local build :-( I also tried doing what ehsan suggested and setting a breakpoint on mozHunspell::SetDictionary, but my lack of RAM/ancient CPU makes MSVC debugging pretty much unusable :-(( (even ignoring the incessant assertions on startup and on gmail, eg bug 714398 - how people put up with debug builds is beyond me - though maybe things are less annoying on an i7 or something)
I couldn't reproduce this with the latest nightly builds. Paul, are you able to get this still?
I'm seeing something similar -- a switch from en-CA to fr-BE, happening over and over again. Using 12.0~b3+build1-0ubuntu0.12.04.1~mfn2 (so, an ubuntu-built package) on ubuntu precise. inspection of prefs.js shows user_pref("spellchecker.dictionary", "fr_BE"); But about:config shows: spellchecker.dictionary;en_CA not sure what the issue is...
Blocks: spell-lang-change
Seeing similar problem in latest 64 bit nightly. 38.0a1 STR: Change dictionary to English Australia. Shut browser down Restart Browser Dictionary has reverted to en-us
I also continue to see this problem, with current FDE and also with Firefox Beta 36.0b2 (both on Arch linux)
This was analysed in bug 1200533. The problem is that en-AU or en-GB should be stored as so-called "content preference", however, these only get stored if the dictionary is set to another language. So setting de-DE or es-ES on an "en" site sticks, setting en-AU and en-GB is explicitly discarded. Code here: https://dxr.mozilla.org/mozilla-central/source/editor/composer/nsEditorSpellCheck.cpp#616
Attached patch Proposed solution (deleted) — — Splinter Review
(In reply to Ehsan Akhgari (don't ask for review please) from comment #2) > Ed, did you get a chance to debug this? I'm not Ed, but yes, this has been analysed. Perhaps, despite Ehsan's general veto to touch this code, we can land a small fix here that affects many English speakers who prefer "en-GB" or "en-AU" over "en-US".
Attachment #8655814 - Flags: review?(roc)
Comment on attachment 8655814 [details] [diff] [review] Proposed solution Review of attachment 8655814 [details] [diff] [review]: ----------------------------------------------------------------- I agree
Attachment #8655814 - Flags: review?(roc) → review+
(In reply to Ehsan Akhgari (don't ask for review please) from comment #2) > Ed, did you get a chance to debug this?
Assignee: nobody → mozilla
Keywords: checkin-needed
Jorg, does the patch also cover the case where some other non-"en" language is propagated to the default (via the preference), causing it to pop up on all windows with neither content pref or lang attribute? E.g. in the situation described in the reproduction, where you also have the set of "de" dictionaries installed, and visit a "de" site? (There's also the fact that those sites pick a “random” de, usually de-at, even if I tell them I want de-de — but this I think should be covered by the patch)
The patch covers exactly the problem described. A specific en-XX choice gets reverted to en-US. Of course similar examples exist for de-XX being reverted to de-YY. When you first visit an "en" site, an en-XX dictionary will be chosen at random, equally for a "de" site, perhaps de-AT. If you changed this, before this user selection was discarded, now it will stick. The other thing you're complaining about, if I understand it correctly, is not part of the bug. The following problem still persists: Have two windows, window 1 and window 2. In window 2 show a site that has no "lang" attribute and where no content preference has been set. So this site uses a fallback, typically from the preference "spellchecker.dictionary" (or if not set, from the locale). Now in window 1, change the language. This will currently always set "spellchecker.dictionary". If you now return to window 2, the language will change to the one set in window 1 based on the preference. This is (sadly) established behaviour. Changing this behaviour was subject to bug 1073840(*), but that has met too much opposition. I am hoping to fix up the fallback behaviour a little in bug 1200533, but the latter will maintain the established behaviour. (*) The aim of bug 1073840 was *not* to change "spellchecker.dictionary" every time a dictionary was set elsewhere.
Can you please add a test for this? test_add_remove_dictionaries.xul shows you how you can install dictionaries in a test and helper_bug1170484.js used in test_bug1170484.html shows you how to click on the spell checking context menu to change the language (helper_bug1170484.js shows how to pick a spelling suggestion from the context menu which should be similar.)
I can, but only if do you a (kind) review of bug 1200533 for me (keeping in mind that the first part of the patch already landed here) ;-) Since it's doable to add dictionaries while testing, bug 1200533 should be fully testable.
Keywords: leave-open
I really don't have enough bandwidth these days. Please ask roc. He should have not r+'ed without a test. ;-)
Flags: needinfo?(ehsan)
Attached patch Test for the patch (v1) (obsolete) (deleted) — — Splinter Review
Here's the test so we don't get into trouble with Ehsan ;-) Limited try run here: https://treeherder.mozilla.org/#/jobs?repo=try&revision=130f54e04540 (no idea how I managed to push 19 changesets, I intended to push two, see whether it works). I should say that I copied this from test_bug678842.html and test_add_remove_dictionaries.xul. So if it's not right, I blame it on the original author(s) ;-)
Attachment #8656061 - Attachment is obsolete: true
Attachment #8656164 - Flags: review?(roc)
I don't know if this is even the right bug to answer your question, but I'm getting kind of confused with all the activity, thanks a lot by the way for wading into this. (In reply to Jorg K (GMT+2) from comment #14) > The other thing you're complaining about, if I understand it correctly, is > not part of the bug. > > The following problem still persists: > Have two windows, window 1 and window 2. In window 2 show a site that has no > "lang" attribute and where no content preference has been set. So this site > uses a fallback, typically from the preference "spellchecker.dictionary" (or > if not set, from the locale). > > Now in window 1, change the language. This will currently always set > "spellchecker.dictionary". If you now return to window 2, the language will > change to the one set in window 1 based on the preference. Almost, but not quite. Let me try to put it into these terms. - In window 1, visit a site with no lang attribute and no content preference. It uses the fallback, which, let's just assume, will be the correct dictionary. - In window 2, visit a site with a "lang" attribute of "de". This sets "spellchecker.dictionary" to "de". Both windows at this point have the correct dictionary set (I think). - Restart the browser (with "Show my windows and tabs from last time"). - Now both windows have some "de-XX" dictionary selected. - Bonus: if I then select "en-gb" on window 1, and restart the browser again, window 1 will be back to "de-XX". Can probably also be replicated by closing window 1 and opening the same site again on window 3. I'm not willing to test right now (I have a dirty work-around in place) but I can if you say it would help.
This bug is not a good place to discuss anything any more, the fix has landed, and as soon as the test lands, this will be marked fixed. Also, your description is sadly wrong: Window 1 will use a fallback, most likely the last value of "spellchecker.dictionary". If in window 2 you visit a site with lang="de", this WILL NOT set the "spellchecker.dictionary". "spellchecker.dictionary" will only be set when you actively select another dictionary via the right-click (context) menu. As detailed before, this can affect all other windows that are based on "spellchecker.dictionary". The "spellchecker.dictionary" is always set to a full dictionary name, never the language code ("de") alone. This behaviour won't change. My remaining fixes are in bug 1200533. However, what you're describing is not covered there, so there would also not be the right place to comment. If you want to be heard, please give a 100% reproducible description of what you are experiencing and put it into the meta-bug 1073827. From there, we can allocate it to the right bug. I repeat: The problem must be 100% reproducible, you should remove/rename your content-prefs.sqlite before you start, or start on a new profile (start Firefox with the -p option). Please specify all the dictionaries you have installed and all the websites you visit.
Thanks. My list of installed dictionaries won't change any time soon, so with a fresh profile, I can reproduce this easily at any point. To avoid creating redundant work for you, what I'll do is I'll wait for your batch of fixes to land, then build that tree, and see if it still happens. I have a feeling it won't, because what I'm seeing might be a side-effect of one of the things you're fixing. If it does happen, I'll give all the detail I can produce.
The fix has landed (comment #22). Of course you can use a Nightly build in a day or so. Sadly my try server build was for Linux only.
Attached patch Test for the patch (v2) (deleted) — — Splinter Review
Need to remove en-GB dictionary after use. If not, later tests use it and fail, since it's a fake dictionary with nothing in it. New try run (just for the "oth" test that failed before): https://treeherder.mozilla.org/#/jobs?repo=try&revision=9d83581b40d5
Attachment #8656164 - Attachment is obsolete: true
Attachment #8656164 - Flags: review?(roc)
Attachment #8656246 - Flags: review?(roc)
Oops, got the try run wrong, it's -o for "other", not -oth. So here we go again with: try: -b o -p linux64 -u mochitest-o[x64] -t none https://treeherder.mozilla.org/#/jobs?repo=try&revision=cd977eb0180f
Try run green, so all good. Dear Sheriff: Please check in the second patch "Test for the patch (v2)". The first part was already checked here: (comment #22) https://hg.mozilla.org/mozilla-central/rev/c11d024e51fd Sorry about the extra hassle.
Keywords: leave-open → checkin-needed
Thanks for the test!
And there will be many more tests in bug 1200533 ;-)
Status: NEW → RESOLVED
Closed: 9 years ago
status-firefox43: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
Thank you for fixing this Jorg! :-)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: