Closed
Bug 1168945
Opened 9 years ago
Closed 9 years ago
Inline spell check behaving strangely on reply, regression from bug 967494
Categories
(Thunderbird :: Mail Window Front End, defect)
Thunderbird
Mail Window Front End
Tracking
(thunderbird38+ fixed, thunderbird39 affected, thunderbird40 fixed, thunderbird41 fixed)
RESOLVED
FIXED
Thunderbird 41.0
People
(Reporter: jorgk-bmo, Assigned: jorgk-bmo)
Details
(Keywords: regression)
Attachments
(1 file, 2 obsolete files)
(deleted),
patch
|
mkmelin
:
review+
neil
:
feedback+
rkent
:
approval-comm-aurora+
rkent
:
approval-comm-beta-
rkent
:
approval-comm-esr38+
|
Details | Diff | Splinter Review |
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•9 years ago
|
Flags: needinfo?(neil)
Assignee | ||
Updated•9 years ago
|
Component: Message Compose Window → Editor
Product: Thunderbird → Core
Comment hidden (obsolete) |
Assignee | ||
Updated•9 years ago
|
Keywords: regression
Assignee | ||
Updated•9 years ago
|
Component: Editor → Mail Window Front End
Product: Core → Thunderbird
Target Milestone: --- → Thunderbird 38.0
Comment hidden (obsolete) |
Comment hidden (obsolete) |
Assignee | ||
Updated•9 years ago
|
Assignee: nobody → mozilla
Status: NEW → ASSIGNED
Comment hidden (obsolete) |
Comment 6•9 years ago
|
||
I don't believe that it makes sense to block on this, but we would consider a patch.
tracking-thunderbird_esr38:
--- → 39+
Assignee | ||
Comment 7•9 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•9 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 13•9 years ago
|
||
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•9 years ago
|
||
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 15•9 years ago
|
||
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•9 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•9 years ago
|
||
Wires crossed again. Thank you again!
Comment 18•9 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•9 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•9 years ago
|
||
OK, land it today then ;-)
Assignee | ||
Comment 21•9 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•9 years ago
|
Keywords: checkin-needed
Comment 22•9 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: Thunderbird 38.0 → Thunderbird 41.0
Comment 23•9 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•9 years ago
|
Assignee | ||
Comment 24•9 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.
Description
•