onpopupshowing does not expose accesskeys (event.shiftKey)

NEW
Unassigned

Status

()

--
enhancement
14 years ago
a month ago

People

(Reporter: nrlz, Unassigned)

Tracking

({testcase})

Trunk
x86
Windows XP
testcase
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 1 obsolete attachment)

757 bytes, application/vnd.mozilla.xul+xml
Details
9.04 KB, patch
enndeakin
: review+
Details | Diff | Splinter Review
(Reporter)

Description

14 years ago
User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7.8) Gecko/20050511 Firefox/1.0.4
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7.8) Gecko/20050511 Firefox/1.0.4

The onpopupshowing event does not expose the state of accesskeys (shiftKey,
ctrlKey) when the event is fired. If this is as designed, then I am requesting
that it be changed. By exposing the accesskeys, it is possible to modify the
popupmenu depending on the accesskeys pressed and therefore offer a more dynamic
popupmenu.

ADVANTAGE:
The best use of this feature, that I can think of, is to hide complexity. I am
trying to write an extension that will only show one menuitem on the contextmenu
and will be used in 90% of the usage scenarios. But when the shift key is
pressed, it will instead show a sub-menu with more advanced menuitems that are
only applicable in the other 10% of the usage scenarios.

By leveraging accesskeys, I can:
- keep the context-menu clean 90% of the time, while still being able to reveal
the advanced options when needed.
- increase the speed at which users can access the menuitem. When only 1
menuitem is visible, I can directly place it on the main contextmenu which is
only 1 click away. Otherwise it will be displayed in a submenu which takes more
time to access.
- reduce the complexity, since novices are unlikely to use the advanced
menuitems and therefore will be invisible to them.

Reproducible: Always

Steps to Reproduce:
(see testcase)
1. click button
Actual Results:  
(see testcase)
onpopupshowing always has shiftKey/ctrlKey == false

Expected Results:  
shiftKey|ctrlKey should reflect the state of accesskeys
(Reporter)

Comment 1

14 years ago
(Reporter)

Comment 2

14 years ago
I forgot to include some related bug reports.
bug 126189 is for the oncommand event and supports it
bug 154306 comment 3 is also for oncommand, but is against it

Updated

14 years ago
Keywords: testcase
All sorts of things can and do trigger popupshowing events; left-clicks on menus
and menubuttons; right-clicks (which is platform dependent; some use mouse down
and some use mouse up); hover timeouts; script. In the case of the latter two
any information about modifiers is inaccurate and/or irrelevant.
Isn't this just invalid? Onpupshowing isn't a mouse event.
(Reporter)

Comment 5

14 years ago
(In reply to comment #3)
> All sorts of things can and do trigger popupshowing events; left-clicks on 
> menus and menubuttons; right-clicks (which is platform dependent; some use 
> mouse down and some use mouse up); hover timeouts; script. In the case of the 
> latter two any information about modifiers is inaccurate and/or irrelevant.

There is similar dicussion about this in bug 126189 which is for the |oncommand|
event. That one is also not a mouse event, because it can be called via scripts
or via keyboard. But the discussion there concluded that it was a good
special-case to support and so that oncommand now also preserves the modifier keys.

I'm suggesting that |onpopupshowing| can deserve special treatment too.
Assignee: events → nobody
QA Contact: ian → events

Comment 6

8 years ago
I need this for bug 617528
Assignee: nobody → Jan.Varga

Updated

8 years ago
Blocks: 617528
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true

Comment 7

8 years ago
Posted patch patch v1 (obsolete) — Splinter Review
Attachment #544268 - Flags: review?(enndeakin)
Comment on attachment 544268 [details] [diff] [review]
patch v1

># HG changeset patch
># Parent f9f7e6821ba6e826def148bc57b2db7f34f1a66f
># User Jan Varga <jan.varga@gmail.com>
>Bug 295285 - onpopupshowing does not expose accesskeys (event.shiftKey)
>
>diff --git a/layout/xul/base/public/nsXULPopupManager.h b/layout/xul/base/public/nsXULPopupManager.h
>--- a/layout/xul/base/public/nsXULPopupManager.h
>+++ b/layout/xul/base/public/nsXULPopupManager.h
>@@ -762,16 +762,22 @@ protected:
> 
>   // range parent and offset set in SetTriggerEvent
>   nsCOMPtr<nsIDOMNode> mRangeParent;
>   PRInt32 mRangeOffset;
>   // Device pixels relative to the showing popup's presshell's
>   // root prescontext's root frame.
>   nsIntPoint mCachedMousePoint;
> 
>+  // cached modifiers
>+  PRBool mIsAlt;
>+  PRBool mIsControl;
>+  PRBool mIsShift;
>+  PRBool mIsMeta;

I'd use bitfields here, or at least PRPackedBools. Or just use a single field with defines for each flag and then you can set/reset just by clearing the value once rather than four times.
Attachment #544268 - Flags: review?(enndeakin) → review+
Also, a test is needed. You can just add something to popup_trigger.js if you like.
Ok, thanks.
Yeah, I planned to use bit fields, then I found out it's a singleton object ...
I'll rework it.
Actually, I would call the patch "Part 1" because it works only when an event is passed to the xul popup manager.
For example, nsMenuFrame opens popups by calling nsXULPopupManager::ShowMenu()

I tried to test it in popup_trigger.js, but that one uses <button type="menu">
So almost all those popups in the test are opened by calling ShowMenu().

However, it works ok for context menus which are opened by calling ShowPopupAtScreen()

Neil, are you ok with pushing the patch w/o an automatic test ?

The shift right click will be tested in test_contextmenu.html anyway, that will by updated for bug 617528.
Posted patch patch v2Splinter Review
replaced bools with flags, updated popup_shared.js to support modifiers mask and updated popup_trigger.js to check the shift key flag for popups initialized with an event
Attachment #544268 - Attachment is obsolete: true
Attachment #544725 - Flags: review?(enndeakin)
Comment on attachment 544725 [details] [diff] [review]
patch v2

Looks ok, although I'd add an extra test for the other modifiers as well.
Attachment #544725 - Flags: review?(enndeakin) → review+
Jan, is anything else to be done here?
(In reply to Neil Deakin from comment #15)
> Jan, is anything else to be done here?

yeah, the popupshowing event fired for e.g. <button type="menu"> doesn't expose accesskeys

I tried to describe the problem in comment 11.

Basically, accesskeys are currently exposed in the popupshowing event only when an event is passed to nsXULPopupManager.

Updated

7 months ago
Assignee: jvarga → nobody
Status: ASSIGNED → NEW
You need to log in before you can comment on or make changes to this bug.