145532 - selection is moved when using a keyboard shortcut with focus on left pane

Status: VERIFIED FIXED

(Core :: XUL, defect)

Description

[Nahor]

With build 2002051808 from trunk, when using a shortcut and when the focus is on
the left pane (list of mail folder/newgroups), the selection changes to the next
folder/group starting with the letter of the shortcut.

Steps:
  1. have a news group starting with f (e, v, g, m, t, w and h work too).
  2. Have this group/folder visible (i.e branch expanded)
  3. Select any folder/groups other than the one from step 1.
  4. Press "alt+f" to open the file menu (or one of the other menu shortcut
depending of your choice at step 1.)

result:
  The menu is open and the selected folder/group changes to a folder/group
starting with letter f (e,v ... respectively).

expected result:
  Menu opens but selection is *not* moved.

Alternatives:
  - Try to do a search for a particular mail/news (ctrl+shift+f) and the
selection moves to the folder/groups whose first letter is a f.
  - "Mark all read" (ctrl+alt+c) has the same issue.

Remarks:
  Moving the selection when a letter is pressed is a nice feature but the
selection should not moved if a modifier (alt or ctrl) is pressed (I think shift
would be ok).

Comment 1

[Andrew Schultz]

confirmed with Linux build 2002051808 (trunk).

regression between 2002051507 (trunk) and 2002051621 (trunk)

probably bug 133366

cc'ing Kyle Yuan

Comment 2

[Daniel Bratell]

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

Comment 3

[Andrew Schultz]

this also occurs in Javascript Debugger and DOM Inspector

==> Trees (Menus?)

Comment 4

[Nahor]

For notice, there is a similar problem (bug 145403) in MailNews with shortcuts
that don't have a modifier (shortcuts in go menu: f, n, t, b and p)

Comment 5

[Kyle Yuan]

this regression is caused by bug 133366. taking...

Comment 6

[Kyle Yuan]

Attachment: patch

1. Disable the keynavigation feature in mailnews' folderpane, because there are
some single letter shortcut key in mailnew module (B N T F ...)
2. Filter out the keypress that combined with alt, ctrl, shift, meta

Comment 7

[Kyle Yuan]

asking Jan for r= ...

Jan, you are right. We still need to disable this feature in folderPane.xul.

Comment 8

[Nahor]

-         else if (!this.disableKeyNavigation && event.charCode > 0) {
+         else if (!this.disableKeyNavigation && event.charCode > 0 &&
+                  !event.altKey && !event.ctrlKey && !event.shiftKey &&
!event.metaKey) {

Do we really want shift to disable navigation?
Because to me, like <letter> shortcuts, <shift>+<letter> shortcuts should not be
used. Too likely to be a problem if a textfield appears in the window that
didn't use to have one.
And on case-sensitive systems, people maybe enclined to use shift if an item of
the tree starts with a capital.

Comment 9

[Kyle Yuan]

Nabor: 
1. I think <shift>+<letter> is supposed to be a combine shortcut, even we 
haven't used them yet.
2. In key-navigation code, I use *case-insensitive* string compare.

Comment 10

[Jan Varga [:janv]]

Comment on attachment 84213 [details] [diff] [review]
patch

r=varga

Comment 11

[Nahor]

Kyle:

1. I guess. If <letter> can be used as a shortcut, there is no reason that
<shiht>+<letter> would not be. But to me, that's seems to be a bad design that
can cause problem later. <ctrl> and <alt> are good for shortcuts because they
are not used to type normal text, but <shift> is.
2. I know, I wasn't talking about changing the search into a case-sensitive case
but to allow people to type a name as it is written on screen. It's not really
an important issue, it's just one of those little details that may make life
easier. :)

Comment 12

[Johnny Stenback (:jst)]

Comment on attachment 84213 [details] [diff] [review]
patch

sr=jst

Comment 13

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

here's what bienvenu reported:

when the folder pane has focus, if I hit "n" (for next unread folder), it loads 
a folder that starts with n.

this was caused by the checkin for #145532.

the mailnews part of the attached patch fixes this problem.

I've landed the mailnews part, now to land the xpfe part since it has r/sr.

Comment 14

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

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

Comment 15

[Greg Onufer]

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

Comment 16

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

sr=sspitzer on the mailnews part.

both mailnews and xpfe fix landed on the trunk.

Comment 17

[Andrew Schultz]

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

Comment 18

[Nahor]

Build 2002052104

- The Mail/News is fixed (no more navigation)
- I tried navigation on the bookmark window and the preference dialog. It works
as I would like it to work but not as the patch intended it. <ctrl> and <alt>
disable the navigation but I can still navigate using <shift>+<letter>.

So, again, I'm not complaining about the behavior, I like it that way, but if
the patch above has been commited "as is", either "event.shift" is
never/incorrectly set or something shunt the test somehow.

Comment 19

[Kyle Yuan]

Nahor, thanks for your careful testing. 
I found that event.shiftKey is true only when you pressed <ctrl>+<shift> on the 
same time. It's right your desire :)

Comment 20

[Nahor]

Does it need to be filed as a new bug or is it bug 104371 (or maybe bug 126189)?

Comment 21

[Kyle Yuan]

yes, those two bugs are aimed at this problem, let me checking...

Comment 22

[Kyle Yuan]

See:
GTK:
http://lxr.mozilla.org/seamonkey/source/widget/src/gtk/nsGtkEventHandler.cpp#400
Windows:
http://lxr.mozilla.org/seamonkey/source/widget/src/windows/nsWindow.cpp#2832

Seems the designer want to turn shift off when you pressed <shift>+<ascii key>. 
I don't think this is a bug.

Comment 23

[Nahor]

In Mail/News, shortcut <n> (select next unread message) is different from
<shift>+<n> (do nothing) but with your patch the two are the same? Go figure...
But well, I'm just a free loader of Mozilla :p... In all case, the bug fix is
closed and verifed. This argumentation should go into another bug or one of the
above.
Thanks Kyle. :)

Comment 24

[Kyle Yuan]

I don't think we are arguing, this is a helpful discussion :)

The fact in mailnews is:
If you turned off CapsLock, <n> works, but <shift>+<n> doesn't;
If you turned on CapsLock, <shift>+<n> works, but <n> doesn't;
because <n> is the shortcut key, but <N> isn't.

Comment 25

[Nahor]

Ok then, lets go on *grin*:

But whatever the status of capslock, if you want to mark all mail as read, you
have to press <ctrl>+<shift>+<c>. And from the gtk link you gave, it says "make
Ctrl+uppercase functional as same as Ctrl+lowercase".
So <n> and <N>(capslock) should behave the same way and <shift>+<n> and
<shift>+<N> too.
In other word, shortcuts should be based on keycodes and not on ascii codes.