Last Comment Bug 731878 - Implement DOM3 mouse event's buttons and getModifierState()
: Implement DOM3 mouse event's buttons and getModifierState()
Status: RESOLVED FIXED
: dev-doc-needed
Product: Core
Classification: Components
Component: DOM: Events (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla15
Assigned To: Masayuki Nakano [:masayuki] (Mozilla Japan)
:
: Andrew Overholt [:overholt]
Mentors:
: 738107 (view as bug list)
Depends on: 749553 749560 749500 753256 769190 957490 1223366
Blocks: 630811 719320
  Show dependency treegraph
 
Reported: 2012-02-29 17:30 PST by Masayuki Nakano [:masayuki] (Mozilla Japan)
Modified: 2015-11-11 08:58 PST (History)
8 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
part.1 Implement D3E initMouseEvent() and getModifierState() but they shouldn't be public (17.95 KB, patch)
2012-02-29 22:11 PST, Masayuki Nakano [:masayuki] (Mozilla Japan)
no flags Details | Diff | Splinter Review
part.1 Implement D3E initMouseEvent() and getModifierState() but they shouldn't be public (27.63 KB, patch)
2012-03-02 05:27 PST, Masayuki Nakano [:masayuki] (Mozilla Japan)
no flags Details | Diff | Splinter Review
part.2 Implement D3E MouseEvent.buttons attribute (24.49 KB, patch)
2012-03-02 05:31 PST, Masayuki Nakano [:masayuki] (Mozilla Japan)
bugs: review+
Details | Diff | Splinter Review
part.3 Set modifiers and buttons of nsMouseEvent on Windows (8.28 KB, patch)
2012-03-02 05:32 PST, Masayuki Nakano [:masayuki] (Mozilla Japan)
jmathies: review+
Details | Diff | Splinter Review
part.4 Set modifiers and buttons of nsMouseEvent on GTK (8.22 KB, patch)
2012-03-02 05:32 PST, Masayuki Nakano [:masayuki] (Mozilla Japan)
no flags Details | Diff | Splinter Review
part.5 Set modifiers and buttons of nsMouseEvent on Mac (3.36 KB, patch)
2012-03-02 05:33 PST, Masayuki Nakano [:masayuki] (Mozilla Japan)
no flags Details | Diff | Splinter Review
part.1 Implement D3E initMouseEvent() and getModifierState() but they shouldn't be public (27.63 KB, patch)
2012-03-20 18:01 PDT, Masayuki Nakano [:masayuki] (Mozilla Japan)
bugs: review+
Details | Diff | Splinter Review
part.1 Implement D3E initMouseEvent() and getModifierState() but they shouldn't be public (27.67 KB, patch)
2012-03-21 21:07 PDT, Masayuki Nakano [:masayuki] (Mozilla Japan)
no flags Details | Diff | Splinter Review
part.2 Implement D3E MouseEvent.buttons attribute (23.80 KB, patch)
2012-03-21 21:40 PDT, Masayuki Nakano [:masayuki] (Mozilla Japan)
jst: superreview+
Details | Diff | Splinter Review
part.5 Set modifiers and buttons of nsMouseEvent on Mac (4.30 KB, patch)
2012-03-28 01:15 PDT, Masayuki Nakano [:masayuki] (Mozilla Japan)
smichaud: review+
Details | Diff | Splinter Review
part.6 Initialize modifiers at dispatching events from nsDOMWindowUtils (5.04 KB, patch)
2012-04-11 18:37 PDT, Masayuki Nakano [:masayuki] (Mozilla Japan)
no flags Details | Diff | Splinter Review
part.1 Implement D3E initMouseEvent() and getModifierState() but they shouldn't be public (27.91 KB, patch)
2012-04-18 18:32 PDT, Masayuki Nakano [:masayuki] (Mozilla Japan)
no flags Details | Diff | Splinter Review
part.2 Implement D3E MouseEvent.buttons attribute (23.96 KB, patch)
2012-04-18 18:33 PDT, Masayuki Nakano [:masayuki] (Mozilla Japan)
no flags Details | Diff | Splinter Review
part.4 Set modifiers and buttons of nsMouseEvent on GTK (7.04 KB, patch)
2012-04-18 18:34 PDT, Masayuki Nakano [:masayuki] (Mozilla Japan)
no flags Details | Diff | Splinter Review
part.4 Set modifiers and buttons of nsMouseEvent on GTK (6.08 KB, patch)
2012-04-23 02:07 PDT, Masayuki Nakano [:masayuki] (Mozilla Japan)
karlt: review+
Details | Diff | Splinter Review

Description Masayuki Nakano [:masayuki] (Mozilla Japan) 2012-02-29 17:30:09 PST
When I'm implementing D3E WheelEvent, I realized that I need to implement some attributes of D3E MouseEvent in nsDOMMouseEvent.
Comment 1 Masayuki Nakano [:masayuki] (Mozilla Japan) 2012-02-29 22:11:28 PST
Created attachment 601873 [details] [diff] [review]
part.1 Implement D3E initMouseEvent() and getModifierState() but they shouldn't be public
Comment 2 Masayuki Nakano [:masayuki] (Mozilla Japan) 2012-03-02 05:27:28 PST
Created attachment 602320 [details] [diff] [review]
part.1 Implement D3E initMouseEvent() and getModifierState() but they shouldn't be public

This implements getModifierState() internally. I think that we shouldn't make it scriptable for now because it cannot be initialized for untrusted events. And I think it's better to make it when bug 630811 for consistent with KeyboardEvent.

Additionally, this patch implements anther initMouseEvent() internally. This should be scriptable but we should wait it's specced. This is necessary for bug 719320.
Comment 3 Masayuki Nakano [:masayuki] (Mozilla Japan) 2012-03-02 05:31:15 PST
Created attachment 602321 [details] [diff] [review]
part.2 Implement D3E MouseEvent.buttons attribute

This patch implements MouseEvent.buttons in XP side. I'm not sure why this cannot be initialized by initMouseEvent()...
Comment 4 Masayuki Nakano [:masayuki] (Mozilla Japan) 2012-03-02 05:32:31 PST
Created attachment 602322 [details] [diff] [review]
part.3 Set modifiers and buttons of nsMouseEvent on Windows
Comment 5 Masayuki Nakano [:masayuki] (Mozilla Japan) 2012-03-02 05:32:57 PST
Created attachment 602324 [details] [diff] [review]
part.4 Set modifiers and buttons of nsMouseEvent on GTK
Comment 6 Masayuki Nakano [:masayuki] (Mozilla Japan) 2012-03-02 05:33:26 PST
Created attachment 602325 [details] [diff] [review]
part.5 Set modifiers and buttons of nsMouseEvent on Mac
Comment 7 Masayuki Nakano [:masayuki] (Mozilla Japan) 2012-03-02 05:37:21 PST
FYI: part.3 patch depends on bug 672175.
Comment 8 Masayuki Nakano [:masayuki] (Mozilla Japan) 2012-03-20 18:01:51 PDT
Created attachment 607803 [details] [diff] [review]
part.1 Implement D3E initMouseEvent() and getModifierState() but they shouldn't be public

fix a bug in parser of modifierList.
Comment 9 Olli Pettay [:smaug] 2012-03-21 13:32:42 PDT
Comment on attachment 607803 [details] [diff] [review]
part.1 Implement D3E initMouseEvent() and getModifierState() but they shouldn't be public

ase NS_MOZTOUCH_EVENT:
>     {
>-      newEvent = new nsMozTouchEvent(false, msg, nsnull,
>-                                     static_cast<nsMozTouchEvent*>(mEvent)->streamId);
>-      NS_ENSURE_TRUE(newEvent, NS_ERROR_OUT_OF_MEMORY);
>+      nsMozTouchEvent* oldMozTouchEvent = static_cast<nsMozTouchEvent*>(mEvent);
>+      nsMozTouchEvent* mozTouchEvent =
>+        new nsMozTouchEvent(false, msg, nsnull,
>+                            static_cast<nsMozTouchEvent*>(mEvent)->streamId);
>+      NS_ENSURE_TRUE(mozTouchEvent, NS_ERROR_OUT_OF_MEMORY);
>       isInputEvent = true;
>+      mozTouchEvent->modifiers = oldMozTouchEvent->modifiers;
You never assign mozTouchEvent to newEvent


> 
>+// XXX Following struct and array are used only in
>+//     nsDOMUIEvent::ComputeModifierState(), but if we define them in it,
>+//     we fail to build on Mac at calling mozilla::ArrayLength().
>+struct nsModifierPair {
{ should be in the next line
Comment 10 Masayuki Nakano [:masayuki] (Mozilla Japan) 2012-03-21 20:24:51 PDT
Comment on attachment 602321 [details] [diff] [review]
part.2 Implement D3E MouseEvent.buttons attribute

Thank you, smaug.

Could you sr this, jst?
Comment 11 Masayuki Nakano [:masayuki] (Mozilla Japan) 2012-03-21 20:31:01 PDT
Comment on attachment 602325 [details] [diff] [review]
part.5 Set modifiers and buttons of nsMouseEvent on Mac

Smaug:

I need your feedback. Mac doesn't have NumLock key even with PC's keyboard.

So, we can think that NumLock is always locked on Mac.

Therefore, I think that getModifierState("NumLock") always should return true on Mac for compatibility with other platforms. Do you agree with this?

Simon:

I don't find the API for getting current mouse button state on 10.5. If you know it, let me know. So, MouseEvent.buttons works only 10.6 and later.
Comment 12 Masayuki Nakano [:masayuki] (Mozilla Japan) 2012-03-21 21:07:49 PDT
Created attachment 608218 [details] [diff] [review]
part.1 Implement D3E initMouseEvent() and getModifierState() but they shouldn't be public
Comment 13 Masayuki Nakano [:masayuki] (Mozilla Japan) 2012-03-21 21:40:43 PDT
Created attachment 608222 [details] [diff] [review]
part.2 Implement D3E MouseEvent.buttons attribute

updating for the latest m-c.
Comment 14 Olli Pettay [:smaug] 2012-03-22 05:41:19 PDT
*** Bug 738107 has been marked as a duplicate of this bug. ***
Comment 15 Olli Pettay [:smaug] 2012-03-22 06:26:08 PDT
 
> So, we can think that NumLock is always locked on Mac.
> 
> Therefore, I think that getModifierState("NumLock") always should return
> true on Mac for compatibility with other platforms. Do you agree with this?
Does PC keyboard work like that on OSX? Does it behave like NumLock would be on?
Comment 16 Masayuki Nakano [:masayuki] (Mozilla Japan) 2012-03-22 07:03:46 PDT
(In reply to Olli Pettay [:smaug] from comment #15)
>  
> > So, we can think that NumLock is always locked on Mac.
> > 
> > Therefore, I think that getModifierState("NumLock") always should return
> > true on Mac for compatibility with other platforms. Do you agree with this?
> Does PC keyboard work like that on OSX? Does it behave like NumLock would be
> on?

PC keyboard's NumLock key works as Clear key which is same position NumLock key on full keyboard for Mac. And e.g., 4 key never becomes left arrow key on Mac even with PC keyboard.
Comment 17 Steven Michaud [:smichaud] (Retired) 2012-03-22 13:33:31 PDT
Comment on attachment 602325 [details] [diff] [review]
part.5 Set modifiers and buttons of nsMouseEvent on Mac

Review of attachment 602325 [details] [diff] [review]:
-----------------------------------------------------------------

(In reply to comment #11)

> I don't find the API for getting current mouse button state on
> 10.5. If you know it, let me know. So, MouseEvent.buttons works only
> 10.6 and later.

There's an undocumented CGSGetCurrentMouseButtonState() method
available on OS X 10.5, which I could reverse-engineer if you want.
But (from looking at the assembly code) it appears to only support the
left and right mouse buttons.

In any case I think it's better to get the mouse button state from
'inEvent', by calling [inEvent buttonNumber].  Beware that calling
'buttonNumber' on a non-mouse event (for example a scroll-wheel event)
may throw an Objective-C exception.  And if 'inEvent' is nil, you'll
(of course) only be able to get the information on OS X 10.6 and up.
Also beware that you may not *want* to get this information if
'inEvent' is nil, even where you can:  It's not uncommon for more than
one pointing device to be attached (for example a mouse and a built-in
trackpad), and I'm not sure how the OS decides which device's
button-state to give you. 

All the mouse-event-specific stuff should, I think, be moved from
[ChildView convertGenericCocoaEvent:toGeckoEvent:] to [ChildView
convertCocoaMouseEvent:toGeckoEvent:].

Like Smaug, I have my doubts about always setting MODIFIER_NUMLOCK on
OS X.  As you've pointed out, the Mac doesn't have any support for
numlock functionality, even when used with a PC keyboard.  What do you
mean when you say that we should always set numlock on the Mac "for
compatibility with other platforms"?  Isn't numlock off by default in
Windows?
Comment 18 Karl Tomlinson (back Dec 13 :karlt) 2012-03-22 15:00:28 PDT
I don't know what the default is (I recall a BIOS setting to change the default at least until the OS resets it), but (as I hear it) the point is more that the Mac keyboard behaves as if NumLock is on.

If we send a code for a numeric keypad 4 (and perhaps we don't have a distinct code from the alphanumeric 4), it would seem strange to send that without NumLock on.
Comment 19 Masayuki Nakano [:masayuki] (Mozilla Japan) 2012-03-22 17:01:53 PDT
(In reply to Steven Michaud from comment #17)
> There's an undocumented CGSGetCurrentMouseButtonState() method
> available on OS X 10.5, which I could reverse-engineer if you want.
> But (from looking at the assembly code) it appears to only support the
> left and right mouse buttons.

Hmm, if we cannot support it completely on 10.5, I think we don't need to support it. I don't think that buttons attribute isn't important and it's not been supported on WebKit yet.

> In any case I think it's better to get the mouse button state from
> 'inEvent', by calling [inEvent buttonNumber].  Beware that calling
> 'buttonNumber' on a non-mouse event (for example a scroll-wheel event)
> may throw an Objective-C exception.  And if 'inEvent' is nil, you'll
> (of course) only be able to get the information on OS X 10.6 and up.
> Also beware that you may not *want* to get this information if
> 'inEvent' is nil, even where you can:  It's not uncommon for more than
> one pointing device to be attached (for example a mouse and a built-in
> trackpad), and I'm not sure how the OS decides which device's
> button-state to give you. 

I think that the class method's behavior is similar to other platforms' API. So, I think it should be used.

Windows API gets all mice's button state. And perhaps, it's same on GTK, but I cannot confirm it because I don't have actual Linux machine, only VM. So, it's not problem that buttons attribute may indicate the buttons' status of all mouse devices in the system.

And note that the buttons attribute is need by all inherited events from nsMouseEvent_base, like wheel events and d&d events. Therefore, inEvent's method cannot be used.

And also, modifiers will be needed for keyboard events too (bug 630811).

> Like Smaug, I have my doubts about always setting MODIFIER_NUMLOCK on
> OS X.  As you've pointed out, the Mac doesn't have any support for
> numlock functionality, even when used with a PC keyboard.  What do you
> mean when you say that we should always set numlock on the Mac "for
> compatibility with other platforms"?  Isn't numlock off by default in
> Windows?

As Karlt said, we can think following example:

function onEvent(aEvent)
{
  if (aEvent.getModifierState("NumLock")) {
    // Mac never runs this code if we're not set the NumLock flag even with full keyboard.
  }
}

It may be better if we can get the current keyboard type -- whether it has numpad or not. If it has, NumLock state should be set, otherwise, shouldn't be set.
Comment 20 Masayuki Nakano [:masayuki] (Mozilla Japan) 2012-03-22 17:02:57 PDT
(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #19)
> (In reply to Steven Michaud from comment #17)
> > There's an undocumented CGSGetCurrentMouseButtonState() method
> > available on OS X 10.5, which I could reverse-engineer if you want.
> > But (from looking at the assembly code) it appears to only support the
> > left and right mouse buttons.
> 
> Hmm, if we cannot support it completely on 10.5, I think we don't need to
> support it. I don't think that buttons attribute isn't important and it's
> not been supported on WebKit yet.

I meant that we don't need to support it on 10.5.
Comment 21 Masayuki Nakano [:masayuki] (Mozilla Japan) 2012-03-26 01:22:50 PDT
Another possible idea is, we should only set NumLock state when a key in NumPad is pressed. So, in this bug, it is always not set.
Comment 22 Steven Michaud [:smichaud] (Retired) 2012-03-26 08:14:57 PDT
(In reply to comment #19)

>> In any case I think it's better to get the mouse button state from
>> 'inEvent', by calling [inEvent buttonNumber].  Beware that calling
>> 'buttonNumber' on a non-mouse event (for example a scroll-wheel
>> event) may throw an Objective-C exception.  And if 'inEvent' is
>> nil, you'll (of course) only be able to get the information on OS X
>> 10.6 and up.  Also beware that you may not *want* to get this
>> information if 'inEvent' is nil, even where you can: It's not
>> uncommon for more than one pointing device to be attached (for
>> example a mouse and a built-in trackpad), and I'm not sure how the
>> OS decides which device's button-state to give you.
>
>I think that the class method's behavior is similar to other
> platforms' API. So, I think it should be used.
>
> Windows API gets all mice's button state. ...

What do you mean by "gets all mice's button state"?  If you have two
mice attached, and press one mouse's left button and the other mouse's
right button, does the Windows API tell you that both the left and
right mouse buttons are pressed?

If so, this doesn't seem right.

As for the NumLock business, I confess I really	don't know what	to
think.  I do (now) understand that on a full-size Mac keyboard, the
keys that are in the same location as the PC keyboard's "number pad"
have only numbers (and no arrows).  So if the Mac had a NumLock key,
it would always be on.

But ...

On the one hand the Mac doesn't have a NumLock key, and it's very
confusing to talk about NumLock state on a Mac.

But on the other hand very few (if any) web developers program
specifically for the Mac, so it makes sense to have one definition of
NumLock state that fits all platforms.

> Another possible idea is, we should only set NumLock state when a
> key in NumPad is pressed. So, in this bug, it is always not set.

So (on the Mac) the NumLock state would be off for all keys but those
in the number pad?  This makes sense to me.
Comment 23 Masayuki Nakano [:masayuki] (Mozilla Japan) 2012-03-26 19:36:17 PDT
(In reply to Steven Michaud from comment #22)
> (In reply to comment #19)
> 
> >> In any case I think it's better to get the mouse button state from
> >> 'inEvent', by calling [inEvent buttonNumber].  Beware that calling
> >> 'buttonNumber' on a non-mouse event (for example a scroll-wheel
> >> event) may throw an Objective-C exception.  And if 'inEvent' is
> >> nil, you'll (of course) only be able to get the information on OS X
> >> 10.6 and up.  Also beware that you may not *want* to get this
> >> information if 'inEvent' is nil, even where you can: It's not
> >> uncommon for more than one pointing device to be attached (for
> >> example a mouse and a built-in trackpad), and I'm not sure how the
> >> OS decides which device's button-state to give you.
> >
> >I think that the class method's behavior is similar to other
> > platforms' API. So, I think it should be used.
> >
> > Windows API gets all mice's button state. ...
> 
> What do you mean by "gets all mice's button state"?  If you have two
> mice attached, and press one mouse's left button and the other mouse's
> right button, does the Windows API tell you that both the left and
> right mouse buttons are pressed?

Yes, it does.

> If so, this doesn't seem right.

Hmm, basically, it shouldn't happen. However, for the compatibility with other platforms, I recommend we should use similar behavior.

And don't you misunderstand the buttons attribute of the DOM event? Looks like the buttonNumber gives the pressed button *by* the event. It's button attribute of DOM MouseEvent. button_s_ is the state of all buttons on mice of the system.
http://dev.w3.org/2006/webapi/DOM-Level-3-Events/html/DOM3-Events.html#events-MouseEvent-buttons

> As for the NumLock business, I confess I really	don't know what	to
> think.  I do (now) understand that on a full-size Mac keyboard, the
> keys that are in the same location as the PC keyboard's "number pad"
> have only numbers (and no arrows).  So if the Mac had a NumLock key,
> it would always be on.
> 
> But ...
> 
> On the one hand the Mac doesn't have a NumLock key, and it's very
> confusing to talk about NumLock state on a Mac.
> 
> But on the other hand very few (if any) web developers program
> specifically for the Mac, so it makes sense to have one definition of
> NumLock state that fits all platforms.
> 
> > Another possible idea is, we should only set NumLock state when a
> > key in NumPad is pressed. So, in this bug, it is always not set.
> 
> So (on the Mac) the NumLock state would be off for all keys but those
> in the number pad?  This makes sense to me.

Okay, then, we should use this way. It's very simple. The keyboard viewer of MacOS has the physical keyboard layout. However, I cannot find such API which tells us what keys exist. So, I guess the keyboard viewer has database of each keyboard type or using hidden/internal APIs.
Comment 24 warcraftthreeft 2012-03-26 21:05:19 PDT
While you're implementing DOM 3 mouse events you might want to pay special attention to the following bug:
https://bugzilla.mozilla.org/show_bug.cgi?id=738105
I just got those changes implemented in the editor draft for defining mouse event default actions. They might require discussion if people find a fault with those proposed changes. (I got them added because I didn't see any conflicts and it looked like a sane solution, but it should be mentioned they are very recent additions).
Comment 25 Steven Michaud [:smichaud] (Retired) 2012-03-27 10:56:00 PDT
(In reply to comment #23)

Ah, now I understand a little better.  I didn't realize the DOM3 spec
has both MouseEvent.button and MouseEvent.buttons, and that your patch
implements the latter.

> button_s_ is the state of all buttons on mice of the system.

However, I don't see the above statement (or its equivalent) in the
part of the spec you quoted.  Most of the text under
MouseEvent.buttons is ambiguous -- it could refer to either all the
buttons of a single "device" or all the buttons of all "devices".  But
in a couple of places the text does imply that it's talking about a
single "device":

"1 must indicate the primary button of the device ..."
"... for an arbitrary number of mouse buttons on a device, ..."

Karl, what do you think? 

And no, most people won't have two mice.  But many *will* have a mouse
and a trackpad.
Comment 26 warcraftthreeft 2012-03-27 17:46:41 PDT
(In reply to Steven Michaud from comment #25)
> And no, most people won't have two mice.  But many *will* have a mouse
> and a trackpad.
What are you expecting with two mice? I'd expect all devices to be binary ORed. (I'm the one that got buttons added to the spec and it was to explicitly get rid of any ambiguities. I've submitted a bug and it should hopefully be added in as a note when it's seen).
https://www.w3.org/Bugs/Public/show_bug.cgi?id=8406#c16
Comment 27 Karl Tomlinson (back Dec 13 :karlt) 2012-03-27 19:42:31 PDT
I get the impression that the spec is interpreting mouse events from mice that
control the main pointer (cursor).  It seems reasonable to me that the buttons
of all mice (and other pointing devices) that control the main pointer be
considered.

I don't see the wording of the spec as leading either way.  "which combination
of mouse buttons are currently being pressed" and "0 must indicates no button
is currently active" may have weak unintended suggestions.  Similarly, I doubt
the portions quoted in comment 25 were intended to mean anything about
multiple mice.

I have a laptop which has 3 buttons for the trackpoint but only 2 for the
trackpad.  I expect that, if I press the extra button for the trackpoint and
use the trackpad, the mouse motion events should be the same as if there had
been such a button on the trackpoint and I had that pressed.

On X11, button state in mouse events (available only for the first 5 buttons)
includes buttons that are down on other mice that are sending core events.  If
that is the same on WINNT, then I guess the spec authors just assumed that
behavior.
Comment 28 Masayuki Nakano [:masayuki] (Mozilla Japan) 2012-03-28 01:15:44 PDT
Created attachment 610036 [details] [diff] [review]
part.5 Set modifiers and buttons of nsMouseEvent on Mac
Comment 29 Masayuki Nakano [:masayuki] (Mozilla Japan) 2012-03-28 01:20:04 PDT
(In reply to Karl Tomlinson (:karlt) from comment #27)
> I get the impression that the spec is interpreting mouse events from mice
> that
> control the main pointer (cursor).  It seems reasonable to me that the
> buttons
> of all mice (and other pointing devices) that control the main pointer be
> considered.

That sounds good reason to combine all mouse buttons state.

> On X11, button state in mouse events (available only for the first 5 buttons)
> includes buttons that are down on other mice that are sending core events. 
> If
> that is the same on WINNT, then I guess the spec authors just assumed that
> behavior.

Same on Windows. And I think Windows and Linux cannot get the buttons state separately for each mouse. Therefore, I think there is no reason to behave differently only on Mac.
Comment 30 Steven Michaud [:smichaud] (Retired) 2012-03-28 07:38:28 PDT
Comment on attachment 610036 [details] [diff] [review]
part.5 Set modifiers and buttons of nsMouseEvent on Mac

OK, this looks fine to me.

>+  // Be aware, NSFunctionKeyMask is included when arrow keys, home key or some
>+  // other keys are pressed. We cannot check whether 'fn' key is pressed or
>+  // not by the flag.

We might get into trouble either way on this issue (of whether or not to support MODIFIER_FN on OS X).  But I think it's probably safer not to support it, so I agree with what Masayuki has done.

However we may need to revisit this at some point.
Comment 31 Steven Michaud [:smichaud] (Retired) 2012-03-28 07:56:54 PDT
It seems I'm the only one here who thinks it's strange to have
MouseEvent.buttons represent the state (ORed together) of the buttons
on all pointing devices.  So I'll consent to be outvoted.

I hope the ambiguity at
http://dev.w3.org/2006/webapi/DOM-Level-3-Events/html/DOM3-Events.html#events-MouseEvent-buttons
is resolved by the time this code gets into a release.	But if we
decide to stay with the current interpretation of MouseEvent.buttons,
I can foresee having to add a MouseEvent.buttonz attribute :-) This
would represent the state of all buttons on the "current pointing
device" (as obtained from the "current pointing-device event").

> And I think Windows and Linux cannot get the buttons state
> separately for each mouse.

The same seems to be true on OS X.

The reason (I think) is that it's impossible, just by querying
pointing devices, to decide which one is the "current" device.
However, this decision is very easy to make if the
pointing-device-state information comes from a native event.  In this
case the "current" device is the one corresponding to the
pointing-device event that's currently being processed.  And if no
pointing-device event is currently being processed, there is no
"current" pointing device.
Comment 32 warcraftthreeft 2012-03-28 11:00:57 PDT
(In reply to Steven Michaud from comment #31)
> This
> would represent the state of all buttons on the "current pointing
> device" (as obtained from the "current pointing-device event").
Oh yeah this is probably where your confusion lies. There is no such things as "current pointing device". Windows doesn't even have that concept. You can use a trackpad and a mouse at the same time and it just sums the results of the movement. So the only intuitive solution is to assume the results are ORed together. Hope that helps.
Comment 33 Steven Michaud [:smichaud] (Retired) 2012-03-28 11:18:55 PDT
> There is no such things as "current pointing device".

Sure there is.  I gave a viable definition in comment #31:

> the "current" device is the one corresponding to the
> pointing-device event that's currently being processed.  And if no
> pointing-device event is currently being processed, there is no
> "current" pointing device.

But I'm not sure the standards makers or most developers care about this.  I seem to be the only one who does here :-)
Comment 34 Jim Mathies [:jimm] 2012-04-11 17:57:57 PDT
Comment on attachment 602322 [details] [diff] [review]
part.3 Set modifiers and buttons of nsMouseEvent on Windows

Review of attachment 602322 [details] [diff] [review]:
-----------------------------------------------------------------

sorry for the delay, look good.
Comment 35 Masayuki Nakano [:masayuki] (Mozilla Japan) 2012-04-11 18:37:25 PDT
Created attachment 614247 [details] [diff] [review]
part.6 Initialize modifiers at dispatching events from nsDOMWindowUtils

Oops, I've forgotten to post an additional patch for synthesizing events.
Comment 36 Olli Pettay [:smaug] 2012-04-16 11:56:31 PDT
Comment on attachment 614247 [details] [diff] [review]
part.6 Initialize modifiers at dispatching events from nsDOMWindowUtils

So why do we need both .isShift/.isControl/.isAlt/.isMeta and .modifiers?
Comment 37 Masayuki Nakano [:masayuki] (Mozilla Japan) 2012-04-16 16:21:02 PDT
(In reply to Olli Pettay [:smaug] from comment #36)
> Comment on attachment 614247 [details] [diff] [review]
> part.6 Initialize modifiers at dispatching events from nsDOMWindowUtils
> 
> So why do we need both .isShift/.isControl/.isAlt/.isMeta and .modifiers?

.isShift/.isControl/.isAlt/.isMeta are defined in nsInputEvent. .modifiers is defined in nsMouseEvent_base (and will be defined in nsKeyEvent). After we fix bug 630811, we can remove them from nsInputEvent, but we shouldn't do it now.
Comment 38 Olli Pettay [:smaug] 2012-04-16 23:54:51 PDT
We could move modifiers to nsInputEvent even now, right?
Comment 39 Masayuki Nakano [:masayuki] (Mozilla Japan) 2012-04-17 00:04:13 PDT
(In reply to Olli Pettay [:smaug] from comment #38)
> We could move modifiers to nsInputEvent even now, right?

If we do so, some developers might try using .modifiers of unsupported events...
Comment 40 Olli Pettay [:smaug] 2012-04-17 00:07:50 PDT
I mean replace .isShift/.isControl/.isAlt/.isMeta with .modifiers.
Right now I'm worried that we will have duplicate data in mouse events. Is it guaranteed that
when one data is updated, also the other one is updated.
Comment 41 Masayuki Nakano [:masayuki] (Mozilla Japan) 2012-04-17 00:35:46 PDT
Hmm, it must become a big patch.
http://mxr.mozilla.org/comm-central/search?string=%28isShift|isControl|isMeta|isAlt%29&regexp=on&find=&findi=&filter=^[^\0]*%24&hitlimit=&tree=comm-central

However, looks like comm-central specific code doesn't reference them directly. I guess I can write the patch in a couple of days. Okay, I'll add replacing patch as part.7.
Comment 42 Masayuki Nakano [:masayuki] (Mozilla Japan) 2012-04-17 02:03:29 PDT
Oops, it's wrong. widget needs more reviews.

I'll post replacing patches in bug 630811. And when it's fixed, landing this bug's patches at same time. Do you agree?
Comment 43 Olli Pettay [:smaug] 2012-04-17 02:56:37 PDT
Sounds ok.
Comment 44 Olli Pettay [:smaug] 2012-04-17 03:26:48 PDT
Comment on attachment 614247 [details] [diff] [review]
part.6 Initialize modifiers at dispatching events from nsDOMWindowUtils

I assume this will change.
Comment 45 Masayuki Nakano [:masayuki] (Mozilla Japan) 2012-04-18 18:32:56 PDT
Created attachment 616413 [details] [diff] [review]
part.1 Implement D3E initMouseEvent() and getModifierState() but they shouldn't be public

updated for latest trunk.
Comment 46 Masayuki Nakano [:masayuki] (Mozilla Japan) 2012-04-18 18:33:37 PDT
Created attachment 616414 [details] [diff] [review]
part.2 Implement D3E MouseEvent.buttons attribute
Comment 47 Masayuki Nakano [:masayuki] (Mozilla Japan) 2012-04-18 18:34:06 PDT
Created attachment 616415 [details] [diff] [review]
part.4 Set modifiers and buttons of nsMouseEvent on GTK
Comment 48 Karl Tomlinson (back Dec 13 :karlt) 2012-04-22 20:50:21 PDT
Comment on attachment 616415 [details] [diff] [review]
part.4 Set modifiers and buttons of nsMouseEvent on GTK

>+    // XXX Is this correct?
>+    if (keymapWrapper->AreModifiersActive(SUPER, aModifierState) ||
>+        keymapWrapper->AreModifiersActive(HYPER, aModifierState)) {
>+        mouseEvent.modifiers |= MODIFIER_WIN;
>+    }

Yes, I think this is the best approach.

>+    if (aEvent.message == NS_MOUSE_BUTTON_DOWN ||
>+        aEvent.message == NS_CONTEXTMENU) {

I think I'd prefer a test on (aGdkEvent->type != GDK_BUTTON_RELEASE) because
it is simpler and because the range of possible GdkEventButton types feels
more tightly defined than the range of nsMouseEvent (for which the inclusion
of NS_CONTEXTMENU feels loose to me).  Does that seem OK to you?

>-    if (aMsg == NS_DRAGDROP_OVER) {
>-        InitDragEvent(event);
>-    }
>+    InitDragEvent(event);

Modifiers usually should be used by the source of the drag to change the type
of drag, so I struggle to imagine how destinations could also use the
modifiers.

For NS_DRAGDROP_EXIT events I find it even harder to imagine how they could be
useful.

The behavior of the drop should have already been determined before the
NS_DRAGDROP_DROP occurs (before or during the NS_DRAGDROP_OVER event), so
again I'd hope that destinations won't use modifiers on NS_DRAGDROP_DROP.

Can you remove the drag event change from this patch, please, or explain why
this is necessary?
Comment 49 Karl Tomlinson (back Dec 13 :karlt) 2012-04-22 21:11:37 PDT
Comment on attachment 616415 [details] [diff] [review]
part.4 Set modifiers and buttons of nsMouseEvent on GTK

>+    if (aModifierState & GDK_BUTTON4_MASK) {
>+        mouseEvent.buttons |= nsMouseEvent::e4thButtonFlag;
>+    }
>+    if (aModifierState & GDK_BUTTON5_MASK) {
>+        mouseEvent.buttons |= nsMouseEvent::e5thButtonFlag;
>+    }

On X11, which is the only platform supported by our GTK port, the bits corresponding to buttons 4 and 5 are only set during synthetic button release events for scroll wheels (up and down).  GDK doesn't pass on these events to us (and converts the corresponding Press events to GDK_SCROLL events) so there is no need for this code.
Comment 50 Masayuki Nakano [:masayuki] (Mozilla Japan) 2012-04-22 22:25:16 PDT
(In reply to Karl Tomlinson (:karlt) from comment #48)
> Comment on attachment 616415 [details] [diff] [review]
> part.4 Set modifiers and buttons of nsMouseEvent on GTK
> >+    if (aEvent.message == NS_MOUSE_BUTTON_DOWN ||
> >+        aEvent.message == NS_CONTEXTMENU) {
> 
> I think I'd prefer a test on (aGdkEvent->type != GDK_BUTTON_RELEASE) because
> it is simpler and because the range of possible GdkEventButton types feels
> more tightly defined than the range of nsMouseEvent (for which the inclusion
> of NS_CONTEXTMENU feels loose to me).  Does that seem OK to you?

Yeah, sounds better.

> >-    if (aMsg == NS_DRAGDROP_OVER) {
> >-        InitDragEvent(event);
> >-    }
> >+    InitDragEvent(event);
> 
> Modifiers usually should be used by the source of the drag to change the type
> of drag, so I struggle to imagine how destinations could also use the
> modifiers.
> 
> For NS_DRAGDROP_EXIT events I find it even harder to imagine how they could
> be
> useful.
> 
> The behavior of the drop should have already been determined before the
> NS_DRAGDROP_DROP occurs (before or during the NS_DRAGDROP_OVER event), so
> again I'd hope that destinations won't use modifiers on NS_DRAGDROP_DROP.
> 
> Can you remove the drag event change from this patch, please, or explain why
> this is necessary?

IE9 has already supported D3E getModifierState() (I guess the IE's behavior has been documented in the spec, though). And IE9 has supported it on all D&D events actually. So, I think it's better for web developers we to support the same behavior. On the other hand, if that would cause some problems for our users, I think we shouldn't prefer the compatibility with other browsers.

Currently, I think we should prefer the compatibility because we don't find actual problems which are caused by supporting it. Do you have idea of some problems by them?
Comment 51 Karl Tomlinson (back Dec 13 :karlt) 2012-04-22 23:12:58 PDT
(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #50)
> Do you have idea of some problems by them?

A prospective destination receiving a drag event does not know how the source has interpreted the modifier state.  If the destination actually tries to use the modifier state, then it's interpretation will likely be in conflict with the interpretation at the source (even if both source and destination receive the same modifier state info).

In the GTK port, the modifier state for NS_DRAGDROP_OVER events currently comes from a query issued after the drag message is received from the source.  That means that the modifier state does not necessarily match the state that the source had when it sent the event.

The destination should instead use the effectAllowed (or maybe dropEffect) information in the event, which may be derived from the modifier state when the source sent the event.
Comment 52 Masayuki Nakano [:masayuki] (Mozilla Japan) 2012-04-22 23:53:39 PDT
(In reply to Karl Tomlinson (:karlt) from comment #51)

Um, I still don't think we shouldn't set .modifiers for D&D events...

I agree with that web developers shouldn't use getModifierState() for deciding D&D effect at the destination. However, I don't like "not good usage" for the reason because I think platform for applications should provide many information as far as possible. They *might* use the information for other purpose, we shouldn't limit such possibility.

Smaug, how do you think about this? If you think we shouldn't set .modifiers and .buttons for some drag events, we shouldn't set them on Windows too. (Note, on Mac, currently doesn't set .modifiers correctly due to platform's limitation(?). Cocoa/Carbon API doesn't return actual state.)
Comment 53 Masayuki Nakano [:masayuki] (Mozilla Japan) 2012-04-23 00:01:28 PDT
And, if we do the limitation, I think widget should set them always. And nsPresShell or something should clear them for ensuring to keep the consistency on all platforms.
Comment 54 Masayuki Nakano [:masayuki] (Mozilla Japan) 2012-04-23 02:07:07 PDT
Created attachment 617421 [details] [diff] [review]
part.4 Set modifiers and buttons of nsMouseEvent on GTK

Okay, I discussed the issue with Smaug. For now, we shouldn't change the behavior by this bug. I'll file a new bug for the issue after this fix.

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