977003 - setPointerCapture and releasePointerCapture methods should fire asyncronius got/lostPointerCapture events

Status: RESOLVED FIXED

(Firefox for Metro Graveyard :: General, defect)

Description

[Maksim Lebedev]

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_releasePointerCapture have 14.2 test which have to be passed on MetroFireFox


Actual results:

Nothing happens


Expected results:

Upon invocation of the releasePointerCapture method, if the provided pointerId value does not match any of the active pointers, a DOMException with the name InvalidPointerId must be thrown.

Comment 1

[Maksim Lebedev]

Attachment: async_gotlost_pointer_capture_events_ver1.diff

Make asyncronius got/lostPointerCapture events

Comment 2

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

Comment on attachment 8473775 [details] [diff] [review]
async_gotlost_pointer_capture_events_ver1.diff

So we should land the test change with this.

Comment 3

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

But see 
https://dvcs.w3.org/hg/pointerevents/raw-file/tip/pointerEvents.html#firing-events-using-the-pointerevent-interface
"Process Pending Pointer Capture"

Comment 4

[Maksim Lebedev]

Attachment: async_gotlost_pointer_capture_events_test_ver1.diff

+ Added test for check async got/lost pointer capture events

Comment 5

[Maksim Lebedev]

Test without patch: https://tbpl.mozilla.org/?tree=Try&rev=ea421867e041

Comment 6

[Maksim Lebedev]

Test with patch: https://tbpl.mozilla.org/?tree=Try&rev=77bd3852bb63

Comment 7

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

Comment on attachment 8475794 [details] [diff] [review]
async_gotlost_pointer_capture_events_test_ver1.diff

You call turnOnPointerEvents first in the parent document, and then in the iframe.

Please use the correct event names in the messages, not
capitalized versions.
So gotPointerCapture, not GotPointerCapture etc.

I don't understand what in this test ensures that the event should fire asynchronously.

Comment 8

[Maksim Lebedev]

(In reply to Olli Pettay [:smaug] from comment #7)
> You call turnOnPointerEvents first in the parent document, and then in the iframe.
Yes. We need call it twice. Unfortunately, test doesn't work with only one call.
> I don't understand what in this test ensures that the event should fire asynchronously.
I am trying to explain. Please see:
>function DownHandler(event) {
>  target.setPointerCapture(event.pointerId);
>  test_send_got = true;
>}
>function GotPCHandler(event) {
>  if(test_send_got)
>    test_got_async = true;
>}
If gotpointercapture is synchronoun event, then GotPCHandler executes before finish of DownHandler
(I mean before "test_send_got = true"). In this case test_got_async will be false in GotPCHandler.
If gotpointercapture is asynchronoun event, then full code of DownHandler are executed.
And only after that GotPCHandler will be called.
In this case test_send_got will be true. And test_got_async will be true too.

Comment 9

[Maksim Lebedev]

Attachment: async_gotlost_pointer_capture_events_ver2.diff

+ Changes: Added r=smaug in comments

Comment 10

[Maksim Lebedev]

Attachment: async_gotlost_pointer_capture_events_test_ver2.diff

+ Changes: Removed capitalized versions of pointer events names

Comment 11

[Maksim Lebedev]

Attachment: async_gotlost_pointer_capture_events_issue_ver1.diff

+ Changes: Added changes for test968148 and test976963 related with asynchronoun events

Comment 12

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

Comment on attachment 8482277 [details] [diff] [review]
async_gotlost_pointer_capture_events_ver2.diff

As I hinted earlier, this doesn't implement 
https://dvcs.w3.org/hg/pointerevents/raw-file/tip/pointerEvents.html#firing-events-using-the-pointerevent-interface
"Process Pending Pointer Capture"
As far as I see we can fire the events async, but those events need to be processed before other pointer events.
The spec isn't super clear about this.

Comment 13

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

Comment on attachment 8482279 [details] [diff] [review]
async_gotlost_pointer_capture_events_issue_ver1.diff

Looks wrong per the spec.

Comment 14

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

Comment on attachment 8482278 [details] [diff] [review]
async_gotlost_pointer_capture_events_test_ver2.diff

I'll review this once the patch implements what the spec says.

Comment 15

[Maksim Lebedev]

Attachment: async_gotlost_pointer_capture_events_ver3.diff

+ Changes: Use additional info for make async behavior of got/lostpointercapture events

Comment 16

[Maksim Lebedev]

Attachment: async_gotlost_pointer_capture_events_ver4.diff

+ Changes: Use pending content in additionally information
+ Changes: Dispatch sync and async events in different cases

Comment 17

[Maksim Lebedev]

https://tbpl.mozilla.org/?tree=Try&rev=0fdccf2d77c0

Comment 18

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

Comment on attachment 8485757 [details] [diff] [review]
async_gotlost_pointer_capture_events_ver4.diff

>+  struct PointerCaptureInfo
>+  {
>+    nsIContent* mPendingContent;
>+    nsIContent* mOverrideContent;
I don't see anything guaranteeing these member variables don't start to point to deleted objects.

>@@ -7174,31 +7186,43 @@ PresShell::HandleEvent(nsIFrame* aFrame,
>         frame = capturingFrame;
>       }
>     }
> 
>     if (aEvent->mClass == ePointerEventClass &&
>         aEvent->message != NS_POINTER_DOWN) {
>       if (WidgetPointerEvent* pointerEvent = aEvent->AsPointerEvent()) {
>         uint32_t pointerId = pointerEvent->pointerId;
>-        nsIContent* pointerCapturingContent = GetPointerCapturingContent(pointerId);
>-
>-        if (pointerCapturingContent) {
>-          if (nsIFrame* capturingFrame = pointerCapturingContent->GetPrimaryFrame()) {
>-            // If pointer capture is set, we should suppress pointerover/pointerenter events
>-            // for all elements except element which have pointer capture. (Code in EventStateManager)
>-            pointerEvent->retargetedByPointerCapture = (frame != capturingFrame);
>-            frame = capturingFrame;
>+        PointerCaptureInfo* pointerCaptureInfo = nullptr;
>+        gPointerCaptureList->Get(pointerId, &pointerCaptureInfo);
>+        if (pointerCaptureInfo) {
Could you perhaps move all this pointercapture handling to some helper method. 
PresShell::HandleEvent is rather complicated method already.


> nsIPresShell::DispatchGotOrLostPointerCaptureEvent(bool aIsGotCapture,
>+                                                   bool aSync,
>                                                    uint32_t aPointerId,
>                                                    nsIContent* aCaptureTarget)
> {
>   PointerEventInit init;
>   init.mPointerId = aPointerId;
>   init.mBubbles = true;
>   nsRefPtr<mozilla::dom::PointerEvent> event;
>   event = PointerEvent::Constructor(aCaptureTarget,
>                                     aIsGotCapture
>                                       ? NS_LITERAL_STRING("gotpointercapture")
>                                       : NS_LITERAL_STRING("lostpointercapture"),
>                                     init);
>   if (event) {
>-    bool dummy;
>-    aCaptureTarget->DispatchEvent(event->InternalDOMEvent(), &dummy);
>+    if (aSync) {
>+      bool dummy;
>+      aCaptureTarget->DispatchEvent(event->InternalDOMEvent(), &dummy);
>+    } else {
>+      nsRefPtr<AsyncEventDispatcher> asyncEventDispatcher =
>+        new AsyncEventDispatcher(aCaptureTarget, event->InternalDOMEvent());
>+      asyncEventDispatcher->PostDOMEvent();
>+    }
Why can we ever just post an AsyncEventDispatcher and forget it then? What if there is another pointerevent coming, and it is processed before
the AsyncEventDispatcher object?
Don't you want to initiate AsyncEventDispatcher-kind of thing here, but keep a reference to it and possibly cancel it if got/lostpointercapture
is dispatched sync because of some other pointer event being dispatched? We don't want to dispatch events twice.

Comment 19

[Maksim Lebedev]

Attachment: async_gotlost_pointer_capture_events_ver5.diff

+ Added: nsCOMptr to pending and override contents
+ Added: Checking to dispatch lostpointercapture only once
+ Added: Async dispatching lostpointercapture
+ Added: Function was allocated to check pointer capture state

Comment 20

[Maksim Lebedev]

Attachment: async_gotlost_pointer_capture_events_ver6.diff

+ Changes: Simplify some callers
- Changes: Remove unwanted code

Comment 21

[Maksim Lebedev]

https://tbpl.mozilla.org/?tree=Try&rev=5c520a1f2ce3

Comment 22

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

Comment on attachment 8487159 [details] [diff] [review]
async_gotlost_pointer_capture_events_ver6.diff

>+  struct PointerCaptureInfo
>+  {
>+    nsCOMPtr<nsIContent> mPendingContent;
>+    nsCOMPtr<nsIContent> mOverrideContent;
>+    bool                 mReleaseContent;
>+    PointerCaptureInfo(nsIContent* aPendingContent) :
>+      mPendingContent(aPendingContent), mReleaseContent(false) {}
>+    bool Empty()
A newline before bool Empty() would be nice.
Could you please add MOZ_COUNT_CTOR to the constructor of PointerCaptureInfo
and add a destructor and add MOZ_COUNT_DTOR there.





>+    {
>+      return !(mPendingContent.get() || mOverrideContent.get());
>+    }
Couldn't you drop .get() here.

>+class AsyncCheckPointerCaptureStateCaller : public nsRunnable
>+{
>+public:
>+  AsyncCheckPointerCaptureStateCaller(int32_t aPointerId) : mPointerId(aPointerId) {}
>+  NS_IMETHOD Run()
Perhaps a newline before NS_IMETHOD Run().





> nsIPresShell::SetPointerCapturingContent(uint32_t aPointerId, nsIContent* aContent)
> {
>-  nsIContent* content = GetPointerCapturingContent(aPointerId);
>+  PointerCaptureInfo* pointerCaptureInfo;
I'd prefer initializing to nullptr

>+nsIPresShell::CheckPointerCaptureState(uint32_t aPointerId)
>+{
>+  bool check = false;
>+  PointerCaptureInfo* pointerCaptureInfo = nullptr;
>+  if (gPointerCaptureList->Get(aPointerId, &pointerCaptureInfo) && pointerCaptureInfo) {
>+    // If pendingContent exist or overrideContent should be released
>+    // we should dispatch lostpointercapture event to overrideContent
>+    if (pointerCaptureInfo->mPendingContent || pointerCaptureInfo->mReleaseContent) {
>+      if (pointerCaptureInfo->mOverrideContent) {
>+        nsCOMPtr<nsIContent> content;
>+        pointerCaptureInfo->mOverrideContent.swap(content);
>+        pointerCaptureInfo->mReleaseContent = false;
>+        DispatchGotOrLostPointerCaptureEvent(false, aPointerId, content);
Hmm, in theory dispatching an event may delete pointerCaptureInfo in case some event listener
ends up doing something which enters this method again and executes gPointerCaptureList->Remove(aPointerId);

I think we should call gPointerCaptureList->Remove(aPointerId); before dispatching any event, and then possibly add the
pointer back to the hashtable at the end of the method (create a new PointerCaptureInfo instance) if needed.
The points to the pendingContent and override content should be stored in nsCOMPtr variables.
Would like to see this issue fixed, otherwise looking good.




>+    if (aEvent->mClass == ePointerEventClass) {
>+      if (WidgetPointerEvent* pointerEvent = aEvent->AsPointerEvent()) {
>+        uint32_t pointerId = pointerEvent->pointerId;
>+        CheckPointerCaptureState(pointerId);
Maybe just 
CheckPointerCaptureState(pointerEvent->pointerId);
No need for the pointerId variable

Comment 23

[Maksim Lebedev]

Attachment: async_gotlost_pointer_capture_events_ver7.diff

+/- Changes: According with comments

Comment 24

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

Comment on attachment 8487843 [details] [diff] [review]
async_gotlost_pointer_capture_events_ver7.diff

Maybe I'm missing something, 
but what code does
"For the specified pointerId, clear the pending pointer capture target override, if set." from https://dvcs.w3.org/hg/pointerevents/raw-file/tip/pointerEvents.html#releasing-pointer-capture

Comment 25

[Maksim Lebedev]

Attachment: async_gotlost_pointer_capture_events_test_ver3.diff

+ Changes: Adding test case with Listener

Comment 26

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

Comment on attachment 8487890 [details] [diff] [review]
async_gotlost_pointer_capture_events_test_ver3.diff

>+    function DownHandler(event) {
>+      logger("Receive event: " + event.type);
>+      logger("Try send pointerCapture to Listener");
>+      listener.setPointerCapture(event.pointerId);
>+      logger("Try clear pointerCapture from Listener");
>+      listener.releasePointerCapture(event.pointerId);
>+      logger("Send setPointerCapture to Target");
>+      target.setPointerCapture(event.pointerId);
>+      logger("setPointerCapture was executed");
>+      test_send_got = true;

We want also a test which does only
listener.setPointerCapture and then immediately calls listener.releasePointerCapture.
This test calls target.setPointerCapture after those.

Comment 27

[Maksim Lebedev]

Attachment: async_gotlost_pointer_capture_events_test_ver4.diff

- Removed: complicated test
+ Added: test for detect async behavior
+ Added: test with immediately calling set/releasePointerCapture
+ Added: previous test with current capturing state
+ Added: previous test with inverse sequence of calling

Comment 28

[Maksim Lebedev]

(In reply to Olli Pettay [:smaug] from comment #24)
> Maybe I'm missing something, but what code does
> "For the specified pointerId, clear the pending pointer capture target override, if set." from
> https://dvcs.w3.org/hg/pointerevents/raw-file/tip/pointerEvents.html#releasing-pointer-capture
async_gotlost_pointer_capture_events_ver7.diff is fully according with specification.
Possibly, current state of specification is wrong, because step 10.2.3 terminate sequence of steps.
And if we expect, that specification will be changed I would like to provide version 8 of patch.

Comment 29

[Maksim Lebedev]

Attachment: async_gotlost_pointer_capture_events_ver8.diff

+ Added: Checking element with "pending pcto" status

Possibly expected, that releasePointerCapture clears pointer capture.
I mean that "pending pcto" and "pct-override" will be empty after it.
Patch 8 implements such behavior.

Comment 30

[Maksim Lebedev]

Test without patch: https://tbpl.mozilla.org/?tree=Try&rev=b0a590a1eef5
Test with patch: https://tbpl.mozilla.org/?tree=Try&rev=587d7c35123c

Comment 31

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

Comment on attachment 8488518 [details] [diff] [review]
async_gotlost_pointer_capture_events_test_ver4.diff

># HG changeset patch
># Parent 776fa9cf70cd4605243f4dbf927a4ea5b6ea3124
># User Maksim Lebedev <[e-mail]>
>Bug 977003 - Test for check async got/lostpointercapture events. r=smaug
>
>diff --git a/layout/base/tests/bug977003_inner_1.html b/layout/base/tests/bug977003_inner_1.html
>new file mode 100644
>--- /dev/null
>+++ b/layout/base/tests/bug977003_inner_1.html
>@@ -0,0 +1,86 @@
>+<!DOCTYPE HTML>
>+<html>
>+<!--
>+https://bugzilla.mozilla.org/show_bug.cgi?id=977003
>+-->
>+<head>
>+  <meta charset="utf-8">
>+  <title>Test for Bug 977003</title>
>+  <meta name="author" content="Maksim Lebedev" />
>+  <script type="application/javascript" src="/tests/SimpleTest/SimpleTest.js"></script>
>+  <script type="application/javascript" src="/tests/SimpleTest/EventUtils.js"></script>
>+  <link rel="stylesheet" type="text/css" href="/tests/SimpleTest/test.css"/>
>+  <style>
>+    #target{ background: yellow; }
>+  </style>
>+  <script type="application/javascript">
>+    var target = undefined;
>+    var test_send_got = false;
>+    var test_got_async = false;
>+    var test_send_lost = false;
>+    var test_lost_async = false;

I'd really prefer if you tested the right ordering too here, 
not just that the event happened.
And make also sure you get only one event.


>+  <script type="application/javascript">
>+    var target = undefined;
>+    var middler = undefined;
>+    var listener = undefined;
>+    var test_down_got = false;
>+    var test_middler_got = false;
>+    var test_middler_lost = false;
>+    var test_listener = false;
middler sounds odd. Why not target2 or some such?


I wonder if you could have combined tests. There is plenty of overhead by just having so many files.
Like, you could have had just one top level document and it could load all the iframes one by one.

Comment 32

[Maksim Lebedev]

(In reply to Olli Pettay [:smaug] from comment #31)
> I'd really prefer if you tested the right ordering too here, not just that the event happened.
test_got_async and test_lost_async are the tests, which check correct async behavior.
> middler sounds odd. Why not target2 or some such?
"middler" means that it is situated in the middle of target and listener.

Comment 33

[Maksim Lebedev]

Attachment: async_gotlost_pointer_capture_events_test_ver5.diff

+ Changes: Middler -> Medium
+ Changes: All tests are made as parts of one test page

Comment 34

[Maksim Lebedev]

Tests without patch: https://tbpl.mozilla.org/?tree=Try&rev=02bba771e1c8
Tests with patch: https://tbpl.mozilla.org/?tree=Try&rev=a674570676b7

Comment 35

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

medium sounds still a bit odd. Are you looking for something like broker or mediator?
Not that I think either of those is quite right.

Comment 36

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

Comment on attachment 8489948 [details] [diff] [review]
async_gotlost_pointer_capture_events_test_ver5.diff

So change medium to... something. broker perhaps ?

And checking the order would be good.
So, check that when test_got_async is about to be set true, we have got
gotpointercapture event, etc.

Those fixed, r+

Comment 37

[Maksim Lebedev]

Attachment: async_gotlost_pointer_capture_events_test_ver6.diff

+ Changes: Medium -> Mediator

Comment 38

[Maksim Lebedev]

(In reply to Olli Pettay [:smaug] from comment #36)
> So change medium to... something. broker perhaps ?
"Broker" sounds like from financial world. I chose "mediator"
> And checking the order would be good.
> So, check that when test_got_async is about to be set true, we have got
> gotpointercapture event, etc.
Unfortunately, I didn't understand what You mean.
>function DownHandler(event) {
>  target.setPointerCapture(event.pointerId);
>  test_send_got++;
>}
>function GotPCHandler(event) {
>  if(test_send_got)
>    test_got_async++;
>}
I think that "if(test_send_got) got_async++" is explicity check of async behavior.

Comment 39

[Maksim Lebedev]

Tests and patch: https://tbpl.mozilla.org/?tree=Try&rev=f3ac7640b153

Comment 40

[Maksim Lebedev]

At-first, I expect that this patches will be landed to mozilla-central (trunc version).
At-second, will be great, if this patches will be landed to project/metro version too.

Comment 41

[Jim Mathies [:jimm]]

(In reply to Maksim Lebedev from comment #40)
> At-first, I expect that this patches will be landed to mozilla-central
> (trunc version).
> At-second, will be great, if this patches will be landed to project/metro
> version too.

I can do an mc merge. I think I'll need to handle bug 1059626 but hopefully that won't be a big deal.

Comment 42

[Ryan VanderMeulen [:RyanVM]]

(In reply to Maksim Lebedev from comment #39)
> Tests and patch: https://tbpl.mozilla.org/?tree=Try&rev=f3ac7640b153

The e10s mochitest-bc failures show up in every Try push of yours. Looks like a real problem to me?

Comment 43

[Ryan VanderMeulen [:RyanVM]]

Oh nevermind, you just pushed on top of a 2 week old m-c parent. Try to update your checkout before pushing to Try in the future?
https://hg.mozilla.org/integration/mozilla-inbound/rev/3493fb6e1e7d
https://hg.mozilla.org/integration/mozilla-inbound/rev/c0021e5609b2

Comment 44

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/3493fb6e1e7d
https://hg.mozilla.org/mozilla-central/rev/c0021e5609b2

Comment 45

[Maksim Lebedev]

(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #42)
> (In reply to Maksim Lebedev from comment #39)
> > Tests and patch: https://tbpl.mozilla.org/?tree=Try&rev=f3ac7640b153
> The e10s mochitest-bc failures show up in every Try push of yours. Looks
> like a real problem to me?
Since some time I always try to push my patches without patch of code changing.
And always try compare results. For example: comment #34
> Tests without patch: https://tbpl.mozilla.org/?tree=Try&rev=02bba771e1c8
> Tests with patch: https://tbpl.mozilla.org/?tree=Try&rev=a674570676b7

Comment 46

[Maksim Lebedev]

(In reply to Jim Mathies [:jimm] from comment #41)
> I can do an mc merge. I think I'll need to handle bug 1059626 but hopefully
> that won't be a big deal.
Do You want merge all pathces from m-c to project/metro?
As I said previously I think that it is not right way when m-c have no ability to testing metro version.
Current version of m-c have issues with compilation metro and have several issues with working metro, which I know.
And possibly have a lot of another issues, which we don't know because we have no ability to test metro in m-c.
I think that we should repair metro in m-c even though only for testing. And we can keep project/metro as stable version.