970199 - Switch mouse event button type from ushort to short according to spec change

Status: RESOLVED FIXED

(Core :: DOM: Events, defect)

Description

[Maksim Lebedev]

Attachment: Test_Assertions_for_pointermove_events_(5.8).patch

User Agent: Mozilla/5.0 (Windows NT 6.2; WOW64; rv:26.0) Gecko/20100101 Firefox/26.0 (Beta/Release)
Build ID: 20131205075310

Steps to reproduce:

http://www.w3.org/wiki/PointerEvents/TestAssertions#Test_Assertions_for_setPointerCapture have a test 5.8 which have to be passed on MetroFireFox.


Actual results:

If we will use trunc version FF we will get wrong value 0.


Expected results:

By specification we should get value -1.

Comment 1

[Maksim Lebedev]

Main thought:
WidgetMouseEventBase have member int16_t button;
nsDOMMouseEvent have method uint16_t Button();
This dissonance have main role in getting right value in test (That's why we have to change type uint16_t to int16_t)

Comment 2

[Jim Mathies [:jimm]]

Marco, these bugs are related to a feature that isn't pref'd on yet.

Comment 3

[Oleg Romashin (:romaxa)]

According to this:
http://www.w3.org/TR/DOM-Level-3-Events/#widl-MouseEvent-button

Button is unsigned... looks like we have conflict here

Comment 4

[Olli Pettay [:smaug]]

This is related to https://www.w3.org/Bugs/Public/show_bug.cgi?id=20236

Comment 5

[Maksim Lebedev]

Attachment: For_test_5.8_and_13.3_updated.patch

Some changes related with current version and future patches of pointer events.
Patch have changes for two bugs 970199 and 970220 (Test 5.8 and 13.3)

Comment 6

[Oleg Romashin (:romaxa)]

Comment on attachment 8377176 [details] [diff] [review]
For_test_5.8_and_13.3_updated.patch

>+    newPointerEvent->relatedTarget = nsIPresShell::GetPointerCapturingContent(sourcePointer->pointerId)
>+                                    ? sourcePointer->relatedTarget
>+                                    : aRelatedContent;
>+    event = newPointerEvent.forget();

looks like you mix patches from different bugs here...

>-
>-  // Helper for making sure event ptrs get freed.
>-  class AutoDeleteEvent
>-  {
>-  public:
>-    AutoDeleteEvent(WidgetGUIEvent* aPtr) :
>-      mPtr(aPtr) {}
>-    ~AutoDeleteEvent() {
>-      if (mPtr) {
>-        delete mPtr;
>-      }
>-    }
>-    WidgetGUIEvent* mPtr;
>-  };

I believe this is unrelated change to problem described in this bug

Comment 7

[Maksim Lebedev]

(In reply to Oleg Romashin (:romaxa) from comment #6)

> looks like you mix patches from different bugs here...

Yes. Because two bugs have cross places in one file.

> I believe this is unrelated change to problem described in this bug

Yes. But we want to delete this unuseful class earlier.

Comment 8

[Oleg Romashin (:romaxa)]

I mean original patch which you attached first time: https://bugzilla.mozilla.org/attachment.cgi?id=8373168
looks much better and cleaner then second one.

Comment 9

[Maksim Lebedev]

Attachment: Switch Mouse button from unsigned to signed type.

All changes related with only repair test 5.8

Comment 10

[Oleg Romashin (:romaxa)]

Comment on attachment 8377495 [details] [diff] [review]
Switch Mouse button from unsigned to signed type.

Please add also patch header in proper format. see 
https://developer.mozilla.org/en-US/docs/Creating_a_patch_that_can_be_checked_in

Comment 11

[Oleg Romashin (:romaxa)]

Comment on attachment 8377495 [details] [diff] [review]
Switch Mouse button from unsigned to signed type.

>-  uint16_t button = (uint16_t)-1;
>+  int16_t button = -1;
>   mouseEvent->GetButton(&button);
...
>   if (!mouseEvent)
>     return NS_OK;
> 
>-  uint16_t button = 0;
>+  int16_t button = 0;
>   mouseEvent->GetButton(&button);
> 
....

> 
>-  uint16_t button;
>+  int16_t button;
>   mouseEvent->GetButton(&button);
>   if (button != 0)
>     return NS_OK;
> 

>+    if (buttonPressed) {
>+      return WidgetMouseEvent::buttonType::eRightButton;
>+    }
>+    return -1;
>+  }


I don't think we should ever return -1 value for non initialized mouse button type. 
IIRC from discussions in https://www.w3.org/Bugs/Public/show_bug.cgi?id=20236#c4 and mailing list
button can be signed now, but for mouse event it MUST be >= 0

Comment 12

[Oleg Romashin (:romaxa)]

Hmm Spec change says:
--- Some pointing devices provide or simulate more buttons, and values higher than 2 MAY be used to represent such buttons.
+++ Some pointing devices provide or simulate more button states, and values higher than 2 or lower than 0 MAY be used to represent such buttons.

so value for mouse with special device could be < 0... but I think it still valid that non-initialized state should be 0, not -1 for mouse events

Comment 13

[Maksim Lebedev]

> >+    if (buttonPressed) {
> >+      return WidgetMouseEvent::buttonType::eRightButton;
> >+    }
> >+    return -1;
> >+  }
> 
> I don't think we should ever return -1 value for non initialized mouse
> button type. 

There is one place, where WidgetMouseEvent are created. If we want that test 5.8 will be passed and MetroFireFox have full support specification of pointer events, we should put correct value for button. At first we check pressed buttons, and if we cannot find them we return -1.

Comment 14

[Maksim Lebedev]

Attachment: switch_mouse_button_type.patch

Comment was added in a head of patch

Comment 15

[Oleg Romashin (:romaxa)]

Comment on attachment 8378141 [details] [diff] [review]
switch_mouse_button_type.patch

My point was to not create -1 value for existing mouse events, and only create -1 value for pointer events (somewhere in Mouse->Pointer converter which require bug 970964 to be landed)
How about another platforms? are we going to be ok with only this fix?

Comment 16

[Oleg Romashin (:romaxa)]

Comment on attachment 8378141 [details] [diff] [review]
switch_mouse_button_type.patch



>diff --git a/dom/interfaces/events/nsIDOMMouseEvent.idl b/dom/interfaces/events/nsIDOMMouseEvent.idl
>--- a/dom/interfaces/events/nsIDOMMouseEvent.idl
>+++ b/dom/interfaces/events/nsIDOMMouseEvent.idl
>@@ -25,17 +25,17 @@ interface nsIDOMMouseEvent : nsIDOMUIEve
>   readonly attribute long               clientX;
>   readonly attribute long               clientY;
> 
>   readonly attribute boolean            ctrlKey;
>   readonly attribute boolean            shiftKey;
>   readonly attribute boolean            altKey;
>   readonly attribute boolean            metaKey;
> 
>-  readonly attribute unsigned short     button;
>+  readonly attribute short              button;

You must update UUID here too

Comment 17

[Olli Pettay [:smaug]]

Comment on attachment 8378141 [details] [diff] [review]
switch_mouse_button_type.patch

yes, update uuid

>+  int16_t
>+  ButtonFromPointProperties(UI::Input::IPointerPointProperties* aProps) {
{ should go to its own line here.


I don't quite understand comment 15.
Or course we need right .button and .buttons values everywhere.
mouse events shouldn't be changed, but when converting mouse events to pointer events, those
pointer events need right .button and .buttons, as the spec says.

We need tests for .button and .buttons, especially in case of
mouse events and pointer events.
Sounds like bug 970964 needs still some fixes.

Comment 18

[Oleg Romashin (:romaxa)]

Attachment: Switch mouse event button type from ushort to short

Same patch, with IDL updated and without Metro/Pointer related stuff... only Mouse events changes

Comment 19

[Oleg Romashin (:romaxa)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ac8adf61a96b

Actual work with pointer specific button tricks will be done in bug 970964

Comment 20

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/ac8adf61a96b