Closed Bug 974305 Opened 7 years ago Closed 6 years ago

Needs a test for WidgetPointerEvent::AssignPointerEventData() if it's possible

Categories

(Core :: DOM: UI Events & Focus Handling, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla40
Tracking Status
firefox40 --- fixed

People

(Reporter: masayuki, Assigned: alessarik)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 2 obsolete files)

I don't understand PointerEvent well, though.

If it's can be synthesized via nsIWidget, i.e., via nsIDOMWindowUtils::Send*Event(), some tests should be added into widget/tests/test_assign_event_data.html.
http://mxr.mozilla.org/mozilla-central/source/widget/tests/test_assign_event_data.html?force=1

This compares all attributes of the DOM event at dispatching and after dispatched (e.g., the event stored to global variable from an event handler and referred later).

If you don't understand how to add the test well, feel free to ask me.
Component: File Handling → Event Handling
Is it still needed?
Flags: needinfo?(masayuki)
(In reply to Maksim Lebedev from comment #1)
> Is it still needed?

Yes, if we can synthesize PointerEvent.
Flags: needinfo?(masayuki)
Attached patch assign_data_test_ver1.diff (obsolete) — Splinter Review
+ Added two tests.

Comment and suggestion and objections are very welcome.
Flags: needinfo?(masayuki)
Attachment #8594750 - Flags: review?(bugs)
Attachment #8594750 - Flags: feedback?(masayuki)
Comment on attachment 8594750 [details] [diff] [review]
assign_data_test_ver1.diff

>   <input id="input-text">
>   <button id="button">button</button>
>   <a id="a" href="about:blank">hyper link</a>
>   <div id="scrollable-div" style="overflow: auto; width: 30px; height: 30px;"><div id="scrolled-div" style="width: 10px; height: 10px;"></div></div>
>   <form id="form"></form>
>   <div id="animated-div"></div>
>+  <div id="pointer-div"></div>

If you can use <input> or <button> instead of adding new <div>, you should use it. However, if you need the new <div>, it's okay.

>+  { description: "PointerEvent (pointerdown)",
>+    targetID: "pointer-div", eventType: "pointerdown",
>+    dispatchEvent: function () {
>+      var elem = document.getElementById(this.targetID);
>+      var rect = elem.getBoundingClientRect();
>+      synthesizePointer(elem, rect.width/2, rect.height/2,
>+                        { type: this.eventType, button: 1, clickCount: 1, inputSource: 2, pressure: 0.25, isPrimary: true });
>+    },
>+    canRun: function () {
>+      return true;
>+    },
>+    todoMismatch: [ ],

nit: "[]", in this test.

>+  },
>+  { description: "PointerEvent (pointerup)",
>+    targetID: "pointer-div", eventType: "pointerup",
>+    dispatchEvent: function () {
>+      var elem = document.getElementById(this.targetID);
>+      var rect = elem.getBoundingClientRect();
>+      synthesizePointer(elem, rect.width/2, rect.height/2,
>+                        { type: this.eventType, button: -1, ctrlKey: true, shiftKey: true, altKey: true, isSynthesized: false });
>+    },
>+    canRun: function () {
>+      return true;
>+    },
>+    todoMismatch: [ ],

ditto.


> TEST-UNEXPECTED-ERROR | widget/tests/test_assign_event_data.html | Assertion count 34 is greater than expected range 0-32 assertions.

On windows, these tests increase assertion count. And the new assertions are not in the scope of dom/events (layout), so, please change the acceptable assertion count 0-34.
Flags: needinfo?(masayuki)
Attachment #8594750 - Flags: feedback?(masayuki) → feedback+
(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #5)
> If you can use <input> or <button> instead of adding new <div>, you should
> use it. However, if you need the new <div>, it's okay.
I don't see any reasons for using <button> instead of <div>. 
Let me know, if any reason exist. I always prefer use <div>.
Attached patch assign_data_test_ver2.diff (obsolete) — Splinter Review
+ Changed number of test assertions
Attachment #8594750 - Attachment is obsolete: true
Attachment #8594750 - Flags: review?(bugs)
Attachment #8595221 - Flags: review?(bugs)
Attachment #8595221 - Flags: feedback?(masayuki)
(In reply to Maksim Lebedev from comment #6)
> (In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #5)
> > If you can use <input> or <button> instead of adding new <div>, you should
> > use it. However, if you need the new <div>, it's okay.
> I don't see any reasons for using <button> instead of <div>. 
> Let me know, if any reason exist. I always prefer use <div>.

When the test is run with other tests (i.e., not alone), tests are run in small <iframe>. Therefore, the area we can synthesize mouse events is not so wide. If other tests which will be added will need new element, somebody needs to rewrite the test with opening new window. Therefore, if you don't need to create the new <div>, we can avoid such risk.
(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #9)
> When the test is run with other tests (i.e., not alone), tests are run in
> small <iframe>. Therefore, the area we can synthesize mouse events is not so wide.
Seems reasonable.
+ pointer div -> pointer span.
+ Add styles for elements.

Let's make the world a brighter!
Attachment #8595221 - Attachment is obsolete: true
Attachment #8595221 - Flags: review?(bugs)
Attachment #8595221 - Flags: feedback?(masayuki)
Attachment #8595371 - Flags: review?(bugs)
Attachment #8595371 - Flags: feedback?(masayuki)
Attachment #8595371 - Flags: feedback?(masayuki) → feedback+
Comment on attachment 8595371 [details] [diff] [review]
assign_data_test_ver3.diff

rs+
Attachment #8595371 - Flags: review?(bugs) → review+
If there are no objections, latest changes can be pushed into m-c.
Can anybody put checkin-needed flag? (I have no permissions for do that).
Blocks: 1122211
https://hg.mozilla.org/mozilla-central/rev/100b9eafbbfe
Status: UNCONFIRMED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
Why it did not happen?
Assignee: oleg.romashin → alessarik
Component: Event Handling → User events and focus handling
You need to log in before you can comment on or make changes to this bug.