Closed Bug 970199 Opened 6 years ago Closed 6 years ago

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

Categories

(Core :: DOM: Events, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla30

People

(Reporter: alessarik, Assigned: alessarik)

References

Details

(Whiteboard: [pointer-events])

Attachments

(1 file, 4 obsolete files)

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.
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)
Whiteboard: [triage]
Marco, these bugs are related to a feature that isn't pref'd on yet.
No longer blocks: metrov1backlog
Status: UNCONFIRMED → NEW
Ever confirmed: true
Whiteboard: [triage] → [pointer-events]
Attachment #8373168 - Attachment is patch: true
Attachment #8373168 - Attachment mime type: text/x-patch → text/plain
According to this:
http://www.w3.org/TR/DOM-Level-3-Events/#widl-MouseEvent-button

Button is unsigned... looks like we have conflict here
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)
Attachment #8373168 - Attachment is obsolete: true
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
Attachment #8377176 - Flags: review-
(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.
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.
All changes related with only repair test 5.8
Attachment #8377176 - Attachment is obsolete: true
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
Attachment #8377495 - Flags: review?(bugs)
Attachment #8377495 - Flags: feedback+
Attachment #8377495 - Attachment description: For_test_5.8_updated.patch → Switch Mouse button from unsigned to signed type.
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
Attachment #8377495 - Flags: feedback+ → feedback-
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
> >+    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.
Attached patch switch_mouse_button_type.patch (obsolete) — Splinter Review
Comment was added in a head of patch
Attachment #8377495 - Attachment is obsolete: true
Attachment #8377495 - Flags: review?(bugs)
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?
Attachment #8378141 - Flags: review?(bugs)
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 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.
Attachment #8378141 - Flags: review?(bugs) → review+
Summary: If pointerType is "mouse" and no mouse button is depressed, then the button attribute of the pointermove event must be -1 and the buttons attribute must be 0. → Switch mouse event button type from ushort to short according to spec change
Same patch, with IDL updated and without Metro/Pointer related stuff... only Mouse events changes
Attachment #8378141 - Attachment is obsolete: true
Attachment #8379130 - Flags: review+
Component: General → DOM: Events
Product: Firefox for Metro → Core
https://hg.mozilla.org/integration/mozilla-inbound/rev/ac8adf61a96b

Actual work with pointer specific button tricks will be done in bug 970964
https://hg.mozilla.org/mozilla-central/rev/ac8adf61a96b
Assignee: nobody → alessarik
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla30
You need to log in before you can comment on or make changes to this bug.