190734 - "Spell" button on message composition invokes the wrong (on-send) spelling dialog

Status: RESOLVED FIXED

(MailNews Core :: Composition, defect)

Description

[Ian Neal]

Using BuildID 2003011508 on Win XP SP1 with latest Spellchecker (dated 15th Dec).
The 2nd patch in bug 173046 introduced a 'Send' button when the window being
spellchecked is a 'msgcompose' window.

Steps to reproduce
1. Compose an email
2. Click on 'Spelling' icon
3. Spellchecker window pops up with 'Send' button on it
4. Click on 'Send' button

Actual results
1. Spellchecker window closes but email isn't sent

Expected results
1. Spellchecker window closes, email is sent and compose window closes.

Not sure if this should be in Browser, Editor Compose (where bug 173046 was
logged against) or under MailNews, Composition.

Comment 1

[Aleksey Nogin]

The "Send" button is not new. There used to be two different spellcheck dialogs
for message composition:

Dlg 1 has the "Close" button

Dlg 2 has the "Send" and "Stop" buttons. Also, the dlg 2 has the feature that it
only pops up when there are spelling mistakes (possibly that part is
pref-controlled, not sure), while Dlg 1 always pops up, even when there are no
spelling mistakes.

It used to be the case that the "Spell" button would invoke Dlg 1, while the
"Send" button would invoke Dlg 2 (provided the "spellcheck before sending" pref
is on).

The real bug is that now both buttons invoke the Dlg 2. This has several
unintended consequences - such as presence of the not working "Send" button as
well as dialog not popping up when "Spell" is clicked, if there are no misspellings.

Comment 2

[Aleksey Nogin]

The "on-send" version of the dialog was added in bug 132697. The regression was
likely caused by bug 173046.

Comment 3

[[e-mail]]

The culprit
seems to be a checkin to ComposerCommads.js from December 17. It was checked in
as part of the patch for block quotes:

http://bugzilla.mozilla.org/show_bug.cgi?id=173046

Changing the last parameter on line 2400 from spellCheckMail to "false" will
resolve both bug 2821 and bug 2992:

http://lxr.mozilla.org/seamonkey/source/editor/ui/composer/content/ComposerCommands.js#2400

Comment 4

[[e-mail]]

see mozdev bugs: 

Spellchecker does not appear if there are no misspelled words
http://mozdev.org/bugs/show_bug.cgi?id=2992

Clicking 'Spell' icon shows 'Send' and 'Stop' buttons instead of 'Close' button
http://mozdev.org/bugs/show_bug.cgi?id=2821

Comment 5

[[e-mail]]

Attachment: patch v1.0

Comment 6

[Aleksey Nogin]

Comment on attachment 112714 [details] [diff] [review]
patch v1.0

My guess (haven't tried it yet) is that if you just replace it with false, then
it would not skip quoted text when clicking the "Spell" button. Please check it
and feel free to remove the "review-"  if my guess is wrong.

Basically, I am guessing that the parameter that used to mean "do we want Dlg 1
or Dlg 2" was overloaded to mean "do we want to skip blockquotes". This
overloading is a mistake, since when "Spell" button is clicked, we want "Dlg 1"
(the default HTML editor spellcheck dlg), but still want to skip blockquotes.
In short, it needs an additional parameter...

Comment 7

[[e-mail]]

Comment on attachment 112714 [details] [diff] [review]
patch v1.0

Aleksey,

you are absolutely correct. 

We need to distinguish between calls from ComposerCommands.js (via spell check
button in mail and HTML composer) and MsgComposeCommands.js (via send button in
mail composer).

http://lxr.mozilla.org/seamonkey/search?string=edspellcheck

BTW although I've reversed the patch, I've had trouble to get the blockquote
filter to work at all. Am I missing anything? Is there any regression to the
new text filter?

Comment 8

[[e-mail]]

Attachment: patch v2.0

Comment 9

[Aleksey Nogin]

Comment on attachment 112719 [details] [diff] [review]
patch v2.0

The new patch looks correct to me, but I do not feel qualified to give a
positive review.

Comment 10

[[e-mail]]

Comment on attachment 112719 [details] [diff] [review]
patch v2.0

Aleksey, thanks for looking over it though! 

I believe Charlie Manske is gone, perhaps Rod Spears would want to have a look
at this?

Comment 11

[Asa Dotzler [:asa]]

unsetting blocking1.3b+ flag. blocking bugs are determined by
[e-mail]. to nominate a bug as blocking a release plese use the
blocking1.3b? bug flag. to request approval to land a patch please use the
approval1.3b? patch flag.

Comment 12

[[e-mail]]

Aleksey, just for expediency, could you r= the bug? Meanwhile I'll try to find a
superreviewer.

Comment 13

[Kathleen :Brade]

Comment on attachment 112719 [details] [diff] [review]
patch v2.0

I have not tested this but here are some questions I need clarified to do a
review:
  * in EdSpellCheck.js you declare a global "gSkipblockQuotes" but then it is
only used in one place; shouldn't it be a local variable or am I missing
something?
  * Also in EdSpellCheck.js, it seems like gSendMailMessageMode doesn't need to
be a global either (local variable in Startup() seems to be sufficient).  Could
you fix that also while you're in there?

Comment 14

[[e-mail]]

Attachment: patch v2.1

Kathleen, thanks for looking over the changes! Without volunteering his time,
perhaps Aleksey could help with testing as well? I'm not sure if I can
reproduce the desired blockquote filter behavior - with the patch or without
it.

Comment 15

[Aleksey Nogin]

You need to use the HTML compose, the blockquotes are still not skipped in plain
text compose. Since I never use the HTML composition, I have no idea how exactly
it's supposed to work in HTML compose.

BTW, you should test that spellchecker does not attempt to skip quotes in
non-Mail Editor.

Comment 16

[Asa Dotzler [:asa]]

not going to hold beta for this. We'd probably take the fix in beta or final,
though, if reviews happen pretty quickly.

Comment 17

[[e-mail]]

ellocogato, we can't set 1.3b blocking ourselves. Asa will kick our butt again,
see comment 11.

Comment 18

[Kathleen :Brade]

Comment on attachment 112811 [details] [diff] [review]
patch v2.1

r=brade

Comment 19

[Gus Scheidt]

Yeah, that was quite the accident.  And I didn't check my email until just now
to see that I had done that.  Sorry!

Comment 20

[Asa Dotzler [:asa]]

Comment on attachment 112811 [details] [diff] [review]
patch v2.1

a=asa (on behalf of drivers) for checkin to 1.3beta.

Comment 21

[[e-mail] (away - not reading bugmail)]

patch has been checked in