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

RESOLVED FIXED in Thunderbird 49.0

Status

Thunderbird
Message Compose Window
RESOLVED FIXED
a year ago
a year ago

People

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

Tracking

(Blocks: 1 bug)

Trunk
Thunderbird 49.0
Dependency tree / graph

Thunderbird Tracking Flags

(thunderbird46 wontfix, thunderbird47 wontfix, thunderbird48 fixed, thunderbird49 fixed, thunderbird_esr45 wontfix)

Details

Attachments

(1 attachment, 3 obsolete attachments)

(Assignee)

Description

a year ago
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.
(Assignee)

Comment 1

a year ago
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.
(Assignee)

Comment 2

a year ago
Created attachment 8759393 [details] [diff] [review]
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.
Flags: needinfo?(richard.marti)
Attachment #8759393 - Flags: review?(acelists)
Yes, this is better with the consistency between the status bar indicator and language dropdown in button.
Flags: needinfo?(richard.marti)
(Assignee)

Comment 4

a year ago
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

a year ago
And it even produces an error in the console.

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

Comment 6

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

Comment 7

a year ago
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)?
(Assignee)

Comment 8

a year ago
Created attachment 8759966 [details] [diff] [review]
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.
Attachment #8759393 - Attachment is obsolete: true
Attachment #8759393 - Flags: review?(acelists)
(Assignee)

Updated

a year ago
Attachment #8759966 - Flags: review?(acelists)

Comment 9

a year ago
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
Attachment #8759966 - Flags: review?(acelists) → review+
(Assignee)

Comment 10

a year ago
Created attachment 8759973 [details] [diff] [review]
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.
Attachment #8759966 - Attachment is obsolete: true
Attachment #8759973 - Flags: review?(acelists)

Comment 11

a year ago
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.
Attachment #8759973 - Flags: review?(acelists)

Updated

a year ago
Status: NEW → ASSIGNED
Keywords: polish
OS: Unspecified → All
Hardware: Unspecified → All
(Assignee)

Comment 12

a year ago
Created attachment 8759975 [details] [diff] [review]
Proposed solution (v2b) earning 100+10 rubber points ;-)

Changed comment.
Attachment #8759973 - Attachment is obsolete: true
Attachment #8759975 - Flags: review+

Comment 13

a year ago
Nice.
(Assignee)

Comment 14

a year ago
https://hg.mozilla.org/comm-central/rev/6b49a9fa86f6
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
status-thunderbird46: --- → wontfix
status-thunderbird47: --- → wontfix
status-thunderbird48: --- → affected
status-thunderbird49: --- → fixed
status-thunderbird_esr45: --- → wontfix
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 49.0
(Assignee)

Comment 15

a year ago
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.
Attachment #8759975 - Flags: approval-comm-aurora+
(Assignee)

Comment 16

a year ago
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.
Keywords: polish
Summary: 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, language indicator can get out of step a full spell check starts with wrong language
(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 :-)
(Assignee)

Comment 18

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

Updated

a year ago
status-thunderbird48: affected → fixed

Updated

a year ago
Blocks: 1279055
You need to log in before you can comment on or make changes to this bug.