Closed Bug 1169996 Opened 5 years ago Closed 4 years ago

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

Categories

(Core :: Editor, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla41
Tracking Status
firefox40 --- fixed
firefox41 --- fixed

People

(Reporter: jorgk, Assigned: jorgk)

References

Details

Attachments

(1 file, 1 obsolete file)

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.
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.
Damn those typos:
I'll look for another fix
I'm looking for another fix.
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: nobody → mozilla
Component: Message Compose Window → Editor
Product: Thunderbird → Core
Attached patch Patch to preserve flags. (obsolete) — Splinter Review
This is a little urgent as we might want to include the fix in TB38.
Attachment #8613328 - Flags: review?(ehsan)
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+
Carrying over Ehsan's review+
Attachment #8613328 - Attachment is obsolete: true
Attachment #8613537 - Flags: review+
Status: NEW → ASSIGNED
Keywords: checkin-needed
OS: Unspecified → All
Hardware: Unspecified → All
Blocks: 1170181
https://hg.mozilla.org/mozilla-central/rev/4f371fcc09cc
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
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?
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+
Sorry, last time (bug 1140617, comment #53) I waited more than a week ;-(
Thanks for taking it!
You need to log in before you can comment on or make changes to this bug.