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

Status: RESOLVED FIXED

(Thunderbird :: Mail Window Front End, defect)

Description

[Jorg K (CEST = GMT+2)]

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.

Comment 1

[Jorg K (CEST = GMT+2)]

Addition to the description "Click on the main window": You don't need to click onto the main TB window, you can make any window current, so the message compose window loses focus. When it regains focus, the language that was set, will be the current language.

===

Damn, it's a M-C problem.

First debugging shows that that also for a recycled windows we call InitEditor and set the language to the preference language.

However, somehow the core engine doesn't acknowledge the setting until after the window is brought to the background and then to the foreground again.

Obviously pushing windows to the background and then pulling it forward again is under M-C control.

Comment 2

[Jorg K (CEST = GMT+2)]

Having second thoughts now.

The recycled window works perfectly as a Spanish window until it's pushed to the back. When brought back forward, the M-C editor gets the language from the "lang" attribute of the document's document element and this was set to English. So the M-C core editor actually works as desired. I have the impression that TB is lacking one step when recycling the window.

Comment 3

[Jorg K (CEST = GMT+2)]

Found it. Patch will come in ten minutes.

Comment 4

[Jorg K (CEST = GMT+2)]

Attachment: Proposed patch.

This is the patch from bug 1168624 (attachment 8611058 [details] [diff] [review], which received feedback+ from Neil) with one added line:
document.documentElement.setAttribute("lang",
  Services.prefs.getCharPref("spellchecker.dictionary"));

The patch without the extra line only contains cosmetic changes to fix up some clumsiness criticised by Neil (bug 1163395, comment #2, SeaMonkey port).

The extra line fixes the problem described in this bug.

Comment 5

[Jorg K (CEST = GMT+2)]

Attachment: Proposed patch (fixed overlong line)

Comment 6

[Kent James (:rkent)]

I don't believe that it makes sense to block on this, but we would consider a patch.

Comment 7

[Jorg K (CEST = GMT+2)]

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?

Comment 8

[[e-mail]]

(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 9

[Jorg K (CEST = GMT+2)]

First of all, this additional patch fixes a regression of Jan Horak's patch from bug 967494. It resets the document language back to the preference for the window to be recycled. When the window is actually recycled, M-C will retrieve the language from the document language and all is working as expected. If we don't do this, the recycled window comes back as Spanish, since this was the previous document language, but changes to English with the next pulling forward, since M-C picks up the document language again. So the fix is not optional.

As for your test. I did this:
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 in TB's preferences.
Open the draft.
The "hoy" comes back with the red underline, but as soon as I click into the subject or the body, the red underline goes away and the dictionary is set to Spanish as per the preference.
Not perfect, but tolerable. I'm happy to hear a suggestion for an improvement.

Comment 10

[Jorg K (CEST = GMT+2)]

Actually, if you reply to the draft, the message is set to Spanish immediately with no underlines.
As I said, if you edit the draft, the underlines disappear after the click.

So there is something different in the "edit" and "reply" processing. Neil, you are surely the best person to point me to the code, so I can add whatever needs to be added in the "edit" case.

If we can't fix this little idiosyncrasy now, I'd suggest to land the patch as it is now (given the very tight time frame) to fix the regression.

Comment 11

[Jorg K (CEST = GMT+2)]

The best solution would of course be to save the language with the draft and restore it when the draft is edited or replied to. This could go into the X-Mozilla-Draft-Info header. This would of course be another bug.

Comment 12

[Jorg K (CEST = GMT+2)]

Hmm, maybe I can see what you're trying to say: My fix is not optimal. It would be better to set the document language when pulling out the windows during the recycling process so that M-C can pick up the correct language straight away. But where is that spot?

Comment 13

[[e-mail]]

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?

Comment 14

[Jorg K (CEST = GMT+2)]

Attachment: Proposed patch, revised after Neil's input.

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.

Comment 15

[[e-mail]]

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 ;-)

Comment 16

[Jorg K (CEST = GMT+2)]

> 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!

Comment 17

[Jorg K (CEST = GMT+2)]

Wires crossed again. Thank you again!

Comment 18

[Magnus Melin [:mkmelin]]

Comment on attachment 8612159 [details] [diff] [review]
Proposed patch, revised after Neil's input.

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

LGTM! r=mkmelin

Comment 19

[Kent James (:rkent)]

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

Comment 20

[Jorg K (CEST = GMT+2)]

OK, land it today then ;-)

Comment 21

[Jorg K (CEST = GMT+2)]

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 ;-)

Comment 22

[Magnus Melin [:mkmelin]]

https://hg.mozilla.org/comm-central/rev/554c22895acc -> FIXED

Comment 23

[Kent James (:rkent)]

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)

Comment 24

[Jorg K (CEST = GMT+2)]

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.