295285 - onpopupshowing does not expose accesskeys (event.shiftKey)

Status: NEW

(Core :: DOM: Events, enhancement)

Description

[nrlz]

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

Comment 1

[nrlz]

Attachment: testcase with onpopupshowing

Comment 2

[nrlz]

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

Comment 3

[[e-mail]]

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.

Comment 4

[Martijn Wargers (dead)]

Isn't this just invalid? Onpupshowing isn't a mouse event.

Comment 5

[nrlz]

(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.

Comment 6

[Jan Varga [:janv]]

I need this for bug 617528

Comment 7

[Jan Varga [:janv]]

Attachment: patch v1

Comment 8

[Neil Deakin]

Comment on attachment 544268 [details] [diff] [review]
patch v1

># HG changeset patch
># Parent f9f7e6821ba6e826def148bc57b2db7f34f1a66f
># User Jan Varga <[e-mail]>
>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.

Comment 9

[Neil Deakin]

Also, a test is needed. You can just add something to popup_trigger.js if you like.

Comment 10

[Jan Varga [:janv]]

Ok, thanks.
Yeah, I planned to use bit fields, then I found out it's a singleton object ...
I'll rework it.

Comment 11

[Jan Varga [:janv]]

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.

Comment 12

[Jan Varga [:janv]]

Attachment: patch v2

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 13

[Neil Deakin]

Comment on attachment 544725 [details] [diff] [review]
patch v2

Looks ok, although I'd add an extra test for the other modifiers as well.

Comment 14

[Jan Varga [:janv]]

http://hg.mozilla.org/mozilla-central/rev/060d1ceb4f2b

Comment 15

[Neil Deakin]

Jan, is anything else to be done here?

Comment 16

[Jan Varga [:janv]]

(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.