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

RESOLVED FIXED in Thunderbird 42.0

Status

Thunderbird
Message Compose Window
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: Thomas D. (currently busy elsewhere; needinfo?me), Assigned: Jorg K (GMT+2))

Tracking

({regression})

38 Branch
Thunderbird 42.0
regression
Dependency tree / graph

Thunderbird Tracking Flags

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

Details

Attachments

(1 attachment, 1 obsolete attachment)

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)
(Assignee)

Comment 1

2 years ago
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
(Assignee)

Comment 2

2 years ago
Created attachment 8626813 [details] [diff] [review]
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.
Attachment #8626813 - Flags: review?(mkmelin+mozilla)
(Assignee)

Updated

2 years ago
Assignee: nobody → mozilla
(Assignee)

Updated

2 years ago
status-thunderbird39: --- → unaffected
status-thunderbird40: --- → affected
status-thunderbird41: --- → affected
status-thunderbird42: --- → affected
status-thunderbird_esr38: --- → affected
tracking-thunderbird_esr38: --- → ?
(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!
(Assignee)

Comment 4

2 years ago
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)
(Assignee)

Comment 5

2 years ago
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 6

2 years ago
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+
(Assignee)

Comment 7

2 years ago
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

2 years ago
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-
(Assignee)

Comment 9

2 years ago
Created attachment 8627351 [details] [diff] [review]
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.
Attachment #8626813 - Attachment is obsolete: true
Attachment #8626813 - Flags: review?(iann_bugzilla)
Attachment #8627351 - Flags: review?(neil)
(Assignee)

Comment 10

2 years ago
Make that 38.1 ;-)

Updated

2 years ago
Attachment #8627351 - Flags: review?(neil) → review+
(Assignee)

Updated

2 years ago
Keywords: checkin-needed
(Assignee)

Comment 11

2 years ago
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?

Comment 12

2 years ago
https://hg.mozilla.org/comm-central/rev/65e8c84d6665
status-thunderbird42: affected → fixed
Keywords: checkin-needed
Target Milestone: --- → Thunderbird 42.0

Updated

2 years ago
Status: NEW → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
(Assignee)

Comment 13

2 years ago
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

2 years ago
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+

Updated

2 years ago
status-thunderbird40: affected → fixed
status-thunderbird41: affected → fixed
status-thunderbird_esr38: affected → fixed
tracking-thunderbird_esr38: ? → 39+

Updated

2 years ago
status-thunderbird38: --- → affected

Updated

2 years ago
Blocks: 1177041
Jorg and everyone, thank you! 9 days is a really cool bug turnaround time :)
(Assignee)

Comment 16

2 years ago
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).

Updated

2 years ago
Blocks: 1183290
You need to log in before you can comment on or make changes to this bug.