Changing the spell check language in the message subject of a recycled message via right-click changes the composition language preference

RESOLVED FIXED in Firefox 40

Status

()

Core
Editor
RESOLVED FIXED
2 years ago
2 years ago

People

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

Tracking

Trunk
mozilla41
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox40 fixed, firefox41 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

2 years ago
Since the completion of bug 967494, the composition language preference should not change when the language is changed via the right-click menu in subject or message body.

This works in the message body but not the message subject, if the message is recycled.

What happens is the the eEditorMailMask (0x40) flag gets lost for the subject on a recycled message.
https://dxr.mozilla.org/comm-central/source/mozilla/editor/composer/nsEditorSpellCheck.cpp#609

New message: flags = 0x143.
Recycled message: flags = 0x103.
(Assignee)

Comment 1

2 years ago
This problem was already known, see bug 967494, comment #41 and Jan's original patch (attachment 8527645 [details] [diff] [review]) where he said:
+    // We need to set this flag on focus event because when reusing compose window 
+    // flags get reset by updateEditableFields when changing 'disabled' attribute.
+    // Reset of flags is done after javascript code is finished so there's no other 
+    // way to set the eEditorDontUseContentPref flag during opening of reused compose window.

I have to take the blame for not fixing the issue before presenting his modified patch for review :-(

Back then I couldn't reproduce the problem and so I removed Jan's solution which had been rejected in the review.

I'm look for another fix.
(Assignee)

Comment 2

2 years ago
Damn those typos:
I'll look for another fix
I'm looking for another fix.
(Assignee)

Comment 3

2 years ago
This is damn complicated, not easy to see where the eEditorMailMask (0x40) gets lost.

Some debugging shows, that for a new composition window, when first clicking into the subject field, this gets called:
nsEditor::SetFlags: 0x143

For a recycled composition window, when first clicking into the subject field, this gets called:
nsEditor::SetFlags: 0x103 (eEditorMailMask missing).

This occurs although the flag is set every time in InitEditor (MsgComposeCommands.js).

In Jan's original patch, he simply added the eEditorMailMask every time the subject field gained focus. This approach was rejected in a review, and I foolishly removed it without any replacement.
(Assignee)

Updated

2 years ago
Assignee: nobody → mozilla
Component: Message Compose Window → Editor
Product: Thunderbird → Core
(Assignee)

Comment 4

2 years ago
Created attachment 8613328 [details] [diff] [review]
Patch to preserve flags.

This is a little urgent as we might want to include the fix in TB38.
Attachment #8613328 - Flags: review?(ehsan)
(Assignee)

Comment 5

2 years ago
I don't think this change will affect anything, but here's a try run for completeness:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=6cd41a67c0e4
Comment on attachment 8613328 [details] [diff] [review]
Patch to preserve flags.

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

::: dom/html/nsTextEditorState.cpp
@@ +1299,5 @@
>  
>      newEditor = mEditor; // just pretend that we have a new editor!
> +
> +    // Don't lose application flags in the process.
> +    uint32_t f;

Nit: please give this variable a better name, such as originalFlags.  Also, please initialize it to 0.
Attachment #8613328 - Flags: review?(ehsan) → review+
(Assignee)

Comment 7

2 years ago
Created attachment 8613537 [details] [diff] [review]
Patch to preserve flags (nit fixed)

Carrying over Ehsan's review+
Attachment #8613328 - Attachment is obsolete: true
Attachment #8613537 - Flags: review+
(Assignee)

Updated

2 years ago
Status: NEW → ASSIGNED
Keywords: checkin-needed
OS: Unspecified → All
Hardware: Unspecified → All
(Assignee)

Updated

2 years ago
Blocks: 1170181

Comment 8

2 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/4f371fcc09cc
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/4f371fcc09cc
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox41: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
(Assignee)

Comment 10

2 years ago
Comment on attachment 8613537 [details] [diff] [review]
Patch to preserve flags (nit fixed)

Approval Request Comment
[Feature/regressing bug #]: 967494
[User impact if declined]:
This patch represents a little tweak/addition to something what was implemented in bug 967494. It would be good if it could be landed on mozilla40, so Thunderbird 40 can take advantage, since bug 967494 was landed on mozilla40 (bug 967494 comment #63).

[Risks and why]: 
No risk for Firefox. This code is only here for Thunderbird.

[String/UUID change made/needed]:
None.
Flags: needinfo?(lmandel)
Attachment #8613537 - Flags: approval-mozilla-aurora?
status-firefox40: --- → affected
Comment on attachment 8613537 [details] [diff] [review]
Patch to preserve flags (nit fixed)

Please don't n-i the release manager without a good reason, we are scanning the uplifts.
Anyway, taking it as it seems low risk.
Flags: needinfo?(lmandel)
Attachment #8613537 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
(Assignee)

Comment 12

2 years ago
Sorry, last time (bug 1140617, comment #53) I waited more than a week ;-(
Thanks for taking it!
https://hg.mozilla.org/releases/mozilla-aurora/rev/8557ef112c11
status-firefox40: affected → fixed

Comment 14

2 years ago
http://hg.mozilla.org/releases/mozilla-esr38/rev/8818cfba3036 for THUNDERBIRD_38_VERBRANCH (See bug 1170181)

Updated

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