Closed Bug 1176671 Opened 4 years ago Closed 4 years ago

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

Categories

(Thunderbird :: Message Compose Window, defect)

38 Branch
defect
Not set

Tracking

(thunderbird38 affected, thunderbird39 unaffected, thunderbird40 fixed, thunderbird41 fixed, thunderbird42 fixed, thunderbird_esr3839+ fixed)

RESOLVED FIXED
Thunderbird 42.0
Tracking Status
thunderbird38 --- affected
thunderbird39 --- unaffected
thunderbird40 --- fixed
thunderbird41 --- fixed
thunderbird42 --- fixed
thunderbird_esr38 39+ fixed

People

(Reporter: bugzilla2007, Assigned: jorgk)

References

Details

(Keywords: regression)

Attachments

(1 file, 1 obsolete file)

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)
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.
Keywords: regression
I suggest to include this in TB 38.1 since it's a regression.
Attachment #8626813 - Flags: review?(mkmelin+mozilla)
Assignee: nobody → mozilla
(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 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 ;-)
Attachment #8626813 - Attachment description: One line change to fix this → One line change to fix this (would be good to ship with TB 38.1)
Attachment #8626813 - Flags: superreview?
Attachment #8626813 - Flags: review?(neil)
Attachment #8626813 - Flags: review?(iann_bugzilla)
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.
Attachment #8626813 - Flags: superreview?
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.
Attachment #8626813 - Flags: review?(mkmelin+mozilla) → feedback+
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 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?
Attachment #8626813 - Flags: review?(neil) → review-
It would be great if you could review this again a.s.a.p., since Kent wants to land stuff for TB 31.1.
Attachment #8626813 - Attachment is obsolete: true
Attachment #8626813 - Flags: review?(iann_bugzilla)
Attachment #8627351 - Flags: review?(neil)
Make that 38.1 ;-)
Attachment #8627351 - Flags: review?(neil) → review+
Keywords: checkin-needed
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.
Attachment #8627351 - Flags: approval-comm-esr38?
Attachment #8627351 - Flags: approval-comm-aurora?
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
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 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
Attachment #8627351 - Flags: approval-comm-esr38?
Attachment #8627351 - Flags: approval-comm-esr38+
Attachment #8627351 - Flags: approval-comm-beta+
Attachment #8627351 - Flags: approval-comm-aurora?
Attachment #8627351 - Flags: approval-comm-aurora+
Jorg and everyone, thank you! 9 days is a really cool bug turnaround time :)
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).
Blocks: 1183290
You need to log in before you can comment on or make changes to this bug.