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

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

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 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 on attachment 8370747 [details] [diff] [review] Implement pointer capture feature. I assume there will be a new patch.

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 ?

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

(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

Some changes were made thanks by comments in thread

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

This test works better on try with special powers

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 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.

Minor cleanup + set default mouse capture

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

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 on attachment 8376052 [details] [diff] [review] Pointer capture version which compiles and runs Is this the one which leaked on try?

Fixed Memory leak.

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

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 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 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?

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

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.)

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

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

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 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.

Added some changes in implementation from related bug 976963

(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 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?

(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 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.

(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.

Some changes according with comments of bug 968148 and bug 976963

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

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 ;)

(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.

Some changes

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 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

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

I have still no idea why LostPointerCaptureDispatcher is needed.

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.

(But the spec bug is not fixed yet.)

+ add code from related bug 970220

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.)

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

(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.

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.

(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.

What does IE do?

> 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).

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

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

(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)

(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 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

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

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 on attachment 8392257 [details] [diff] [review] pointerCapture_updated_ver8.diff ># HG changeset patch ># Parent 570c4ac17aa53b87583ad274d11d749d0aa35803 ># User Oleg Romashin <oleg.romashin@microsoft.com> > >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

- 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

(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?

(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.

+ pointerId -> WidgetPointerHelper + Update PresShell::UpdateActivePointerState

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

(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?

+ add check in SetPointerCapturingContent + add my favorite class LostPointerCaptureDispatcher

(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 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!

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

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.

+ 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 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

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

+ 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 on attachment 8397838 [details] [diff] [review] pointerCapture_updated_ver14.diff Ok, I see, please update tests, and check patches on try server

+ update LostPointerCaptureDispatcher -> ReleasePointerCaptureCaller All mochitests are passed successfully.

(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 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?

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

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

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

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?