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).

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

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.

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).

reassigning to saari

reassigning QA to me, updating component.

Taking menu/popup bugs.

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

bulk accepting xpmenu/popup bugs. sigh.

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

Mass moving M19 bugs to M20

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

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.

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.

See also bug 36665.

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.

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

Mass-moving all M20-M30 XPToolkit bugs to Future

*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.

Is this bug still valid?

Yes.

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).

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

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.

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 :)

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= :-)

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

+ // 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 ...

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.

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.

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?

> 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.

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.

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=.

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

sr=hyatt

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

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.

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"!

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.

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

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

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

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

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

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.