Preference Composition/Spelling/Language is ignored, and changing spellcheck language in one composition window affects all open and new compositions

RESOLVED FIXED in Firefox 40

Status

()

Core
Editor
RESOLVED FIXED
3 years ago
2 years ago

People

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

Tracking

Trunk
mozilla40
x86_64
Linux
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox40 fixed)

Details

Attachments

(2 attachments, 9 obsolete attachments)

8.10 KB, patch
Jorg K (GMT+2)
: review+
Details | Diff | Splinter Review
4.75 KB, patch
Magnus Melin
: review+
Details | Diff | Splinter Review
(Reporter)

Description

3 years ago
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
(Reporter)

Updated

3 years ago
Flags: needinfo?(ehsan)
Hmm, the problem with using a pref for your needs is that it's global per application, which doesn't play well with for example using one language for the subject and another one for the body, or for using two different languages in two open compose windows.

Currently, here are the things that we look at: the contentprefs service so that we remember the language last used on a website, then we fall back to the pref, then we fall back to the language of the application.

We can add another step after looking at the contentprefs specifically for Thunderbird.  I'm not the best person to say what would be the best way to represent this information on the Thunderbird side though.  If the editor objects are separate between the subject and the field, perhaps we can add a Thunderbird-specific attribute on nsIEditor to set a dictionary name, and have that be used before looking at the global pref?
Flags: needinfo?(ehsan)
(Reporter)

Comment 2

3 years ago
Created attachment 8374000 [details] [diff] [review]
sync-dicts2.patch

I've made a rather simple fix for mail compose. It ignores obtained content pref when Editor has flag eEditorMailMask. It also save chosen dictionary to application preferences when dictionary is changed during editing (only for editor with eEditorMailMask flag).

Drawback is that content pref is queried and result is thrown away.

This also solves bug 717292.
Attachment #8374000 - Flags: review?(ehsan)
Comment on attachment 8374000 [details] [diff] [review]
sync-dicts2.patch

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

It would be nice if you could add a test case for this (using a <editor editortype="htmlmail">).  See test_bug616590.xul for example.  Since this code won't be tested in Firefox builds, it will be even more prone to breaking in the future than the usual code.

Thanks!

::: toolkit/modules/InlineSpellChecker.jsm
@@ +272,5 @@
>        return;
>      var spellchecker = this.mInlineSpellChecker.spellChecker;
>      spellchecker.SetCurrentDictionary(this.mDictionaryNames[index]);
>      this.mInlineSpellChecker.spellCheckRange(null); // causes recheck
> +    // Save chosen dictionary to preferences only when writing mail 

Nit: trailing space

@@ +274,5 @@
>      spellchecker.SetCurrentDictionary(this.mDictionaryNames[index]);
>      this.mInlineSpellChecker.spellCheckRange(null); // causes recheck
> +    // Save chosen dictionary to preferences only when writing mail 
> +    if (this.mEditor.flags & this.mEditor.eEditorMailMask) {
> +      Components.utils.import("resource://gre/modules/Services.jsm")

Nit: trailing ;
Attachment #8374000 - Flags: review?(ehsan) → review+
(Reporter)

Comment 4

3 years ago
Thanks for the review. I'm a bit puzzled how to write the test now.
(Reporter)

Comment 5

3 years ago
I don't know how to write this test with only one dictionary installed because of all fallback code. If I set content pref to unknown language (to test that htmlmail editor is not using content prefs) the spellchecking is still 'en' lang because of locale and LANG variable fallback code.
Jan, does your patch here solve the problem that in TB31, it's not possible to have two different spelling languages set for two different composition windows which are open simultaneously (and user switches between them and wants each language to be preserved)?
Assignee: nobody → jhorak
Blocks: 717292
Status: NEW → ASSIGNED
Flags: needinfo?(jhorak)
See Also: → bug 1059835
Ehsan, thanks for looking into this issue which is affecting Thunderbird composition as a consumer, causing several bugs wrt spellcheck.

This bug has a patch with review+ by you and is now stalled for adding tests per your request of comment 3. Patch author tried to write tests but is facing difficulties because inherent language fallback code design makes it hard/impossible to test for correct language against the default installation of only one language. It would be a pity for this patch to rot away.
Could you advise on the way forward? Perhaps we can get away without test?

(In reply to Jan Horak from comment #5)
> I don't know how to write this test with only one dictionary installed
> because of all fallback code. If I set content pref to unknown language (to
> test that htmlmail editor is not using content prefs) the spellchecking is
> still 'en' lang because of locale and LANG variable fallback code.
Flags: needinfo?(ehsan.akhgari)
See comment 3?  r+ means the patch can be checked in.  I said in comment 3 that it would be nice to add a test here, but since this is mailnews specific code, I'm not going to hold the patch back for the lack of having a test, especially since it seems that testing this is difficult.
Flags: needinfo?(ehsan.akhgari)
Created attachment 8487332 [details] [diff] [review]
sync-dicts2.patch by Jan Horak (nitfixed)

r=ehsan from comment 3

This is Jan Horak's original patch attachment 8374000 [details] [diff] [review], with comment 3 nits fixed, and Jan's authorship added to patch header.

Thanks a lot Jan for this patch which fixes the currently very irritating UX.
Attachment #8374000 - Attachment is obsolete: true
Attachment #8487332 - Flags: review+
Created attachment 8487345 [details] [diff] [review]
sync-dicts2.patch by Jan Horak (nitfixed) - see comment 2

Sorry, I broke the last patch, so here's the right one.

r=ehsan from comment 3

This is Jan Horak's original patch attachment 8374000 [details] [diff] [review], as described in comment 2, with comment 3 nits fixed, and Jan's authorship added to patch header.

Thanks a lot Jan for this patch which fixes several bugs around the currently very irritating UX.
Attachment #8487332 - Attachment is obsolete: true
Comment on attachment 8487345 [details] [diff] [review]
sync-dicts2.patch by Jan Horak (nitfixed) - see comment 2

(In reply to Thomas D. from comment #10)
>
> r=ehsan from comment 3
> 
> This is Jan Horak's original patch attachment 8374000 [details] [diff] [review]
> [review], as described in comment 2, with comment 3 nits fixed, and Jan's
> authorship added to patch header.
> 
> Thanks a lot Jan for this patch which fixes several bugs around the
> currently very irritating UX.
Attachment #8487345 - Flags: review+
So this is now ready for checkin.

(In reply to Thomas D. from comment #6)
> Jan, does your patch here solve the problem that in TB31, it's not possible
> to have two different spelling languages set for two different composition
> windows which are open simultaneously (and user switches between them and
> wants each language to be preserved)?

On closer reading of comment 0, yes.
Flags: needinfo?(jhorak)
Keywords: checkin-needed
Summary: Preference Composition/Spelling/Language is ignored → Preference Composition/Spelling/Language is ignored, and changing spellcheck language in one composition window affects all open and new compositions
See Also: bug 1059835
Duplicate of this bug: 1059835
Duplicate of this bug: 694556
Duplicate of this bug: 723231
The patch doesn't apply cleanly.  I will fix it and land it soon.
Component: Message Compose Window → Editor
Keywords: checkin-needed
Product: Thunderbird → Core
https://hg.mozilla.org/integration/mozilla-inbound/rev/02fbb6ada9cb
And backed out because of build bustage:

https://hg.mozilla.org/integration/mozilla-inbound/rev/2c53d5f78afd
https://tbpl.mozilla.org/php/getParsedLog.php?id=47820642&tree=Mozilla-Inbound#error0

Please fix the error and resubmit the patch.  Thanks!
And mochitest-other failures. Please verify that this is green on Try before setting checkin-needed.
https://tbpl.mozilla.org/php/getParsedLog.php?id=47822946&tree=Mozilla-Inbound
And reftest failures.
https://tbpl.mozilla.org/php/getParsedLog.php?id=47824261&tree=Mozilla-Inbound
FTR, this is the current patch from comment 17 which failed per comment 18, comment 19, and comment 20.
Jan, could you look into your patch again and see why it fails? Tia!

Then, per comment 19, we'll have to request a try-build with this first.

(In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from comment #17)
> https://hg.mozilla.org/integration/mozilla-inbound/rev/02fbb6ada9cb

(In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from comment #18)
> And backed out because of build bustage:
> 
> https://hg.mozilla.org/integration/mozilla-inbound/rev/2c53d5f78afd
> https://tbpl.mozilla.org/php/getParsedLog.php?id=47820642&tree=Mozilla-
> Inbound#error0

> /builds/slave/m-in-lx-d-00000000000000000000/build/editor/composer/nsEditorSpellCheck.cpp:745:17:
> error: 'nsIPlaintextEditor' has not been declared

> 
> Please fix the error and resubmit the patch.  Thanks!

(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #19)
> And mochitest-other failures. Please verify that this is green on Try before
> setting checkin-needed.
> https://tbpl.mozilla.org/php/getParsedLog.php?id=47822946&tree=Mozilla-
> Inbound

(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #20)
> And reftest failures.
> https://tbpl.mozilla.org/php/getParsedLog.php?id=47824261&tree=Mozilla-
> Inbound
Flags: needinfo?(jhorak)
Created attachment 8487724 [details] [diff] [review]
967494.sync-dicts2.rev2.patch (offset-fixed; causing build bustage)

FTR, this is the current patch from comment 17 which failed per comment 18, comment 19, and comment 20.
Jan, could you look into your patch again and see why it fails? Tia!

Then, per comment 19, we'll have to request a try-build with this first.

(In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from comment #17)
> https://hg.mozilla.org/integration/mozilla-inbound/rev/02fbb6ada9cb

(In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from comment #18)
> And backed out because of build bustage:
> 
> https://hg.mozilla.org/integration/mozilla-inbound/rev/2c53d5f78afd
> https://tbpl.mozilla.org/php/getParsedLog.php?id=47820642&tree=Mozilla-
> Inbound#error0

> /builds/slave/m-in-lx-d-00000000000000000000/build/editor/composer/nsEditorSpellCheck.cpp:745:17:
> error: 'nsIPlaintextEditor' has not been declared

> 
> Please fix the error and resubmit the patch.  Thanks!

(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #19)
> And mochitest-other failures. Please verify that this is green on Try before
> setting checkin-needed.
> https://tbpl.mozilla.org/php/getParsedLog.php?id=47822946&tree=Mozilla-
> Inbound

(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #20)
> And reftest failures.
> https://tbpl.mozilla.org/php/getParsedLog.php?id=47824261&tree=Mozilla-
> Inbound
Attachment #8487345 - Attachment is obsolete: true
Thomas, copying information from previous comments over and over again only makes the bug harder to read, and therefore harder to fix.  I'd appreciate if you just waited for Jan to get back to us.  Thanks!
(Reporter)

Comment 24

3 years ago
(In reply to Thomas D. from comment #6)
> Jan, does your patch here solve the problem that in TB31, it's not possible
> to have two different spelling languages set for two different composition
> windows which are open simultaneously (and user switches between them and
> wants each language to be preserved)?

I'm afraid it does not. Now each time window get focus it use nsEditorSpellCheck::UpdateCurrentDictionary to get current dictionary from content preferences related to about:blank uri in case of message body is focused or composer xul uri when subject is selected (or vice versa, I don't remember exactly now).

My patch actually skips content preferences for maileditor and in fact it fallback to dictionary which was set in Preferences/Composition/Language and if you later change dictionary by context menu in maileditor the Preferences/Composition/Language is updated.

While thinking about it this is probably what we don't want to do. To fix your problem with different dictionaries for multiple editor windows we would need to save dictionary when user pick different one to window context/object (any hint where?) and obtain it in nsEditorSpellCheck::UpdateCurrentDictionary or nsEditorSpellCheck::DictionaryFetched and use it instead of Preferences/Composition/Language somehow.

:Eshan any hint where to save user selected dictionary for specific maileditor window that it is available from subject and body elements?
Flags: needinfo?(jhorak)
(In reply to Jan Horak from comment #24)
> :Eshan

Spelling mistake!  s/sh/hs/  :-)

> any hint where to save user selected dictionary for specific
> maileditor window that it is available from subject and body elements?

I don't think we have a good place to store this information, unfortunately.  Whatever you want to do, we'd need to add specific code for it, since the existing code only looks at lang attribute on the element, the root content, and the document URI when it tries to fetch the content prefs.

Have you tried to see if we can persist the user's choice somewhere and then set the lang attribute on these nodes accordingly?  (I am not sure whether that even works in XUL documents...)

Comment 26

3 years ago
(In reply to :Ehsan Akhgari from comment #25)
> (In reply to Jan Horak from comment #24)
> 
> > any hint where to save user selected dictionary [...]?
> 
> I don't think we have a good place to store this information, unfortunately.

I don't know how that would fit within the code, but using a Content-Language
header (RFC 3282) could preserve user's choice across re-editing Drafts, and
even across replies if both mailers use it appropriately.
(In reply to Alessandro Vesely from comment #26)
> (In reply to :Ehsan Akhgari from comment #25)
> > (In reply to Jan Horak from comment #24)
> > 
> > > any hint where to save user selected dictionary [...]?
> > 
> > I don't think we have a good place to store this information, unfortunately.
> 
> I don't know how that would fit within the code, but using a Content-Language
> header (RFC 3282) could preserve user's choice across re-editing Drafts, and
> even across replies if both mailers use it appropriately.

There are no headers involved here, this is about a XUL application.
(Assignee)

Comment 28

3 years ago
When you guys have a moment, please take a look at bug 1073936. This is about the spelling not updating under certain circumstances when the dictionary is changed.
Flags: needinfo?(ehsan.akhgari)
Flags: needinfo?(ehsan.akhgari)
(Reporter)

Comment 29

3 years ago
I've made fix for this problem but it turns to be more complicated that I thought at first time.

What my solution needs in editor code:
1) some way to skip using content preferences to determine which dictionary to use. I'm using new 'eEditorDontUseContentPref' flag to do so.
2) fire event 'spellcheck-changed' with each time user change dictionary

In mail code I'm using 'spellcheck-changed' event to set 'lang' attribute of msgcomposeWindow in messengercompose.xul. This 'lang' attribute is used in nsEditorSpellCheck::UpdateCurrentDictionary by GetLang method to set current dictionary. Initially 'lang' attribute is set to spellchecker.dictionary preference.

This allows to use same dictionary for subject and message body while user can pick different dictionary in another compose window. It also works with compose window reuse.
(Reporter)

Comment 30

3 years ago
Created attachment 8527645 [details] [diff] [review]
mail part v1

attaching relevant patches.
Attachment #8487724 - Attachment is obsolete: true
Attachment #8527645 - Flags: review?(ehsan.akhgari)
(Reporter)

Comment 31

3 years ago
Created attachment 8527646 [details] [diff] [review]
editor changes
Attachment #8527646 - Flags: review?(ehsan.akhgari)
Comment on attachment 8527646 [details] [diff] [review]
editor changes

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

Apologies for my delay, I was on vacation!

The patch looks good, but I'd like to see a version that addresses my comments below.

::: dom/base/nsIContent.h
@@ +909,5 @@
>          // xml:lang has precedence over lang on HTML elements (see
>          // XHTML1 section C.7).
>          bool hasAttr = content->GetAttr(kNameSpaceID_XML, nsGkAtoms::lang,
>                                            aResult);
> +        if (!hasAttr && (content->IsHTML() || content->IsSVG() || content->IsXUL())) {

I think this is fine, but I'm not certain.  I would like to have Boris' opinion on this change.

::: editor/composer/nsEditorSpellCheck.cpp
@@ +604,5 @@
>      } else {
>        langCode.Assign(aDictionary);
>      }
>  
> +    uint32_t flags;

Nit: please initialize this to 0.

@@ +704,5 @@
>  
>    nsRefPtr<DictionaryFetcher> fetcher =
>      new DictionaryFetcher(this, aCallback, mDictionaryFetcherGroup);
> +
> +  // Try to get topmost document for embedded mail editor

Nit: s/document/document's document element/.

@@ +705,5 @@
>    nsRefPtr<DictionaryFetcher> fetcher =
>      new DictionaryFetcher(this, aCallback, mDictionaryFetcherGroup);
> +
> +  // Try to get topmost document for embedded mail editor
> +  uint32_t flags;

Nit: please initialize this to 0.

@@ +712,5 @@
> +    nsCOMPtr<nsIDocument> ownerDoc = rootContent->OwnerDoc();
> +    NS_ENSURE_TRUE(ownerDoc, NS_ERROR_FAILURE);
> +    nsIDocument* parentDoc = ownerDoc->GetParentDocument();
> +    if (parentDoc) {
> +      rootContent = do_QueryInterface(parentDoc->GetDocumentElement());

You should null check rootContent since the document is not guaranteed to have a document element.

In fact, you can probably move this code up before we check rootContent for nullness.

::: editor/nsIPlaintextEditor.idl
@@ +45,5 @@
>    const long eEditorLeftToRight         = 0x2000;
>    // when this flag is set, the editor's text content is not spell checked.
>    const long eEditorSkipSpellCheck      = 0x4000;
> +  // when this flag is set, the editor doesn't use spell checker dictionary from content pref.
> +  const long eEditorDontUseContentPref  = 0x8000;

I'm not convinced that it's worth adding a flag for this.  Let's just keep this behavior specific to Thunderbird by using eEditorMailMask.

::: toolkit/modules/InlineSpellChecker.jsm
@@ +241,2 @@
>            return function(evt) {
>              me.selectDictionary(val);

Please add a comment saying why you're dispatching the event.

@@ +241,3 @@
>            return function(evt) {
>              me.selectDictionary(val);
> +            var spellcheckChangeEvent = new menu.ownerDocument.defaultView.CustomEvent("spellcheck-changed", 

Nit: perhaps make this a bit more readable by introducing a helper variable for menu.ownerDocument.defaultView?

@@ +241,5 @@
>            return function(evt) {
>              me.selectDictionary(val);
> +            var spellcheckChangeEvent = new menu.ownerDocument.defaultView.CustomEvent("spellcheck-changed", 
> +                {"detail":{"dictionary":dictName}}
> +                );

Nit: you can just say: {detail: {dictionary: dictName}}
Attachment #8527646 - Flags: review?(ehsan.akhgari)
Attachment #8527646 - Flags: review?(bzbarsky)
Attachment #8527646 - Flags: feedback+
Comment on attachment 8527645 [details] [diff] [review]
mail part v1

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

I don't know this code, Mike said he could review this!
Attachment #8527645 - Flags: review?(ehsan.akhgari) → review?(mconley)
Comment on attachment 8527646 [details] [diff] [review]
editor changes

r=me on adding the IsXUL() bit there.
Attachment #8527646 - Flags: review?(bzbarsky) → review+
Comment hidden (obsolete)
Comment hidden (obsolete)
Comment hidden (obsolete)
Comment hidden (obsolete)
(Reporter)

Comment 39

3 years ago
(In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from comment #32)
> ::: editor/nsIPlaintextEditor.idl
> @@ +45,5 @@
> >    const long eEditorLeftToRight         = 0x2000;
> >    // when this flag is set, the editor's text content is not spell checked.
> >    const long eEditorSkipSpellCheck      = 0x4000;
> > +  // when this flag is set, the editor doesn't use spell checker dictionary from content pref.
> > +  const long eEditorDontUseContentPref  = 0x8000;
> 
> I'm not convinced that it's worth adding a flag for this.  Let's just keep
> this behavior specific to Thunderbird by using eEditorMailMask.
> 
Thanks for the review. I was checking using eEditorMailMask in mxr:
http://mxr.mozilla.org/mozilla-central/search?string=eEditorMailMask and saw that it's used in more places so I've decided to use new flag instead of adding eEditorMailMask to subject input which could break some other behaviour. Maybe you can help me to invalid my thoughts.
Comment on attachment 8527645 [details] [diff] [review]
mail part v1

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

::: mail/components/compose/content/MsgComposeCommands.js
@@ +4679,5 @@
>  
>  function InitEditor()
>  {
> +  // Set eEditorMailMask flag to avoid using content prefs for spell checker otherwise
> +  // dictionary setting in prefereces is ignored and dictionary is inconsistent in subject

sp: "preferences"

@@ +4681,5 @@
>  {
> +  // Set eEditorMailMask flag to avoid using content prefs for spell checker otherwise
> +  // dictionary setting in prefereces is ignored and dictionary is inconsistent in subject
> +  // and message body.
> +  var editorElem = document.getElementById('content-frame');

let, not var

@@ +4685,5 @@
> +  var editorElem = document.getElementById('content-frame');
> +  var contentEditor = editorElem.getEditor(editorElem.contentWindow);
> +  contentEditor.flags = contentEditor.flags | Components.interfaces.nsIPlaintextEditor.eEditorDontUseContentPref;
> +  document.getElementById('msgSubject').addEventListener("focus", function(event) {
> +    // We need to set this flag on focus event because when reusing compose window 

This event listener seems really flimsy to me. :/ I'd like to consider some other options first.

Can you explain to me in more details where and how the flags are being reset?

@@ +4721,5 @@
>    gAttachmentNotifier.init(editor.document);
> +
> +  // Set document language to preferred dictionary
> +  document.getElementById("msgcomposeWindow").setAttribute("lang", Services.prefs.getCharPref("spellchecker.dictionary"));
> +  // Listen for spellchecker changes, set document language to picked dictionary 

Nit - trailing ws
Attachment #8527645 - Flags: review?(mconley) → review-
(Reporter)

Comment 41

3 years ago
First thanks for the review.

(In reply to Mike Conley (:mconley) - Needinfo me! from comment #40)
> @@ +4685,5 @@
> > +  var editorElem = document.getElementById('content-frame');
> > +  var contentEditor = editorElem.getEditor(editorElem.contentWindow);
> > +  contentEditor.flags = contentEditor.flags | Components.interfaces.nsIPlaintextEditor.eEditorDontUseContentPref;
> > +  document.getElementById('msgSubject').addEventListener("focus", function(event) {
> > +    // We need to set this flag on focus event because when reusing compose window 
> 
> This event listener seems really flimsy to me. :/ I'd like to consider some
> other options first.
> 
> Can you explain to me in more details where and how the flags are being
> reset?
> 
I know this looks awful and I'll gladly be able to make it better, but I don't know how.

Problem is that init of the editor (ie. clear flags) is fired by PrepareEditorEvent::Run() as Timer event. This is done after javascript updateEditableFields function is finished, so I'm unable to reinstall flags at the updateEditableFields end. Stacktrace may be more helpful

#0  nsEditor::SetFlags(unsigned int) (this=, aFlags=16643) at /home/jhorak/mozilla/comm-central/mozilla/editor/libeditor/nsEditor.cpp:463
#1  nsEditor::Init(nsIDOMDocument*, nsIContent*, nsISelectionController*, unsigned int, nsAString_internal const&) (this=, aDoc=, aRoot=, aSelCon=, aFlags=16643, aValue=...) at /home/jhorak/mozilla/comm-central/mozilla/editor/libeditor/n
sEditor.cpp:220
#2  nsPlaintextEditor::Init(nsIDOMDocument*, nsIContent*, nsISelectionController*, unsigned int, nsAString_internal const&) (this=, aDoc=, aRoot=, aSelCon=, aFlags=16643, aInitialValue=...) at /home/jhorak/mozilla/comm-central/mozilla/ed
itor/libeditor/nsPlaintextEditor.cpp:136
#3  nsTextEditorState::PrepareEditor(nsAString_internal const*) (this=, aValue=) at /home/jhorak/mozilla/comm-central/mozilla/dom/html/nsTextEditorState.cpp:1289
#4  PrepareEditorEvent::Run() (this=) at /home/jhorak/mozilla/comm-central/mozilla/dom/html/nsTextEditorState.cpp:1076
#5  nsContentUtils::RemoveScriptBlocker() () at /home/jhorak/mozilla/comm-central/mozilla/dom/base/nsContentUtils.cpp:5029
#6  nsAutoScriptBlocker::~nsAutoScriptBlocker() (this=, __in_chrg=<optimized out>) at /home/jhorak/mozilla/comm-central/mozilla/dom/base/nsContentUtils.h:2336
#7  PresShell::FlushPendingNotifications(mozilla::ChangesToFlush) (this=, aFlush=...) at /home/jhorak/mozilla/comm-central/mozilla/layout/base/nsPresShell.cpp:4304
#8  nsRefreshDriver::Tick(long, mozilla::TimeStamp) (this=, aNowEpoch=1420461461196741, aNowTime=...) at /home/jhorak/mozilla/comm-central/mozilla/layout/base/nsRefreshDriver.cpp:1247
#9  mozilla::RefreshDriverTimer::TickDriver(nsRefreshDriver*, long, mozilla::TimeStamp) (driver=, jsnow=1420461461196741, now=...) at /home/jhorak/mozilla/comm-central/mozilla/layout/base/nsRefreshDriver.cpp:175
#10 mozilla::RefreshDriverTimer::Tick() (this=) at /home/jhorak/mozilla/comm-central/mozilla/layout/base/nsRefreshDriver.cpp:166
#11 mozilla::RefreshDriverTimer::TimerTick(nsITimer*, void*) (aTimer=, aClosure=) at /home/jhorak/mozilla/comm-central/mozilla/layout/base/nsRefreshDriver.cpp:192
#12 nsTimerImpl::Fire() (this=) at /home/jhorak/mozilla/comm-central/mozilla/xpcom/threads/nsTimerImpl.cpp:621
#13 nsTimerEvent::Run() (this=) at /home/jhorak/mozilla/comm-central/mozilla/xpcom/threads/nsTimerImpl.cpp:714
#14 nsThread::ProcessNextEvent(bool, bool*) (this=, aMayWait=true, aResult=) at /home/jhorak/mozilla/comm-central/mozilla/xpcom/threads/nsThread.cpp:830
Flags are reset after updateEditableFields re-enable previously disabled editor element by 
nsTextEditorState::PrepareEditor. For some reason the mEditorInitialized is set to false, so nsPlaintextEditor::Init is called with new default flags.
(Assignee)

Comment 42

2 years ago
What is the status of this fix? No movement in two months.
(Assignee)

Comment 43

2 years ago
Created attachment 8597742 [details] [diff] [review]
Editor patch (Jan Horak)

Carrying forward Ehsan's and Boris Zbarsky's review+

Refreshed the patch and addressed the issues from the review in comment #32.
This is NOT ready for checkin yet, since I need to look at the C-C part next.

In the meantime, let's see whether the patch breaks anything so far:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=f012bd744735

Oops, I just saw, there was some disagreement on whether to use eEditorMailMask or a new flag (proposed was eEditorDontUseContentPref = 0x8000;) (comment #39) That needs to be resolved, too.
Assignee: jhorak → mozilla
Attachment #8527646 - Attachment is obsolete: true
Attachment #8597742 - Flags: review+
(Assignee)

Comment 44

2 years ago
Created attachment 8598161 [details] [diff] [review]
Client patch (Jan Horak, adapted slightly by Jorg K)

I took the liberty to pick up this bug.
I addressed the review problems. The client part was rejected due to a "flimsy" event listener, that was used to set a flag on the subject field on focus.

Unless I missed something, this is actually not necessary. The flag can be set at init time and is not reset by "updateEditableFields", since this function only toggles one bit (nsIPlaintextEditorMail.eEditorReadonlyMask,
http://mxr.mozilla.org/mozilla-central/source/editor/nsIPlaintextEditor.idl#20).

If tested with two open messages, one in English, one in Spanish. I changed the body and subject language and observed that the other part follows. Also, when making the message compose window current, the language selected in this window is re-established. So as far as I can see, this is working as desired.
Attachment #8527645 - Attachment is obsolete: true
Attachment #8598161 - Flags: review?(mconley)
(Assignee)

Comment 45

2 years ago
@Ehsan:
Attachment 8597742 [details] [diff] already received a review+. I've refreshed the patch and addressed the nits.

However, there is one question open:
In your original review in comment #32 you said:
"Please add a comment saying why you're dispatching the event"
referring to this code:

-        var callback = function(me, val) {
+        var callback = function(me, val, dictName) {
           return function(evt) {
             me.selectDictionary(val);
+            // Notify change of dictionary, especially for mail client,
+            // which is otherwise not notified any more.
+            var spellcheckChangeEvent =
+              new menu.ownerDocument.defaultView.CustomEvent(
+                "spellcheck-changed", {detail: { dictionary: dictName}}
+              );
+            menu.ownerDocument.dispatchEvent(spellcheckChangeEvent);  <====
           }
         };
-        item.addEventListener("command", callback(this, i), true);
+        item.addEventListener
+          ("command", callback(this, i, sortedList[i].id), true);
       }

I guess you wanted a comment for the "dispatchEvent".

I added the comment to the best of my knowledge but I don't want to slip this patch into the codebase without your final approval.
(Assignee)

Updated

2 years ago
Attachment #8597742 - Flags: feedback?(ehsan)
Comment on attachment 8597742 [details] [diff] [review]
Editor patch (Jan Horak)

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

So, Jan seemed to want a separate eEditorDontUseContentPref flag.  If that's needed for Thunderbird, that's fine by me.  Since you removed it, and he last said on the bug that he wants it, did you check with him?

(I don't need to see a version of the patch that addresses the two nits below, unless you end up making new changes!)

::: toolkit/modules/InlineSpellChecker.jsm
@@ +241,3 @@
>            return function(evt) {
>              me.selectDictionary(val);
> +            // Notify change of dictionary, especially for mail client,

s/mail client/Thunderbird/.

@@ +242,5 @@
>              me.selectDictionary(val);
> +            // Notify change of dictionary, especially for mail client,
> +            // which is otherwise not notified any more.
> +            var spellcheckChangeEvent =
> +              new menu.ownerDocument.defaultView.CustomEvent(

Please address my comment about this.
Attachment #8597742 - Flags: feedback?(ehsan) → feedback+
(Assignee)

Comment 47

2 years ago
Created attachment 8598855 [details] [diff] [review]
Editor patch (Jan Horak, after second review by Ehsan)

Carrying forward Ehsan's review+ after addressing two review issues.
Attachment #8597742 - Attachment is obsolete: true
Attachment #8598855 - Flags: review+
Comment hidden (obsolete)
(Assignee)

Comment 49

2 years ago
Created attachment 8598911 [details] [diff] [review]
Client patch (Jan Horak, adapted slightly by Jorg K, take 2)

Now working when changing the language in the Thunderbird spelling selector. This always got the subject out of sync (see bug 717292) but is fixed now.
Ignore my (now obsolete) comment #48. It had nothing to do with losing the flag we set previously.

As far as I can see, this is ready to go.

You can check the spell language any way you like, in the Thunderbird spelling selector or in the subject or message body (via right click > Languages) and it will be changed consistently. Also, other compositions are not affected.
Attachment #8598161 - Attachment is obsolete: true
Attachment #8598911 - Flags: review?(mconley)
(Assignee)

Comment 50

2 years ago
Comment on attachment 8598911 [details] [diff] [review]
Client patch (Jan Horak, adapted slightly by Jorg K, take 2)

Magnus, can you review this? It would be nice to have this in TB38. The M-C part is already approved. The C-C has been reviewed once, I addressed the complaint (by simply removing the code in question) and added one line:
+    // Update the document language as well (needed to synchronise
+    // the subject).
+    document.getElementById("msgcomposeWindow")
+            .setAttribute("lang", event.target.value);
Attachment #8598911 - Flags: review?(mkmelin+mozilla)

Comment 51

2 years ago
Has the m-c patch been sent to try?

Comment 52

2 years ago
Comment on attachment 8598911 [details] [diff] [review]
Client patch (Jan Horak, adapted slightly by Jorg K, take 2)

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

::: mail/components/compose/content/MsgComposeCommands.js
@@ +3120,5 @@
>    var spellChecker = gSpellChecker.mInlineSpellChecker.spellChecker;
>    if (spellChecker.GetCurrentDictionary() != event.target.value)
>    {
>      spellChecker.SetCurrentDictionary(event.target.value);
> +    

trailing whitespace

@@ +4686,5 @@
> +  contentEditor.flags = contentEditor.flags |
> +    Components.interfaces.nsIPlaintextEditor.eEditorMailMask;
> +  msgSubject.editor.flags = msgSubject.editor.flags |
> +    Components.interfaces.nsIPlaintextEditor.eEditorMailMask;
> +    

trailing whitespace

@@ +4717,5 @@
> +
> +  // Set document language to preferred dictionary.
> +  document.getElementById("msgcomposeWindow")
> +          .setAttribute(
> +            "lang", Services.prefs.getCharPref("spellchecker.dictionary"));

odd wrapping 

  document.getElementById("msgcomposeWindow").setAttribute("lang",
    Services.prefs.getCharPref("spellchecker.dictionary"));

@@ +4719,5 @@
> +  document.getElementById("msgcomposeWindow")
> +          .setAttribute(
> +            "lang", Services.prefs.getCharPref("spellchecker.dictionary"));
> +  // Listen for spellchecker changes, set document language to
> +  // picked dictionary.

picked up dictionary language.
(Assignee)

Comment 53

2 years ago
(In reply to Magnus Melin from comment #51)
> Has the m-c patch been sent to try?

Of course!! Comment #43.

Thanks for the review. Is this a review+ or review- or what?

I can fix the whitespace, wrapping and comment issues right now if you want.
(Assignee)

Comment 54

2 years ago
Created attachment 8602953 [details] [diff] [review]
Client patch (Jan Horak, adapted slightly by Jorg K, take 3)

Fixed the whitespace, wrapping and comment issues.
Attachment #8598911 - Attachment is obsolete: true
Attachment #8598911 - Flags: review?(mkmelin+mozilla)
Attachment #8598911 - Flags: review?(mconley)
Attachment #8602953 - Flags: review?(mkmelin+mozilla)

Comment 55

2 years ago
didn't have time to test it yet. let's get the m-c patch landed
Keywords: checkin-needed, leave-open
(Assignee)

Updated

2 years ago
Component: Editor → Message Compose Window
Product: Core → Thunderbird
Target Milestone: --- → Thunderbird 38.0
Version: Trunk → 38
(Assignee)

Comment 56

2 years ago
I moved this from Core to Thunderbird, so we can apply our tracking flag.
The Core part is done anyway, and it's not really a Core problem, Core works fine.

Kent: This will be the last bug I'm nagging about, I promise ;-)

If you want to read the extended narrative, please click here: bug 1162396 comment #0
tracking-thunderbird38: --- → ?

Comment 57

2 years ago
I interpreted Ehsan's last statement of feedback+ in comment 46 as meaning that he wanted to see the patch again before giving review+ So I think you were hasty in carrying over the review+, as that was on a previous version. I suggest that you ask for his review again.
(Assignee)

Comment 58

2 years ago
Kent: You interpret this incorrectly and you didn't read comment #46. Let's read it together (quote):

> I don't need to see a version of the patch that addresses the two nits below, 
> unless you end up making new changes!

I didn't carry over anything hastily. The M-C part is approved and with all due respect, I won't ask for the same approval again. Ehsan is a busy man, I annoy him daily with reviews and questions, so I won't do it when it's not necessary.
Please don't move Core bugs to Thunderbird.  If there's stuff that needs to happen on top of the core changes on Thunderbird, that stuff should go to its own bug.  Same if you need to change tracking flags.
Component: Message Compose Window → Editor
Product: Thunderbird → Core
Target Milestone: Thunderbird 38.0 → ---
Version: 38 → Trunk
(Assignee)

Updated

2 years ago
Blocks: 1163086

Comment 60

2 years ago
Comment on attachment 8602953 [details] [diff] [review]
Client patch (Jan Horak, adapted slightly by Jorg K, take 3)

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

LGTM. Thx for driving this in, Jorg! r=mkmelin
Attachment #8602953 - Flags: review?(mkmelin+mozilla) → review+

Comment 61

2 years ago
mail/ part landed: https://hg.mozilla.org/comm-central/rev/0631809c3173

Updated

2 years ago
Blocks: 1163395

Comment 62

2 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/ac0f231f4342
Keywords: checkin-needed

Updated

2 years ago
Keywords: leave-open
https://hg.mozilla.org/mozilla-central/rev/ac0f231f4342
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox40: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
Cool! 1 yr 3 months is probably a pretty good turnaround time for an editor bug...
Great team work with the pings and pongs from everyone to keep this on track for swifter success!

RKent, this would be cool to have for TB38 if feasible, another eloquent proof for the added stability that comes with TB 38.

Comment 65

2 years ago
Comment on attachment 8602953 [details] [diff] [review]
Client patch (Jan Horak, adapted slightly by Jorg K, take 3)

This is a request for THUNDERBIRD_38_BRANCH on comm-beta, which will put this patch into TB 38. Not taking for now on other branches (39, 40) since the corresponding editor patch only exists currently on Mozilla 41.
Attachment #8602953 - Flags: approval-mozilla-beta?

Comment 66

2 years ago
Comment on attachment 8602953 [details] [diff] [review]
Client patch (Jan Horak, adapted slightly by Jorg K, take 3)

http://hg.mozilla.org/releases/comm-beta/rev/552a9ad0f909

a=rkent

(And ignore my previous setting of tracking-mozilla-beta I was looking for comm-beta and missed)
Attachment #8602953 - Flags: approval-mozilla-beta?

Comment 67

2 years ago
I'm removing the thunderbird-38 tracking since I don't have any way to set status-thunderbird 38, and I want this to quit showing up on my blocking bug list.
tracking-thunderbird38: ? → ---
(Assignee)

Comment 68

2 years ago
Since the landings have taken place in two bugs, here a summary:
TB38:
M-C: http://hg.mozilla.org/releases/mozilla-esr38/rev/3c181756da20 (bug 1163086 comment #3)
C-C: http://hg.mozilla.org/releases/comm-beta/rev/552a9ad0f909 (comment #66)
TB39: Not fixed.
TB40:
M-C: https://hg.mozilla.org/mozilla-central/rev/ac0f231f4342 (comment #63)
C-C: https://hg.mozilla.org/comm-central/rev/0631809c3173 (comment #61)

You've got to love release management.

Updated

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