Last Comment Bug 126189 - event.shiftKey always false in oncommand="" of <menuentry> when triggered by mouse/keyboard
: event.shiftKey always false in oncommand="" of <menuentry> when triggered by ...
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: DOM: Events (show other bugs)
: Trunk
: x86 Linux
: -- normal with 1 vote (vote)
: ---
Assigned To: Neil Deakin
:
: Andrew Overholt [:overholt]
Mentors:
Depends on:
Blocks: 108348 59132 72361 79990
  Show dependency treegraph
 
Reported: 2002-02-18 06:52 PST by Christian :Biesinger (don't email me, ping me on IRC)
Modified: 2011-04-13 11:28 PDT (History)
11 users (show)
dbaron: blocking1.8b-
enndeakin: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
propagate state of event modifier keys to menu command handlers (2.87 KB, patch)
2002-09-26 10:33 PDT, Dan M
jag-mozilla: review+
bryner: superreview+
Details | Diff | Splinter Review
Testcase (729 bytes, application/vnd.mozilla.xul+xml)
2004-03-28 06:44 PST, Wladimir Palant
no flags Details
Additional patch (15.63 KB, patch)
2004-03-28 06:48 PST, Wladimir Palant
no flags Details | Diff | Splinter Review
Additional patch (updated) (13.45 KB, patch)
2005-05-07 10:11 PDT, Wladimir Palant
bryner: review+
Details | Diff | Splinter Review
handle key events (10.56 KB, patch)
2010-07-26 18:35 PDT, Neil Deakin
neil: review+
Details | Diff | Splinter Review

Description Christian :Biesinger (don't email me, ping me on IRC) 2002-02-18 06:52:25 PST
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.
Comment 1 Chris Nebergall (cneberg@gmail.com) 2002-03-29 13:24:58 PST
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.
Comment 2 J Luh 2002-04-17 06:24:17 PDT
See also bug 108348.
Comment 3 Akkana Peck 2002-06-25 13:02:16 PDT
I'll take a look at this and try to figure out what's needed.
Comment 4 Chris Nebergall (cneberg@gmail.com) 2002-06-25 21:09:51 PDT
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.
Comment 5 gabriel 2002-07-26 04:57:19 PDT
.
Comment 6 Dan M 2002-09-26 10:30:07 PDT
un-futuring
Comment 7 Dan M 2002-09-26 10:33:30 PDT
Created attachment 100733 [details] [diff] [review]
propagate state of event modifier keys to menu command handlers
Comment 8 jag (Peter Annema) 2002-09-26 11:20:40 PDT
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
Comment 9 Dan M 2002-09-26 13:23:40 PDT
  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))
?
Comment 10 Dan M 2002-09-26 13:31:53 PDT
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 11 Brian Ryner (not reading) 2002-09-26 16:15:10 PDT
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.
Comment 12 Dan M 2002-09-26 18:20:39 PDT
.
Comment 13 Wladimir Palant 2004-03-28 04:56:38 PST
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.
Comment 14 Wladimir Palant 2004-03-28 06:44:49 PST
Created attachment 144956 [details]
Testcase

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.
Comment 15 Wladimir Palant 2004-03-28 06:48:02 PST
Created attachment 144957 [details] [diff] [review]
Additional patch

Propagate keyboard events to menu command handlers
Comment 16 Ben Basson 2005-02-04 17:07:07 PST
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.
Comment 17 Asa Dotzler [:asa] 2005-02-08 08:37:40 PST
Johnny, can you take a look at this bug? Do you think it's worth blocking the
1.8 releases for?
Comment 18 David Baron :dbaron: ⌚️UTC-10 2005-02-17 11:24:01 PST
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 19 Wladimir Palant 2005-05-07 10:11:20 PDT
Created attachment 182881 [details] [diff] [review]
Additional patch (updated)
Comment 20 Brian Ryner (not reading) 2006-02-19 00:31:28 PST
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.
Comment 21 neil@parkwaycc.co.uk 2006-03-24 13:13:08 PST
Since nsIMenuParent is an implementation detail, i.e. an interface internal to Gecko, then does it still fall under normal branch rules?
Comment 22 Neil Deakin 2010-07-26 09:48:51 PDT
Fixed long ago.
Comment 23 Wladimir Palant 2010-07-26 09:53:22 PDT
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...
Comment 24 Neil Deakin 2010-07-26 09:56:21 PDT
(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.
Comment 25 Neil Deakin 2010-07-26 18:35:42 PDT
Created attachment 460407 [details] [diff] [review]
handle key events

This patch handles key events and passes the modifiers onwards to the command event The call to Execute() is changed to take the event.
Comment 26 :Ehsan Akhgari 2010-11-19 09:31:16 PST
This can't land unless it's approved for 2.0.

Note You need to log in before you can comment on or make changes to this bug.