Closed
Bug 126189
Opened 23 years ago
Closed 14 years ago
event.shiftKey always false in oncommand="" of <menuentry> when triggered by mouse/keyboard
Categories
(Core :: DOM: Events, defect)
Tracking
()
RESOLVED
FIXED
People
(Reporter: Biesinger, Assigned: enndeakin)
References
(Blocks 1 open bug)
Details
Attachments
(3 files, 2 obsolete files)
2.87 KB,
patch
|
jag+mozilla
:
review+
bryner
:
superreview+
|
Details | Diff | Splinter Review |
729 bytes,
application/vnd.mozilla.xul+xml
|
Details | |
10.56 KB,
patch
|
neil
:
review+
|
Details | Diff | Splinter Review |
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•23 years ago
|
||
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.
Comment 3•22 years ago
|
||
I'll take a look at this and try to figure out what's needed.
Assignee: joki → akkana
Comment 4•22 years ago
|
||
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.
Updated•22 years ago
|
Target Milestone: --- → Future
un-futuring
Assignee: akkana → danm
Target Milestone: Future → mozilla1.2beta
Comment 8•22 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+
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•22 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.
Comment 11•22 years ago
|
||
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 13•21 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•21 years ago
|
||
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•21 years ago
|
||
Propagate keyboard events to menu command handlers
Updated•21 years ago
|
Assignee: danm-moz → trev
Status: REOPENED → ASSIGNED
Updated•21 years ago
|
Attachment #144957 -
Flags: review?(jag)
Comment 16•20 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•20 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•20 years ago
|
||
Attachment #144957 -
Attachment is obsolete: true
Attachment #182881 -
Flags: review?(bryner)
Updated•20 years ago
|
Attachment #144957 -
Flags: review?(jag)
Updated•19 years ago
|
Target Milestone: mozilla1.2beta → ---
Comment 20•19 years ago
|
||
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•19 years ago
|
||
Since nsIMenuParent is an implementation detail, i.e. an interface internal to Gecko, then does it still fall under normal branch rules?
Updated•15 years ago
|
QA Contact: vladimire → events
Assignee | ||
Comment 22•14 years ago
|
||
Fixed long ago.
Status: ASSIGNED → RESOLVED
Closed: 22 years ago → 14 years ago
Resolution: --- → FIXED
Comment 23•14 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 → ---
Assignee | ||
Comment 24•14 years ago
|
||
(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•14 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•14 years ago
|
Assignee: trev.moz → nobody
Assignee | ||
Comment 25•14 years ago
|
||
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•14 years ago
|
Attachment #460407 -
Flags: review?(neil) → review+
Assignee | ||
Updated•14 years ago
|
Keywords: checkin-needed
Assignee | ||
Comment 27•14 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 14 years ago → 14 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•