1323158 - [PointerEvents][web-platform-tests] Fail to get expect result status when running "pointerevent_capture_suppressing_mouse-manual"

Status: RESOLVED FIXED

(Core :: DOM: Events, defect, P3)

Description

[Cynthia Tang [:cynthiatang]]

Test Description: 
This test checks if setCapture/releaseCapture functions works properly. 

Steps to reproduce:
 1. Launch Firefox with Pointer Events
 2. Go to http://w3c-test.org/pointerevents/pointerevent_capture_suppressing_mouse-manual.html
 3. Start the manual. Complete the following actions:
    (1) Put your mouse over the black rectangle. pointerover and pointerenter should be logged inside of it.
    (2) Move your mouse out of the black rectangle. pointerout and pointerleave should be logged inside of it
    (3) Put your mouse over the purple rectangle. pointerover and pointerenter should be logged inside of it.
    (4) Move your mouse out of the purple rectangle. pointerout and pointerleave should be logged inside of it
    (5) Press and hold left mouse button over "Set Capture" button and move mouse a litte inside the button. "gotpointercapture", "pointerover", and "pointerenter" should be logged in the black rectangle
    (6) Put your mouse over the purple rectangle and then move it out. Nothing should happen
    (7) Put your mouse over the black rectangle and then move it out. Nothing should happen.
    (8) Put your mouse over the purple rectangle and then release left mouse button. "lostpointercapture" should be logged in the black rectangle. Move your mouse in the purple rectangle a little. "pointerout" and "pointerleave" should be logged in the black rectangle. Also "pointerover" and "pointerenter" should be logged in the purple rectangle"
    Test passes if the proper behaviour of the events is observed.
   

Expected result:
 - Return result status is "PASS".

Actual result:
 - 8 Pass. 2 Not Run. 3 Fail.
   - Not Run:
     (1) pointerout event after lostpointercapture received
     (2) pointerleave event after lostpointercapture received

   - Fail: 
     (1) relatedTarget is not null for boundary events even when the capture is set.
     (2) pointerout shouldn't be sent to captured node as all the events are targeted at the capturing node.
     (3) pointerleave shouldn't be sent to captured node as all the events are targeted at the capturing node.
   
   - Error Message: 
     "assert_not_equals: relatedTarget should behave the same as when the capture is not set got disallowed value null
     run/</<@http://w3c-test.org/pointerevents/pointerevent_capture_suppressing_mouse-manual.html:93:33
     Test.prototype.step@http://w3c-test.org/resources/testharness.js:1406:20
     test@http://w3c-test.org/resources/testharness.js:501:9
     run/<@http://w3c-test.org/pointerevents/pointerevent_capture_suppressing_mouse-manual.html:92:29
     EventListener.handleEvent*on_event@http://w3c-test.org/resources/testharness.js:665:9
     run@http://w3c-test.org/pointerevents/pointerevent_capture_suppressing_mouse-manual.html:86:17
     window.onload@http://w3c-test.org/pointerevents/pointerevent_capture_suppressing_mouse-manual.html:82:17
     EventHandlerNonNull*@http://w3c-test.org/pointerevents/pointerevent_capture_suppressing_mouse-manual.html:62:13
   
Environment:
 - Devices: Surface 3
 - OS: Windows 10 Pro. (64-bit)
 - Firefox: 53.0a1 (2016-11-30) (64-bit)

Comment 1

[Ming-Chou Shih [:stone]]

Attachment: Fire pointer and mouse boundary events when capturing the pointer

Spec says setPointerCapture or releasePointerCapture might have changed the hit test target, and we should fire boundary events when a pointing device is moved into or out of the hit test boundaries of an element. So we don't need WidgetPointerHelper::retargetedByPointerCapture to suppress boundary events anymore. Also, we should set MouseEvent::relatedTarget to follow spec.

Comment 2

[Ming-Chou Shih [:stone]]

Attachment: Part2: Remove those PE boundary events test cases which are out of date

Comment 3

[Ming-Chou Shih [:stone]]

Attachment: Part3: Fire boundary events when dispatching gotpointercapture

PE spec v2 says we should fire boundary events when the hit test target is changed by pointer capture. Using PresShell::HandleEventWithTarget to dispatch gotpointercapture so that we can fire boundary events in EventStateManager::PreHandleEvent. We don't fire boundary events when implicitly releasing a pointer and use the next pointermove to trigger them.

Comment 4

[Ming-Chou Shih [:stone]]

Attachment: Part4: Refine PE boundary events related testcases since boundary events are fired after capturing or releasing the pointer

Comment 5

[Ming-Chou Shih [:stone]]

Attachment: Part5: Update test_pointerevent_releasepointercapture_events_to_original_target-manual.html

Update pointerevent_releasepointercapture_events_to_original_target-manual.html from wpt test case and refine synthesized events to trigger it.

Comment 6

[Ming-Chou Shih [:stone]]

Attachment: Part1: Fire pointer and mouse boundary events when capturing the pointer

Comment 7

[Ming-Chou Shih [:stone]]

Attachment: Part3: Fire boundary events when dispatching gotpointercapture

Comment 8

[Olli Pettay [:smaug][[e-mail]]]

So some of the tests are in testing/web-platform/tests/pointerevents and also 
dom/events/test/pointerevents/pointerevent_releasepointercapture_events_to_original_target-manual.html

Do the patches make dom/* work like wpt?

Comment 9

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8843671 [details] [diff] [review]
Part5: Update test_pointerevent_releasepointercapture_events_to_original_target-manual.html

please don't add dump() calls.

rs+

Comment 10

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8843694 [details] [diff] [review]
Part1: Fire pointer and mouse boundary events when capturing the pointer

setPointerCapture says
"For subsequent events of the pointer, the capturing target will substitute the normal hit testing result as if the pointer is always over the capturing target, and they MUST always be targeted at this element until capture is released."

but for example pointerleave "A user agent MUST fire a pointer event named pointerleave when a pointing device is moved out of the hit test boundaries of an element and all of its descendants"
So I don't now understand what the spec is trying to say

Comment 11

[Olli Pettay [:smaug][[e-mail]]]

The spec also says 
"If the pointer capture target override has been set for the pointer, set the target to pointer capture target override object." when firing a pointer event.

Comment 12

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8843694 [details] [diff] [review]
Part1: Fire pointer and mouse boundary events when capturing the pointer

So, my current interpretation isn't this.
Could you explain, and if I've missed something, ask review again.

I also filed https://github.com/w3c/pointerevents/issues/184

Comment 13

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8843695 [details] [diff] [review]
Part3: Fire boundary events when dispatching gotpointercapture


>+nsIPresShell::DispatchGotOrLostPointerCaptureEvent(
>+                bool aIsGotCapture,
>+                const WidgetPointerEvent* aPointerEvent,
>+                nsIContent* aCaptureTarget)
>+{
>+  nsIDocument* targetDoc = aCaptureTarget->OwnerDoc();
>+  nsIPresShell* shell = targetDoc->GetShell();
Please keep shell alive while dispatching events.
So, nsCOMPtr.

Comment 14

[Olli Pettay [:smaug][[e-mail]]]

Ok, the spec has plenty of issues, but after chatting in #pointerevents, I think I understand this better.

Comment 15

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8843694 [details] [diff] [review]
Part1: Fire pointer and mouse boundary events when capturing the pointer

>-    newPointerEvent->relatedTarget =
>-      nsIPresShell::GetPointerCapturingContent(sourcePointer->pointerId)
>-        ? nullptr
>-        : aRelatedContent;
>+    newPointerEvent->relatedTarget = aRelatedContent;
This isn't spec'ed anywhere (I did file a spec issue). But please ensure this is what other browsers do - do they have
relatedTarget set when pointer is captured

>+++ b/layout/base/PresShell.cpp
>@@ -7584,40 +7584,38 @@ PresShell::HandleEvent(nsIFrame* aFrame,
>           }
>           if (targetContent) {
>             SetPointerCapturingContent(pointerEvent->pointerId, targetContent);
>           }
>         }
>       }
>     }
> 
>-    if (aEvent->mClass == ePointerEventClass &&
>-        aEvent->mMessage != ePointerDown) {
>-      if (WidgetPointerEvent* pointerEvent = aEvent->AsPointerEvent()) {
>-        uint32_t pointerId = pointerEvent->pointerId;
>+    // Mouse events should be fired to the same target as their mapped pointer
>+    // events
>+    if ((aEvent->mClass == ePointerEventClass ||
>+         aEvent->mClass == eMouseEventClass) &&
Please verify other browsers capture mouse events too. Do we have any tests for it?

Comment 16

[Ming-Chou Shih [:stone]]

(In reply to Olli Pettay [:smaug] (high review load, queue closed for couple of days) from comment #15)
> Comment on attachment 8843694 [details] [diff] [review]
> Part1: Fire pointer and mouse boundary events when capturing the pointer
> 
> >-    newPointerEvent->relatedTarget =
> >-      nsIPresShell::GetPointerCapturingContent(sourcePointer->pointerId)
> >-        ? nullptr
> >-        : aRelatedContent;
> >+    newPointerEvent->relatedTarget = aRelatedContent;
> This isn't spec'ed anywhere (I did file a spec issue). But please ensure
> this is what other browsers do - do they have
> relatedTarget set when pointer is captured
Chrome Canary (59.0.3033.0) sets relatedTarget to the capturing node.
Edge (38.14393.0.0) sets relatedTarget to null.

> >+++ b/layout/base/PresShell.cpp
> >@@ -7584,40 +7584,38 @@ PresShell::HandleEvent(nsIFrame* aFrame,
> >           }
> >           if (targetContent) {
> >             SetPointerCapturingContent(pointerEvent->pointerId, targetContent);
> >           }
> >         }
> >       }
> >     }
> > 
> >-    if (aEvent->mClass == ePointerEventClass &&
> >-        aEvent->mMessage != ePointerDown) {
> >-      if (WidgetPointerEvent* pointerEvent = aEvent->AsPointerEvent()) {
> >-        uint32_t pointerId = pointerEvent->pointerId;
> >+    // Mouse events should be fired to the same target as their mapped pointer
> >+    // events
> >+    if ((aEvent->mClass == ePointerEventClass ||
> >+         aEvent->mClass == eMouseEventClass) &&
> Please verify other browsers capture mouse events too. Do we have any tests
> for it?
Both Chrome Canary and Edge fire mouse events to the same target as pointer events.

Comment 17

[Ming-Chou Shih [:stone]]

(In reply to Olli Pettay [:smaug] (high review load, queue closed for couple of days) from comment #8)
> So some of the tests are in testing/web-platform/tests/pointerevents and
> also 
> dom/events/test/pointerevents/
> pointerevent_releasepointercapture_events_to_original_target-manual.html
> 
> Do the patches make dom/* work like wpt?

Yes. dom/events/test/pointerevents/* should be the same as testing/web-platform/tests/pointerevents. And test_pointerevent* are the wrapper files to synthesized input events to trigger those test automatically.

Comment 18

[Ming-Chou Shih [:stone]]

Attachment: Part6: Checks target and relatedTarget of mouse events are the same as their corresponding pointer events

Comment 19

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8845276 [details] [diff] [review]
Part6: Checks target and relatedTarget of mouse events are the same as their corresponding pointer events

>+  done.addEventListener("mousedown", () => {
>+    SimpleTest.finish();
>+  });
I don't understand this, when you after dispatch mouseup to done afterwards.
Shouldn't you add the listener to mouseup? Or just explicitly call 
SimpleTest.finish(); after dispatching all the events

That fixed, r+

Comment 20

[Ming-Chou Shih [:stone]]

Attachment: Part1: Fire pointer and mouse boundary events when capturing the pointer

Comment 21

[Ming-Chou Shih [:stone]]

Attachment: Part2: Remove those PE boundary events test cases which are out of date

Comment 22

[Ming-Chou Shih [:stone]]

Attachment: Part3: Fire boundary events when dispatching gotpointercapture

Comment 23

[Ming-Chou Shih [:stone]]

Attachment: Part4: Refine PE boundary events related testcases since boundary events are fired after capturing or releasing the pointer

Comment 24

[Ming-Chou Shih [:stone]]

Attachment: Part5: Update test_pointerevent_releasepointercapture_events_to_original_target-manual.html

Comment 25

[Ming-Chou Shih [:stone]]

Attachment: Part6: Checks target and relatedTarget of mouse events are the same as their corresponding pointer events

Comment 26

[Ming-Chou Shih [:stone]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=984450ef0db16c85f5caa4e4554790b11b7720d4

Comment 27

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4311e8a7e14f
Part 1: Fire pointer and mouse boundary events when capturing the pointer. r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/feadd76c7f3c
Part 2: Remove those PE boundary events test cases which are out of date. r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/947e223f23a1
Part 3: Fire boundary events when dispatching gotpointercapture. r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/de1f28ab01a1
Part 4: Refine PE boundary events related testcases since boundary events are fired after capturing or releasing the pointer. r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/01ffdf50f96f
Part 5: Update test_pointerevent_releasepointercapture_events_to_original_target-manual.html. r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/f923de5a1110
Part 6: Check target and relatedTarget of mouse events are the same as their corresponding pointer events. r=smaug

Comment 28

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/4311e8a7e14f
https://hg.mozilla.org/mozilla-central/rev/feadd76c7f3c
https://hg.mozilla.org/mozilla-central/rev/947e223f23a1
https://hg.mozilla.org/mozilla-central/rev/de1f28ab01a1
https://hg.mozilla.org/mozilla-central/rev/01ffdf50f96f
https://hg.mozilla.org/mozilla-central/rev/f923de5a1110