Closed Bug 126189 Opened 22 years ago Closed 13 years ago

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

Categories

(Core :: DOM: Events, defect)

x86
Linux
defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: Biesinger, Assigned: enndeakin)

References

(Blocks 1 open bug)

Details

Attachments

(3 files, 2 obsolete files)

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.
Blocks: 59132
See also bug 108348.
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.
.
Target Milestone: --- → Future
un-futuring
Assignee: akkana → danm
Target Milestone: Future → mozilla1.2beta
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+
  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.
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+
.
Status: NEW → RESOLVED
Closed: 22 years ago
Resolution: --- → FIXED
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 → ---
Attached file 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.
Attached patch Additional patch (obsolete) — Splinter Review
Propagate keyboard events to menu command handlers
Assignee: danm-moz → trev
Status: REOPENED → ASSIGNED
Attachment #144957 - Flags: review?(jag)
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?
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-
Attached patch Additional patch (updated) (obsolete) — Splinter Review
Attachment #144957 - Attachment is obsolete: true
Attachment #182881 - Flags: review?(bryner)
Attachment #144957 - Flags: review?(jag)
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+
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
Closed: 22 years ago14 years ago
Resolution: --- → FIXED
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.
Summary: event.shiftKey always false in oncommand="" of <menuentry> → event.shiftKey always false in oncommand="" of <menuentry> when triggered by mouse/keyboard
Assignee: trev.moz → nobody
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)
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
Closed: 14 years ago13 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: