Closed Bug 731878 Opened 12 years ago Closed 12 years ago

Implement DOM3 mouse event's buttons and getModifierState()

Categories

(Core :: DOM: Events, defect)

Product:
Core
Component:
DOM: Events
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla15

People

(Reporter: masayuki, Assigned: masayuki)

References

(Depends on 2 open bugs)

Details

(Keywords: dev-doc-needed)

Attachments

(5 files, 10 obsolete files)

part.3 Set modifiers and buttons of nsMouseEvent on Windows
8.28 KB, patch
jimm
: review+
Details | Diff | Splinter Review
part.5 Set modifiers and buttons of nsMouseEvent on Mac
4.30 KB, patch
smichaud
: review+
Details | Diff | Splinter Review
part.1 Implement D3E initMouseEvent() and getModifierState() but they shouldn't be public
27.91 KB, patch
Details | Diff | Splinter Review
part.2 Implement D3E MouseEvent.buttons attribute
23.96 KB, patch
Details | Diff | Splinter Review
part.4 Set modifiers and buttons of nsMouseEvent on GTK
6.08 KB, patch
karlt
: review+
Details | Diff | Splinter Review 
When I'm implementing D3E WheelEvent, I realized that I need to implement some attributes of D3E MouseEvent in nsDOMMouseEvent.
Summary: Implement DOM3 mouse event → Implement DOM3 mouse event (at least internally)
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.
Attachment #601873 - Attachment is obsolete: true
Attachment #602320 - Flags: review?(bugs)
This patch implements MouseEvent.buttons in XP side. I'm not sure why this cannot be initialized by initMouseEvent()...
Attachment #602321 - Flags: review?(bugs)
FYI: part.3 patch depends on bug 672175.
fix a bug in parser of modifierList.
Attachment #602320 - Attachment is obsolete: true
Attachment #607803 - Flags: review?(bugs)
Attachment #602320 - Flags: review?(bugs)
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
Attachment #607803 - Flags: review?(bugs) → review+
Attachment #602321 - Flags: review?(bugs) → review+
Comment on attachment 602321 [details] [diff] [review]
part.2 Implement D3E MouseEvent.buttons attribute

Thank you, smaug.

Could you sr this, jst?
Attachment #602321 - Flags: superreview?(jst)
Attachment #602322 - Flags: review?(jmathies)
Attachment #602324 - Flags: review?(karlt)
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.
Attachment #602325 - Flags: review?(smichaud)
Attachment #602325 - Flags: feedback?(bugs)
updating for the latest m-c.
Attachment #602321 - Attachment is obsolete: true
Attachment #608222 - Flags: superreview?(jst)
Attachment #602321 - Flags: superreview?(jst)
 
> 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?
(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 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?
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.
(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.
(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.
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.
(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.
(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.
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).
(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.
(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
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.

        Attachment #602325 -
        Attachment is obsolete: true

        Attachment #610036 -
        Flags: review?(smichaud)

        Attachment #602325 -
        Flags: review?(smichaud)

        Attachment #602325 -
        Flags: feedback?(bugs)

    
    

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

        Attachment #610036 -
        Flags: review?(smichaud) → review+

    
    

    
  
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.

    
    

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

    
    

    
  
> 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 :-)

        Attachment #608222 -
        Flags: superreview?(jst) → superreview+

    
    

    
  
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.

        Attachment #602322 -
        Flags: review?(jmathies) → review+

    
    

    
  


  
Oops, I've forgotten to post an additional patch for synthesizing events.

        Attachment #614247 -
        Flags: review?(bugs)

    
    

    
  
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?

    
    

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

    
    

    
  
We could move modifiers to nsInputEvent even now, right?

    
    

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

    
    

    
  
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.

    
    

    
  
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.

    
    

    
  
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 on attachment 614247 [details] [diff] [review]
part.6 Initialize modifiers at dispatching events from nsDOMWindowUtils

I assume this will change.

        Attachment #614247 -
        Flags: review?(bugs)

    
    

    
  


  

        Attachment #608218 -
        Attachment is obsolete: true

        Attachment #608222 -
        Attachment is obsolete: true

    
    

    
  


  

        Attachment #602324 -
        Attachment is obsolete: true

        Attachment #614247 -
        Attachment is obsolete: true

        Attachment #602324 -
        Flags: review?(karlt)

        Attachment #616415 -
        Flags: review?(karlt)

    
    

    
  
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?

        Attachment #616415 -
        Flags: review?(karlt)

    
    

    
  
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.

    
    

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

    
    

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

    
    

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

    
    

    
  
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.

    
    

    
  


  
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.

        Attachment #616415 -
        Attachment is obsolete: true

        Attachment #617421 -
        Flags: review?(karlt)

        Attachment #617421 -
        Flags: review?(karlt) → review+
Depends on: 749500
Depends on: 957490

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






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: