+++ 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

Straight port from https://hg.mozilla.org/comm-central/rev/0631809c3173

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.)

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()); + }

>>+ // 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()); >> + }

Can we fix the somewhat sub-optimal code in Thunderbird as well, please ;-), or do we need a separate bug for that?

I guess, it was a silly question, since this is a SeaMonkey bug. I created bug 1168624 for TB.

Before committing this, you might want to watch bug 1168945. The change made in bug 967494 doesn't work if the window is recycled.

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

You're good to go, just add this extra line: document.documentElement.setAttribute("lang", Services.prefs.getCharPref("spellchecker.dictionary"));

Actually, no extra line, just moved this line to earlier on in the processing as per Neil's suggestion.

>>+ // 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()); >> + }

Sorry, still another problem, see bug 1169996.

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.

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.

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.