1277658 - When "Spell Check as you type" is switched off, language indicator can get out of step a full spell check starts with wrong language

Status: RESOLVED FIXED

(Thunderbird :: Message Compose Window, defect)

Description

[Jorg K (CEST = GMT+2)]

When "Spell Check as you type" is switched off, language indicator can get out of step an full spell check starts with wrong language:

When "Spell Check as you type" is switched off, strange things happen:

STR:
1) Create a new message, straight away switch off "Spell Check as you type".
2) Inspect the spelling button. Nothing selected.
   Language in status bar shows default language.
3) Use the spelling button and select a language, not the default.
   Result: Full spell check starts in default language. Not good.
4) Dismiss spell check panel.
5) Inspect the spelling button. Previously selected language now selected,
   status bar still shows the default.

STR Variation:
1) Create a new message.
2) Inspect the spelling button. Default selected.
   Language in status bar shows default language.
2a) Switch off "Spell Check as you type".

This is broken since TB 31 at least, the wrong language in the status bar joined in TB 45.

I'll look into it once bug 728775 has landed.

Comment 1

[Jorg K (CEST = GMT+2)]

The problem is, that if inline spell checking is switched off, the user shouldn't use the language selector on the button. He should just click the button and then select the language on the spelling panel.

The language indicator is actually correct. That is the language they will get when enabling the inline spell check again. The only thing that is wrong in the selection on the button, and that's already wrong in TB 31 (!!).

So I haven't broken anything.

Comment 2

[Jorg K (CEST = GMT+2)]

Attachment: Proposed solution (v1).

Richard, can you please check that this fixes the problem you saw, that is, the inconsistency between language in status bar and language in button.

What I haven't fixed and have no intention to fix is that when you use the menu dropdown and select a different language, the spell check panel will be initialised to that language. It never worked that way and we never had any complaints. With no inline spell check enabled, any action on the button will start the spell check in the last selected language.

Should the patch not apply, apply bug 728775 first.

Comment 3

[Richard Marti (:Paenglab)]

Yes, this is better with the consistency between the status bar indicator and language dropdown in button.

Comment 4

[Jorg K (CEST = GMT+2)]

The problem was this line:
  let curLang = spellChecker.GetCurrentDictionary();
That has been there since day one.
If inline spell checking is switched off, this returns null. This has now been replaced since the "lang" attribute reliably tells us the language of the document.

Comment 5

[:aceman]

And it even produces an error in the console.

Error: TypeError: spellChecker is null
Source File: chrome://messenger/content/messengercompose/MsgComposeCommands.js
Line: 3281

Comment 6

[Jorg K (CEST = GMT+2)]

You mean without the patch, right? I never run without inline spell checking.

Comment 7

[:aceman]

Yes, without the patch the error is shown just opening the language popup on the button.

But with the patch choosing another language still opens the spell checking dialog and throws that error at:

Error: TypeError: spellChecker is null
Source File: chrome://messenger/content/messengercompose/MsgComposeCommands.js
Line: 3324

Yes, you say about that dialog in comment 2. Can't that be fixed in the same way as the popup (getting the lang from the document if inline speller is off) ? If you do not intend to do that, can you at least mitigate the error (do nothing if it is null)?

Comment 8

[Jorg K (CEST = GMT+2)]

Attachment: Proposed solution (v2) earning 100 rubber points ;-)

I'm glad someone pays attention to the error console, haha.

How about a complete restructure of this stuff then?

This even fixes the stuff I didn't intend to fix, so I should get 100% rubber points ;-)

Starting the full spell check was an *error* since stopPropagation() never ran due to the error you spotted. Sigh.

Comment 9

[:aceman]

Comment on attachment 8759966 [details] [diff] [review]
Proposed solution (v2) earning 100 rubber points ;-)

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

Perfect, thanks!

It even works that if you disable inline speller, then change language via the menu, then enable spelling, it checks (underlines) in the correct new language.

::: mail/components/compose/content/MsgComposeCommands.js
@@ +3317,5 @@
>  
>  function ChangeLanguage(event)
>  {
> +  // We need to change the dictionary language and if we are using inline
> +  // spell check, recheck the message.

+10 points for moving this comment above the function and making it the /** */ syntax and documenting event :)

@@ +3325,5 @@
>      // Update the document language as well (needed to synchronise
>      // the subject).
> +    document.documentElement.setAttribute("lang", newLang);
> +
> +    var spellChecker = gSpellChecker.mInlineSpellChecker.spellChecker;

When you are already touching the variables, use 'let' when not in top-level blocks.

@@ +3335,5 @@
> +        gSpellChecker.mInlineSpellChecker.spellCheckRange(null);
> +
> +        // Also force a recheck of the subject. If for some reason the spell
> +        // checker isn't ready yet, don't auto-create it, hence pass 'false'.
> +        var inlineSpellChecker =

let

Comment 10

[Jorg K (CEST = GMT+2)]

Attachment: Proposed solution (v2a) earning 100+10 rubber points ;-)

I would document it if I know what event actually is ;-(

Can you please take another look and make a better suggestion if needed.

Comment 11

[:aceman]

Comment on attachment 8759973 [details] [diff] [review]
Proposed solution (v2a) earning 100+10 rubber points ;-)

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

::: mail/components/compose/content/MsgComposeCommands.js
@@ +3318,5 @@
> +/**
> + * Change the language of the composition and if we are using inline
> + * spell check, recheck the message with the new dictionary.
> + *
> + * @param event  The popupshowing event of the opening menu.

I think it is actually the event of clicking an item in the menupopup. That is why then event.target is the specific menuitem that was clicked and you fetch .value from it.

Comment 12

[Jorg K (CEST = GMT+2)]

Attachment: Proposed solution (v2b) earning 100+10 rubber points ;-)

Changed comment.

Comment 13

[:aceman]

Nice.

Comment 14

[Jorg K (CEST = GMT+2)]

https://hg.mozilla.org/comm-central/rev/6b49a9fa86f6

Comment 15

[Jorg K (CEST = GMT+2)]

Comment on attachment 8759975 [details] [diff] [review]
Proposed solution (v2b) earning 100+10 rubber points ;-)

[Approval Request Comment]
Regression caused by (bug #): No regression. Wrong from day one.
User impact if declined: Puzzling behaviour of language selection when
inline spell check is turned off.
Testing completed (on c-c, etc.): Manual.
Risk to taking this patch (and alternatives if risky):
Low risk.

Comment 16

[Jorg K (CEST = GMT+2)]

Richard, please note that the full extent of the error got fixed.

With inline spell check disabled
1) The selection of a new language from the drop-down now does *not* start the full spell check.
   That was an error, see comment #7 and comment #8.
2) The full spell check starts with the chosen language.
3) The spelling button and the status text don't get out of step any more.

Since there were three functional problems here, I don't think the attribute "polish" applies to this bug.

Comment 17

[Richard Marti (:Paenglab)]

(In reply to Jorg K (GMT+2, PTO during summer, NI me) from comment #16)
> Richard, please note that the full extent of the error got fixed.

Noted :-)

Comment 18

[Jorg K (CEST = GMT+2)]

Aurora (TB 48):
https://hg.mozilla.org/releases/comm-aurora/rev/80242da9baf3