1176671 - Changing dictionary language from spellcheck dialogue is not persisted after closing the dialogue

Status: RESOLVED FIXED

(Thunderbird :: Message Compose Window, defect)

Description

[Thomas D. (:thomas8)]

STR

0 default spell check language set to EN in Options > composition; install DE-DE dictionary (e.g. right-click on composition msg body > languages > add dictionaries)

1 compose email msg, for demonstration with DE and EN language text
subject: Hello World - Hallo zusammen
body: This is English with - Das ist Deutsch

2 place focus in body (because spelling button is wrongly disabled in subject, unrecorded bug, the other half of bug 368915)

3 click "Spelling" button

4 change dictionary from English/Untited States to German/Germany and watch inline spelling change in background

5 optionally click any button like recheck, ignore, replace to confirm that you're actually using the new dictionary (but this shouldn't be necessary)

6 Verify and remember which language you have chosen (DE) and then click "close" button in spelling dialogue

7 Look at inline spelling marks, and verify current language from dual spelling button

Actual result

4) when changing dictionary, inline spelling is updated as long as spell dialogue is open (to DE)
7) but after closing the dialogue, it reverts back to the original language setting which was EN

Expected result

7) Chosing another dictionary in spell dialogue must be persisted after closing the dialogue (with or without further action)

(Also, it looks like another bug, probably in editor's spell-check dialogue, that TB immediately applies the new language to the text in background but DOES NOT update the list of suggestions *in* the dialogue to the new language - this should be synchronized somehow)

Comment 1

[Jorg K (CEST = GMT+2)]

This is another regression from bug 967494. Previously, changing the dictionary anywhere in the user interface, set the spellchecker.dictionary preference to the new value. This affected all open compositions. This is now no longer done
http://mxr.mozilla.org/comm-central/source/mozilla/editor/composer/nsEditorSpellCheck.cpp#609
since we have the eEditorMailMask set.

BTW, the full spelling dialogue changes the dictionary here:
http://mxr.mozilla.org/comm-central/source/editor/ui/dialogs/content/EdSpellCheck.js#422

The fix is to set the "lang" attribute of the document from which the spell check was initiated.

Comment 2

[Jorg K (CEST = GMT+2)]

Attachment: One line change to fix this (would be good to ship with TB 38.1)

I suggest to include this in TB 38.1 since it's a regression.

Comment 3

[Thomas D. (:thomas8)]

(In reply to Jorg K from comment #2)
> Created attachment 8626813 [details] [diff] [review]
> One line change to fix this
> 
> I suggest to include this in TB 38.1 since it's a regression.

+1. Thanks Jorg for providing the patch! Rapid bug turnaround time, nice!

Comment 4

[Jorg K (CEST = GMT+2)]

Comment on attachment 8626813 [details] [diff] [review]
One line change to fix this (would be good to ship with TB 38.1)

Looking for a reviewer for a one-line-change ;-)

Comment 5

[Jorg K (CEST = GMT+2)]

Comment on attachment 8626813 [details] [diff] [review]
One line change to fix this (would be good to ship with TB 38.1)

I've just tested it again: It works correctly regardless of whether you start the spell check via the button, <CTRL><SHIFT>P or the menu.

Comment 6

[Magnus Melin [:mkmelin]]

Comment on attachment 8626813 [details] [diff] [review]
One line change to fix this (would be good to ship with TB 38.1)

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

I'm not an editor/ reviewer, but looks like the right thing to do to me.

Comment 7

[Jorg K (CEST = GMT+2)]

Thanks for the feedback+. So funny, since we have the same code here:
https://dxr.mozilla.org/comm-central/source/mail/components/compose/content/MsgComposeCommands.js#3173

Comment 8

[[e-mail]]

Comment on attachment 8626813 [details] [diff] [review]
One line change to fix this (would be good to ship with TB 38.1)

I only want this to apply to compose windows; fortunately window.arguments[1] is true in this case. Please could you update the patch and also add a comment?

Comment 9

[Jorg K (CEST = GMT+2)]

Attachment: Fix, corrected as per Neil's suggestion

It would be great if you could review this again a.s.a.p., since Kent wants to land stuff for TB 31.1.

Comment 10

[Jorg K (CEST = GMT+2)]

Make that 38.1 ;-)

Comment 11

[Jorg K (CEST = GMT+2)]

Comment on attachment 8627351 [details] [diff] [review]
Fix, corrected as per Neil's suggestion

[Approval Request Comment]
Regression caused by (bug #): 967494
User impact if declined: Results in behaviour that leaves the user puzzled.
Risk to taking this patch (and alternatives if risky):
Small change, not risky, basically only sets an attribute of the document's document element.

Note: bug 967494 didn't land on TB 39, so beta approval is not requested.

Comment 12

[Kent James (:rkent)]

https://hg.mozilla.org/comm-central/rev/65e8c84d6665

Comment 13

[Jorg K (CEST = GMT+2)]

Can you please land this on TB38, 40 and 41 as well, since they all have the problem. All the fixed we put into TB38 also went into 40 and 41.

Comment 14

[Kent James (:rkent)]

Comment on attachment 8627351 [details] [diff] [review]
Fix, corrected as per Neil's suggestion

[Triage Comment]

http://hg.mozilla.org/releases/comm-aurora/rev/d4c7c9330493
http://hg.mozilla.org/releases/comm-beta/rev/f9a9f3cfcdaa
http://hg.mozilla.org/releases/comm-esr38/rev/8d5d97576d43

Comment 15

[Thomas D. (:thomas8)]

Jorg and everyone, thank you! 9 days is a really cool bug turnaround time :)

Comment 16

[Jorg K (CEST = GMT+2)]

I only saw the bug on the 26th of June, so for me it's four days turnaround (26th to 30th).
Next time you see a spelling/dictionary problem, please CC me straight away.
I sincerely hope that this was the very last regression of bug 967494, I've counted four regressions (bug 1175908, bug 1170181/bug 1169996, bug 1168945 and this one).