User Agent: Mozilla/5.0 (Windows NT 6.2; WOW64; rv:26.0) Gecko/20100101 Firefox/26.0 (Beta/Release) Build ID: 20131205075310 Steps to reproduce: http://www.w3.org/wiki/PointerEvents/TestAssertions#Test_Assertions_for_releasePointerCapture have 14.1 test which have to be passed on MetroFireFox Actual results: Test fails Expected results: According with specification test have to be passed

This patch fix behavior. FireFox will pass specification-test.

I proposed to move this fix into 968148 and land it all together. That would be easier.

Comment on attachment 8381983 [details] [diff] [review] For_test_14.1_lost_pointer_capture.diff >--- a/layout/base/nsIPresShell.h >+++ b/layout/base/nsIPresShell.h >@@ -1185,16 +1185,19 @@ public: > static bool gPreventMouseEvents; > > // 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; > static nsClassHashtable<nsUint32HashKey, ActivePointerState>* gActivePointersIds; > >+ static uint32_t lostCapturePointerId; >+ static nsIContent* lostPointerCaptureTarget; >+ Pointer by definition > 1, and this code would not work for multi-touch

Comment on attachment 8381983 [details] [diff] [review] For_test_14.1_lost_pointer_capture.diff Style >+ if(nsIPresShell::lostPointerCaptureTarget) { ^ - spaces needed here >+ WidgetPointerEvent* pointerEvent = aMouseEvent->AsPointerEvent(); >+ if(pointerEvent) ^ spaces if (foo) { } >+ if(pointerEvent->pointerId == nsIPresShell::lostCapturePointerId) >+ dispatch = true; >+ }

Changes for multiple pointers

Comment on attachment 8383657 [details] [diff] [review] bug976963.diff >+ if (content) >+ nsIPresShell::DispatchPointerCaptureEvent(false, mPointerId, content); if (foo) { } >+ } else { >+ nsIContent* content = nullptr; >+ gLostPointerCaptureList->Get(aPointerId, &content); nsIContent* content; if (gLostPointerCaptureList->Get(aPointerId, &content)) { return content; } >+ return content; >+ } >+ return nullptr; >+} Could you add automatic mochitest for this issue? into layout/base/tests/bug968148_inner.html or new test file.

Some changes.

Internal mochitest for testing bug 976963

Comment on attachment 8384508 [details] [diff] [review] For_test_14.1_bug976963_fix.diff ># HG changeset patch ># Parent f92440c4386f99ca4ca8ca24cdeb56caf9dddb0d ># User Maksim Lebedev <alessarik@gmail.com> >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 >@@ -633,17 +633,16 @@ public: > 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); >- nsIPresShell::DispatchPointerCaptureEvent(false, 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()) { >diff --git a/dom/events/nsEventStateManager.cpp b/dom/events/nsEventStateManager.cpp >--- a/dom/events/nsEventStateManager.cpp >+++ b/dom/events/nsEventStateManager.cpp >@@ -4191,16 +4191,46 @@ public: > > nsEventStateManager* mESM; > nsCOMArray<nsIContent> mTargets; > nsCOMPtr<nsIContent> mRelatedTarget; > WidgetMouseEvent* mMouseEvent; > uint32_t mType; > }; > >+class LostPointerCaptureDispatcher >+{ >+public: >+ LostPointerCaptureDispatcher(WidgetMouseEvent* aMouseEvent) { { goes to its own line >+ dispatch = false; >+ mPointerId = 0; Initialize member variables in constructor using mDispatch(false), mPointerId(0) syntax >+ WidgetPointerEvent* pointerEvent = aMouseEvent->AsPointerEvent(); >+ if (pointerEvent) { >+ nsIContent* content = nsIPresShell::UpdateLostPointerCapturingContent(false, pointerEvent->pointerId); >+ if (content) { >+ dispatch = true; >+ mPointerId = pointerEvent->pointerId; >+ } >+ } >+ } >+ ~LostPointerCaptureDispatcher() { { goes to its own line >+ if (dispatch) { >+ nsIContent* content = nsIPresShell::UpdateLostPointerCapturingContent(false, mPointerId); >+ if (content) { >+ nsIPresShell::DispatchPointerCaptureEvent(false, mPointerId, content); >+ } >+ nsIPresShell::UpdateLostPointerCapturingContent(true, mPointerId); >+ } >+ } >+ >+private: >+ bool dispatch; bool mDispatch; >+ int32_t mPointerId; >+}; >+ > void > nsEventStateManager::NotifyMouseOut(WidgetMouseEvent* aMouseEvent, > nsIContent* aMovingInto) > { > OverOutElementsWrapper* wrapper = GetWrapperByEventID(aMouseEvent); > > if (!wrapper->mLastOverElement) > return; >@@ -4241,16 +4271,18 @@ nsEventStateManager::NotifyMouseOut(Widg > // two nearby elements both deep in the DOM tree that would be defeated by > // switching the hover state to null here. > bool isPointer = aMouseEvent->eventStructType == NS_POINTER_EVENT; > if (!aMovingInto && !isPointer) { > // Unset :hover > SetContentState(nullptr, NS_EVENT_STATE_HOVER); > } > >+ LostPointerCaptureDispatcher lostPointerCaptureDispatcher(aMouseEvent); This doesn't look right. Per spec lostpointercapture should be dispatched (asynchronously) when capturing is done. What has that to do with pointerover/out ?

(In reply to Olli Pettay [:smaug] from comment #9) > { goes to its own line Should I write "class LostPointerCaptureDispatcher {" ? > Initialize member variables in constructor using mDispatch(false), mPointerId(0) syntax Ok. I will do it. > >+ WidgetPointerEvent* pointerEvent = aMouseEvent->AsPointerEvent(); > >+ if (pointerEvent) { > >+ nsIContent* content = nsIPresShell::UpdateLostPointerCapturingContent(false, pointerEvent->pointerId); > >+ if (content) { > >+ dispatch = true; > >+ mPointerId = pointerEvent->pointerId; > >+ } > >+ } > >+ } > >+ ~LostPointerCaptureDispatcher() { > { goes to its own line Which "{" should I change? > bool mDispatch; Ok. I will do it. > This doesn't look right. > Per spec lostpointercapture should be dispatched (asynchronously) when > capturing is done. > What has that to do with pointerover/out ? At first: When a user up his mouse (touch, pen, etc) from device we can determine POINTER_UP event in PresShell::HandleEvent. But if anybody call object.ReleasePointerCapture() we don't receive POINTER_UP in this case. At second: PointerOver/Out events looks syntetic for me. And LostPointerCapture event should be receive the last in the queue for listener (element which received pointer capture). That's why I created special class for send LostPointerCapture event. And put it immediately before EnterLeaveDispatcher class.

+ Change pointer capture behavior for same behavior as IE

Comment on attachment 8433404 [details] [diff] [review] change_pointer_capture_behavior_ver1.diff The patch is hard to read since you comment out some code and apparently move it to a separate method in PresShell. It is not clear to me why we even need to move that code. s/cuptureContent/capturingContent/ If you conditionally need to create a stack object, like EnterLeaveDispatcher, use Maybe<>. Please don't make random changes to DispatchPointerFromMouseOrTouch. Those just increase the size of the patch.

+ Changes: Added some comments + Changes: Added using Maybe-template + Changes: Deleted unwanted code (In reply to Olli Pettay [:smaug] from comment #13) > The patch is hard to read since you comment out some code and > apparently move it to a separate method in PresShell. > It is not clear to me why we even need to move that code. FireFox have many big functions - this is a big problem to understand behavior. When we need to change behavior, big function will be more bigger (It will make understanding more harder). That's why I allocated some of code to independent function PresShell::DispatchEvent()

Comment on attachment 8434834 [details] [diff] [review] change_pointer_capture_behavior_ver2.diff >+ // NB: pointerOut/pointerLeave will be suppressing in auto mode I don't understand this comment > if (pointerCapturingContent) { >- if (nsIFrame* capturingFrame = pointerCapturingContent->GetPrimaryFrame()) { >- frame = capturingFrame; >- } So we'll end up sort-of handling the event in the presshell of the hit testing target, but then dispatch to the document of the capturing content. I don't quite like that setup. We should handle the event in the presshell/EventStateManager of the target of the event. >+PresShell::DispatchEvent(WidgetEvent* aEvent, >+ nsEventStatus* aStatus, >+ nsPresShellEventCB* aEventCB) { >+ nsresult rv = NS_OK; >+ nsCOMPtr<nsINode> eventTarget; >+ // If pointer capture is set, all events should fired on capturing element >+ if (WidgetMouseEvent* mouseEvent = aEvent->AsMouseEvent()) { >+ eventTarget = GetPointerCapturingContent(mouseEvent->pointerId); ...so in other words, this is really hackish.

(In reply to Olli Pettay [:smaug] from comment #16) > >+ // NB: pointerOut/pointerLeave will be suppressing in auto mode > I don't understand this comment In common case wrapper->mLastOverElement have element which will receive PointerOut/pointerLeave in future NotifyMouseOut. If we have capturing state, we put nullptr into wrapper->mLastOverElement if we have element without pointer capture. Code is below: >+ if (dispatch) { >+ wrapper->mLastOverElement = aContent; >+ } else { >+ wrapper->mLastOverElement = nullptr; >+ }

(In reply to Olli Pettay [:smaug] from comment #16) > So we'll end up sort-of handling the event in the presshell of the hit > testing target, but then > dispatch to the document of the capturing content. I don't quite like that > setup. > We should handle the event in the presshell/EventStateManager of the target > of the event. > ...so in other words, this is really hackish. Sequence of functions in PresShell::HandleEvent is like below: PresShell::HandlePositionedEvent() PresShell::HandleEventInternal() EventStateManager::PreHandleEvent() EventStateManager::NotifyMouseOut() EventStateManager::NotifyMouseOver() PresShell::DispatchEvent() EventDispatcher::Dispatch() EventStateManager::PostHandleEvent() EventStateManager::NotifyMouseOut() * 'target' - element in normal state (other words it have mouse hover state) * 'listener' - element which receive pointer capture ('target' not equals 'listener') If we change target frame as 'listener' in PresShell::HandleEvent, we get calling NotifyMouseOver with 'listener', because wrapper->mLastOverElement have 'target'. In this case 'listener' will receive pointerOver/pointerEnter events. But this is wrong behavior, because pointer (mouse) can be out of boundingRect 'listener' (according specification). If we want suppress wrong events like pointerOver/pointerOut for all elements except 'listener', we should change target frame only in PresShell::DispatchEvent. In this case we will suppress sending pointerOver/pointerEnter events for all element, which are not as 'listener'. And if mouse moves into boundingRect of 'listener', in this case, we can send right pointerOver/pointerEnter events for 'listener'.

+ Changes: small changes in PresShell::DispatchEvent - Changes: removed unwanted comment

+ Changes: Changes which can detect correct behavior (like IE behavior) (according with specification of PointerEvents)

Comment on attachment 8440739 [details] [diff] [review] change_pointer_capture_behavior_ver3.diff >+PresShell::DispatchEvent(WidgetEvent* aEvent, >+ nsEventStatus* aStatus, >+ nsPresShellEventCB* aEventCB) { >+ nsresult rv = NS_OK; >+ nsCOMPtr<nsINode> eventTarget; >+ // If pointer capture is set, all events should fired on capturing element >+ if (WidgetPointerEvent* pointerEvent = aEvent->AsPointerEvent()) { >+ eventTarget = GetPointerCapturingContent(pointerEvent->pointerId); GetPointerCapturingContent may return element which is in different document than mDocument. We really want to handle event in the right presshell/eventstatemanager, so you need to forward stuff to the right presshell which then dispatches the event correctly.

In other words, we really shouldn't use ESM[1] of some presshell, but yet dispatch event to some totally different document. If we need to suppress some events, do that in ESM. [1] EventStateManager

Comment on attachment 8440778 [details] [diff] [review] change_pointer_capture_behavior_test_ver2.diff >+ function run() { >+ var listener = document.getElementById("listener"); >+ var target = document.getElementById("target"); >+ target.style["touchAction"] = "none"; >+ >+ // target and listener - handle all events >+ for (var i = 0; i < All_Pointer_Events.length; i++) { >+ on_event(target, All_Pointer_Events[i], targetEventHandler); >+ on_event(listener, All_Pointer_Events[i], listenerEventHandler); >+ } >+ } Could you rename this function to something like setEventListeners() or just init()

+ Changes: Added WidgetPointerHelper::retargetedByPointerCapture Looks like test succesfully passed, but I wonder in another correct behaviors

Comment on attachment 8442148 [details] [diff] [review] change_pointer_capture_behavior_ver4.diff >+ // If pointer capture is set, we should suppress pointerOver/pointerEnter events There are no pointerOver/pointerEnter events. pointerover/pointerenter ;) >@@ -6827,12 +6827,15 @@ PresShell::HandleEvent(nsIFrame* aFrame, > if (WidgetPointerEvent* pointerEvent = aEvent->AsPointerEvent()) { > uint32_t pointerId = pointerEvent->pointerId; > nsIContent* pointerCapturingContent = GetPointerCapturingContent(pointerId); > > if (pointerCapturingContent) { > if (nsIFrame* capturingFrame = pointerCapturingContent->GetPrimaryFrame()) { Technically capturingFrame can be null here, although we have capturing content. Anyhow I think that is a case we can fix in a different bug. > } else { >- nsCOMPtr<nsINode> eventTarget = mCurrentEventContent.get(); >- nsPresShellEventCB* eventCBPtr = &eventCB; >- if (!eventTarget) { >- nsCOMPtr<nsIContent> targetContent; >- if (mCurrentEventFrame) { >- rv = mCurrentEventFrame-> >- GetContentForEvent(aEvent, getter_AddRefs(targetContent)); >- } >- if (NS_SUCCEEDED(rv) && targetContent) { >- eventTarget = do_QueryInterface(targetContent); >- } else if (mDocument) { >- eventTarget = do_QueryInterface(mDocument); >- // If we don't have any content, the callback wouldn't probably >- // do nothing. >- eventCBPtr = nullptr; >- } >- } >- if (eventTarget) { >- if (aEvent->eventStructType == NS_COMPOSITION_EVENT || >- aEvent->eventStructType == NS_TEXT_EVENT) { >- IMEStateManager::DispatchCompositionEvent(eventTarget, >- mPresContext, aEvent, aStatus, eventCBPtr); >- } else { >- EventDispatcher::Dispatch(eventTarget, mPresContext, >- aEvent, nullptr, aStatus, eventCBPtr); >- } >- } >+ rv = DispatchEvent(aEvent, aStatus, &eventCB); > } > } > > nsContentUtils::SetIsHandlingKeyBoardEvent(wasHandlingKeyBoardEvent); > > // 3. Give event to event manager for post event state changes and >@@ -7451,12 +7428,44 @@ nsIPresShell::DispatchGotOrLostPointerCa > if (event) { > bool dummy; > aCaptureTarget->DispatchEvent(event->InternalDOMEvent(), &dummy); > } > } > >+nsresult >+PresShell::DispatchEvent(WidgetEvent* aEvent, >+ nsEventStatus* aStatus, >+ nsPresShellEventCB* aEventCB) { >+ nsresult rv = NS_OK; >+ nsCOMPtr<nsINode> eventTarget = mCurrentEventContent.get(); >+ if (!eventTarget) { >+ nsCOMPtr<nsIContent> targetContent; >+ if (mCurrentEventFrame) { >+ rv = mCurrentEventFrame->GetContentForEvent(aEvent, getter_AddRefs(targetContent)); >+ } >+ if (NS_SUCCEEDED(rv) && targetContent) { >+ eventTarget = do_QueryInterface(targetContent); >+ } else if (mDocument) { >+ eventTarget = do_QueryInterface(mDocument); >+ // If we don't have any content, the callback wouldn't probably do nothing >+ aEventCB = nullptr; >+ } >+ } >+ if (eventTarget) { >+ if (aEvent->eventStructType == NS_COMPOSITION_EVENT || >+ aEvent->eventStructType == NS_TEXT_EVENT) { >+ IMEStateManager::DispatchCompositionEvent(eventTarget, mPresContext, >+ aEvent, aStatus, aEventCB); >+ } else { >+ EventDispatcher::Dispatch(eventTarget, mPresContext, >+ aEvent, nullptr, aStatus, aEventCB); >+ } >+ } >+ return rv; >+} So there isn't need for this change. Remove it from the patch please.

+ Changes: Added detection when pointer leaves rect of capturing content - Changes: Removed unwanted code (In reply to Olli Pettay [:smaug] from comment #25) > Comment on attachment 8442148 [details] [diff] [review] > change_pointer_capture_behavior_ver4.diff > So there isn't need for this change. Remove it from the patch please. I removed this code, but my opinion that if we will have small functions, we can simply change behavior and suporting will be simplier.

+ Changes: Added checks to determine old and new behavior

I am confusing a bit by results of testing. >uncaught exception - TypeError: listener.setPointerCapture is not a function

+ Changes: Resolved issues with unknown function

Try-build with only test: https://tbpl.mozilla.org/?tree=Try&rev=ad2973642f81 Try-build with test and patch: https://tbpl.mozilla.org/?tree=Try&rev=f45d54b169d3 It means, that patch change behavior FF from wrong form to right form (right form is as IE behavior. According with specification of pointer events).

Can we checkin this bug?

Comment on attachment 8447164 [details] [diff] [review] change_pointer_capture_behavior_test_ver4.diff >+ <meta charset="utf-8"> >+ <title>Test for Bug 976963</title> >+ <script type="application/javascript" src="/tests/SimpleTest/SimpleTest.js"></script> >+ <link rel="stylesheet" type="text/css" href="/tests/SimpleTest/test.css"/> >+ <script type="text/javascript"> >+ function setRemoteFrame() { >+ var iframe = document.getElementById("testFrame"); >+ iframe.src = "bug976963_inner.html"; >+ function messageListener(event) { >+ eval(event.data); >+ } >+ window.addEventListener("message", messageListener, false); You could have just called parent.foo in the iframe. I'd prefer that. Also for things like SimpleTest.finish(); etc. >+ function prepareTest() { >+ SimpleTest.waitForExplicitFinish(); >+ SpecialPowers.pushPrefEnv({ >+ "set": [ >+ ["dom.w3c_pointer_events.enabled", true] >+ ] >+ }, setRemoteFrame); >+ } Could you rename setRemoteFrame to startTest or such. "remote frame" tends to mean separate process in Gecko

+ Changes according with comments

Comment on attachment 8449339 [details] [diff] [review] change_pointer_capture_behavior_test_ver5.diff >diff --git a/layout/base/tests/bug976963_inner.html b/layout/base/tests/bug976963_inner.html >new file mode 100644 >--- /dev/null >+++ b/layout/base/tests/bug976963_inner.html >@@ -0,0 +1,243 @@ >+<!DOCTYPE HTML> >+<html> >+<!-- >+https://bugzilla.mozilla.org/show_bug.cgi?id=976963 >+--> >+<head> >+ <meta charset="utf-8"> >+ <title>Test for Bug 976963</title> >+ <meta name="author" content="Maksim Lebedev" /> >+ <script type="application/javascript" src="/tests/SimpleTest/SimpleTest.js"></script> >+ <script type="application/javascript" src="/tests/SimpleTest/EventUtils.js"></script> Why you need these in the _inner ? >+ function prepareTest() { >+ SimpleTest.waitForExplicitFinish(); >+ SpecialPowers.pushPrefEnv({ >+ "set": [ >+ ["dom.w3c_pointer_events.enabled", true] >+ ] >+ }, executeTest); >+ } Please remove this from the _inner ... >+<body onload="prepareTest()"> So this could be executeTest()

(In reply to Olli Pettay [:smaug] from comment #36) > Why you need these in the _inner ? > >+ function prepareTest() { > >+ SimpleTest.waitForExplicitFinish(); > >+ SpecialPowers.pushPrefEnv({ > >+ "set": [ > >+ ["dom.w3c_pointer_events.enabled", true] > >+ ] > >+ }, executeTest); > >+ } > Please remove this from the _inner > ... > >+<body onload="prepareTest()"> > So this could be executeTest() Unfortunately, I cannot make this changes, because after changes, test will be failed. Look's like, iFrame loads default preferences, and we should change preferences again.

https://hg.mozilla.org/integration/mozilla-inbound/rev/54252e150439 https://hg.mozilla.org/integration/mozilla-inbound/rev/4cd69faa8bc0

Backed out for B2G mochitest failures. https://hg.mozilla.org/integration/mozilla-inbound/rev/558b30253cbe https://tbpl.mozilla.org/php/getParsedLog.php?id=43048268&tree=Mozilla-Inbound

Additional patch to resolve issues with fails during running tests on B2G systems

Comment on attachment 8451551 [details] [diff] [review] resolve_test_issues_on_b2g_ver1.diff Review of attachment 8451551 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/base/nsDOMWindowUtils.cpp @@ +848,5 @@ > > nsEventStatus status; > + if (aToWindow) { > + nsCOMPtr<nsIPresShell> presShell = presContext->PresShell(); > + if (!presShell) - nit wrap these checks

+ Changes: Allocated function GetViewToDispatchEvent

+ Changes: resolved compilation issues on *nix platforms

If no objections from anybody, I will land this patches.

https://hg.mozilla.org/integration/mozilla-inbound/rev/f5dd9a7794e6 https://hg.mozilla.org/integration/mozilla-inbound/rev/69da62f5cb29 https://hg.mozilla.org/integration/mozilla-inbound/rev/ab6ad70d1a3e

https://hg.mozilla.org/mozilla-central/rev/f5dd9a7794e6 https://hg.mozilla.org/mozilla-central/rev/69da62f5cb29 https://hg.mozilla.org/mozilla-central/rev/ab6ad70d1a3e