Closed
Bug 1163395
Opened 9 years ago
Closed 9 years ago
Port Bug 967494 to SeaMonkey (Preference Composition/Spelling/Language is ignored, and changing spellcheck language in one composition window affects all open and new compositions)
Categories
(SeaMonkey :: MailNews: Composition, defect)
SeaMonkey
MailNews: Composition
Tracking
(seamonkey2.35 fixed, seamonkey2.36 wontfix, seamonkey2.37 fixed, seamonkey2.38 fixed)
RESOLVED
FIXED
seamonkey2.38
People
(Reporter: philip.chee, Assigned: philip.chee)
References
Details
Attachments
(1 file, 2 obsolete files)
(deleted),
patch
|
neil
:
review+
iannbugzilla
:
approval-comm-aurora+
iannbugzilla
:
approval-comm-release+
|
Details | Diff | Splinter Review |
+++ This bug was initially created as a clone of Bug #967494 +++
> I've hit inconsistency with dictionary settings in Thunderbird. When you
> have multiple dictionaries, start composing message and use RMB to change
> your dictionary, your new dictionary setting is remembered and whenever
> you compose another message it is out of sync with
> Composition/Spelling/Language.
> Its due to nsEditorSpellCheck::DictionaryFetched start to look for
> dictionary from content prefs associated with document URI (about:blank
> in case of Thunderbird) and only when it fails it continues with
> spellchecker.dictionary. See:
> http://mxr.mozilla.org/comm-central/source/mozilla/editor/composer/src/nsEditorSpellCheck.cpp#713
> That works fine for Firefox, where we have a lot of content prefs but not
> for Thunderbird where's only "about:blank" page.
> Question is how to solve it. We could probably want to sync settings that
> when we change language in composer the spellchecker.dictionary pref also
> change. Composer and also Subject [1] input should use only
> spellchecker.dictionary preference for dictionary.
> I can start to work on it, but I'd like to get some feedback to avoid
> wasting my time.
> [1] https://bugzilla.mozilla.org/show_bug.cgi?id=717292
Assignee | ||
Comment 1•9 years ago
|
||
Straight port from https://hg.mozilla.org/comm-central/rev/0631809c3173
Comment 2•9 years ago
|
||
Comment on attachment 8606739 [details] [diff] [review]
Patch v1.0 Just Port it.
>+ // Stop listening to changes in the spell check dictionary.
>+ document.removeEventListener("spellcheck-changed", updateDocumentLanguage);
Why is this here and not in ComposeUnload/ComposeLoad?
> function InitEditor(editor)
> {
>+ // Set the eEditorMailMask flag to avoid using content prefs for the spell
>+ // checker, otherwise the dictionary setting in preferences is ignored and
>+ // the dictionary is inconsistent between the subject and message body.
>+ var editorElement = document.getElementById("content-frame");
>+ var contentEditor = editorElement.getEditor(editorElement.contentWindow);
Same editor as the editor parameter to InitEditor?
>+ msgSubject.editor.flags |= eEditorMailMask;
GetMsgSubjectElement()
>+ // Set the document language to preferred dictionary.
>+ document.getElementById("msgcomposeWindow")
>+ .setAttribute("lang",
>+ Services.prefs.getCharPref("spellchecker.dictionary"));
Why the whole document and not just the subject? (But document.documentElement works better.)
Comment 3•9 years ago
|
||
Neil: This code was ported from the fix to bug 967494 by Jan Horak, attachment 8602953 [details] [diff] [review].
There was no movement on the bug for months, so I took the liberty to do what necessary to get it landed.
It seems to work fine. I'm sorry if some of it is clumsy and/or suboptimal.
As for the last question: I think the idea is to make sure that subject and body have the same language. If you look at the m-c patch, attachment 8598855 [details] [diff] [review], you can see that the language is retrieved from the document:
+ if (flags & nsIPlaintextEditor::eEditorMailMask) {
+ nsCOMPtr<nsIDocument> ownerDoc = rootContent->OwnerDoc();
+ NS_ENSURE_TRUE(ownerDoc, NS_ERROR_FAILURE);
+ nsIDocument* parentDoc = ownerDoc->GetParentDocument();
+ if (parentDoc) {
+ rootContent = do_QueryInterface(parentDoc->GetDocumentElement());
+ }
Assignee | ||
Comment 4•9 years ago
|
||
>>+ // Stop listening to changes in the spell check dictionary.
>>+ document.removeEventListener("spellcheck-changed", updateDocumentLanguage);
> Why is this here and not in ComposeUnload/ComposeLoad?
I think this is because this is where the InlineSpellChecker is enabled/disabled so it's logical to group any other spellchecking startup/shutdown functions here as well.
>> function InitEditor(editor)
>> {
>>+ // Set the eEditorMailMask flag to avoid using content prefs for the spell
>>+ // checker, otherwise the dictionary setting in preferences is ignored and
>>+ // the dictionary is inconsistent between the subject and message body.
>>+ var editorElement = document.getElementById("content-frame");
>>+ var contentEditor = editorElement.getEditor(editorElement.contentWindow);
> Same editor as the editor parameter to InitEditor?
Yes it is. I've simplified the code since we already have the current editor.
>>+ msgSubject.editor.flags |= eEditorMailMask;
> GetMsgSubjectElement()
Fixed.
> (But document.documentElement works better.)
I've rewritten the patch to use document.documentElement when possible.
>>+ // Set the document language to preferred dictionary.
>>+ document.getElementById("msgcomposeWindow")
>>+ .setAttribute("lang",
>>+ Services.prefs.getCharPref("spellchecker.dictionary"));
> Why the whole document and not just the subject?
(Accoding to Jorg K from comment #3)
>> As for the last question: I think the idea is to make sure that subject and
>> body have the same language. If you look at the m-c patch, attachment
>> 8598855 [details] [diff] [review], you can see that the language is
>> retrieved from the document:
>>
>> + if (flags & nsIPlaintextEditor::eEditorMailMask) {
>> + nsCOMPtr<nsIDocument> ownerDoc = rootContent->OwnerDoc();
>> + NS_ENSURE_TRUE(ownerDoc, NS_ERROR_FAILURE);
>> + nsIDocument* parentDoc = ownerDoc->GetParentDocument();
>> + if (parentDoc) {
>> + rootContent = do_QueryInterface(parentDoc->GetDocumentElement());
>> + }
Attachment #8606739 -
Attachment is obsolete: true
Attachment #8606739 -
Flags: review?(neil)
Attachment #8610841 -
Flags: review?(neil)
Comment 5•9 years ago
|
||
Can we fix the somewhat sub-optimal code in Thunderbird as well, please ;-), or do we need a separate bug for that?
Comment 6•9 years ago
|
||
I guess, it was a silly question, since this is a SeaMonkey bug. I created bug 1168624 for TB.
Comment 7•9 years ago
|
||
Before committing this, you might want to watch bug 1168945.
The change made in bug 967494 doesn't work if the window is recycled.
Assignee | ||
Comment 8•9 years ago
|
||
Comment on attachment 8610841 [details] [diff] [review]
Patch v2.0 address review issues.
(In reply to Jorg K from comment #7)
> Before committing this, you might want to watch bug 1168945.
> The change made in bug 967494 doesn't work if the window is recycled.
Thanks
Attachment #8610841 -
Flags: review?(neil)
Comment 9•9 years ago
|
||
You're good to go, just add this extra line:
document.documentElement.setAttribute("lang", Services.prefs.getCharPref("spellchecker.dictionary"));
Comment 10•9 years ago
|
||
Actually, no extra line, just moved this line to earlier on in the processing as per Neil's suggestion.
Assignee | ||
Comment 11•9 years ago
|
||
>>+ // Stop listening to changes in the spell check dictionary.
>>+ document.removeEventListener("spellcheck-changed", updateDocumentLanguage);
> Why is this here and not in ComposeUnload/ComposeLoad?
I think this is because this is where the InlineSpellChecker is enabled/disabled so it's logical to group any other spellchecking startup/shutdown functions here as well.
>> function InitEditor(editor)
>> {
>>+ // Set the eEditorMailMask flag to avoid using content prefs for the spell
>>+ // checker, otherwise the dictionary setting in preferences is ignored and
>>+ // the dictionary is inconsistent between the subject and message body.
>>+ var editorElement = document.getElementById("content-frame");
>>+ var contentEditor = editorElement.getEditor(editorElement.contentWindow);
> Same editor as the editor parameter to InitEditor?
Yes it is. I've simplified the code since we already have the current editor.
>>+ msgSubject.editor.flags |= eEditorMailMask;
> GetMsgSubjectElement()
Fixed.
> (But document.documentElement works better.)
I've rewritten the patch to use document.documentElement when possible.
>>+ // Set the document language to preferred dictionary.
>>+ document.getElementById("msgcomposeWindow")
>>+ .setAttribute("lang",
>>+ Services.prefs.getCharPref("spellchecker.dictionary"));
> Why the whole document and not just the subject?
(Accoding to Jorg K from comment #3)
>> As for the last question: I think the idea is to make sure that subject and
>> body have the same language. If you look at the m-c patch, attachment
>> 8598855 [details] [diff] [review], you can see that the language is
>> retrieved from the document:
>>
>> + if (flags & nsIPlaintextEditor::eEditorMailMask) {
>> + nsCOMPtr<nsIDocument> ownerDoc = rootContent->OwnerDoc();
>> + NS_ENSURE_TRUE(ownerDoc, NS_ERROR_FAILURE);
>> + nsIDocument* parentDoc = ownerDoc->GetParentDocument();
>> + if (parentDoc) {
>> + rootContent = do_QueryInterface(parentDoc->GetDocumentElement());
>> + }
Attachment #8610841 -
Attachment is obsolete: true
Attachment #8612870 -
Flags: review?(neil)
Comment 12•9 years ago
|
||
Sorry, still another problem, see bug 1169996.
Updated•9 years ago
|
Attachment #8612870 -
Flags: review?(neil) → review+
Comment 13•9 years ago
|
||
I don't know whether SeaMonkey has recycled windows like TB. In any case, the fix to bug 1169996 is in M-C, so SeaMonkey will automatically get it.
Assignee | ||
Comment 14•9 years ago
|
||
status-firefox40:
affected → ---
status-seamonkey2.37:
--- → affected
status-seamonkey2.38:
--- → fixed
Target Milestone: --- → seamonkey2.38
Assignee | ||
Comment 15•9 years ago
|
||
Comment on attachment 8612870 [details] [diff] [review]
Patch v2a Set the document language before enabling the inline spell check
[Approval Request Comment]
Regression caused by (bug #): has always been broken or something
User impact if declined: Preference Composition/Spelling/Language is ignored, and changing spellcheck language in one composition window affects all open and new compositions
Testing completed (on m-c, etc.): Mozilla central patch landed on Gecko40
Risk to taking this patch (and alternatives if risky): low risk bug fix.
String changes made by this patch: none.
Attachment #8612870 -
Flags: approval-comm-aurora?
Assignee | ||
Comment 16•9 years ago
|
||
> Comment on attachment 8612870 [details] [diff] [review]
> Patch v2a Set the document language before enabling the inline spell check
>
> [Approval Request Comment]
> Regression caused by (bug #): has always been broken or something
> User impact if declined: Preference Composition/Spelling/Language is
> ignored, and changing spellcheck language in one composition window affects
> all open and new compositions
> Testing completed (on m-c, etc.): Mozilla central patch landed on Gecko40
> Risk to taking this patch (and alternatives if risky): low risk bug fix.
> String changes made by this patch: none.
Flags: needinfo?(iann_bugzilla)
Flags: needinfo?(iann_bugzilla)
Attachment #8612870 -
Flags: approval-comm-aurora? → approval-comm-aurora+
Assignee | ||
Comment 17•9 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Assignee | ||
Updated•9 years ago
|
Blocks: SM2.35-Uplift
Assignee | ||
Comment 18•9 years ago
|
||
Comment on attachment 8612870 [details] [diff] [review]
Patch v2a Set the document language before enabling the inline spell check
[Approval Request Comment]
Regression caused by (bug #): unknown
User impact if declined: Preference Composition/Spelling/Language is ignored, and changing spellcheck language in one composition window affects all open and new compositions
Testing completed (on m-c, etc.): mozilla-central, thunderbird.
Risk to taking this patch (and alternatives if risky):
String changes made by this patch:
Note Bug 967494 has two parts one in mozilla-* and another in comm-* So both should land in SM 2.35 or neither.
Attachment #8612870 -
Flags: approval-comm-release?
Attachment #8612870 -
Flags: approval-comm-release? → approval-comm-release+
Assignee | ||
Comment 19•9 years ago
|
||
status-seamonkey2.35:
--- → fixed
status-seamonkey2.36:
--- → wontfix
You need to log in
before you can comment on or make changes to this bug.
Description
•