Last Comment Bug 639338 - Unable to specify XBL modifiers for all possible mouse events
: Unable to specify XBL modifiers for all possible mouse events
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: XBL (show other bugs)
: unspecified
: All All
: -- normal (vote)
: ---
Assigned To: neil@parkwaycc.co.uk
:
: Andrew Overholt [:overholt]
Mentors:
Depends on:
Blocks: 650950 659968
  Show dependency treegraph
 
Reported: 2011-03-06 09:41 PST by neil@parkwaycc.co.uk
Modified: 2011-05-26 09:16 PDT (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch for DOMMouseScroll only (1.15 KB, patch)
2011-03-06 09:43 PST, neil@parkwaycc.co.uk
no flags Details | Diff | Splinter Review
Patch for mouse, scroll and drag events (3.54 KB, patch)
2011-03-07 06:15 PST, neil@parkwaycc.co.uk
bugs: feedback+
enndeakin: feedback+
Details | Diff | Splinter Review
With test (6.94 KB, patch)
2011-04-07 16:04 PDT, neil@parkwaycc.co.uk
bugs: review+
Details | Diff | Splinter Review

Description neil@parkwaycc.co.uk 2011-03-06 09:41:29 PST
XBL special-cases key and mouse events so that it can check for modifiers. Key events are special-cased in nsXBLBinding::InstallEventHandlers, while mouse events are special-cased in NS_NewXBLEventHandler. However I noticed that DOMMouseScroll is missing from this list, which (for instance) means that instead of being able to use modifiers="accel", search.xml needs to #ifdef its modifier based on the platform.

I don't have an exhaustive list of mouse event names so I don't know whether any other mouse events are missing, thus CCing some people who might know.
Comment 1 neil@parkwaycc.co.uk 2011-03-06 09:43:21 PST
Created attachment 517279 [details] [diff] [review]
Patch for DOMMouseScroll only
Comment 2 Neil Deakin 2011-03-06 12:42:42 PST
At least also: MozMouseHittest, dragstart, drag, dragleave, drop, dragend, popupshowing, popupshown, popuphiding, popuphidden

Plus, all of the gesture and touch events listed at
  http://mxr.mozilla.org/mozilla-central/source/content/base/src/nsContentUtils.cpp#675

It may be easier and more efficient just to add an indicator the events that are or inherit from mouse event to the table in nsContentUtils::InitializeEventTable.
Comment 3 neil@parkwaycc.co.uk 2011-03-06 13:06:10 PST
(In reply to comment #2)
Thanks for the list!

> At least also: MozMouseHittest
Surely it makes no sense to consider modifiers in this case?

> dragstart, drag, dragleave, drop, dragend
Ah, those are the new drag events? I see the old ones are there already.

> popupshowing, popupshown, popuphiding, popuphidden
I'm not convinced that we need to consider modifiers. IIRC these are only mouse events because they need to have coordinates.

> Plus, all of the gesture and touch events
Again, I remain unconvinced.

> It may be easier and more efficient just to add an indicator the events that
> are or inherit from mouse event to the table in
> nsContentUtils::InitializeEventTable.
Unfortunately that table has "on" prefixes for all of its event atom names.
Comment 4 Neil Deakin 2011-03-06 13:45:41 PST
The list is just the events I saw that inherit from mouse event. I don't even know what some of the events are for.

The drag events at least should be added.
Comment 5 Olli Pettay [:smaug] 2011-03-07 03:43:25 PST
(In reply to comment #3)
> > It may be easier and more efficient just to add an indicator the events that
> > are or inherit from mouse event to the table in
> > nsContentUtils::InitializeEventTable.
> Unfortunately that table has "on" prefixes for all of its event atom names.

You can access entries in the table either with onfoo atom, or using foo string.
see sAtomEventTable and sStringEventTable
Comment 6 neil@parkwaycc.co.uk 2011-03-07 06:15:02 PST
Created attachment 517404 [details] [diff] [review]
Patch for mouse, scroll and drag events
Comment 7 Olli Pettay [:smaug] 2011-03-07 06:25:47 PST
Comment on attachment 517404 [details] [diff] [review]
Patch for mouse, scroll and drag events


>   /**
>+   * Return the category for the event with the given name. The name is the
>+   * event name *without* the 'on' prefix. Returns NS_EVENT if the event
>+   * doesn't match a known event name.
>+   *
>+   * @param aName the event name to look up
>+   */
>+  static PRUint32 GetEventType(const nsAString& aName);

Could you call this GetEventStructType? Or
GetEventCategory()


>+  switch (nsContentUtils::GetEventType(nsDependentAtomString(aEventType))) {
>+    case NS_MOUSE_EVENT:
>+    case NS_MOUSE_SCROLL_EVENT:
>+    case NS_DRAG_EVENT:
Add also NS_SIMPLE_GESTURE_EVENT


Some tests, please.
Comment 8 Neil Deakin 2011-03-07 06:42:40 PST
Comment on attachment 517404 [details] [diff] [review]
Patch for mouse, scroll and drag events

>+   * Return the category for the event with the given name. The name is the
>+   * event name *without* the 'on' prefix. Returns NS_EVENT if the event
>+   * doesn't match a known event name.

Some known events will return NS_EVENT as well. Some also return NS_EVENT_NULL so there doesn't seem to be a good return value. But I'd change the comment to be clearer.
Comment 9 neil@parkwaycc.co.uk 2011-03-07 06:43:32 PST
Enn, do we have any existing XBL handler modifier tests?
Comment 10 Neil Deakin 2011-03-07 06:44:11 PST
Don't know.
Comment 11 neil@parkwaycc.co.uk 2011-04-07 16:04:10 PDT
Created attachment 524522 [details] [diff] [review]
With test
Comment 12 neil@parkwaycc.co.uk 2011-04-18 14:01:09 PDT
Pushed changeset c65c39ca8b11 to mozilla-central.

Note You need to log in before you can comment on or make changes to this bug.