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

RESOLVED FIXED in Firefox 55

Status

()

defect
P3
normal
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: cynthiatang, Assigned: stone)

Tracking

(Blocks 1 bug)

53 Branch
mozilla55
x86_64
Windows 10
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox55 fixed)

Details

Attachments

(6 attachments, 8 obsolete attachments)

13.39 KB, patch
stone
: review+
Details | Diff | Splinter Review
12.35 KB, patch
stone
: review+
Details | Diff | Splinter Review
13.18 KB, patch
stone
: review+
Details | Diff | Splinter Review
17.98 KB, patch
stone
: review+
Details | Diff | Splinter Review
12.94 KB, patch
stone
: review+
Details | Diff | Splinter Review
5.38 KB, patch
stone
: review+
Details | Diff | Splinter Review
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)
Priority: -- → P3
Assignee: nobody → sshih
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.
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.
Update pointerevent_releasepointercapture_events_to_original_target-manual.html from wpt test case and refine synthesized events to trigger it.
Attachment #8843669 - Attachment is obsolete: true
Attachment #8843694 - Flags: review?(bugs)
Attachment #8843695 - Flags: review?(bugs)
Attachment #8843671 - Flags: review?(bugs)
Attachment #8843670 - Flags: review?(bugs)
Attachment #8843668 - Flags: review?(bugs)
Attachment #8843668 - Flags: review?(bugs) → review+
Attachment #8843670 - Flags: review?(bugs) → review+
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 on attachment 8843671 [details] [diff] [review]
Part5: Update test_pointerevent_releasepointercapture_events_to_original_target-manual.html

please don't add dump() calls.

rs+
Attachment #8843671 - Flags: review?(bugs) → review+
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
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 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
Attachment #8843694 - Flags: review?(bugs) → review-
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.
Attachment #8843695 - Flags: review?(bugs) → review+
Ok, the spec has plenty of issues, but after chatting in #pointerevents, I think I understand this better.
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?
Attachment #8843694 - Flags: review- → review+
(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.
(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.
Attachment #8845276 - Flags: review?(bugs)
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+
Attachment #8845276 - Flags: review?(bugs) → review+
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
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
Keywords: checkin-needed
Duplicate of this bug: 1323161
Duplicate of this bug: 1323160
Duplicate of this bug: 1292079
Duplicate of this bug: 1292077
Duplicate of this bug: 1293176
You need to log in before you can comment on or make changes to this bug.