14368 - Win32 - F10 key does not activate the menu bar

Status: VERIFIED FIXED

(Core :: XUL, defect, P3)

Description

[cpratt]

Build ID: 1999092013
Platform: Windows NT (this bug is NOT applicable to Linux or Mac OS)

To reproduce:
- Launch apprunner
- Click in the body of the browser window to give it focus
- Press F10

Result: Nothing happens.

Expected result: the menu bar should be activated (see legacy Netscape browsers
for expected behavior).

Comment 1

[joki (gone)]

I'm guessing function keys aren't correctly coming through the widget library.

Comment 2

[rods (gone)]

Yes, F10 is coming through. The char code is 0 but the VK is set. I would
imagine the menubar is looking for it.

Assigning to hyatt.

Comment 3

[David Hyatt]

My hands have deteriorated to the point where I can no longer type.  I need
help.  If you think you can fix this bug on your own, please take it away from
me.  If you'd like to volunteer to be my hands for a specific bug, then I'll be
happy to come up to your cube and sit with you and fix the bug (assuming you
have the patience for that).

Comment 4

[Peter Trudelle]

reassigning to saari

Comment 5

[sairuh (rarely reading bugmail)]

reassigning QA to me, updating component.

Comment 6

[Mike Pinkerton (not reading bugmail)]

Taking menu/popup bugs.

Comment 7

[leger]

BULK MOVE: Changing component from XP Menus to XP Toolkit/Widgets: Menus.  XP 
Menus component will be deleted.

Comment 8

[Mike Pinkerton (not reading bugmail)]

bulk accepting xpmenu/popup bugs. sigh.

Comment 9

[Mike Pinkerton (not reading bugmail)]

moving all key-related menu bugs to hyatt, per his request.

Comment 10

[Peter Trudelle]

Mass moving M19 bugs to M20

Comment 11

[:deanis74]

* SPAM *
I blame this bug for my missing going to the gym tonight.

Comment 12

[:deanis74]

Check this.  I had some problems with not properly trapping the F10 all of the 
time so it would activate the menu bar, next press would deactivate the menu 
bar, then the next press would activate the system menu instead of activating 
the menu bar again.  Fixed that.

It behaves almost exactly like F10 does in Windows.  F10 alone or Ctrl+F10 will 
activate the menu bar.  I didn't know what to do with Shift+F10, because this 
keystroke should really pop up a context menu if applicable, so for now that 
gets eaten.  The only difference is that I do it on KeyPress and in Windows it 
occurs on KeyUp.  I just didn't want to go through the pain of tracking the 
F10 KeyDown then the subsequent KeyUp.  If someone complains about this, they're 
way too picky.

Comment 13

[:deanis74]

Attachment: Here it is. My source is a week or two old, but I don't think much has changed in these files recently.

Comment 14

[:deanis74]

Oh yeah, I'm using the weird #ifdef:

#if defined XP_PC && !defined XP_OS2

because XP_WIN isn't properly implemented yet.  Once it is, that's what should 
be used.

Comment 15

[Matthew T (active 1999-2002)]

See also bug 36665.

Comment 16

[Mike Young]

F10 doesn't bring up the file menu, but it does bring up the Windows menu. (or
would if you press the down arrow) This bug seems to be related to bug 13042 in
that it doesn't allow the user to cycle through the menus.

Comment 17

[Marko Macek]

This should work on Linux/Unix too. Maybe there should be #ifndef MAC

Comment 18

[Peter Trudelle]

Mass-moving all M20-M30 XPToolkit bugs to Future

Comment 19

[sairuh (rarely reading bugmail)]

*spam*: transferring current XP Menu bugs over to jrgm, the new component owner.
feel free to add me to the cc list (unless am the Reporter) of any of these, if
you have any questions/etc.

Comment 20

[Dan Rosen]

Is this bug still valid?

Comment 21

[David Hyatt]

Yes.

Comment 22

[John Morrison]

Er, so is Dean's patch valid? (Perhaps this can go in the trunk if it's the
right thing to do; probably can't take it on the branch though).

Comment 23

[:deanis74]

Oh wow, I'd forgotten about this patch.  It probably needs some changes, 
especially given Marko's comments from 05-09.

Comment 24

[Mike Young]

Now that NS6 is out the door can we get this bug looked at? It has a patch
(although now most problably old) And when we do that someone should look at bug
13042 which might be affected by this bug.

Comment 25

[Mark Hammond [:markh] [:mhammond]]

I have attached a new patch for this.  It was based on Dean's, but has been 
updated against the trunk (as of today) and has also been simplified - there is 
no need to track the state of F10 across events - just handle it in KeyPress 
and be done with it.

The patch also touches widget/src/windows/nsWindow.cpp to give VK_F10 the same 
special case as VK_MENU.  This patch to nsWindow is also needed if XUL wants to 
use F10 - so if this patch isn't accepted here for some reason I will file a 
separate bug for that, as it is a problem for Komodo that does want F10 
captured in XUL.

I explicitly do not handle Shift-F10 as required for bug 36665, as it seems 
that the listener for this key would be elsewhere.

Once this patch is in, F10 gives the same basic behaviour as standard Windows 
apps.

I made no attempt to look at bug 13042 (System menu missing), as I believe that 
is not a serious issue - as bug 13042 states neither Office nor IE do this.

As I mentioned, this patch has the added advantage of fixing an unfiled bug 
regarding other F10 handling in XUL - so if we can get traction on this we kill 
2 bugs with one stone :)

Comment 26

[Mark Hammond [:markh] [:mhammond]]

Attachment: Updated and simplified patch.

Comment 27

[Mark Hammond [:markh] [:mhammond]]

I should also note that as Hyatt has many things on his plate, I would be happy 
to have it assigned to me - but would still be counting on Hyatt's sr= :-)

Comment 28

[John Morrison]

Sounds like a plan to me. Dean or Chris, can you r=?

Comment 29

[timeless]

+      // Also need to handle F10/Shift-F10 specially on Windows.
+      else if (theChar == NS_VK_F10) {

+        if (!(shift || alt || meta)) {
this doesn't seem to handle shift-f10 ...

Comment 30

[:deanis74]

The comment shouldn't mention Shift+F10.  When I wrote the original patch, I
didn't have a concept of all the listeners, so I thought didn't realize that
there was a separate listener for the context menu.

At first glance, the patch looks pretty good.  But I don't have my computer set
up at home right now, so I can't test it.

Comment 31

[Mark Hammond [:markh] [:mhammond]]

I will correct the comment, but believe the code is correct.

Another style nit I will fix is that one file uses #ifdef and the other #if 
defined.

Comment 32

[:deanis74]

Mark: Yeah, the code does look correct to me.  Nice clean-up, by the way!  I
think that way back when I wrote this, I had never delved into nsWindow.cpp, so
I tried to  do it all in the listener.

John: I'd like to r= this, but can I do that if I haven't installed and tested it?

Comment 33

[Mark Hammond [:markh] [:mhammond]]

Attachment: Slightly modified patch - removed the reference from "Shift-F10" in the comment, and used "#ifdef" instead of "#if defined" to be consistent with other code already in the file. No code has changed.

Comment 34

[John Morrison]

> John: I'd like to r= this, but can I do that if I haven't installed and 
> tested it?

Yes. It's primarily a review of the correctness of the code (handles all 
conditions correctly, uses appropriate style, follows coding guidelines). But, 
I'm a QA weenie, not a developer weenie, so don't take my word as gospel :-]

FWIW, I have built and poked at this, and it works fine. (One trivial nit is
that if a menu is already open, pressing F10 will rollup the menu (same as a
win32 app would), but leaves none of the menus active whereas the native menu
will leave the rolledup menu active (i.e. a down arrow will reopen that menu
for the native case). But if that is never "fixed", it would be fine by me.

Comment 35

[:deanis74]

OK, then...  r=me

John, your last comment there didn't make any sense.  From what you say ("the 
native menu will leave the rolledup menu active"), things work exactly as they 
should.  In Windows, at least Win2000 which is what I have in front of me, if I 
press F10 to dismiss a menu, the menus are no longer active.  This patch should 
behave the same way.

Comment 36

[John Morrison]

Well, dang, I thought I was seeing it remain active for native menus, but 
now I don't see that (I was/am on crack). What it is is that :hover is not
set, whereas native does show their :hover equivalent. But this is in the
'who cares' category and I shouldn't have even mentioned it.

markh - send email to hyatt about an sr=.

Comment 37

[timeless]

alt, f10 behave the same.
jgrm is thinking about esc which leaves focus in the menu bar, but the menu 
rolled up.

Comment 38

[David Hyatt]

sr=hyatt

Comment 39

[:deanis74]

There's an r= and an sr= on this.  Is anything else needed, or can someone check 
it in before it rots?

Comment 40

[Mark Hammond [:markh] [:mhammond]]

There is some karma here :)  I've been away for a week, and was just looking at 
this as the mail came in!  Will check in now.

Comment 41

[Mark Hammond [:markh] [:mhammond]]

Checked in rev 3.322 of nsWindow.cpp, rev 1.52 of nsMenuBarListener.cpp.  
Marking as "fixed" - I hope that is correct - this is my first real "fix"!

Comment 42

[:deanis74]

Hmmm.... I'm assuming that I'm checking this on a build that has your changes. 
I'm using 2001031204, and F10 activates the menu but a subsequent press doesn't
de-activate it.

Are you sure you checked in nsMenuListener.cpp?  I can see the changes to
nsMenuBarListener.cpp, but there's nothing in there for nsMenuListener.cpp for
the past month.

Re-opening because of this.

Comment 43

[Mark Hammond [:markh] [:mhammond]]

Apologies - my mistake.  rev 1.9 of nsMenuListener.cpp checked in.  Back to 
FIXED.

Comment 44

[:deanis74]

Looks much better in LXR.  Have to wait until tomorrow's re-spin to test.

Comment 45

[:deanis74]

Works great.

(Holy, them's some big radio buttons on HTML forms today!)

Comment 46

[Aaron Leventhal]

We need F10 for all operating systems, not just windows.

Comment 47

[:deanis74]

Then open a new bug.  Don't mess with a bug that's already fixed and verified.  
Moving back to fixed.

Comment 48

[:deanis74]

Verifying based on my original verification from yesterday.

ps. Aaron, we don't need it on all OSes, do we?  I don't think Mac needs this.