53029 - Menu highlight by ALT key should be cleared by next key event

Status: RESOLVED FIXED

(Core :: XUL, defect, P5)

Description

[Masaki Katakai]

ALT key will highlight Menu item for keyboard navigation.
For example, on Message Compose window,

1) type ALT key
File menu will be highlighted.

2) type DOWN key
contents of File menu will be shown

However, the highlight isn't cleared by next key event.

1) type ALT key
File menu will be highlighted.

2) type 'a' key
Mail Composer does not have 'a' menu, so the
highlight should be cleared and 'a' needs to be
inserted into input field.

Please try the same operations on notepad on
Windows. ALT key also will highlight the File menu,
but next 'a' will clear the highlight and 'a'
will be inserted.

ALT key is used for ALT+HANKAKU/ZENKAKU to
enable/disable IME. So I often meet this problem.
Menu highlight grabs the key event.

Comment 1

[:deanis74]

Using Notepad on NT4, this is not the behavior.  If I press and release Alt, the 
File item on the menu bar becomes active.  Pressing 'A' then results in a beep, 
but the highlight is _not_ removed.

....

Of course, thank MS for changing this on Win2000.  Pressing the 'A' key now 
de-activates the menu bar.  But nothing is inserted into the document.

Comment 2

[Masaki Katakai]

I'm very sorry my first report is not correct.
The behavior on Win98 is the same with Win2K that
'a' will de-activate menu bar, will not be inserted.

However, on Mozilla, 'a' will not de-active the menu bar.
That's problem.

Comment 3

[Masaki Katakai]

ALT+HANKAKU/ZENKAKU enables IME. However, ALT+HANKAKU/ZENKAKU combination
often activates menu bar on Mozilla. So menu bar will grab key event
while composing. For example, return key should move to next line, but
it will show File menu because menubar is active. On Notepad, I can not
see this problem. However, on Mozilla, the same operation often activates
menu bar. I'll file a separate bug.

Comment 4

[Masaki Katakai]

The separate bug - http://bugzilla.mozilla.org/show_bug.cgi?id=53528
has been fixed. So now I think priority/severity is not high.

Comment 5

[Viswanath Ramachandran]

Since Don has left, Vishy is taking his bugs in bulk, pending reassignment.
thanks,
	Vishy

Comment 6

[Frank Tang]

yokoyama said it should be intl but not nsbeta1

Comment 7

[Aaron Leventhal]

Is there any compelling reason not to mark this WONTFIX?

Comment 8

[Masaki Katakai]

I don't have any objection to mark this as WONTFIX. The separate
bug 53528 has been fixed.

Comment 9

[:deanis74]

No, leave it open.  If doing this on Win2000 deactivates the menu bar, we 
should do it.  Keep up with the conventions of the latest OS.  But don't change 
the severity from trivial.

An important implementation note is that the Win2000 behavior is different for 
menu bars and menus.  If I press "A" when the top-level menu bar is active and 
there's no corresponding item, the menu bar is deactivated.  If I have a menu 
open, such as File, and press "A" when there's no corresponding item, the 
system beeps and the menu remains open.  Shouldn't be too tough.

Comment 10

[timeless]

w2k nc4.75 alt;qq => *beep*q.  The beep to be played per system sound rules and 
the menu should lose focus.

Comment 11

[:deanis74]

That's what I said, "If I press "A" when the top-level menu bar is active and 
there's no corresponding item, the menu bar is deactivated", isn't it?

Comment 12

[timeless]

you left out the beep.

Comment 13

[:deanis74]

Sorry, must have had my speakers too low!  To restate...

An important implementation note is that the Win2000 behavior is different for 
menu bars and menus.  If I press "A" when the top-level menu bar is active and 
there's no corresponding item, the system beeps and the menu bar is deactivated. 
 If I have a menu open, such as File, and press "A" when there's no 
corresponding item, the system beeps and the menu remains open.  Shouldn't be 
too tough.

Comment 14

[Blake Ross]

Dean, wanna take this?  Probably needs to go to pink if not.

Comment 15

[:deanis74]

Sure.  One question - how do I sound a beep?

Since I'll be doing this for Windows only, I'll throw in the behavior where 
pressing the accelerator for a disabled menu item closes the popup chain.

Comment 16

[Blake Ross]

http://lxr.mozilla.org/seamonkey/source/widget/src/windows/nsSound.cpp#82

That menus dismiss when access keys for disabled items are pressed seems like a 
Windows flaw to me. Matthew, what do you think?

Comment 17

[:deanis74]

Personally, I like to maintain consistency with the platform unless there's a 
_really_ good reason not to.  But whatever, I'll hold off on that last part for 
now.

Comment 18

[:deanis74]

Attachment: here it is

Comment 19

[:deanis74]

This patch adds what I described before...

   If I press "A" when the top-level menu bar is active and
   there's no corresponding item, the system beeps and the
   menu bar is deactivated. If I have a menu open, such as
   File, and press "A" when there's no corresponding item,
   the system beeps and the menu remains open.

It's only for Windows.

Comment 20

[:deanis74]

While I'm here, I'll attach a patch for the controversial behavior of pressing 
the access key for a disabled menu item dismissing the chain.  I maintain that 
this is pp and we should do it.

Comment 21

[:deanis74]

Attachment: My cat's breath smells like cat food.

Comment 22

[:deanis74]

Attachment: Whoops, that last patch wasn't quite right.

Comment 23

[:deanis74]

Now I've got two patches here.

1) attachment 32259 [details] [diff] [review]: handles what to do on Windows when pressing an access key 
that doesn't correspond to a menu item.
2) attachment 32264 [details] [diff] [review]: dismisses the chain when pressing the access key for a 
disabled menu item or when pressing Enter when it's highlighted.

Both of these mimic standard Windows behavior.  Take what you want.

Comment 24

[:deanis74]

cc: pink - Mike, can you check these patches out (see my previous comment)?  
They're pretty straight-forward.

(I promise, everyone, this is my last note on this bug for a while - sorry)

Comment 25

[Matthew T (active 1999-2002)]

> That menus dismiss when access keys for disabled items are pressed seems like
> a Windows flaw to me. Matthew, what do you think?

Since when has the flawedness of an internally inconsistent Windows menu 
behavior discouraged us from reimplementing it faithfully?

... Ahem. In this case yes, I think the Windows 2000 behavior is correct. The 
most likely case is that the user has typed the mnemonics (and/or arrow keys) 
for a menu item at high speed, not realizing that the menu item in question is 
disabled. If we ding then close the menu, the user is alerted to the fact that 
the menu item didn't work. But if we leave the menu open, the user's subsequent 
key-presses (which they are expecting to go in the content area) may in fact 
activate other menu items, possibly causing dataloss.

Comment 26

[:deanis74]

OK, then I've got patches for both.  Looking for an r=.  My only concern is 
whether I did the beeping properly.

Comment 27

[:deanis74]

mpt: There's no beep for disabled menu items.  I could add it, though, because I 
think it's a good idea.  But I'm with you in that I think the menu should be 
dismissed.

Comment 28

[:deanis74]

timeless, care to review?

Comment 29

[timeless]

r=timeless for 1) and 2) [skipping cat's breath]

Comment 30

[:deanis74]

Pink, Blake wanted me to get an r= from you on this one.  Can you take a quick look?

Comment 31

[:deanis74]

Hyatt, can you take a look at this patch?  It's windows-only.

Comment 32

[:deanis74]

moving up to 0.9.8.  it's low-risk and windows-only.  just need an sr=.

Comment 33

[:deanis74]

I'm tired of the struggle to get my patches reviewed. --> default owner

Comment 34

[Brendan Eich [:brendan]]

Dean, I don't think hyatt reads bugmail well, or at all.  Did you try e-mailing
him directly?  I'm doing that now.

/be

Comment 35

[Peter Trudelle]

->99

Comment 36

[Mike Pinkerton (not reading bugmail)]

dean, can you refresh my memory on what's going on here. The menu code has been
swapped out and I can't find it in my paging table.

why would we be calling DismissChain() here, and at what stage are we when this
is being called?

Comment 37

[:deanis74]

Sure thing, Mike.  I needed a refresher as well!

attachment 32259 [details] [diff] [review]: Adds the behavior I summarized in comment 19.  The patch for
nsMenuBarFrame.cpp is in FindMenuWithShortcut().  If a Windows user presses Alt
and then a key that isn't a valid access key on the menu bar (eg. 'X'), a beep
is sounded and DismissChain() is called to deactivate the menu bar.  Similar for
the nsMenuPopupFrame.cpp patch, except it's obviously for popup menus and the
menu remains open.

attachment 32264 [details] [diff] [review]: Adds what I described in comment 20, and which mpt
(grudgingly?) agreed with in comment 25.  DismissChain() closes the menu.

Comment 38

[Mike Pinkerton (not reading bugmail)]

ok 2 comments (now that I understand)

1) did you want to beep when dismissing the popup in patch 32264?

2) i'd prefer you not use the CID for creating a sound object if at all
possible. use the prog id ("@mozilla.org/sound;1") instead.

fix (2) and r=pink on both.

Comment 39

[:deanis74]

1) See comment 25 and comment 27.  Windows doesn't beep, so my patch makes our
menus work the same.

2) Sure thing.  Uhh... how?

Comment 40

[Mike Pinkerton (not reading bugmail)]

do_createInstance() can take a progid instead of a CID. just pass the string
instead of the CID.

Comment 41

[:deanis74]

let's try this again  -->me

Comment 42

[:deanis74]

Attachment: new single patch, addressing pink's concern

Comment 43

[:deanis74]

Comment on attachment 69200 [details] [diff] [review]
new single patch, addressing pink's concern

r=pink from his comments.  Need an sr=, please.

Comment 44

[Mike Pinkerton (not reading bugmail)]

yes, r=pink ;)

Comment 45

[:deanis74]

Comment on attachment 69200 [details] [diff] [review]
new single patch, addressing pink's concern

Hyatt sez sr=hyatt

Comment 46

[:deanis74]

checked in