If I check for event.shiftKey in the oncommand handler of an <menuitem>, it is always false even if shift was pressed. It should be true if shift was pressed and false otherwise. Blocks bug 79990.

None of the regular events are set event.shiftKey, event.ctrlKey, and event.button. This is also needed for bug 59132 so items on Personal Toolbar sub menus can be treated as normal links.

See also bug 108348.

I'll take a look at this and try to figure out what's needed.

I filed bug 154306 for event.button not being set for oncommand. Feel free to dup it against this one if you want. But since I'm not sure any controls set event.button for oncommand I didn't know if it it would require a seperate fix.

.

un-futuring

Comment on attachment 100733 [details] [diff] [review] propagate state of event modifier keys to menu command handlers Could you change the test to == NS_INPUT_EVENT so we'll also grab the modifier states when a user arrows down to the menu item and hits enter? r=jag with that

if (aEvent && (aEvent->eventStructType == NS_INPUT_EVENT || aEvent->eventStructType == NS_MOUSE_EVENT || aEvent->eventStructType == NS_MOUSE_SCROLL_EVENT || aEvent->eventStructType == NS_KEY_EVENT || aEvent->eventStructType == NS_TEXT_EVENT || aEvent->eventStructType == NS_ACCESSIBLE_EVENT)) ?

going with if (aEvent && (aEvent->eventStructType == NS_MOUSE_EVENT || aEvent->eventStructType == NS_KEY_EVENT || aEvent->eventStructType == NS_ACCESSIBLE_EVENT)) { , the subset of event classes which inherit from nsInputEvent and seem likely to happen in menus.

Comment on attachment 100733 [details] [diff] [review] propagate state of event modifier keys to menu command handlers I'd suggest creating a temporary pointer variable with the static cast so you don't need to repeat it 4 times. Other than that, sr=bryner.

.

This fixes only one part of bug - the modifiers are still not set when the user chooses a menu item with keyboard (because of |Execute(0)| in nsMenuFrame::Enter()), relevant for bug 72361. Reopening.

The testcase has a menu and a context menu. Selecting a menu item with the mouse shows correct values for modifier keys. Selecting it with the keyboard (Enter or shortcut key) always shows false for both shiftKey and ctrlKey.

Propagate keyboard events to menu command handlers

Can we finally get this one sorted out? The necessary workarounds are quite irritating. Nominating for 1.8b, since there are patches available, although they will most likely need updating.

Johnny, can you take a look at this bug? Do you think it's worth blocking the 1.8 releases for?

I don't think this is a blocker, but there's nothing wrong with getting a fix. If you want to help it get fixed, update the patches and request review from people who are around (e.g., try bryner instead of jag).

Comment on attachment 182881 [details] [diff] [review] Additional patch (updated) >@@ -1663,17 +1663,17 @@ nsMenuPopupFrame::ShortcutNavigation(nsI > // This applies to us. Let's see if one of the shortcuts applies > PRBool action; > nsIMenuFrame* result = FindMenuWithShortcut(aKeyEvent, action); > if (result) { > // We got one! > aHandledFlag = PR_TRUE; > SetCurrentMenuItem(result); > if (action) >- result->Enter(); >+ result->Enter(NS_STATIC_CAST(nsIDOMEvent*, aKeyEvent)); Casting here should not be necessary. >--- layout/xul/base/src/nsIMenuParent.h 18 Apr 2004 14:30:36 -0000 1.24 >+++ layout/xul/base/src/nsIMenuParent.h 7 May 2005 17:06:40 -0000 >@@ -163,17 +163,17 @@ public: > NS_IMETHOD RemoveKeyboardNavigator() = 0; > > // Used to move up, down, left, and right in menus. > NS_IMETHOD KeyboardNavigation(PRUint32 aKeyCode, PRBool& aHandledFlag) = 0; > NS_IMETHOD ShortcutNavigation(nsIDOMKeyEvent* aKeyEvent, PRBool& aHandledFlag) = 0; > // Called when the ESC key is held down to close levels of menus. > NS_IMETHOD Escape(PRBool& aHandledFlag) = 0; > // Called to execute a menu item. >- NS_IMETHOD Enter() = 0; >+ NS_IMETHOD Enter(nsIDOMEvent* aEvent) = 0; > You should change the IID for nsIMenuParent. Also, unfortunately I think this interface change will prevent us from taking this fix for FF2. If you can come up with a way to do this that doesn't change an interface, we can probably consider it. >--- layout/xul/base/public/nsIMenuFrame.h 11 Feb 2005 13:18:40 -0000 1.14 >+++ layout/xul/base/public/nsIMenuFrame.h 7 May 2005 17:06:40 -0000 >@@ -64,17 +65,17 @@ public: > NS_IMETHOD MenuIsOpen(PRBool& aResult) = 0; > NS_IMETHOD MenuIsContainer(PRBool& aResult) = 0; > NS_IMETHOD MenuIsChecked(PRBool& aResult) = 0; > NS_IMETHOD MenuIsDisabled(PRBool& aResult) = 0; > > NS_IMETHOD SelectFirstItem() = 0; > > NS_IMETHOD Escape(PRBool& aHandledFlag) = 0; >- NS_IMETHOD Enter() = 0; >+ NS_IMETHOD Enter(nsIDOMEvent* aEvent) = 0; You'll need to rev this IID as well. r=me with those fixes.

Since nsIMenuParent is an implementation detail, i.e. an interface internal to Gecko, then does it still fall under normal branch rules?

Fixed long ago.

See comment 13 - this issue is still not fixed for the keyboard. Reopening again. Unfortunately, I don't have time to update my original patch and I am not sure anybody else cares. So if you want to resolve as won't fix I won't object...

(In reply to comment #23) > Unfortunately, I don't have time to update my original patch and I am not sure > anybody else cares. So if you want to resolve as won't fix I won't object... OK, you should either file a different bug, or make it clear in the bug title.

This patch handles key events and passes the modifiers onwards to the command event The call to Execute() is changed to take the event.

This can't land unless it's approved for 2.0.