Closed Bug 717292 Opened 12 years ago Closed 9 years ago

Spell check language setting for subject and body not synchronized, but temporarily appears so when changing language and depending on focus (confusing ux)

Categories

(Thunderbird :: Message Compose Window, defect)

9 Branch
x86_64
Windows 7
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: cwesterhof, Unassigned)

References

Details

(Keywords: ux-mode-error, ux-userfeedback, Whiteboard: [STR comment 22] [Fixed by bug 967494])

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:9.0.1) Gecko/20100101 Firefox/9.0.1
Build ID: 20111220165912

Steps to reproduce:

I changed the spell check language for the subject.


Actual results:

Only the language for the subject changed.


Expected results:

The language for the message should be changed also.
(In reply to Cecil Westerhof from comment #0)
Here too I cannot change the dictionary unless I first click into the body section. Once I change the dictionary though, It will spell check the subject part.
User Agent: Mozilla/5.0 (X11; Linux i686; rv:9.0) Gecko/20111222 Thunderbird/9.0.1
Application Build ID: 20111222141531
(In reply to Hashem Masoud from comment #1)
> Here too I cannot change the dictionary unless I first click into the body
> section. Once I change the dictionary though, It will spell check the
> subject part.

That is not my problem. I can change the language for the subject, but that does not change the language for the body. When I change the language for the body, that does not change the language for the subject.

I am using Thunderbird 9.0.1 on Windows.
Cecil do you have extension installed in your Thunderbird ?
Extensions:
British English Dictionary 1.19.1
Provider for Google Calendar 0.9
Test pilot for Thunderbird 1.3.5

Disabled extensions are:
AVG e-mail scanner 12.0.0.1892
Lightning 1.1.1

The following plug-ins:
Adobe Acrobat 10.1.2.45
Java Deployment Toolkit 6.0.300.12
Java (TM) Platform SE 6 U 30 6.0.300.12
Microsoft Office 2010 14.0.4761.1000
Shockwave Flash 11.1.102.55
Shockwave for Director 11.6.3.633
Silverlight Plugin 4.0.60831.0
Whiteboard: dupme?
I can reproduce this one. This is because nsEditorSpellCheck::UpdateCurrentDictionary is called during onfocus signal on the message body and the subsequest call of LastDictionary::FetchLastDictionary use ContentPrefService to query for directory which was picked last time when user changed dictionary of message body input. 

This works fine for Firefox where you can choose to have different dictionary for each domain and Firefox still remembers which dictionary to use on which domain.

This is confusing in Thunderbird, where setting is inconsistent between Subject, Body and Settings. It would be nice to disable lookup to Content Pref Service for Thunderbird (unless you don't want to start to remember what dictionary use for specific recipient for example).
Possible dup of bug 378434 ?
Huh, a little bit complicated. I see one way:
- add UseContentPrefService(bool = true) to nsIEditor, implement in nsIEditor
- add UseContentPrefService to nsIInlineSpellChecker, implement in mozInlineSpellChecker
- add UseContentPrefService to nsIEditorSpellCheck, implement in nsEditorSpellCheck
- in nsEditorSpellCheck::UpdateCurrentDictionary() use content prefs only when mUseContentPrefService is true.
- finally in nsMsgCompose::InitEditor add m_editor->UseContentPrefService(false).

Is it worth doing the patch or my chances to get review+ on this is zero?
(In reply to Cecil Westerhof from comment #0)
> Steps to reproduce:
> I changed the spell check language for the subject.

How do you change language for subject? I can't do that on TB12.
(In reply to jhorak from comment #9)
> Huh, a little bit complicated. I see one way:
...
> Is it worth doing the patch or my chances to get review+ on this is zero?

Perhaps if you briefly explain the resulting behaviour, and compare to current behaviour, it will be easier to judge...
More precisely, detailed steps to reproduce, actual and expected behaviour would be helpful.
Summary: I have to change the language several times → Spell check for subject and body: I have to change the language several times
When I right click on the subject I get a popup menu where I can select the language.
I found something interesting. When I change the language on the body, the subject is spell-checked in the same language. But when I then click on the subject (because I want to change it) it reverts to being spell-checked in the old language.
Component: General → Untriaged
Blocks: 3459
(In reply to Cecil Westerhof from comment #14)
> I found something interesting. When I change the language on the body, the
> subject is spell-checked in the same language. But when I then click on the
> subject (because I want to change it) it reverts to being spell-checked in
> the old language.

Indeed, that's exactly what I see, as describe in bug 378434, comment 24.

(In reply to jhorak from comment #7)
> Possible dup of bug 378434 ?
Yes, bug 378434 and this bug are the same.

(In reply to jhorak from comment #9)
> Is it worth doing the patch or my chances to get review+ on this is zero?
Magnus, could you comment on comment 9 or set needinfo to an appropriate person?

Jhorak, sorry that you didn't get feedback on this so far. I've set needinfo flag for that.
As you seem to be the only forthcoming person who has some insight on this per your comment 6 and comment 9, may I ask you what you'd prefer:

1) dupe bug 378434 to this bug so that you can just continue your valuable analysis and work here
OR
2) dupe this bug to bug 378434, transfer your comment 6 and 9 and continue there

duping to the older bug would be standard procedure, but both bugs aren't ideal and we'll happily make exceptions if it helps to get moving.
Flags: needinfo?(mkmelin+mozilla)
Flags: needinfo?(jhorak)
Summary: Spell check for subject and body: I have to change the language several times → Spell check language setting for subject and body not synchronized: I have to change language several times
Whiteboard: dupme? → [dupme to 378434 or vice versa]
See Also: → 714439
Looks ok to me, but better ask someone that would possibly review it (like Ehsan)? See comment 6 for description, comment 9 for the plan that wants feedback.
Flags: needinfo?(mkmelin+mozilla) → needinfo?(ehsan)
I'm not sure if I understand comment 6 quite well.  Why is it that the content prefs service usage is OK in Firefox but not in Thunderbird?  Note that the Thunderbird UI code can probably set the right language on every spell checkable field based on the user preferences.
Flags: needinfo?(ehsan)
See Also: → 378434
Whiteboard: [dupme to 378434 or vice versa] → [dupme to 378434 or vice versa, see comment 15]
(In reply to :Ehsan Akhgari (needinfo? me!) from comment #17)
> I'm not sure if I understand comment 6 quite well.  Why is it that the
> content prefs service usage is OK in Firefox but not in Thunderbird?  Note
> that the Thunderbird UI code can probably set the right language on every
> spell checkable field based on the user preferences.

Problem lies there:
http://mxr.mozilla.org/mozilla-central/source/editor/composer/src/nsEditorSpellCheck.cpp#681

The nsEditorSpellCheck::UpdateCurrentDictionary is called when Subject or Body get focus.
If Subject gets focus then editor's rootElement with url chrome://messenger/content/messengercompose/messengercompose.xul is used to lookup last used dictionary from prefs. When Body has focus then html editor's document with url about:blank is used to lookup last used dictionary from prefs.

It is possible to set specific/different dictionaries for each url by RMB on Subject or Body.

To fix that we need to disable lookup for htmlEditor (ie. get/set dictionary for about:blank). Since it is deep in nsEditor. In comment 9 I suggested to add UseContentPrefService to nsIEditor and propagate it down to nsEditorSpellCheck. Since nsEditorSpellCheck is hidden from creator of nsEditor I don't know how to make it more directly.
Flags: needinfo?(jhorak)
Hmm, ok thanks for the explanation.  I think the root cause of this problem is that it doesn't really make sense to look up the dictionary for about:blank.  Do you agree?

If yes, perhaps a better fix would be to use the parent document's location in case we find an about:blank.  I think that should fix this bug.  Can you try that please?

Thanks!
See Also: 378434
FTR, here are STR:

1) add English words and some words from another language e.g. German to both subject and body
2) from context menu of either body or subject line, change language
3) with "spell check as you type" enabled, observe spell check error underlining in both body and subject
4) place cursor alternately in subject and body, observe spell checking markings
5) verify language setting in subject and body

Actual result:

3) immediately after changing language, it *looks* as if the other part (body vs. subject) also changes to the same language, as underlinings flip accordingly; but when you actually click into the other part (body vs. subject)...
4) ...you'll see that the underlinings flip back to the language previously active in that part, and...
5) ...language setting of the other part (body vs. subject) is NOT updated/synchronized in the menus.
Current UX is really confusing.

Expected result:

As we don't allow different sections of body to have different language settings (yet?), I assume we want: subject and body = single spell check area.

When user changes language or adds ignored words in either subject or body, ensure synchronizing/updating the spell checking incl. markers in the other part immediately.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Summary: Spell check language setting for subject and body not synchronized: I have to change language several times → Spell check language setting for subject and body not synchronized, but temporarily appears so when changing language and depending on focus (confusing ux)
Whiteboard: [dupme to 378434 or vice versa, see comment 15]
Whiteboard: [STR comment 22]
See Also: → 433181
(In reply to :Ehsan Akhgari (needinfo? me!) from comment #19)
> Hmm, ok thanks for the explanation.  I think the root cause of this problem
> is that it doesn't really make sense to look up the dictionary for
> about:blank.  Do you agree?
> 
> If yes, perhaps a better fix would be to use the parent document's location
> in case we find an about:blank.  I think that should fix this bug.  Can you
> try that please?
> 
> Thanks!

Unfortunatelly it seems to be more complicated. The nsEditorSpellCheck::UpdateCurrentDictionary is called with different mEditor instances. Subject is a textbox element and message body is editor element (see http://mxr.mozilla.org/comm-central/source/mail/components/compose/content/messengercompose.xul#974). Both have different root elements when obtained by GetRootElement. I don't know how to get 'chrome://messenger/content/messengercompose/messengercompose.xul' in case of editor element.
(In reply to comment #23)
> (In reply to :Ehsan Akhgari (needinfo? me!) from comment #19)
> > Hmm, ok thanks for the explanation.  I think the root cause of this problem
> > is that it doesn't really make sense to look up the dictionary for
> > about:blank.  Do you agree?
> > 
> > If yes, perhaps a better fix would be to use the parent document's location
> > in case we find an about:blank.  I think that should fix this bug.  Can you
> > try that please?
> > 
> > Thanks!
> 
> Unfortunatelly it seems to be more complicated. The
> nsEditorSpellCheck::UpdateCurrentDictionary is called with different mEditor
> instances. Subject is a textbox element and message body is editor element (see
> http://mxr.mozilla.org/comm-central/source/mail/components/compose/content/messengercompose.xul#974).
> Both have different root elements when obtained by GetRootElement. I don't know
> how to get 'chrome://messenger/content/messengercompose/messengercompose.xul'
> in case of editor element.

Isn't the editor element just an iframe of some sort?  You should be able to get the parent document by looking at the OwnerDoc() of the parent of the document root, IINM.
Perhaps the comment should go to a different bug ... ;-)

I recently re-installed TB and I'm now at version 24.0 on Windows. Before I used 17.0.x.

Now the spelling is completely broken. Despite having "Check Spelling" checked and a language selected, no spell check is performed. This seems to be happening at random. I need to quit TB and start it again before the spell check works again. So what is going on here. Coping with the wrong dictionary being used was bearable, but having to restart for the spell check to work is just fatal. It's worse than that. How do you know the spell check doesn't work? You need to explicitly misspell a word and then you can see. In the currently broken state no spelling errors can mean:
1) there aren't any errors - cool to send
2) spell check ain't working - no good to send!!
OK, this is a new bug in TB 24 reported as bug 917027 and bug 919184.
@jhorak, did you check comment 24? This problem still exists in current release.
Component: Untriaged → Message Compose Window
jhorak, I think F. Buclin's comment 24 answers your question of comment 23. Perhaps we can initiate some progress here?

(In reply to Frédéric Buclin from comment #27)
> @jhorak, did you check comment 24? This problem still exists in current
> release.
Flags: needinfo?(jhorak)
This bug is fixed by patch in bug 967494. I'm stuck by writing a test for it.
Flags: needinfo?(jhorak)
Depends on: 967494
Per Bug 967494 Comment 2, this will be fixed by Bug 967494.
Fix pending, I just pushed the existing patch there into the checkin queue.
Depends on: 1163086
Fixed by bug 967494, landed on Mozilla40: bug 967494 comment 63, landed on C-C: bug 967494 comment #61.
Will be included in TB38, see bug 1163086.
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Whiteboard: [STR comment 22] → [STR comment 22]Fixed by bug 967494]
Whiteboard: [STR comment 22]Fixed by bug 967494] → [STR comment 22] [Fixed by bug 967494]
You need to log in before you can comment on or make changes to this bug.