128101 - switching focus from message pane to quick search causes typing in quick search to execute commands (like "t","r","n",...)

Status: VERIFIED FIXED

(SeaMonkey :: MailNews: Message Display, defect, P1)

Description

[Steve Parkinson]

Mozilla Build 2002022408, windows 2000

In the mail windows, I am using the quick search filter textbox.

Most characters I type whle the textbox has focus have the correct action -
the letters are inserted into the textbox.

However, some characters with other bindings in the mail window keep
that binding even when the search filter has focus. 
(e.g. 'n' for next message) 

The net effect is that when I type an 'n' in the search filter, the next
mail message is selected, and I lose focus in the search filter textfield.
If there are no unread messages in the current folder, a dialog appears asking
me to move to the next folder.

The same thing happens for me in the AIM password-entry field in
the commercial build.

Comment 1

[laurel]

changing assignees

Comment 2

[laurel]

.

Comment 3

[laurel]

Navin: I think this (see bug 122713) has regressed... it doesn't happen all the
time, but I'm seeing N problems in QuickSearch the first time you focus in the
QSearch text field and use a word starting with n. I'm using feb27 commercial
trunk win98.

Comment 4

[laurel]

I am hitting this more frequently, yet when explicitly trying to reproduce it
doesn't happen. 

Comment 5

[Håkan Waara]

Anyone got any reproducible steps for this one yet?

Comment 6

[laurel]

Both Navin and I are seeing this, so it's reproducible over time, just not every
time. But if you're doing searches using n or t in the string you'll probably
hit it eventually. It's a timing thing where we seem to temporarily drop focus
from the quicksearch text field somehow. 

Comment 7

[Håkan Waara]

Funny, while reading this bugmail I clicked the Quick Search textbox, hit 'n'
and whoops...!  "Do you want to advance to the next unread folder?"  :-)

Reproducible steps:

1. Click the message pane
2. Click the Quick Search textbox
3. Hit 'n'

Comment 8

[laurel]

Hwaara, yes, those are the general steps but it does not happen every time. You
are oversimplifying.

Comment 9

[Håkan Waara]

Attachment: Patch

Well, I can reproduce it everytime with those steps.  Here's the fix.

The problem was that we have view-navigation available in textfields as well,
but since we don't want it to interfere with Quick Search, we need to check
that Quick Search isn't focused before proceeding with the view-navigation
command (e.g. "n" for next message).  (see:
http://lxr.mozilla.org/seamonkey/source/mailnews/base/resources/content/mail3PaneWindowCommands.js#648)

Comment 10

[Håkan Waara]

Calling for reviews.  Naving & Seth?

Comment 11

[laurel]

I'm sorry, you're right.  I was reading message pane and thinking thread pane. 
I also see it when in thread pane sometimes... and, of course, it's easier to
see when no unread items in the folder so that the cross-folder dialog appears.

Comment 12

[(not reading, please use [e-mail] instead)]

naving is on vacation, so hwaara, feel free to take this bug (or let me know 
and I can take it and drive it in.)

I haven't been able to reproduce this bug, but hwaara's fix makes sense.

One suggestion, instead of IsInQuickSearchMode() can you rename your new 
function in a way similar to the existing function MailAreaHasFocus()

Maybe, QuickSearchTextInputHasFocus()?

address that, and sr=sspitzer

Comment 13

[Steve Parkinson]

I will assume this patch takes care of the quick search mode since I
don't build mozilla myself.

However, it was not obvious to me if this will also fix the AIM password
entry field in the commercial builds.

Laurel - do you see this AIM problem too? (Maybe it happens most when the AIM
window is in the sidebar?)

Thanks for your effort.

Comment 14

[(not reading, please use [e-mail] instead)]

wait, I take back my sr= because the bug is bigger than just navigation.

when I type "r" in the quick search, it will mark mark my selected message as 
read, before doing the quick search.

Comment 15

[(not reading, please use [e-mail] instead)]

I'm thinking a better fix for this would be to fix MailAreaHasFocus() to also 
include when the quick search text area has focus.

working on a patch...

Comment 16

[(not reading, please use [e-mail] instead)]

Attachment: patch

testing this patch now...

Comment 17

[(not reading, please use [e-mail] instead)]

Attachment: complete patch, also fixes the "r" problem

Comment 18

[(not reading, please use [e-mail] instead)]

Attachment: patch, again

Comment 19

[(not reading, please use [e-mail] instead)]

ok, my last patch fixes the "r" problem for me, and should fix the "n" problem 
as well, I just can't reproduce that as easily.

I think ssu fixed a bug a while ago where when you'd type in text fields in 
messages, we'd label the selected messages.

this just extends on his fix (so that we allow the quick search text field 
to "grab" keys) and makes it so two other commands 

      case "cmd_markThreadAsRead":
      case "cmd_markAsFlagged":

are disabled when text areas (including quick search) have focus.

I'll see r= from hwaara and ssu

Comment 20

[(not reading, please use [e-mail] instead)]

>I think ssu fixed a bug a while ago where when you'd type in text fields in 
>messages, we'd label the selected messages.

looking at the checkin logs, it might have been naving. 

Comment 21

[(not reading, please use [e-mail] instead)]

take a look at http://bugzilla.mozilla.org/show_bug.cgi?id=122713

according that bug, I'd think the "r" problem was fixed and that existing 
MailAreaHasFocus() implementaion was enough.

I'm not sure my change to MailAreaHasFocus() is correct.

but I think the other change is correct, because the "r" problem was never 
addressed.

I'll continue to investigate.

Comment 22

[(not reading, please use [e-mail] instead)]

whoops, I missed the crucial first step in hwaara's list:

1. Click the message pane
2. Click the Quick Search textbox
3. Hit 'n'

now I can reproduce the "n" problem as well.  i'll continue to investigate...

Comment 23

[(not reading, please use [e-mail] instead)]

ok, I think I'm narrowing in on this bad boy.

to solve the "r" problem we'll need part of my fix.  but my change to 
MailAreaHasFocus() is incorrect.

MailAreaHasFocus() is correct as is.

The problem is that we aren't updating commands when focus goes from message 
pane to quick search text area.  working on that, and a new patch.

Comment 24

[(not reading, please use [e-mail] instead)]

ok, getting closer.

I think the problem is in the DefaultController.

I think it also needs to use "MailAreaHasFocus()"

testing that theory now...

Comment 25

[(not reading, please use [e-mail] instead)]

bad theory.  here's new one:

the DefaultController is ok, but the call to MailAreaHasFocus() is returning 
true.

it's doing that because focusedElement is null when I got from message pane to 
qs text area.  

not sure why yet, but I'm getting closer.

Comment 26

[(not reading, please use [e-mail] instead)]

Attachment: fix

ok, the interesting part of this fix is the change to MailAreaHasFocus()

the rest is just dead code removal and code cleanup.

Comment 27

[Håkan Waara]

Comment on attachment 73167 [details] [diff] [review]
fix

> function MsgMarkThreadAsRead()

I think you should remove this function altogether, and replace users with
direct calls to MarkThreadAsRead() instead.

Let's have as few layers of abstractions as possible.

Do that and r=hwaara.

Comment 28

[(not reading, please use [e-mail] instead)]

Attachment: same patch, but remove MarkThreadAsRead()

Comment 29

[(not reading, please use [e-mail] instead)]

updating summary.

note to QA, here's how I'm testing this:

load a folder, select a message, then click in the message pane.

then, click in quick text search area.  type a letter that is also a command, 
like t,r,n, etc.

in previous builds, this would have executed the command (for t, mark thread as 
read) and then do the quick search.

Comment 30

[David :Bienvenu]

Comment on attachment 73215 [details] [diff] [review]
same patch, but remove MarkThreadAsRead()

sr=bienvenu

Comment 31

[Sean Su]

Comment on attachment 73215 [details] [diff] [review]
same patch, but remove MarkThreadAsRead()

r=ssu

Comment 32

[Håkan Waara]

Comment on attachment 73215 [details] [diff] [review]
same patch, but remove MarkThreadAsRead()

r=hwaara

Comment 33

[Asa Dotzler [:asa]]

Comment on attachment 73215 [details] [diff] [review]
same patch, but remove MarkThreadAsRead()

a=asa (on behalf of drivers) for checkin to the 1.0 trunk

Comment 34

[(not reading, please use [e-mail] instead)]

fixed.

Comment 35

[laurel]

OK using mar22 commercial trunk build: win98, linux rh6.2, mac OS 10.1