282669 - First ("To:") address field not focused in new mail when last mail sent with first address field in focus

Status: VERIFIED FIXED

(MailNews Core :: Composition, defect)

Description

[Wayne Woods]

I've seen this bug for a while, but I never got around to testing it thoroughly
enough to report it, till now. I can't see a duplicate, but I'd be surprised if
it's never been mentioned before.

Normally when you go to compose a message, the To: field is focused automatically.

If you send a mail message (either new mail, or forward, or forward as
attachment, or whatever), and have the "To:" field focused at the time of
sending, then go to write a new mail by clicking the "Write" or "Forward"
buttons ("Forward as Attachment" from the menu works too), then you'll notice
that the To: field isn't focused.

In fact I can't see where the focus is. Habitually you go to type the recipient
address, but the text isn't going anywhere.

Nothing major, just a small glitch :)

Last tested on Thunderbird 1.0 20050217.

Comment 1

[Tim Wilde]

I can confirm that this bug still exists in the released 1.5 (20051201) build.  It's particularly annoying to me when I'm forwarding spam off, because I typically only write the "To" address, then hit send, then do the same thing again several times.

Comment 2

[Wayne Woods]

Yeah, I had that problem, too, until I discovered that Spamcop could accept multiple spam attached to the one submission email :)

I had a look through the code, and I can't tell why the focus is lost. When the "To:" field is focused, it calls something related to the address book, and it may be that, as the call isn't closed off again until the field is unfocused (which it never is). Just a guess.

Comment 3

[Tim Wilde]

(In reply to comment #2)
> Yeah, I had that problem, too, until I discovered that Spamcop could accept
> multiple spam attached to the one submission email :)

Yeah, but I keep TB open, so when new spam comes in after I've recently submitted spam, boom, problem recurs again.  I also was noticing it when I was doing SMTP server testing (and testing of broken behaviors on other bugs) where I was sending myself blank messages.

Hopefully someone who knows more than we do will take pity on us and fix the bug! :)

Comment 4

[Wayne Woods]

Further info... the kind of recipient that the field is set ("To", "Cc" etc.) seems to be irrelevant. It *does* matter that the focus is in the *first* address field, however. If, say, you set the first field to a "Cc" and the second field to a "To", then the bug will still only occur if the focus is in the first field, but not the second.

Comment 5

[Wayne Woods]

Also in Mozilla Mail/News and also on Mac, so ->Core and ->All/All. See also bug 141648 that describes something like this from a few years back.

Comment 6

[Wayne Woods]

Attachment: Proposed fix

Simple, two-line fix:
In the onClose function, add a check to see if an input field is focused, and remove the focus if it is.

It seems that the focus status of addressCol2#1 (the first input address field) is saved when the window closes. If a new window is opened, and it already thinks that field is focused, it fails to reset the focus for it.

As a reference, see the comments and subsequent code at:
http://lxr.mozilla.org/seamonkey/source/mail/components/compose/content/addressingWidgetOverlay.js#565 
That's a related problem with the same fix, where the focus status was carried over when new address input fields were cloned.

The exact same fix should be applicable to the equivalent SeaMonkey file, though I've not tried it.

Scott, is this suitable?

Comment 7

[Wayne Woods]

Comment on attachment 228523 [details] [diff] [review]
Proposed fix

Apologies if I'm not doing this right.. never submitted an actual patch before. I'm guessing by other bugs in this component that David Bienvenu is the best one to request a standard review from.

Comment 8

[David :Bienvenu]

Comment on attachment 228523 [details] [diff] [review]
Proposed fix

seems to work for me. I can check this into the trunk and consider for the branch.

Comment 9

[David :Bienvenu]

fixed on trunk; thanks for the patch Wayne!

Comment 10

[Ian Neal]

Would be useful to get a SeaMonkey version of this patch too. Was fun trying to find this bug number from the bonsai entry :-P

Comment 11

[Wayne Woods]

Attachment: SeaMonkey version of fix (Checked in trunk & 1.8 branch)

The code in the SeaMonkey file is identical, so the fix should be the same. But I haven't got all the SeaMonkey code in my cvs to build and check (just the common ones, which fortunately includes the relevant file). Can anyone else check that this new patch works? If not, hopefully I can download the code tomorrow and test.

Comment 12

[Wayne Woods]

OK, I tested the SeaMonkey version of the patch and it seems to work fine.

David, is this something that can be reviewed and checked in from here, or should I open a new bug for it?

BTW, thanks for the fast review! It was only 2 lines, but it was my first patch. Wheee :)

Comment 13

[Ian Neal]

Comment on attachment 228885 [details] [diff] [review]
SeaMonkey version of fix (Checked in trunk & 1.8 branch)

r=me, ask bienvenu or [e-mail] for sr=

Comment 14

[David :Bienvenu]

it can be reviewed and checked in from here...

Comment 15

[Wayne Woods]

Comment on attachment 228885 [details] [diff] [review]
SeaMonkey version of fix (Checked in trunk & 1.8 branch)

Thanks :) Again, this is something that could be applied to both trunk and branch. Not sure which approval request flags to set, as there doesn't seem to be a 1.9 flag, or a 1.8.

Comment 16

[Scott MacGregor]

not a blocker, but i approved the patch for tb2.

Comment 17

[Ian Neal]

Comment on attachment 228885 [details] [diff] [review]
SeaMonkey version of fix (Checked in trunk & 1.8 branch)

a=me for SM1.1a (SM only patch)

Comment 18

[David :Bienvenu]

fixed for 2.0

Comment 19

[Ian Neal]

Comment on attachment 228885 [details] [diff] [review]
SeaMonkey version of fix (Checked in trunk & 1.8 branch)

Checking in (trunk)
MsgComposeCommands.js;
new revision: 1.380; previous revision: 1.379
Checking in (1.8 branch)
MsgComposeCommands.js;
new revision: 1.369.2.5; previous revision: 1.369.2.4
done

Comment 20

[Wayne Woods]

Thanks :)

Comment 21

[[e-mail]]

Comment on attachment 228885 [details] [diff] [review]
SeaMonkey version of fix (Checked in trunk & 1.8 branch)

Note: I didn't actually verify the cause of the bug.
>+    // Clear the focus
>+    if (top.awInputElement.getAttribute('focused') != '')
>+      top.awInputElement.removeAttribute('focused');
1) unconditionally removing the attribute is cheaper than testing it
2) getAttribute might return null in the future like it does for HTML

Comment 22

[Wayne Woods]

Fair enough. Since the patches are already in TB and SM, I'll just submit a couple of patches that remove the if() and correct the indent of the remaining line. Not sure what to do with them, though (r, sr etc.)

Comment 23

[Wayne Woods]

Attachment: TB patch that removes the check added by the previously applied patch

Here's the Thunderbird one. This doesn't obsolete the previous patch, just corrects it to have no if() check.

Comment 24

[Wayne Woods]

Attachment: SM patch that removes the check added by the previously applied patch

Comment 25

[Wayne Woods]

Comment on attachment 229655 [details] [diff] [review]
TB patch that removes the check added by the previously applied patch

Small correction for Thunderbird, based on Neil's comment 21.

Comment 26

[Wayne Woods]

Comment on attachment 229656 [details] [diff] [review]
SM patch that removes the check added by the previously applied patch

Small correction for SeaMonkey, based on Neil's comment 21.

Comment 27

[Serge Gautherie (:sgautherie)]

[Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.8.1b1) Gecko/20060724 SeaMonkey/1.1a] (nightly) (W98SE)

The fix seems to have caused the following regression:
[
Error: top.awInputElement has no properties
Source File: chrome://messenger/content/messengercompose/MsgComposeCommands.js
Line: 228
]
when I send a message with Ctrl-Enter:
the error seems to show up when the message has been sent and the ComposeWindow+statusDialog are removed.

Comment 28

[Ian Neal]

I suppose an |if (top.awInputElement)| would fix it but be good to have some steps for reproducing and testing.

Comment 29

[Wayne Woods]

Hi Serge, Where are you seeing this error? In a debug window or does something pop up automatically?

I haven't been able to reproduce so far. Do you have any third party extensions or themes installed?

Cheers

Comment 30

[Serge Gautherie (:sgautherie)]

[Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.8.1b1) Gecko/20060725 SeaMonkey/1.1a] (nightly) (W98SE)

I'm using a (almost) stock nighly, and the error shows up in the Error Console.

(In reply to comment #27)
> when I send a message with Ctrl-Enter:

Using the button too: it's not related to keyboard.

> the error seems to show up when the message has been sent and the
> ComposeWindow+statusDialog are removed.

I confirm this.

Here are steps:
1. Start MailNews
2. Display a (plain text) message (to yourself).
3. Reply to it.

I get the error each time I do this in a session, until I _Compose_ a new message;
after which the error doesn't show up any more ... until next session.

Comment 31

[Wayne Woods]

Ah, I see it now. It's in Tbird too. It only seems to occur in replies, which I hadn't tested before.

top.awInputElement mustn't be initialized in reply compositions. So, the question is, should it be? Or do I simply add a |if (top.awInputElement)| check?

Comment 32

[Wayne Woods]

Attachment: Corrective patch to account for when top.awInputElement is null

It looks like focusing is handled quite differently for reply mails. This new Tbird patch obsoletes the previously reviewed (but not checked in) corrective patch, and adds a check for |if (top.awInputElement)|

Needs to be checked in to trunk and 1.8 branch.

SeaMonkey patch to follow.

Comment 33

[Wayne Woods]

Attachment: SeaMonkey corrective patch to account for when top.awInputElement is null

SeaMonkey version of previous patch. Again, this needs to land on trunk and SM 1.1 branch. Thanks!

Comment 34

[David :Bienvenu]

Comment on attachment 230736 [details] [diff] [review]
Corrective patch to account for when top.awInputElement is null

thunderbird2 is appropriate - 1.8.1 isn't - firefox uses that.

Comment 35

[[e-mail]]

Hmm, I guess I should have paid more attention to this one :-(

Does the problem only occur when composing a brand new message?
Does the problem only occur when the previous message had exactly one address?

If either of these are true then can you please try this:
awGetInputElement(0).removeAttribute("focused");

Comment 36

[Serge Gautherie (:sgautherie)]

(In reply to comment #35)
> Hmm, I guess I should have paid more attention to this one :-(

If you are refering to the error in comment 27 with steps in comment 30,
the answers are:

> Does the problem only occur when composing a brand new message?

No, it's the opposite: for replies only.

> Does the problem only occur when the previous message had exactly one address?

I didn't test this,
but, at least, it occurs for the very first reply, when there has been no previous message (written) yet.

Comment 37

[[e-mail]]

Sorry, I was referring to the original bug, not the regression.

Comment 38

[Wayne Woods]

(In reply to comment #35)
> Does the problem only occur when composing a brand new message?

It depends what you mean by brand new. The bug also occurred when composing messages that were forwards.

> Does the problem only occur when the previous message had exactly one address?

The previous message could have had any number of addresses added. But the first address field had to have the focus when the message was sent.

> If either of these are true then can you please try this:
> awGetInputElement(0).removeAttribute("focused");
 
I'll try that tonight.

Comment 39

[Wayne Woods]

(In reply to comment #35) 
> If either of these are true then can you please try this:
> awGetInputElement(0).removeAttribute("focused");

awGetInputElement(0) is null in onClose in all situations I tested, including sending fresh mail compositions and reply mails.

Comment 40

[[e-mail]]

(In reply to comment #39)
>awGetInputElement(0) is null in onClose in all situations I tested, including
>sending fresh mail compositions and reply mails.
My fault; can you please try awGetInputElement(1) instead?

Comment 41

[Wayne Woods]

(In reply to comment #40)
> My fault; can you please try awGetInputElement(1) instead?

Yep. That works! It seems to exist in all cases. In new messages (including forwarded etc.) it's the same as top.awInputElement. In the case of replies, it's something else.

Comment 42

[Wayne Woods]

Attachment: TB correction to use awGetInputElement(1) instead of top.awInputElement [Checkin: Comment 52]

How does this look?

Comment 43

[[e-mail]]

(In reply to comment #42)
>Created an attachment (id=230891) [edit]
>TB correction to use awGetInputElement(1) instead of top.awInputElement
> 
>How does this look?
Looks fine to me.

Comment 44

[Wayne Woods]

Comment on attachment 230891 [details] [diff] [review]
TB correction to use awGetInputElement(1) instead of top.awInputElement
[Checkin: Comment 52]

Cool, we'll go with that, then. SM patch to follow when I get a chance to confirm that it works.

Comment 45

[[e-mail]]

Comment on attachment 230891 [details] [diff] [review]
TB correction to use awGetInputElement(1) instead of top.awInputElement
[Checkin: Comment 52]

This doesn't need my sr.

Comment 46

[Wayne Woods]

Attachment: SM correction to use awGetInputElement(1) instead of top.awInputElement [Checkin: Comment 52]

SeaMonkey version of the same patch. Tested on trunk on Mac and works fine with both sends and replies. I can't test branch. Would need to land on both trunk and branch, however.

Hopefully this is the last patch. Sorry for all the corrections.

Comment 47

[Ian Neal]

Comment on attachment 231079 [details] [diff] [review]
SM correction to use awGetInputElement(1) instead of top.awInputElement
[Checkin: Comment 52]

a=me for SM1.1a

Comment 48

[Serge Gautherie (:sgautherie)]

Wayne:
Is there a reason to wait before checking in the patches ?

Ian:
Is your SM v1.1a approval enough, or do we need another one, or a plussed flag ?

Comment 49

[Wayne Woods]

(In reply to comment #48)
> Wayne:
> Is there a reason to wait before checking in the patches ?

I see no reason to wait. Thanks!



Comment 50

[Wayne Woods]

Sorry, I have no idea how that last post happened.

The patches should be right to go. Now they're approved, do I normally have to request check-in? Or does someone else take care of that? Thanks :)

Comment 51

[Kai Engert (:KaiE:)]

*** Bug 347554 has been marked as a duplicate of this bug. ***

Comment 52

[Mark Banner (:standard8)]

I've just checked the TB & SM correction patches into Trunk & Branch on behalf of Wayne.

Comment 53

[Mike Cowperthwaite]

I can verify this fixes the problem, with TB 3a1-0806, Win2K.

I had been hoping the solution would also fix the problem seen at bug 289339, which symptom appears under very similar conditions as the one described here; but alas, no.  Wayne, maybe you could take a look at that...?

Comment 54

[Wayne Woods]

Thanks for the check-ins, Mark!

I'm not sure about bug 289339, but if nobody gets to it first, I'll try my meagre skills on it as soon as I'm done with a couple of Fx things. :)