The default bug view has changed. See this FAQ.

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

RESOLVED FIXED in Thunderbird 41.0

Status

Thunderbird
Mail Window Front End
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: Jorg K (GMT+1), Assigned: Jorg K (GMT+1))

Tracking

({regression})

Trunk
Thunderbird 41.0
regression

Thunderbird Tracking Flags

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

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

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

Updated

2 years ago
Flags: needinfo?(neil)
(Assignee)

Updated

2 years ago
Component: Message Compose Window → Editor
Product: Thunderbird → Core
Comment hidden (obsolete)
(Assignee)

Updated

2 years ago
Keywords: regression
(Assignee)

Updated

2 years ago
Component: Editor → Mail Window Front End
Product: Core → Thunderbird
Target Milestone: --- → Thunderbird 38.0
Comment hidden (obsolete)
Comment hidden (obsolete)
(Assignee)

Updated

2 years ago
Assignee: nobody → mozilla
Status: NEW → ASSIGNED
Comment hidden (obsolete)

Comment 6

2 years ago
I don't believe that it makes sense to block on this, but we would consider a patch.
tracking-thunderbird38: ? → -
tracking-thunderbird_esr38: --- → 39+
(Assignee)

Comment 7

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

2 years ago
(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 hidden (obsolete)
Comment hidden (obsolete)
Comment hidden (obsolete)
Comment hidden (obsolete)
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?
(Assignee)

Comment 14

2 years ago
Created attachment 8612159 [details] [diff] [review]
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.
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+
(Assignee)

Comment 16

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

Comment 17

2 years ago
Wires crossed again. Thank you again!

Comment 18

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

Comment 19

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

Comment 20

2 years ago
OK, land it today then ;-)
(Assignee)

Comment 21

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

Updated

2 years ago
Keywords: checkin-needed

Comment 22

2 years ago
https://hg.mozilla.org/comm-central/rev/554c22895acc -> FIXED
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-thunderbird41: affected → fixed
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: Thunderbird 38.0 → Thunderbird 41.0

Comment 23

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

Updated

2 years ago
status-thunderbird38: affected → fixed
status-thunderbird40: affected → fixed
tracking-thunderbird38: - → +
tracking-thunderbird_esr38: 39+ → ---
(Assignee)

Comment 24

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