event.shiftKey always false in oncommand="" of <menuentry> when triggered by mouse/keyboard

RESOLVED FIXED

Status

()

Core
DOM: Events
RESOLVED FIXED
16 years ago
6 years ago

People

(Reporter: Biesinger, Assigned: Neil Deakin (not available until Aug 9))

Tracking

(Blocks: 1 bug)

Trunk
x86
Linux
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.8b -
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 2 obsolete attachments)

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.
Blocks: 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.
Blocks: 59132

Comment 2

15 years ago
See also bug 108348.

Comment 3

15 years ago
I'll take a look at this and try to figure out what's needed.
Assignee: joki → akkana
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

15 years ago
.
Blocks: 72361

Updated

15 years ago
Target Milestone: --- → Future

Comment 6

15 years ago
un-futuring
Assignee: akkana → danm
Target Milestone: Future → mozilla1.2beta

Comment 7

15 years ago
Created attachment 100733 [details] [diff] [review]
propagate state of event modifier keys to menu command handlers

Comment 8

15 years ago
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
Attachment #100733 - Flags: review+

Comment 9

15 years ago
  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

15 years ago
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.

Updated

15 years ago
Blocks: 108348
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.
Attachment #100733 - Flags: superreview+

Comment 12

15 years ago
.
Status: NEW → RESOLVED
Last Resolved: 15 years ago
Resolution: --- → FIXED

Comment 13

13 years ago
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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---

Comment 14

13 years ago
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

13 years ago
Created attachment 144957 [details] [diff] [review]
Additional patch

Propagate keyboard events to menu command handlers

Updated

13 years ago
Assignee: danm-moz → trev
Status: REOPENED → ASSIGNED

Updated

13 years ago
Attachment #144957 - Flags: review?(jag)

Comment 16

13 years ago
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.
Flags: blocking1.8b?

Comment 17

13 years ago
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).
Flags: blocking1.8b? → blocking1.8b-

Comment 19

12 years ago
Created attachment 182881 [details] [diff] [review]
Additional patch (updated)
Attachment #144957 - Attachment is obsolete: true
Attachment #182881 - Flags: review?(bryner)

Updated

12 years ago
Attachment #144957 - Flags: review?(jag)

Updated

12 years ago
Target Milestone: mozilla1.2beta → ---
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.
Attachment #182881 - Flags: review?(bryner) → review+

Comment 21

11 years ago
Since nsIMenuParent is an implementation detail, i.e. an interface internal to Gecko, then does it still fall under normal branch rules?
QA Contact: vladimire → events
Fixed long ago.
Status: ASSIGNED → RESOLVED
Last Resolved: 15 years ago7 years ago
Resolution: --- → FIXED

Comment 23

7 years 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...
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(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.

Updated

7 years ago
Summary: event.shiftKey always false in oncommand="" of <menuentry> → event.shiftKey always false in oncommand="" of <menuentry> when triggered by mouse/keyboard

Updated

7 years ago
Assignee: trev.moz → nobody
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.
Assignee: nobody → enndeakin
Attachment #182881 - Attachment is obsolete: true
Status: REOPENED → ASSIGNED
Attachment #460407 - Flags: review?(neil)

Updated

7 years ago
Attachment #460407 - Flags: review?(neil) → review+
Keywords: checkin-needed
This can't land unless it's approved for 2.0.
Keywords: checkin-needed
http://hg.mozilla.org/mozilla-central/rev/fdcb15cbbaed
Status: ASSIGNED → RESOLVED
Last Resolved: 7 years ago6 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.