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
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
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.
(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
I need this for bug 617528
Assignee: nobody → Jan.Varga
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Comment on attachment 544268 [details] [diff] [review] patch v1 ># HG changeset patch ># Parent f9f7e6821ba6e826def148bc57b2db7f34f1a66f ># User Jan Varga <email@example.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.
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
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.
You need to log in before you can comment on or make changes to this bug.