968148 - Implement PointerCapture for pointer events

Status: RESOLVED FIXED

(Core :: DOM: Events, defect)

Description

[Nick Lebedev [:nl]]

User Agent: Mozilla/5.0 (Windows NT 6.2; WOW64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/32.0.1700.107 Safari/537.36

Comment 1

[Sebastian Zartner [:sebo]]

The summary says "evens" instead of "events".

Sebastian

Comment 2

[Nick Lebedev [:nl]]

Attachment: Implement pointer capture feature.

By submitting this patch i'm looking for feedback for the following points:

1. Should pointer capture correlate somehow with the usual mousecapture and pointer lock stuff?
2. How to properly handle the cases when uses keep mouse button pressed and during that the tab changes?

Also, I was asked by annevk to proceed with not adding new DOMException type but with re-using Range Error.
(http://lists.w3.org/Archives/Public/public-pointer-events/2014JanMar/0067.html)
So i'll likely rework it to range error.

Comment 3

[Oleg Romashin (:romaxa)]

Comment on attachment 8370747 [details] [diff] [review]
Implement pointer capture feature.

>-  const unsigned short      INVALID_NODE_TYPE_ERR          = 24;
>-  const unsigned short      DATA_CLONE_ERR                 = 25;
>+  const unsigned short      INDEX_SIZE_ERR                      = 1;
>+  const unsigned short      DOMSTRING_SIZE_ERR                  = 2; // historical

could you not change indent for all const's here?

>+
>+// static
>+void nsIPresShell::GetPointerInfo(nsIContent* aElement, uint32_t aPointerId, bool &outIsValid, bool &outIsActive)
>+{
>+  nsIDocument* document = aElement->GetDocument();
>+
>+  if (document) {
>+    nsIPresShell* shell = document->GetShell();
>+    if (shell) {
>+      nsViewManager* vm = shell->GetViewManager();
>+      if (vm) {
>+        nsIWidget* widget = nullptr;
>+        vm->GetRootWidget(&widget);
>+        if (widget) {
>+          widget->IsPointerActive(aPointerId, outIsValid, outIsActive);
>+        }
>+      }
>+    }
>+  }

What if one this will fail? should not we give nsresult about it, or give better description for function behavior in header?


>-    return HandlePositionedEvent(frame, aEvent, aEventStatus);
>+      handleEventResult = shell->HandlePositionedEvent(frame, aEvent, aEventStatus);
>+
>+      if (shouldDispatchCaptureEvent) {
>+        DispatchLostPointerCaptureEvent(capturePointerId, lostPointerCaptureTarget);
>+      }
>+      return handleEventResult;
>+    }
>+
>+    handleEventResult = HandlePositionedEvent(frame, aEvent, aEventStatus);
>+
>+    if (shouldDispatchCaptureEvent) {
>+      DispatchLostPointerCaptureEvent(capturePointerId, lostPointerCaptureTarget);

Would it be possible with or without nsIRunnable to dispatch DispatchLostPointerCaptureEvent right from SetPointerCapturingContent function?

>+    }
>+    return handleEventResult;
>   }


>+nsIPresShell::DispatchCaptureEvent(const nsAString& aEventName,
>+                                   uint32_t aEventType,
>+                                   uint32_t aPointerId,
>+                                   nsIContent* aCaptureTarget)
>+{
>+  WidgetPointerEvent* pointerEvent =
>+    new WidgetPointerEvent(true, aEventType, nullptr);
>+  pointerEvent->pointerId = aPointerId;
>+
>+  nsIDOMEvent *event = nullptr;
               ^ stick pointer to the type name as above with "WidgetPointerEvent*"


>+  if (inputEvent->eventStructType == NS_POINTER_EVENT) {
>+    WidgetPointerEvent* pointerEvent = static_cast<WidgetPointerEvent*>(inputEvent);
>+
>+    MOZ_ASSERT(pointerEvent);
>+
>+    uint32_t pointerId = pointerEvent->pointerId;
>+    if (pointerEvent->inputSource == nsIDOMMouseEvent::MOZ_SOURCE_MOUSE) {
>+      if (pointerEvent->message == NS_POINTER_ENTER) {
>+        mWidget->UpdateActivePointerId(true, pointerId, false);
>+      } else if (pointerEvent->message == NS_POINTER_LEAVE) {
>+        mWidget->UpdateActivePointerId(false, pointerId, false);
>+      } else if (pointerEvent->message == NS_POINTER_DOWN) {
>+        mWidget->UpdateActivePointerId(true, pointerId, true);
>+      } else if (pointerEvent->message == NS_POINTER_UP) {
>+        mWidget->UpdateActivePointerId(true, pointerId, false);
>+      }

Can we use switch here?

>+    } else if (pointerEvent->inputSource == nsIDOMMouseEvent::MOZ_SOURCE_PEN) {
>+      if (pointerEvent->message == NS_POINTER_DOWN) {
>+        mWidget->UpdateActivePointerId(true, pointerId, true);
>+      } else if (pointerEvent->message == NS_POINTER_UP) {
>+        mWidget->UpdateActivePointerId(false, pointerId, false);
>+      }
>+    }
>+  }
>+


>+  uint32_t pointerId = event->touches[0]->Identifier();

sounds like you may crash here if event->touches.Length() == 0

>   if (event->message == NS_TOUCH_START) {
>     if (event->touches.Length() == 1) {
>       // If this is the first touchstart of a touch session reset some


>+void nsBaseWidget::IsPointerActive(uint32_t aPointerId, bool& aIsValid, bool& aIsActive) const
>+{
>+}
>+

>   NS_IMETHOD              SetModal(bool aModal);
>+  virtual void            IsPointerActive(uint32_t aPointerId, bool& aIsValid, bool& aIsActive) const;

 virtual void            IsPointerActive(uint32_t aPointerId, bool& aIsValid, bool& aIsActive) const {}
and remove empty method from cpp file

Comment 4

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

Comment on attachment 8370747 [details] [diff] [review]
Implement pointer capture feature.

I assume there will be a new patch.

Comment 5

[Oleg Romashin (:romaxa)]

Comment on attachment 8370747 [details] [diff] [review]
Implement pointer capture feature.

>+        if (widget) {
>+          widget->IsPointerActive(aPointerId, outIsValid, outIsActive);
>+        }
>+      }

>   gCaptureTouchList = nullptr;
>+  gPointerCaptureList = nullptr;
> }

>     /**
>+     * Returns whether pointer with given pointerId is active
>+     * and is also in active button state.
>+     */
>+    virtual void IsPointerActive(uint32_t aPointerId, bool& aIsValid, bool& aIsActive) const = 0;
>+

Is it really necessary to push IsPointerActive API down to widget level? 
Would it be more cross platform to track active/inactive pointers somewhere in nsEventStateManager...
even possibly merge it with mPointersEnterLeaveHelper coming in this patch: https://bugzilla.mozilla.org/attachment.cgi?id=8372050&action=edit
?

Comment 6

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

Cross-platform space is better, if just possible. Do we need IsPointerActive somewhere in off the main thread when doing hit testing using APZC?

Comment 7

[Oleg Romashin (MS)]

(In reply to Olli Pettay [:smaug] from comment #6)
> Cross-platform space is better, if just possible. Do we need IsPointerActive
> somewhere in off the main thread when doing hit testing using APZC?

sounds like no we don't, it is only used by Main thread

Comment 8

[Maksim Lebedev]

Attachment: Pointer_capture_changes.patch

Some changes were made thanks by comments in thread

Comment 9

[Oleg Romashin (:romaxa)]

Attachment: Simple pointer capture test converted from test_bug582771.html

Comment 10

[Oleg Romashin (MS)]

Attachment: Widget independent Pointer capture version

Comment 11

[Oleg Romashin (:romaxa)]

Comment on attachment 8374556 [details] [diff] [review]
Simple pointer capture test converted from test_bug582771.html

this has to be iframed in order to work with special powers properly

Comment 12

[Oleg Romashin (MS)]

Attachment: Simple pointer capture test converted from test_bug582771.html

This test works better on try with special powers

Comment 13

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

Comment on attachment 8374557 [details] [diff] [review]
Widget independent Pointer capture version

Need to still go through the existing mouse capture thing.
I think if pointer capture is used for mouse, we should effectively set 
also mouse capture.

Comment 14

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

Comment on attachment 8374640 [details] [diff] [review]
Simple pointer capture test converted from test_bug582771.html

We need some tests for multiple captured pointers

copy from dom/indexedDB/test/test_event_propagation.html
copy to layout/base/tests/test_bug968148.html
is surprising ;) but if that works, fine.

Comment 15

[Oleg Romashin (MS)]

Attachment: Pointer capture version

Minor cleanup + set default mouse capture

Comment 16

[Oleg Romashin (MS)]

Attachment: Simple pointer capture test converted from test_bug582771.html

Simply added two more elements, and duplicated all steps of the test for 2 different pointer ID's

Comment 17

[Oleg Romashin (MS)]

Attachment: Pointer capture version which compiles and runs

Comment 18

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

Comment on attachment 8376051 [details] [diff] [review]
Simple pointer capture test converted from test_bug582771.html

This is rather painful to read.  The original test was simple since it
had to test just one capture, but with several captures this needs some comments.

Comment 19

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

Comment on attachment 8376052 [details] [diff] [review]
Pointer capture version which compiles and runs

Is this the one which leaked on try?

Comment 20

[Oleg Romashin (MS)]

Attachment: Pointer capture

Fixed Memory leak.

Comment 21

[Oleg Romashin (MS)]

Attachment: Pointer capture test, support multi target/pointers

Added comment that test originally came from 582771 and was adopted to test multi pointers capture API

Comment 22

[Oleg Romashin (MS)]

Attachment: Pointer capture test, support multi target/pointers

Renamed test variables, instead of d1...d4, it has now d1 and d2 like original test had, but added prefix test1 and test2 for each Pointer ID accordingly.

Comment 23

[Oleg Romashin (MS)]

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

Comment 24

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

Comment on attachment 8377208 [details] [diff] [review]
Pointer capture test, support multi target/pointers

We need still some tests for setCapture + setPointerCapture, and how
mouse events work in that case. Well, also how pointer events work in that case.

Comment 25

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

Comment on attachment 8376848 [details] [diff] [review]
Pointer capture

I realized a bug in the spec. It says SetPointerCapture would work always with
mouse. But that would lead to various security issues. So SetPointerCapture
must work only between mousedown and mouseup, and released automatically
after mouseup.

>+typedef struct ActivePointerState {
Do you need typedef
{ should go to the next line

>+  // Keeps a map between pointerId and element that currently capturing pointer
>+  // with such pointerId. If pointerId is absent in this map then nobody is
>+  // capturing it.
>+  static nsDataHashtable<nsUint32HashKey, nsIContent*>* gPointerCaptureList;
This is super scary. Keeping a raw value pointing to nsIContent object, and nothing
guarantees the value is cleared before CC/GC have run and possibly deleted the object. 

>+PresShell::UpdateActivePointerState(WidgetGUIEvent* aEvent)
>+{
>+  WidgetPointerEvent* pointerEvent = aEvent->AsPointerEvent();
>+  if (!pointerEvent) {
>+    return;
>+  }
>+
>+  if (pointerEvent->inputSource == nsIDOMMouseEvent::MOZ_SOURCE_TOUCH) {
>+    switch (aEvent->message) {
>+    case NS_POINTER_DOWN:
>+      UpdatePointerIds(pointerEvent, true, true);
>+      break;
>+    case NS_POINTER_UP:
>+      UpdatePointerIds(pointerEvent, false, false);
>+    default:
>+      break;
>+    }
>+  } else {
>+    switch (aEvent->message) {
>+    case NS_POINTER_DOWN:
>+      UpdatePointerIds(pointerEvent, true, true);
>+      break;
>+    case NS_POINTER_UP:
>+      UpdatePointerIds(pointerEvent, true, false);
>+      break;
>+    case NS_POINTER_ENTER:
>+      UpdatePointerIds(pointerEvent, true, false);
>+      break;
>+    case NS_POINTER_LEAVE:
>+      UpdatePointerIds(pointerEvent, false, false);
>+      break;
Why do we need to handle pointerenter/leave here?

Comment 26

[Oleg Romashin (MS)]

Attachment: Pointer capture

Register active pointers only between down <> up, regardless pointer type.
store nsIContent in RefPtr Hash table.

Comment 27

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

Comment on attachment 8377547 [details] [diff] [review]
Pointer capture

>+++ b/dom/base/DOMException.cpp
>@@ -45,16 +45,17 @@ enum DOM4ErrorTypeCodeMap {
>   SecurityError              = nsIDOMDOMException::SECURITY_ERR,
>   NetworkError               = nsIDOMDOMException::NETWORK_ERR,
>   AbortError                 = nsIDOMDOMException::ABORT_ERR,
>   URLMismatchError           = nsIDOMDOMException::URL_MISMATCH_ERR,
>   QuotaExceededError         = nsIDOMDOMException::QUOTA_EXCEEDED_ERR,
>   TimeoutError               = nsIDOMDOMException::TIMEOUT_ERR,
>   InvalidNodeTypeError       = nsIDOMDOMException::INVALID_NODE_TYPE_ERR,
>   DataCloneError             = nsIDOMDOMException::DATA_CLONE_ERR,
>+  InvalidPointerId           = nsIDOMDOMException::INVALID_POINTER_ERR,
Note, this exception may change in the spec. It is not yet sure to which exception.


>+++ b/dom/webidl/EventHandler.webidl
>@@ -47,25 +47,27 @@ interface GlobalEventHandlers {
>            //(Not implemented)attribute EventHandler ondragexit;
>            attribute EventHandler ondragleave;
>            attribute EventHandler ondragover;
>            attribute EventHandler ondragstart;
>            attribute EventHandler ondrop;
>            attribute EventHandler ondurationchange;
>            attribute EventHandler onemptied;
>            attribute EventHandler onended;
>+           attribute EventHandler ongotpointercapture;
>            attribute EventHandler oninput;
>            attribute EventHandler oninvalid;
>            attribute EventHandler onkeydown;
>            attribute EventHandler onkeypress;
>            attribute EventHandler onkeyup;
>            attribute EventHandler onload;
>            attribute EventHandler onloadeddata;
>            attribute EventHandler onloadedmetadata;
>            attribute EventHandler onloadstart;
>+           attribute EventHandler onlostpointercapture;
These two need some [Pref="..."]


>+nsIPresShell::SetPointerCapturingContent(uint32_t aPointerId, nsIContent* aContent)
>+{
>+  uint16_t inputSource = 0;
>+  if (gActivePointersIds->Get(aPointerId, &inputSource) &&
>+      inputSource == nsIDOMMouseEvent::MOZ_SOURCE_MOUSE) {
>+    SetCapturingContent(aContent, CAPTURE_PREVENTDRAG);
>+  }
>+
>+  if (!aContent) {
>+    // Releasing capture for given pointer.
>+    gPointerCaptureList->Remove(aPointerId);
>+  } else {
>+    gPointerCaptureList->Put(aPointerId, aContent);
So what if there is already some content capturing that pointer.
And especially, what if that content is from another document (perhaps even different domain).
The spec if not clear what to do in that case. So, a spec bug please.


>+    nsresult handleEventResult = NS_OK;
nsresult rv


r- because we need to get that spec issue sorted out before landing this.
(The issue is rather important since as far as I see per spec information leaks to other domains would be possible.)

Comment 28

[Oleg Romashin (MS)]

Attachment: Pointer capture implementation

Comment 29

[Oleg Romashin (:romaxa)]

Attachment: Setup pointer related target properly for capturing content

I think it make sense to merge bug 970220 into this one

Comment 30

[Oleg Romashin (MS)]

Attachment: Pointer capture implementation

Fixed some issues according to spec description. Update Active pointers by pref only

Comment 31

[Oleg Romashin (:romaxa)]

Comment on attachment 8382938 [details] [diff] [review]
Setup pointer related target properly for capturing content

Sounds like spec has been changed recently, and this patch is not required anymore

Comment 32

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

Comment on attachment 8382939 [details] [diff] [review]
Pointer capture implementation

>+NS_IMETHOD SetPointerCapture(int32_t pointerId) MOZ_FINAL                     \
>+{                                                                             \
>+  mozilla::ErrorResult rv;                                                    \
>+  Element::SetPointerCapture(pointerId, rv);                                  \
>+  return rv.ErrorCode();                                                      \
>+}                                                                             \
>+NS_IMETHOD ReleasePointerCapture(int32_t pointerId) MOZ_FINAL                 \
>+{                                                                             \
>+  mozilla::ErrorResult rv;                                                    \
>+  Element::ReleasePointerCapture(pointerId, rv);                              \
>+  return rv.ErrorCode();                                                      \
>+}                                                                             \
> NS_IMETHOD SetCapture(bool retargetToElement) MOZ_FINAL                       \
> {                                                                             \
>   Element::SetCapture(retargetToElement);                                     \
>   return NS_OK;                                                               \
> }                                                                             \
> NS_IMETHOD ReleaseCapture(void) MOZ_FINAL                                     \
> {                                                                             \
>   Element::ReleaseCapture();                                                  \
Why do we need these?


>+nsIPresShell::DispatchPointerCaptureEvent(bool aIsGotCapture,
>+                                          uint32_t aPointerId,
>+                                          nsIContent* aCaptureTarget)
>+{
>+  nsAutoPtr<WidgetPointerEvent> pointerEvent;
>+  pointerEvent =
>+    new WidgetPointerEvent(true,
>+                           aIsGotCapture ? NS_POINTER_GOT_CAPTURE :
>+                                           NS_POINTER_LOST_CAPTURE,
>+                           nullptr);
>+  pointerEvent->pointerId = aPointerId;
>+  nsCOMPtr<nsIDOMEvent> event;
>+  nsresult rv =
>+    NS_NewDOMPointerEvent(getter_AddRefs(event), aCaptureTarget, nullptr, pointerEvent);
>+  if (NS_SUCCEEDED(rv)) {
>+    rv = event->InitEvent(aIsGotCapture ? NS_LITERAL_STRING("gotpointercapture") :
>+                                          NS_LITERAL_STRING("lostpointercapture"),
>+                             false, false);
These events should bubble. Missing test obviously.

>+    event->SetTrusted(true);
>+    if (NS_SUCCEEDED(rv)) {
>+      bool rv;
>+      aCaptureTarget->DispatchEvent(event, &rv);
>+    }
>+  }
>+}
This all looks odd.
Why you need WidgetPointerEvent?
Just create PointerEvent with right type and dispatch it.

Comment 33

[Maksim Lebedev]

Attachment: Pointer capture implementation (updated)

Added some changes in implementation from related bug 976963

Comment 34

[Maksim Lebedev]

(In reply to Olli Pettay [:smaug] from comment #32)

> >+NS_IMETHOD SetPointerCapture(int32_t pointerId) MOZ_FINAL                     \
> >+NS_IMETHOD ReleasePointerCapture(int32_t pointerId) MOZ_FINAL                 \
> Why do we need these?
Look like all methods of element should be there. Like a:
> > NS_IMETHOD SetCapture(bool retargetToElement) MOZ_FINAL                       \
> > NS_IMETHOD ReleaseCapture(void) MOZ_FINAL                                     \

Comment 35

[Oleg Romashin (:romaxa)]

Comment on attachment 8385299 [details] [diff] [review]
Pointer capture implementation (updated)

> void
>+nsIPresShell::DispatchPointerCaptureEvent(bool aIsGotCapture,
>+                                          uint32_t aPointerId,
>+                                          nsIContent* aCaptureTarget)
>+{
>+  nsAutoPtr<WidgetPointerEvent> pointerEvent;
>+  pointerEvent =
>+    new WidgetPointerEvent(true,
>+                           aIsGotCapture ? NS_POINTER_GOT_CAPTURE :
>+                                           NS_POINTER_LOST_CAPTURE,
>+                           nullptr);

smaug was asking to remove WidgetPointerEvent and just create dom::PointerEvent here.. could you update it too?

Comment 36

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

(In reply to Maksim Lebedev from comment #34)
> (In reply to Olli Pettay [:smaug] from comment #32)
> 
> > >+NS_IMETHOD SetPointerCapture(int32_t pointerId) MOZ_FINAL                     \
> > >+NS_IMETHOD ReleasePointerCapture(int32_t pointerId) MOZ_FINAL                 \
> > Why do we need these?
> Look like all methods of element should be there. Like a:
> > > NS_IMETHOD SetCapture(bool retargetToElement) MOZ_FINAL                       \
> > > NS_IMETHOD ReleaseCapture(void) MOZ_FINAL                                     \
Stuff from .idl, I think. But new methods go to .webidl only.

Comment 37

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

Comment on attachment 8385299 [details] [diff] [review]
Pointer capture implementation (updated)

>+class LostPointerCaptureDispatcher
>+{
Now I'm lost. This class is also in the patch for bug 976963, which
was in my review queue.

>+nsIPresShell::DispatchPointerCaptureEvent(bool aIsGotCapture,
>+                                          uint32_t aPointerId,
>+                                          nsIContent* aCaptureTarget)
>+{
>+  nsAutoPtr<WidgetPointerEvent> pointerEvent;
>+  pointerEvent =
>+    new WidgetPointerEvent(true,
>+                           aIsGotCapture ? NS_POINTER_GOT_CAPTURE :
>+                                           NS_POINTER_LOST_CAPTURE,
>+                           nullptr);
>+  pointerEvent->pointerId = aPointerId;
>+  nsCOMPtr<nsIDOMEvent> event;
>+  nsresult rv =
>+    NS_NewDOMPointerEvent(getter_AddRefs(event), aCaptureTarget, nullptr, pointerEvent);
>+  if (NS_SUCCEEDED(rv)) {
>+    rv = event->InitEvent(aIsGotCapture ? NS_LITERAL_STRING("gotpointercapture") :
>+                                          NS_LITERAL_STRING("lostpointercapture"),
>+                             false, false);
>+    event->SetTrusted(true);
>+    if (NS_SUCCEEDED(rv)) {
>+      bool rv;
>+      aCaptureTarget->DispatchEvent(event, &rv);
>+    }
>+  }
I have no idea why you need pointerEvent variable.
Just create a PointerEvent and initialize it.
But note, per spec got/lostpointercapture should be dispatch asynchronously.
So you probably want to use nsAsyncDOMEvent to dispatch the event you create here.
It has PostDOMEvent() for async dispatching.

Comment 38

[Maksim Lebedev]

(In reply to Olli Pettay [:smaug] from comment #32)
> Why you need WidgetPointerEvent?
> Just create PointerEvent with right type and dispatch it.
(In reply to Oleg Romashin (:romaxa) from comment #35)
> smaug was asking to remove WidgetPointerEvent and just create
> dom::PointerEvent here.. could you update it too?

How we can create dom:PointerEvent without WidgetPointerEvent?
PointerEvent needs WidgetPointerEvent* in constructor.

(In reply to Olli Pettay [:smaug] from comment #37)
> >+class LostPointerCaptureDispatcher
> >+{
> Now I'm lost. This class is also in the patch for bug 976963, which
> was in my review queue.

Please, do not be discouraged, all it's ok. Previous version of patch 968148 had a bug, which was resolved in patch 976963.
And if you want that FireFox have full support of specification of Pointer Events, we should commit 968148 and 976963 at the same time.
That's why we think that we should combine this patches.
Please, let us know you opinion about our solution.

Comment 39

[Maksim Lebedev]

Attachment: pointerCapture_updated.diff

Some changes according with comments of bug 968148 and bug 976963

Comment 40

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

Combining patches is ok. I just didn't understand how two patches contained the same class.

Comment 41

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

Comment on attachment 8386655 [details] [diff] [review]
pointerCapture_updated.diff

>+NS_IMETHOD SetPointerCapture(int32_t pointerId) MOZ_FINAL                     \
>+{                                                                             \
>+  mozilla::ErrorResult rv;                                                    \
>+  Element::SetPointerCapture(pointerId, rv);                                  \
>+  return rv.ErrorCode();                                                      \
>+}                                                                             \
>+NS_IMETHOD ReleasePointerCapture(int32_t pointerId) MOZ_FINAL                 \
>+{                                                                             \
>+  mozilla::ErrorResult rv;                                                    \
>+  Element::ReleasePointerCapture(pointerId, rv);                              \
>+  return rv.ErrorCode();                                                      \
>+}                                                                             \
As I wondered before... why these? You don't add anything to .idl, only to
.webidl, so why to implement something for .idl?
> 
>+  LostPointerCaptureDispatcher lostPointerCaptureDispatcher(aMouseEvent);
>+
>   EnterLeaveDispatcher leaveDispatcher(this, wrapper->mLastOverElement,
>                                        aMovingInto, aMouseEvent,
>                                        isPointer ? NS_POINTER_LEAVE :
>                                                    NS_MOUSELEAVE);
What has LostPointerCaptureDispatcher to do with pointerleave dispatching?
We should dispatch lostpointercapture asynchronously after we've lost the pointer capture.
Not when some pointerleave is about to be dispatched.
In other words, odd to see this stuff in EventStateManager.

>+nsIPresShell::DispatchPointerCaptureEvent(bool aIsGotCapture,
>+                                          uint32_t aPointerId,
>+                                          nsIContent* aCaptureTarget)
>+{
>+  WidgetPointerEvent pointerEvent(true,
>+                           aIsGotCapture ? NS_POINTER_GOT_CAPTURE :
>+                                           NS_POINTER_LOST_CAPTURE,
>+                           nullptr);
>+  pointerEvent.pointerId = aPointerId;
>+  nsCOMPtr<nsIDOMEvent> event;
>+  nsresult rv =
>+    NS_NewDOMPointerEvent(getter_AddRefs(event), aCaptureTarget, nullptr, &pointerEvent);
>+  if (NS_SUCCEEDED(rv)) {
>+    rv = event->InitEvent(aIsGotCapture ? NS_LITERAL_STRING("gotpointercapture") :
>+                                          NS_LITERAL_STRING("lostpointercapture"),
>+                             false, false);
Some odd indentation here, and these events bubble.

>+    event->SetTrusted(true);
>+    if (NS_SUCCEEDED(rv)) {
>+      bool rv;
rv is for nsresult. bool dummy

>+      aCaptureTarget->DispatchEvent(event, &rv);
This still dispatches event synchronously. Per spec dispatching should happen using a task.
So, use nsAsyncDOMEvent as I told before ;)

Comment 42

[Maksim Lebedev]

(In reply to Olli Pettay [:smaug] from comment #41)
> What has LostPointerCaptureDispatcher to do with pointerleave dispatching?
> We should dispatch lostpointercapture asynchronously after we've lost the
> pointer capture.
> Not when some pointerleave is about to be dispatched.
> In other words, odd to see this stuff in EventStateManager.

As I wrote in related bug 976963 comment #10:
LostPointerCapture event should be received the last in the queue for listener (element which received pointer capture).
I created special class for send LostPointerCapture event exactly in EventStateManager. This is ensures that LostPointerCapture event will be send immediately after last event to listener (PointerLeave). That's why sending LostPointerCapture event is done not in place where it happens. And it's done in another place. That's why we can think that this event is sending asynchronously.

Comment 43

[Maksim Lebedev]

Attachment: pointerCapture_updated_ver3.diff

Some changes

Comment 44

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

Comment on attachment 8387415 [details] [diff] [review]
pointerCapture_updated_ver3.diff

The spec is buggy here
https://www.w3.org/Bugs/Public/show_bug.cgi?id=24971
But I was told IE does async dispatching, so we should do, and
the spec will get fixed.

Comment 45

[Oleg Romashin (MS)]

Comment on attachment 8387415 [details] [diff] [review]
pointerCapture_updated_ver3.diff


>       case NS_POINTER_ENTER:
>       case NS_POINTER_LEAVE:
>         mFlags.mBubbles = false;
>+        mFlags.mCancelable = false;
>+        break;
>+      case NS_POINTER_CANCEL:
>+      case NS_POINTER_GOT_CAPTURE:
>+      case NS_POINTER_LOST_CAPTURE:
>+        mFlags.mCancelable = false;
>         break;
>       default:

This does not looks right, these changes coming from some another bug like https://bugzilla.mozilla.org/show_bug.cgi?id=977695
and more over these changes already landed into upstream
https://hg.mozilla.org/mozilla-central/rev/dd7286173768

Comment 46

[Maksim Lebedev]

Attachment: pointerCapture_updated_ver4.diff

- some code was deleted, which was already comited.
+ correct behavior was added for SetPointerCapture function.

Comment 47

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

I have still no idea why LostPointerCaptureDispatcher is needed.

Comment 48

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

Comment on attachment 8389150 [details] [diff] [review]
pointerCapture_updated_ver4.diff

Based on the conf call we probably should dispatch got/lostpointercapture
before any other new event is dispatched.
So, I have no idea why LostPointerCaptureDispatcher would be needed.

Comment 49

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

(But the spec bug is not fixed yet.)

Comment 50

[Maksim Lebedev]

Attachment: pointerCapture_updated_ver5.diff

+ add code from related bug 970220

Comment 51

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

Comment on attachment 8389805 [details] [diff] [review]
pointerCapture_updated_ver5.diff

I have still no idea why LostPointerCaptureDispatcher is needed.
And https://bugzilla.mozilla.org/show_bug.cgi?id=976963#c10 doesn't explain that.
Why should lostpointercapture be ever postponed to happen after pointerleave events?

And nsIPresShell::DispatchPointerCaptureEvent should be renamed to something like
nsIPresShell::DispatchGotOrLostPointerCaptureEvent.

In nsIPresShell::DispatchPointerCaptureEvent you don't need pointerEvent.
Just create the DOM event and call InitEvent() on it and set it
trusted and dispatch the event.

(And the spec isn't clear whether to dispatch async or sync.)

Comment 52

[Maksim Lebedev]

Attachment: pointerCapture_updated_ver6.diff

+ add PointerEvent::Constructor
+ change internal code in DispatchPointerCaptureEvent
+ rename DispatchPointerCaptureEvent -> DispatchGotOrLostPointerCaptureEvent
+ add correct work with pointerId
+ add if-statement to correct work with pressure

Comment 53

[Maksim Lebedev]

(In reply to Olli Pettay [:smaug] from comment #51)
> I have still no idea why LostPointerCaptureDispatcher is needed.
> And https://bugzilla.mozilla.org/show_bug.cgi?id=976963#c10 doesn't explain that.
> Why should lostpointercapture be ever postponed to happen after pointerleave events?
Simple test without LostPointerCaptureDispatcher:
div.target and div.listener
div.target.onPointerDown -> div.listener.setPointerCapture()
div.listener.onPointerMove -> div.listener.releasePointerCapture()
The log of events can show this sequence of events:
1.Target.pointerOver
2.Target.pointerEnter
3.Target.pointerDown
4.Listener.GotPointerCapture
5.Target.pointerOut
6.Target.pointerLeave
7.Listener.pointerOver
8.Listener.pointerEnter
9.Listener.pointerOut
10.Listener.pointerLeave
11.Target.pointerOver
12.Target.pointerEnter
13.Target.pointerUp
14.Target.pointerOut
15.Target.pointerLeave
One question where should be receive Listener.LostPointerCapture?
My opinion: lostPointerCapture should be receive between 10 and 11.
The main goal of LostPointerCaptureDispatcher is send event between 10 and 11.

Comment 54

[Maksim Lebedev]

But I am confused by results of another simple test:
div.target and div.listener
div.target.onPointerDown -> div.listener.setPointerCapture()
User puts mouse on target and lifts up mouse from target.
The log of events can show this sequence of events:
1.Target.pointerOver
2.Target.pointerEnter
3.Target.pointerDown
4.Listener.gotPointerCapture
5.Target.pointerOut
6.Target.pointerLeave
7.Listener.pointerOver
8.Listener.pointerEnter
9.Listener.pointerUp
10.Listener.lostPointerCapture
11.Listener.pointerOut
12.Assert(Listener.pointerOut)
13.Listener.pointerLeave
14.Assert(Listener.pointerLeave)
15.Target.pointerOver
16.Target.pointerEnter
17.Target.pointerOut
18.Target.pointerLeave
Log shows that any event to listener which is reseived after lostPointerCapture call assert function.
May be right way in this secuence is move lostPointerCapture after pointerLeave.

Comment 55

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

(In reply to Maksim Lebedev from comment #53)
> (In reply to Olli Pettay [:smaug] from comment #51)
> > I have still no idea why LostPointerCaptureDispatcher is needed.
> > And https://bugzilla.mozilla.org/show_bug.cgi?id=976963#c10 doesn't explain that.
> > Why should lostpointercapture be ever postponed to happen after pointerleave events?
> Simple test without LostPointerCaptureDispatcher:
> div.target and div.listener
> div.target.onPointerDown -> div.listener.setPointerCapture()
> div.listener.onPointerMove -> div.listener.releasePointerCapture()
> The log of events can show this sequence of events:
> 1.Target.pointerOver
> 2.Target.pointerEnter
> 3.Target.pointerDown
> 4.Listener.GotPointerCapture
> 5.Target.pointerOut
> 6.Target.pointerLeave
> 7.Listener.pointerOver
> 8.Listener.pointerEnter
> 9.Listener.pointerOut
> 10.Listener.pointerLeave
> 11.Target.pointerOver
> 12.Target.pointerEnter
> 13.Target.pointerUp
> 14.Target.pointerOut
> 15.Target.pointerLeave

This list if missing the pointermove which calls releasePointerCapture. lostpointercapture should happen right after that,
I think. The spec isn't fixed yet, so it is not clear whether to dispatch sync or async or what.
Another possibility is to dispatch it even synchronously, so while we're handling pointermove, we'd get lostpointercapture.


> One question where should be receive Listener.LostPointerCapture?
> My opinion: lostPointerCapture should be receive between 10 and 11.
> The main goal of LostPointerCaptureDispatcher is send event between 10 and
> 11.
Nothing in the spec hints lostpointercapture should happen between 10 and 11.

Comment 56

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

What does IE do?

Comment 57

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

 
> This list if missing the pointermove which calls releasePointerCapture.
> (1) lostpointercapture should happen right after that,
> I think. The spec isn't fixed yet, so it is not clear whether to dispatch
> sync or async or what.
> (2) Another possibility is to dispatch it even synchronously, so while we're
> handling pointermove, we'd get lostpointercapture.
> 
And 3rd option is to dispatch lostpointercapture truly asynchronously.
But the comments in the spec bug hints (2).

Comment 58

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

Er, sorry, the comments hint that (1) should be done. So the next pointerevent for pointerid X after
releasePointerCapture is called should be lostpointercapture.

Comment 59

[Maksim Lebedev]

Attachment: pointerCapture_updated_ver7.diff

- delete build error (with "const" EventTarget* aOwner)
+ small change of work with ReleasePointerCaptureContent()

Comment 60

[Maksim Lebedev]

(In reply to Olli Pettay [:smaug] from comment #55)
> This list if missing the pointermove which calls releasePointerCapture.
> lostpointercapture should happen right after that,
pointerMove event was deleted in log by me, because it happens very often

> > One question where should be receive Listener.LostPointerCapture?
> > My opinion: lostPointerCapture should be receive between 10 and 11.
> > The main goal of LostPointerCaptureDispatcher is send event between 10 and 11.
> Nothing in the spec hints lostpointercapture should happen between 10 and 11.
And nothing in the spec hints that lostpointercapture should NOT happen between 10 and 11.

> Er, sorry, the comments hint that (1) should be done. So the next
> pointerevent for pointerid X after
> releasePointerCapture is called should be lostpointercapture.
We can see test in https://github.com/InternetExplorer/web-platform-tests/blob/submission/Microsoft/PointerEvents/pointerevents/pointerevent_releasePointerCapture_events_to_original_target.html
In this test we can see if listener receives any event after lostpointercapture we call assert(false)

Comment 61

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

(In reply to Maksim Lebedev from comment #60)
> (In reply to Olli Pettay [:smaug] from comment #55)
> > This list if missing the pointermove which calls releasePointerCapture.
> > lostpointercapture should happen right after that,
> pointerMove event was deleted in log by me, because it happens very often
Well, it is a key point to see where the releasePointerCapture is called.


> We can see test in
> https://github.com/InternetExplorer/web-platform-tests/blob/submission/
> Microsoft/PointerEvents/pointerevents/
> pointerevent_releasePointerCapture_events_to_original_target.html
> In this test we can see if listener receives any event after
> lostpointercapture we call assert(false)
Dont' rely on random tests. As far as I know, that is just a test submission from MS, which hasn't even
been reviewed.

Comment 62

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

Comment on attachment 8391210 [details] [diff] [review]
pointerCapture_updated_ver7.diff

Please remove LostPointerCaptureDispatcher, or explain what in the spec says we should delay lostpointercapture to happen after pointerleave events.

The need for gLostPointerCaptureList isn't clear to me either

Comment 63

[Maksim Lebedev]

Attachment: pointerCapture_updated_ver8.diff

- delete class LostPointerCaptureDispatcher
- delete nsIPresShell::UpdateLostPointerCapturingContent
- delete nsIPresShell::gLostPointerCaptureList

Comment 64

[Oleg Romashin (MS)]

Comment on attachment 8392257 [details] [diff] [review]
pointerCapture_updated_ver8.diff


>+    newPointerEvent->relatedTarget = nsIPresShell::GetPointerCapturingContent(sourcePointer->pointerId)
>+                                    ? sourcePointer->relatedTarget
>+                                    : aRelatedContent;
>+    event = newPointerEvent.forget();

You are merging patch from bug 970220, this is fine, but probably better close bug 970220 then.




>   bool convertToPointer;
>+  uint32_t pointerId;
>   uint32_t tiltX;
>   uint32_t tiltY;
> 
>-  WidgetPointerHelper() : convertToPointer(true), tiltX(0), tiltY(0) {}
>+  WidgetPointerHelper() : convertToPointer(true), pointerId(0), tiltX(0), tiltY(0) {}


>-  props->get_Pressure(&pressure);
>+  aPointerPoint->get_PointerId(&aEvent->pointerId);
>+  props->get_Pressure(&aEvent->pressure);
>   props->get_XTilt(&tiltX);


I think these changes are not related to Capture implementation.
move them into separate bug with appropriate description

Comment 65

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

Comment on attachment 8392257 [details] [diff] [review]
pointerCapture_updated_ver8.diff

># HG changeset patch
># Parent 570c4ac17aa53b87583ad274d11d749d0aa35803
># User Oleg Romashin <[e-mail]>
>
>Bug 968148 - Implement PointerCapture for pointer events. r=smaug
>Bug 976963 - After invoking the releasePointerCapture method on an element, subsequent events for the specified pointer must follow normal hit testing mechanisms for determining the event target.
>
>diff --git a/content/base/public/Element.h b/content/base/public/Element.h
>--- a/content/base/public/Element.h
>+++ b/content/base/public/Element.h
>@@ -618,16 +618,42 @@ public:
>   already_AddRefed<nsIHTMLCollection>
>     GetElementsByTagNameNS(const nsAString& aNamespaceURI,
>                            const nsAString& aLocalName,
>                            ErrorResult& aError);
>   already_AddRefed<nsIHTMLCollection>
>     GetElementsByClassName(const nsAString& aClassNames);
>   bool MozMatchesSelector(const nsAString& aSelector,
>                           ErrorResult& aError);
>+  void SetPointerCapture(int32_t aPointerId, ErrorResult& aError)
>+  {
>+    bool activeState = false;
>+    if (!nsIPresShell::GetPointerInfo(aPointerId, activeState)) {
>+      aError.Throw(NS_ERROR_DOM_INVALID_POINTER_ERR);
>+      return;
>+    }
>+    if(!activeState) {
>+      return;
>+    }
>+    nsIPresShell::SetPointerCapturingContent(aPointerId, this);
>+  }
>+  void ReleasePointerCapture(int32_t aPointerId, ErrorResult& aError)
>+  {
>+    bool activeState = false;
>+    if (!nsIPresShell::GetPointerInfo(aPointerId, activeState)) {
>+      aError.Throw(NS_ERROR_DOM_INVALID_POINTER_ERR);
>+      return;
>+    }
>+
>+    // Ignoring ReleasePointerCapture call on incorrect element (on element
>+    // that didn't have capture before).
>+    if (nsIPresShell::GetPointerCapturingContent(aPointerId) == this) {
>+      nsIPresShell::ReleasePointerCapturingContent(aPointerId, this);
>+    }
>+  }
>   void SetCapture(bool aRetargetToElement)
>   {
>     // If there is already an active capture, ignore this request. This would
>     // occur if a splitter, frame resizer, etc had already captured and we don't
>     // want to override those.
>     if (!nsIPresShell::GetCapturingContent()) {
>       nsIPresShell::SetCapturingContent(this, CAPTURE_PREVENTDRAG |
>         (aRetargetToElement ? CAPTURE_RETARGETTOELEMENT : 0));
>@@ -1553,16 +1579,28 @@ NS_IMETHOD GetScrollTopMax(int32_t* aScr
> }                                                                             \
> NS_IMETHOD MozMatchesSelector(const nsAString& selector,                      \
>                               bool* _retval) MOZ_FINAL                        \
> {                                                                             \
>   mozilla::ErrorResult rv;                                                    \
>   *_retval = Element::MozMatchesSelector(selector, rv);                       \
>   return rv.ErrorCode();                                                      \
> }


>+NS_IMETHOD SetPointerCapture(int32_t pointerId) MOZ_FINAL                     \
>+{                                                                             \
>+  mozilla::ErrorResult rv;                                                    \
>+  Element::SetPointerCapture(pointerId, rv);                                  \
>+  return rv.ErrorCode();                                                      \
>+}                                                                             \
>+NS_IMETHOD ReleasePointerCapture(int32_t pointerId) MOZ_FINAL                 \
>+{                                                                             \
>+  mozilla::ErrorResult rv;                                                    \
>+  Element::ReleasePointerCapture(pointerId, rv);                              \
>+  return rv.ErrorCode();                                                      \
>+}                                                                             \
Why do we need these?



>+PointerEvent::Constructor(EventTarget* aOwner,
>                           const nsAString& aType,
>-                          const PointerEventInit& aParam,
>-                          ErrorResult& aRv)
>+                          const PointerEventInit& aParam)
> {
>-  nsCOMPtr<EventTarget> t = do_QueryInterface(aGlobal.GetAsSupports());
>-  nsRefPtr<PointerEvent> e = new PointerEvent(t, nullptr, nullptr);
>-  bool trusted = e->Init(t);
>+  nsRefPtr<PointerEvent> e = new PointerEvent(aOwner, nullptr, nullptr);
>+  bool trusted = e->Init(aOwner);
> 
>-  aRv = e->InitMouseEvent(aType, aParam.mBubbles, aParam.mCancelable,
>+  ErrorResult rv;
>+  rv = e->InitMouseEvent(aType, aParam.mBubbles, aParam.mCancelable,
>                           aParam.mView, aParam.mDetail, aParam.mScreenX,
>                           aParam.mScreenY, aParam.mClientX, aParam.mClientY,
>                           aParam.mCtrlKey, aParam.mAltKey, aParam.mShiftKey,
>                           aParam.mMetaKey, aParam.mButton,
>                           aParam.mRelatedTarget);
>-  if (aRv.Failed()) {
>+  if (rv.Failed()) {
>     return nullptr;
>   }
In practice you can just remove ErrorResult rv; and not deal with those errors.



>+    newPointerEvent->relatedTarget = nsIPresShell::GetPointerCapturingContent(sourcePointer->pointerId)
>+                                    ? sourcePointer->relatedTarget
>+                                    : aRelatedContent;
Shouldn't relatedTarget be null here if we're capturing the pointer.


"If the pointer capture target override has been set for the pointer,
 -Set the relatedTarget attribute of the event to null
 -Fire the event to the pointer capture target override object.
 Otherwise, fire the event to the object returned by normal hit test mechanisms (out of scope for this specification)."

>+  static bool GetPointerInfo(uint32_t aPointerId, bool& aActiveState);
Want to document this method. It is not clear what it returns.

>+nsIPresShell::SetPointerCapturingContent(uint32_t aPointerId, nsIContent* aContent)
>+{
>+  nsIContent* content = GetPointerCapturingContent(aPointerId);
>+
>+  bool activeState = false;
>+  if (!content && gActivePointersIds->Get(aPointerId, &activeState) && activeState) {
>+    SetCapturingContent(aContent, CAPTURE_PREVENTDRAG);
Shouldn't we call this only in case aPointerId is for mouse.


>+nsIPresShell::ReleasePointerCapturingContent(uint32_t aPointerId, nsIContent* aContent)
>+{
>+  bool activeState = false;
>+  if (gActivePointersIds->Get(aPointerId, &activeState)) {
>+    SetCapturingContent(nullptr, CAPTURE_PREVENTDRAG);
>+  }
Also here, only if mouse is being released.


>+nsIPresShell::GetPointerCapturingContent(uint32_t aPointerId)
>+{
>+  nsCOMPtr<nsIContent> content;
>+  if (!gPointerCaptureList->Get(aPointerId, getter_AddRefs(content))) {
>+    return nullptr;
>+  }
>+
>+  return content;

Just
  return gPointerCaptureList->GetWeak(aPointerId); 


>+PresShell::UpdateActivePointerState(WidgetGUIEvent* aEvent)
>+{
>+  WidgetPointerEvent* pointerEvent = aEvent->AsPointerEvent();
>+  if (!pointerEvent) {
>+    return;
>+  }
>+
>+  switch (aEvent->message) {
>+  case NS_POINTER_ENTER:
>+    if(pointerEvent->inputSource != nsIDOMMouseEvent::MOZ_SOURCE_TOUCH) {
>+      gActivePointersIds->Put(pointerEvent->pointerId, false);
>+    }
>+    break;
>+  case NS_POINTER_DOWN:
>+    gActivePointersIds->Put(pointerEvent->pointerId, true);
>+    break;
>+  case NS_POINTER_UP:
>+    if(pointerEvent->inputSource != nsIDOMMouseEvent::MOZ_SOURCE_TOUCH) {
>+      gActivePointersIds->Put(pointerEvent->pointerId, false);
>+    } else {
>+      gActivePointersIds->Remove(pointerEvent->pointerId);
>+    }
>+    break;
>+  case NS_POINTER_LEAVE:
>+    gActivePointersIds->Remove(pointerEvent->pointerId);
>+    break;
Why you need to deal with NS_POINTER_ENTER/LEAVE here?
DOWN/UP should be enough, and perhaps CANCEL


>     WidgetPointerEvent event(*mouseEvent);
>     event.message = pointerMessage;
>     event.button = button;
>+    if (!mouseEvent->buttons) {
>+      event.pressure = 0;
>+    }
Doesn't seem to be for this bug.

>+nsIPresShell::DispatchGotOrLostPointerCaptureEvent(bool aIsGotCapture,
>+                                                    uint32_t aPointerId,
>+                                                    nsIContent* aCaptureTarget)
>+{
>+  PointerEventInit init;
>+  init.mPointerId = aPointerId;
These events do bubble, so init.mBubbles = true;

Looking much better :) Perhaps one more round?
(This means lost/got will be sync always, but that is one possible way to interpret the current spec, and
I think we will even need to spec that behavior.)

This obviously needs tests

Comment 66

[Maksim Lebedev]

Attachment: pointerCapture_updated_ver9.diff

- delete code:
>+NS_IMETHOD SetPointerCapture(int32_t pointerId) MOZ_FINAL
>+NS_IMETHOD ReleasePointerCapture(int32_t pointerId) MOZ_FINAL
- delete additional code
+ add coment for GetPointerInfo
+ update GetPointerCapturingContent
+ add flag mBubbles

Comment 67

[Maksim Lebedev]

(In reply to Olli Pettay [:smaug] from comment #65)

> >+  switch (aEvent->message) {
> >+  case NS_POINTER_ENTER:
> >+    if(pointerEvent->inputSource != nsIDOMMouseEvent::MOZ_SOURCE_TOUCH) {
> >+      gActivePointersIds->Put(pointerEvent->pointerId, false);
> >+    }
> >+    break;
> >+  case NS_POINTER_DOWN:
> >+    gActivePointersIds->Put(pointerEvent->pointerId, true);
> >+    break;
> >+  case NS_POINTER_UP:
> >+    if(pointerEvent->inputSource != nsIDOMMouseEvent::MOZ_SOURCE_TOUCH) {
> >+      gActivePointersIds->Put(pointerEvent->pointerId, false);
> >+    } else {
> >+      gActivePointersIds->Remove(pointerEvent->pointerId);
> >+    }
> >+    break;
> >+  case NS_POINTER_LEAVE:
> >+    gActivePointersIds->Remove(pointerEvent->pointerId);
> >+    break;
> Why you need to deal with NS_POINTER_ENTER/LEAVE here?
> DOWN/UP should be enough, and perhaps CANCEL
Elements can receive NS_POINTER_ENTER/LEAVE when user move mouse over element.
In this case this pointer exist, but not in active buttons state.
NS_POINTER_CANCEL doesn't change state of active pointers.

> Looking much better :) Perhaps one more round?
It's realy only one?

Comment 68

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

(In reply to Maksim Lebedev from comment #67)
> Elements can receive NS_POINTER_ENTER/LEAVE when user move mouse over
> element.
So?

> In this case this pointer exist, but not in active buttons state.
but enter/leave don't change active state, and also, I don't see us ever handling NS_POINTER_ENTER/LEAVE
here. NS_POINTER_ENTER/LEAVE are dispatched by nsEventStateManager which happens higher level than presshell.


> NS_POINTER_CANCEL doesn't change state of active pointers.
In theory it could, I think. Perhaps not in our implementation.

> It's realy only one?
Well, maybe few more.

Comment 69

[Maksim Lebedev]

Attachment: pointerCapture_updated_ver10.diff

+ pointerId -> WidgetPointerHelper
+ Update PresShell::UpdateActivePointerState

Comment 70

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

Comment on attachment 8393343 [details] [diff] [review]
pointerCapture_updated_ver10.diff

>+nsIPresShell::SetPointerCapturingContent(uint32_t aPointerId, nsIContent* aContent)
>+{
>+  nsIContent* content = GetPointerCapturingContent(aPointerId);
>+
>+  bool activeState = false;
>+  if (!content && gActivePointersIds->Get(aPointerId, &activeState) && activeState) {
>+    SetCapturingContent(aContent, CAPTURE_PREVENTDRAG);
>+  }
Why this. SetCapturingContent should be for mouse case only, so shouldn't you check whether certain
pointerid is for mouse?

>+
>+  if (content) {
>+    // Releasing capture for given pointer.
>+    gPointerCaptureList->Remove(aPointerId);
>+    DispatchGotOrLostPointerCaptureEvent(false, aPointerId, content);
>+  }
>+
>+  gPointerCaptureList->Put(aPointerId, aContent);
>+  DispatchGotOrLostPointerCaptureEvent(true, aPointerId, aContent);

Somewhat interesting case... what if lostpointercapture event calls setPointerCapture on some other element'.
That will dispatch gotpointercapture on that element'. Then we here enable pointercapture for aContent, 
and there won't be lostpointercapture for the lost capture for element'.
I think we should handle that somehow. Perhaps after dispatching lostpointercapture check that
gPointerCaptureList really doesn't containt aPointerId, and if it does, return and don't call
->Put() nor dispatch gotpointercapture.

We need tests for this.
>+    if (aEvent->eventStructType == NS_POINTER_EVENT
>+        && aEvent->message != NS_POINTER_DOWN) {
&& to the previous line

>+      if (WidgetPointerEvent* pointerEvent = aEvent->AsPointerEvent()) {
>+        uint32_t pointerId = pointerEvent->pointerId;
>+        nsIContent* pointerCapturingContent = GetPointerCapturingContent(pointerId);
>+
>+        if (pointerCapturingContent) {
>+          if (nsIFrame* capturingFrame = pointerCapturingContent->GetPrimaryFrame()) {
>+            frame = capturingFrame;
>+          }
>+
>+          if (pointerEvent->message == NS_POINTER_UP
>+              || pointerEvent->message == NS_POINTER_CANCEL) {
ditto

>+            // Implicitly releasing capture for given pointer.
>+            ReleasePointerCapturingContent(pointerId, pointerCapturingContent);
This should happen as late as possible, since frame is possibly dead object after this call,
given that ReleasePointerCapturingContent may dispatch a dom event synchronously, and event listeners may
delete nsIFrame objects by changing layout.
I guess we need a stack object here which dispatches lostpointercapture right after NS_POINTER/CANCEL have been handled.



tests tests tests

Comment 71

[Maksim Lebedev]

(In reply to Olli Pettay [:smaug] from comment #70)
> Why this. SetCapturingContent should be for mouse case only, so shouldn't
> you check whether certain pointerid is for mouse?
Unfortunately, there are no word in specification, which say that SetPointerCapturingContent should work only for mouse.

> This should happen as late as possible, since frame is possibly dead object
> after this call,
> given that ReleasePointerCapturingContent may dispatch a dom event
> synchronously, and event listeners may
> delete nsIFrame objects by changing layout.
> I guess we need a stack object here which dispatches lostpointercapture
> right after NS_POINTER/CANCEL have been handled.
What we should do there? Should we remember class LostPointerCaptureDispatcher?

Comment 72

[Maksim Lebedev]

Attachment: pointerCapture_updated_ver11.diff

+ add check in SetPointerCapturingContent
+ add my favorite class LostPointerCaptureDispatcher

Comment 73

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

(In reply to Maksim Lebedev from comment #71)
> (In reply to Olli Pettay [:smaug] from comment #70)
> > Why this. SetCapturingContent should be for mouse case only, so shouldn't
> > you check whether certain pointerid is for mouse?
> Unfortunately, there are no word in specification, which say that
> SetPointerCapturingContent should work only for mouse.
I'm not talking about SetPointerCapturingContent, but SetCapturingContent, which is an API for
mouse. This thing is not coming from the pointer events spec.


> > This should happen as late as possible, since frame is possibly dead object
> > after this call,
> > given that ReleasePointerCapturingContent may dispatch a dom event
> > synchronously, and event listeners may
> > delete nsIFrame objects by changing layout.
> > I guess we need a stack object here which dispatches lostpointercapture
> > right after NS_POINTER/CANCEL have been handled.
> What we should do there? Should we remember class
> LostPointerCaptureDispatcher?
Something similar, but here it is used in a different case.

Comment 74

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

Comment on attachment 8395566 [details] [diff] [review]
pointerCapture_updated_ver11.diff

>+nsIPresShell::SetPointerCapturingContent(uint32_t aPointerId, nsIContent* aContent)
>+{
>+  nsIContent* content = GetPointerCapturingContent(aPointerId);
>+
>+  bool activeState = false;
>+  if (!content && gActivePointersIds->Get(aPointerId, &activeState) && activeState) {
>+    SetCapturingContent(aContent, CAPTURE_PREVENTDRAG);
>+  }
So I don't still understand this. Why we need to call SetCapturingContent in all the cases?
capturing content is basically a mouse thing.  I admit the old setCapturingContent and setPointerCapturingContent
don't play along too well


>+  if (content) {
>+    // Releasing capture for given pointer.
>+    gPointerCaptureList->Remove(aPointerId);
>+    DispatchGotOrLostPointerCaptureEvent(false, aPointerId, content);
>+    if (GetPointerCapturingContent(aPointerId)) {
>+      return;
>+    }
Add some comment there why we have GetPointerCapturingContent check.

>+class LostPointerCaptureDispatcher
>+{
>+public:
>+  LostPointerCaptureDispatcher(int32_t aPointerId, nsIContent* aContent) :
>+                                mPointerId(aPointerId),
>+                                mContent(aContent)
>+  {
>+  }
>+  ~LostPointerCaptureDispatcher()
>+  {
>+    if (mContent) {
>+      nsIPresShell::DispatchGotOrLostPointerCaptureEvent(false, mPointerId, mContent);
>+    }
>+  }
>+
>+private:
>+  int32_t     mPointerId;
>+  nsIContent* mContent;
This looks unsafe. What guarantees that mContent stays alive?
So, nsCOMPtr<mContent>


>   nsIFrame* frame = aFrame;
> 
>-  bool dispatchUsingCoordinates = aEvent->IsUsingCoordinates();
>-  if (dispatchUsingCoordinates) {
>+  if (bool dispatchUsingCoordinates = aEvent->IsUsingCoordinates()) {
>+    nsAutoPtr<LostPointerCaptureDispatcher> lostPointerCaptureDispatcher;
Put LostPointerCaptureDispatcher to stack before the if, and add
SetTarget(nsIContent* aContent).
{
  mContent = aContent;
}
method to it.  No need to use nsAutoPtr



We need tests!

Comment 75

[Maksim Lebedev]

Attachment: pointerCapture_updated_ver12.diff

+ update SetPointerCapturingContent
+ update LostPointerCaptureDispatcher
+ add comments and changes according with comments

Comment 76

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

Comment on attachment 8396383 [details] [diff] [review]
pointerCapture_updated_ver12.diff

>+  typedef struct PointerInfo {
No need for typedef, and { goes to the next line

>+  if (content) {
>+    // Releasing capture for given pointer.
>+    gPointerCaptureList->Remove(aPointerId);
>+    DispatchGotOrLostPointerCaptureEvent(false, aPointerId, content);
>+    // Need checking state, because listener,
Need to check the state because a lostpointercapture listener
may have called setPointerCapture.

>+    // which received LOST_POINTER_CAPTURE event
>+    // can call SetPointerCapture with another element
>+    if (GetPointerCapturingContent(aPointerId)) {
>+      return;
>+    }
>+  }
>+
>+  gPointerCaptureList->Put(aPointerId, aContent);
>+  DispatchGotOrLostPointerCaptureEvent(true, aPointerId, aContent);
This stuff is still underspecified.
I filed https://www.w3.org/Bugs/Public/show_bug.cgi?id=25147
and https://www.w3.org/Bugs/Public/show_bug.cgi?id=24971
is still an open issue.


>+  void SetTarget(uint32_t aPointerId, nsIContent* aContent) {
{ goes to the next line

>+private:
>+  int32_t               mPointerId;
>+  nsCOMPtr<nsIContent>  mContent;
extra space before variable names.

>+nsIPresShell::DispatchGotOrLostPointerCaptureEvent(bool aIsGotCapture,
>+                                                    uint32_t aPointerId,
>+                                                    nsIContent* aCaptureTarget)
align params

>+{
>+  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"),
2 space indentation for ? and : cases


Do the tests still pass with this? I assume got/lost events need some more tests.

Comment 77

[Maksim Lebedev]

Attachment: pointerCapture_updated_ver13.diff

+ some changes according with comments

(In reply to Olli Pettay [:smaug] from comment #76)
> Do the tests still pass with this? I assume got/lost events need some more tests.
Some tests is in attached file "Pointer capture test, support multi target/pointers"

Comment 78

[Oleg Romashin (MS)]

Comment on attachment 8396962 [details] [diff] [review]
pointerCapture_updated_ver13.diff

>+    if(!activeState) {
        ^ space here

 
> //static
    ^ space here
> already_AddRefed<PointerEvent>
>-PointerEvent::Constructor(const GlobalObject& aGlobal,
>+PointerEvent::Constructor(EventTarget* aOwner,
> 
>+//static
    ^ space here 

>+already_AddRefed<PointerEvent>
>+PointerEvent::Constructor(const GlobalObject& aGlobal,
>+                          const nsAString& aType,
>+                          const PointerEventInit& aParam,
>+                          ErrorResult& aRv)
>+{
>+  nsCOMPtr<EventTarget> owner = do_QueryInterface(aGlobal.GetAsSupports());



>diff --git a/layout/base/nsIPresShell.h b/layout/base/nsIPresShell.h
>--- a/layout/base/nsIPresShell.h
>+++ b/layout/base/nsIPresShell.h
>@@ -33,14 +33,16 @@
> #include "nsCompatibility.h"
> #include "nsFrameManagerBase.h"
> #include "mozFlushType.h"
> #include "nsWeakReference.h"
> #include <stdio.h> // for FILE definition
> #include "nsChangeHint.h"
> #include "nsRefPtrHashtable.h"
>+#include "nsClassHashtable.h"
>+#include "nsDataHashtable.h"

are you sure you need nsDataHashtable here? looks like there are no usage for it in this header



> 
>-  bool dispatchUsingCoordinates = aEvent->IsUsingCoordinates();
>-  if (dispatchUsingCoordinates) {
>+  if (bool dispatchUsingCoordinates = aEvent->IsUsingCoordinates()) {

remove dispatchUsingCoordinates variable it is not used anywhere below, also it will cause build error due to unexpected warnings

> class WidgetPointerHelper
> {
> public:
>   bool convertToPointer;
>+  uint32_t pointerId;

This is the changes from bug 977559, keep them in that bug (https://bugzilla.mozilla.org/attachment.cgi?id=8382993&action=edit), these changes are not related to pointer capture functionality at all

Before posting new update please send updated patch with Tests patch to try server, and make sure that all tests are passing.
Because current patch just not compiling for now due to warning errors
see https://tbpl.mozilla.org/?tree=Try&rev=d5f6cc96fd63

Comment 79

[Oleg Romashin (MS)]

https://tbpl.mozilla.org/php/getParsedLog.php?id=36738788&tree=Try#error0 - this test at least must be fixed.

Comment 80

[Maksim Lebedev]

Attachment: pointerCapture_updated_ver14.diff

+ changes according comments

(In reply to Oleg Romashin (MS) from comment #78)
> > class WidgetPointerHelper
> > {
> > public:
> >   bool convertToPointer;
> >+  uint32_t pointerId;
> This is the changes from bug 977559, keep them in that bug
> (https://bugzilla.mozilla.org/attachment.cgi?id=8382993&action=edit), these
> changes are not related to pointer capture functionality at all
Unfortunately, this change have to be in this bug, because we use pointerId value.
You can see: PresShell::UpdateActivePointerState

Comment 81

[Oleg Romashin (:romaxa)]

Comment on attachment 8397838 [details] [diff] [review]
pointerCapture_updated_ver14.diff

Ok, I see, please update tests, and check patches on try server

Comment 82

[Maksim Lebedev]

Attachment: pointerCapture_updated_ver15.diff

+ update LostPointerCaptureDispatcher -> ReleasePointerCaptureCaller

All mochitests are passed successfully.

Comment 83

[Maksim Lebedev]

https://tbpl.mozilla.org/?tree=Try&rev=176602f7ed76

Comment 84

[Oleg Romashin (MS)]

(In reply to Maksim Lebedev from comment #83)
> https://tbpl.mozilla.org/?tree=Try&rev=176602f7ed76

For final try builds it is better to use try: -b do -p all -u all -t all this way you will get all results for all platforms.
I created full run with only attached patches, it looks ok
https://tbpl.mozilla.org/?tree=Try&rev=9199c336777a

Comment 85

[Oleg Romashin (MS)]

Comment on attachment 8399877 [details] [diff] [review]
pointerCapture_updated_ver15.diff

smaug previous patch has r+ from you, do we need your additional r+ to updated patch? or shall we push it as is already?

Comment 86

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

Should be fine for now. We may need to change it a bit once the spec is fixed.

Comment 87

[Oleg Romashin (MS)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/32a25e5194b8
https://hg.mozilla.org/integration/mozilla-inbound/rev/331ea1caab22

Comment 88

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/32a25e5194b8
https://hg.mozilla.org/mozilla-central/rev/331ea1caab22

Comment 89

[Chris Mills [:cmills]]

Setting to dev-doc-complete; we document the main features used for pointercapture: see https://developer.mozilla.org/en-US/docs/Web/API/Pointer_events#Event_types_and_Global_Event_Handlers

Although:

* We don't document hasPointerCapture; I'll look into this.
* We say that it was first implemented behind a pref in 41, but this says 31. I am also not sure we have the information right about when it was enabled. The docs say 59?