Closed Bug 976963 Opened 6 years ago Closed 5 years ago

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.

Categories

(Firefox for Metro Graveyard :: General, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 33

People

(Reporter: alessarik, Assigned: alessarik)

References

Details

(Whiteboard: p=0)

Attachments

(3 files, 13 obsolete files)

6.32 KB, patch
smaug
: review+
Details | Diff | Splinter Review
12.60 KB, patch
smaug
: review+
alessarik
: feedback?
oleg.romashin
Details | Diff | Splinter Review
21.09 KB, patch
smaug
: review+
alessarik
: feedback?
oleg.romashin
Details | Diff | Splinter Review
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.
Blocks: metrobacklog
Whiteboard: p=0
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
Attachment #8381983 - Flags: feedback-
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;
>+    }
Attached patch bug976963.diff (obsolete) — Splinter Review
Changes for multiple pointers
Attachment #8381983 - Attachment is obsolete: true
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.
Attachment #8383657 - Flags: feedback?(bugs)
Attached patch For_test_14.1_bug976963_fix.diff (obsolete) — Splinter Review
Some changes.
Attachment #8383657 - Attachment is obsolete: true
Attachment #8383657 - Flags: feedback?(bugs)
Internal mochitest for testing bug 976963
Attachment #8384508 - Flags: review?(bugs)
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 ?
Attachment #8384508 - Flags: review?(bugs) → review-
(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
Attachment #8433404 - Flags: review?(mbrubeck)
Attachment #8433404 - Flags: review?(jmathies)
Attachment #8433404 - Flags: review?(bugs)
Attachment #8433404 - Flags: feedback?(oleg.romashin)
Attachment #8433404 - Flags: feedback?(nicklebedev37)
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.
Attachment #8433404 - Flags: review?(mbrubeck)
Attachment #8433404 - Flags: review?(jmathies)
Attachment #8433404 - Flags: review?(bugs)
Attachment #8433404 - Flags: review-
Attachment #8433404 - Flags: feedback?(oleg.romashin)
Attachment #8433404 - Flags: feedback?(nicklebedev37)
+ 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()
Attachment #8384508 - Attachment is obsolete: true
Attachment #8433404 - Attachment is obsolete: true
Attachment #8434834 - Flags: review?(bugs)
Attachment #8434834 - Flags: feedback?(oleg.romashin)
Attachment #8434834 - Flags: feedback?(nicklebedev37)
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.
Attachment #8434834 - Flags: review?(bugs) → review-
(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
Attachment #8434834 - Attachment is obsolete: true
Attachment #8434834 - Flags: feedback?(oleg.romashin)
Attachment #8434834 - Flags: feedback?(nicklebedev37)
Attachment #8440739 - Flags: review?(mbrubeck)
Attachment #8440739 - Flags: review?(jmathies)
Attachment #8440739 - Flags: review?(bugs)
Attachment #8440739 - Flags: feedback?(oleg.romashin)
Attachment #8440739 - Flags: feedback?(nicklebedev37)
Attachment #8440739 - Flags: review?(mbrubeck)
+ Changes: Changes which can detect correct behavior (like IE behavior)
(according with specification of PointerEvents)
Attachment #8385247 - Attachment is obsolete: true
Attachment #8440778 - Flags: review?(mbrubeck)
Attachment #8440778 - Flags: review?(jmathies)
Attachment #8440778 - Flags: review?(bugs)
Attachment #8440778 - Flags: feedback?(oleg.romashin)
Attachment #8440778 - Flags: feedback?(nicklebedev37)
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.
Attachment #8440739 - Flags: review?(bugs) → review-
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()
Attachment #8440778 - Flags: review?(mbrubeck)
Attachment #8440778 - Flags: review?(jmathies)
Attachment #8440778 - Flags: review?(bugs)
Attachment #8440778 - Flags: review+
Attachment #8440778 - Flags: feedback?(oleg.romashin)
Attachment #8440778 - Flags: feedback?(nicklebedev37)
Attachment #8440739 - Flags: review?(jmathies)
Attachment #8440739 - Flags: feedback?(oleg.romashin)
Attachment #8440739 - Flags: feedback?(nicklebedev37)
+ Changes: Added WidgetPointerHelper::retargetedByPointerCapture

Looks like test succesfully passed, but I wonder in another correct behaviors
Attachment #8440739 - Attachment is obsolete: true
Attachment #8442148 - Flags: review?(bugs)
Attachment #8442148 - Flags: feedback?(oleg.romashin)
Attachment #8442148 - Flags: feedback?(nicklebedev37)
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.
Attachment #8442148 - Flags: review?(bugs) → review+
+ 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.
Attachment #8442148 - Attachment is obsolete: true
Attachment #8442148 - Flags: feedback?(oleg.romashin)
Attachment #8442148 - Flags: feedback?(nicklebedev37)
Attachment #8443550 - Flags: review?(bugs)
Attachment #8443550 - Flags: feedback?(oleg.romashin)
Attachment #8443550 - Flags: feedback?(nicklebedev37)
Attachment #8443550 - Flags: review?(bugs)
Attachment #8443550 - Flags: review+
Attachment #8443550 - Flags: feedback?(oleg.romashin)
Attachment #8443550 - Flags: feedback?(nicklebedev37)
+ Changes: Added checks to determine old and new behavior
Attachment #8440778 - Attachment is obsolete: true
Attachment #8445186 - Flags: review?(bugs)
Attachment #8445186 - Flags: feedback?(oleg.romashin)
Attachment #8445186 - Flags: feedback?(nicklebedev37)
I am confusing a bit by results of testing.
>uncaught exception - TypeError: listener.setPointerCapture is not a function
+ Changes: Resolved issues with unknown function
Attachment #8445186 - Attachment is obsolete: true
Attachment #8445186 - Flags: review?(bugs)
Attachment #8445186 - Flags: feedback?(oleg.romashin)
Attachment #8445186 - Flags: feedback?(nicklebedev37)
Attachment #8447164 - Flags: review?(bugs)
Attachment #8447164 - Flags: feedback?(oleg.romashin)
Attachment #8447164 - Flags: feedback?(nicklebedev37)
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
Attachment #8447164 - Flags: review?(bugs)
Attachment #8447164 - Flags: feedback?(oleg.romashin)
Attachment #8447164 - Flags: feedback?(nicklebedev37)
+ Changes according with comments
Attachment #8447164 - Attachment is obsolete: true
Attachment #8449339 - Flags: review?(bugs)
Attachment #8449339 - Flags: feedback?(oleg.romashin)
Attachment #8449339 - Flags: feedback?(nicklebedev37)
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()
Attachment #8449339 - Flags: review?(bugs) → review+
(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.
Keywords: checkin-needed
Additional patch to resolve issues with fails during running tests on B2G systems
Attachment #8451551 - Flags: review?(mbrubeck)
Attachment #8451551 - Flags: review?(jmathies)
Attachment #8451551 - Flags: review?(bugs)
Attachment #8451551 - Flags: feedback?(oleg.romashin)
Attachment #8451551 - Flags: feedback?(nicklebedev37)
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
Attachment #8451551 - Flags: review?(jmathies) → review+
Attachment #8451551 - Flags: review?(mbrubeck) → feedback+
+ Changes: Allocated function GetViewToDispatchEvent
Attachment #8451551 - Attachment is obsolete: true
Attachment #8451551 - Flags: review?(bugs)
Attachment #8451551 - Flags: feedback?(oleg.romashin)
Attachment #8451551 - Flags: feedback?(nicklebedev37)
Attachment #8452181 - Flags: review?(bugs)
Attachment #8452181 - Flags: feedback?(oleg.romashin)
Attachment #8452181 - Flags: feedback?(nicklebedev37)
+ Changes: resolved compilation issues on *nix platforms
Attachment #8452181 - Attachment is obsolete: true
Attachment #8452181 - Flags: review?(bugs)
Attachment #8452181 - Flags: feedback?(oleg.romashin)
Attachment #8452181 - Flags: feedback?(nicklebedev37)
Attachment #8452937 - Flags: review?(jmathies)
Attachment #8452937 - Flags: review?(bugs)
Attachment #8452937 - Flags: feedback?(oleg.romashin)
Attachment #8452937 - Flags: feedback?(nicklebedev37)
Attachment #8452937 - Flags: review?(jmathies)
Attachment #8452937 - Flags: review?(bugs)
Attachment #8452937 - Flags: review+
If no objections from anybody, I will land this patches.
Keywords: checkin-needed
Attachment #8449339 - Flags: feedback?(nicklebedev37)
Attachment #8452937 - Flags: feedback?(nicklebedev37)
You need to log in before you can comment on or make changes to this bug.