Inline spell check behaving strangely on reply, regression from bug 967494

RESOLVED FIXED in Thunderbird 41.0

Status

defect
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: jorgk, Assigned: jorgk)

Tracking

({regression})

Trunk
Thunderbird 41.0

Thunderbird Tracking Flags

(thunderbird38+ fixed, thunderbird39 affected, thunderbird40 fixed, thunderbird41 fixed)

Details

Attachments

(1 attachment, 2 obsolete attachments)

Have two dictionaries, English and Spanish, make English the default for the inline spell check.

Create a new message. Dictionary is set to English.
Set the dictionary to Spanish. Type something in Spanish ("hoy" - today).
Save the message as a draft. Close the window.

Reply to a message. Dictionary is set to Spanish! Type "hoy", no red underline.
Click on the main window. Then make the message compose window current again.
Language jumps to English, "hoy" is now underlined.

There's something fishy here. It looks like it has to do with window recycling, since setting mail.compose.max_recycled_windows to 0 fixes the problem.
Flags: needinfo?(neil)
Component: Message Compose Window → Editor
Product: Thunderbird → Core
Keywords: regression
Component: Editor → Mail Window Front End
Product: Core → Thunderbird
Target Milestone: --- → Thunderbird 38.0
Assignee: nobody → mozilla
Status: NEW → ASSIGNED
I don't believe that it makes sense to block on this, but we would consider a patch.
Well, it's pretty bad that the spell check changes when you bring the window back forward.
It's a one line change, so Magnus can review this today and we can include it.
When is the cut-off date for the TB38 release?
(In reply to Jorg K from comment #0)
> Have two dictionaries, English and Spanish, make English the default for the
> inline spell check.
> 
> Create a new message. Dictionary is set to English.
> Set the dictionary to Spanish. Type something in Spanish ("hoy" - today).
> Save the message as a draft. Close the window.
> 
> Reply to a message. Dictionary is set to Spanish! Type "hoy", no red
> underline.
> Click on the main window. Then make the message compose window current again.
> Language jumps to English, "hoy" is now underlined.

OK, so with your patch, what happens in this case:
Create a new message. Dictionary is set to English.
Type something in Spanish ("hoy" - today).
Save the message as a draft. Close the window.
Change the default dictionary to Spanish.
When you open the draft, what is the dictionary set to?
(If it makes a difference, try a reply too.)
Comment on attachment 8611474 [details] [diff] [review]
Proposed patch (fixed overlong line)

>@@ -4716,29 +4715,28 @@ function InitEditor()
>   // still add dictionaries. Else, hide that.
>   document.getElementById('spellCheckAddDictionariesMain')
>           .setAttribute('hidden', gSpellChecker.canSpellCheck);
>   // Then, we enable related UI entries.
>   enableInlineSpellCheck(getPref("mail.spellcheck.inline"));
>   gAttachmentNotifier.init(editor.document);
> 
>   // Set document language to preferred dictionary.
>-  document.getElementById("msgcomposeWindow").setAttribute("lang",
>+  document.documentElement.setAttribute("lang",
>     Services.prefs.getCharPref("spellchecker.dictionary"));
Perhaps we need to do this before enabling the inline spell check?
Now setting document language as early as possible so the M-C editor always sees the language we really want, even for recycled windows and when editing a draft after changing the preference.
Attachment #8611474 - Attachment is obsolete: true
Attachment #8611474 - Flags: review?(mkmelin+mozilla)
Attachment #8611474 - Flags: feedback?(neil)
Attachment #8612159 - Flags: review?(mkmelin+mozilla)
Attachment #8612159 - Flags: feedback?(neil)
Comment on attachment 8612159 [details] [diff] [review]
Proposed patch, revised after Neil's input.

Well, that's certainly before we enable the inline spell check ;-)
Attachment #8612159 - Flags: feedback?(neil) → feedback+
> Perhaps we need to do this before enabling the inline spell check?

Sorry Neil, wires crossed. You are right, I moved it to spot much earlier in the processing. I hope I hit the right spot this time. Seems to work now in all cases, including the test cases you proposed. Thank you so much for your input!
Wires crossed again. Thank you again!
Comment on attachment 8612159 [details] [diff] [review]
Proposed patch, revised after Neil's input.

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

LGTM! r=mkmelin
Attachment #8612159 - Flags: review?(mkmelin+mozilla) → review+
(In reply to Jorg K from comment #7)

> When is the cut-off date for the TB38 release?

Not clear, sometimes between today and a week from today. (Although technically, the cutoff was several months ago.)
OK, land it today then ;-)
Comment on attachment 8612159 [details] [diff] [review]
Proposed patch, revised after Neil's input.

[Approval Request Comment]
Regression caused by (bug #): 967494
User impact if declined: Nasty little regression, very confusing.
Risk to taking this patch (and alternatives if risky):
Not risky at all. One line of processing moved to an earlier point.
Also some cosmetic changes.
Neil gave feedback+, so it must be good ;-)
Attachment #8612159 - Flags: approval-comm-beta?
Attachment #8612159 - Flags: approval-comm-aurora?
Keywords: checkin-needed
https://hg.mozilla.org/comm-central/rev/554c22895acc -> FIXED
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: Thunderbird 38.0 → Thunderbird 41.0
Comment on attachment 8612159 [details] [diff] [review]
Proposed patch, revised after Neil's input.

[Triage Comment]

https://hg.mozilla.org/releases/comm-esr38/rev/0fcc9afa3628
http://hg.mozilla.org/releases/comm-aurora/rev/2b8ceb66d698

Not landed in comm-beta (TB 39) because parent previous patch did not land in comm-beta or mozilla-beta (at gecko 39)
Attachment #8612159 - Flags: approval-comm-esr38+
Attachment #8612159 - Flags: approval-comm-beta?
Attachment #8612159 - Flags: approval-comm-beta-
Attachment #8612159 - Flags: approval-comm-aurora?
Attachment #8612159 - Flags: approval-comm-aurora+
Thanks for landing that on TB38, TB40 and TB41.
When I requested beta approval, I had TB38 in mind. You are quite correct, that neither the M-C nor the C-C patch of bug 967494 were landed on mozilla39 or TB39.
You need to log in before you can comment on or make changes to this bug.