967796 - Implement Pointer Enter/Leave events support.

Status: RESOLVED FIXED

(Core :: DOM: Events, defect)

Description

[Oleg Romashin (MS)]

We need to implement dispatching widget events and create pointer enter/leave events, also support Multi-Pointer handling
https://dvcs.w3.org/hg/pointerevents/raw-file/tip/pointerEvents.html#the-pointerenter-event

Comment 1

[Oleg Romashin (MS)]

Simple test which allow to see over/out events when pointer moved on top of red and blue box

Comment 2

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

Hopefully mouseenter/leave dispatcher can be used here too. And we probably want similar optimization
to dispatch the events only if there are listeners.

Comment 3

[Oleg Romashin (MS)]

Attachment: Pointer Enter Leave with Multi Pointer support, WIP

Still dirty, but kind of work with multi pointers/targets.
Would like to hear if this way would be appropriate for Cycle collector

Comment 4

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

Comment on attachment 8370593 [details] [diff] [review]
Pointer Enter Leave with Multi Pointer support, WIP

So you don't want any wrappercache or _SCRIPT_HOLDER_ stuff, since your object doesn't actually deal with any script objects, but only cycle collectable C++ objects.

Hmm, so we'll dispatch pointer event as mouse event? That is surprising.
Do we really want that? The spec doesn't say that should happen.

Comment 5

[Oleg Romashin (MS)]

Attachment: Pointer Enter Leave with Multi Pointer support.

Cleaned up things

Comment 6

[Oleg Romashin (MS)]

Attachment: Pointer Enter Leave Listeners detection

Comment 7

[Oleg Romashin (MS)]

Comment on attachment 8371066 [details] [diff] [review]
Pointer Enter Leave with Multi Pointer support.

this does not pass tests yet, need to figure out how to fix that

Comment 8

[Oleg Romashin (MS)]

Attachment: Refactor Out/Over element helpers not functional change

Wrap Out/Over element helpers into one class which can be shared between Mouse and Pointer events.
Current patch does not introduce actual implementation for pointer events.

Comment 9

[Oleg Romashin (MS)]

Attachment: Add support for Pointer events Enter/Leave over/out dispatching

Comment 10

[Oleg Romashin (MS)]

https://tbpl.mozilla.org/?tree=Try&rev=102b968132fc - with enabled pointers for desktop FF and enter/leave patches applied.
Now it looks pretty much green

Comment 11

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

Comment on attachment 8372050 [details] [diff] [review]
Refactor Out/Over element helpers not functional change

>+OverOutElementsWrapper*
>+nsEventStateManager::GetWrapperByEventID(WidgetMouseEvent* aEvent)
>+{
>+  if (!mMouseEnterLeaveHelper) {
>+    mMouseEnterLeaveHelper = new OverOutElementsWrapper();
>+  }
>+  return mMouseEnterLeaveHelper.get();
>+}

Ok, this will do something more complicated in the next patch, I assume.

>+PLDHashOperator
>+nsEventStateManager::ResetLastOverForContent(const uint32_t& aIdx,
>+                                             nsRefPtr<OverOutElementsWrapper>& aElemWrapper,
>+                                             void* aClosure)
>+{
>+  nsIContent* content = static_cast<nsIContent*>(aClosure);
>+  if (aElemWrapper && aElemWrapper->mLastOverElement &&
>+      nsContentUtils::ContentIsDescendantOf(aElemWrapper->mLastOverElement, content)) {
>+    aElemWrapper->mLastOverElement = nullptr;
>+  }
>+
>+  return PL_DHASH_NEXT;
>+}
Hmm, this is odd. This looks like a hashtable enum function, but you use it as a 
normal method.

>   /**
>    * Tell this ESM and ESMs in affected child documents that the mouse
>    * has exited this document's currently hovered content.
>    * @param aMouseEvent the event that triggered the mouseout
>    * @param aMovingInto the content node we've moved into.  This is used to set
>    *        the relatedTarget for mouseout events.  Also, if it's non-null
>    *        NotifyMouseOut will NOT change the current hover content to null;
>    *        in that case the caller is responsible for updating hover state.
>    */
>   void NotifyMouseOut(mozilla::WidgetMouseEvent* aMouseEvent,
>-                      nsIContent* aMovingInto);
>+                      nsIContent* aMovingInto, OverOutElementsWrapper* aWrapper);
Please document the 3rd param



This is probably ok, assuming the next patch does what I expect.

Comment 12

[Oleg Romashin (MS)]

Comment on attachment 8372051 [details] [diff] [review]
Add support for Pointer events Enter/Leave over/out dispatching

>+    if (aMessage == NS_POINTER_LEAVE ||
>+        aMessage == NS_POINTER_ENTER) {
>+      pointerEvent->mFlags.mBubbles = false;
>+    }

Will move this to event ctor, similar to http://mxr.mozilla.org/mozilla-central/source/widget/MouseEvents.h#167

Comment 13

[Oleg Romashin (MS)]

> Ok, this will do something more complicated in the next patch, I assume.

yep

> >+}
> Hmm, this is odd. This looks like a hashtable enum function, but you use it
> as a  normal method.

I just wrapped this functionality into one method
in next patch callers will do
// See bug 292146 for why we want to null this out
ResetLastOverForContent(0, mMouseEnterLeaveHelper, aContent);
mPointersEnterLeaveHelper.Enumerate(&nsEventStateManager::ResetLastOverForContent, aContent);

Comment 14

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

Comment on attachment 8372051 [details] [diff] [review]
Add support for Pointer events Enter/Leave over/out dispatching

>+  case NS_POINTER_CANCEL:
>+  case NS_POINTER_UP: {
>+    WidgetPointerEvent* pointerEvent = aEvent->AsPointerEvent();
>+    // Remove helper if touch event get's released
>+    if (pointerEvent->inputSource == nsIDOMMouseEvent::MOZ_SOURCE_TOUCH) {
>+      mPointersEnterLeaveHelper.Remove(pointerEvent->pointerId);
>+    }
I don't understand this. Why do we special case touch?
Or will we remove the relevant thing for mouse when legacy MOUSE_UP is handled?

>@@ -4072,44 +4083,64 @@ nsEventStateManager::DispatchMouseEvent(
So this method isn't anymore for mouse events, but for pointer events too.
Please rename. DispatchMouseOrPointerEvent is a bit ugly, but at least it is clear.

>-  PROFILER_LABEL("Input", "DispatchMouseEvent");
>   nsEventStatus status = nsEventStatus_eIgnore;
>-  WidgetMouseEvent event(aMouseEvent->mFlags.mIsTrusted, aMessage,
>-                         aMouseEvent->widget, WidgetMouseEvent::eReal);
>-  event.refPoint = aMouseEvent->refPoint;
>-  event.modifiers = aMouseEvent->modifiers;
>-  event.buttons = aMouseEvent->buttons;
>-  event.pluginEvent = aMouseEvent->pluginEvent;
>-  event.relatedTarget = aRelatedContent;
>-  event.inputSource = aMouseEvent->inputSource;
>+  WidgetMouseEvent* event = nullptr;
>+  WidgetPointerEvent* sourcePointer = aMouseEvent->AsPointerEvent();
>+  if (sourcePointer) {
>+    PROFILER_LABEL("Input", "DispatchPointerEvent");
>+    WidgetPointerEvent* pointerEvent =
>+      new WidgetPointerEvent(aMouseEvent->mFlags.mIsTrusted, aMessage,
>+                             aMouseEvent->widget);
>+    pointerEvent->isPrimary = sourcePointer->isPrimary;
>+    pointerEvent->pointerId = sourcePointer->pointerId;
>+    pointerEvent->width = sourcePointer->width;
>+    pointerEvent->height = sourcePointer->height;
>+    pointerEvent->inputSource = sourcePointer->inputSource;
>+    if (aMessage == NS_POINTER_LEAVE ||
>+        aMessage == NS_POINTER_ENTER) {
>+      pointerEvent->mFlags.mBubbles = false;
Hmm, spec has a bug, I think. pointerenter/leavy shouldn't be cancelable.
But anyhow, could you move this mBubbles = false to the same place where we do that for MOUSEENTER/LEAVE.
For now pointerleave/enter could be cancelable.

> class MouseEnterLeaveDispatcher
Could you rename to EnterLeaveDispatcher or MouseOrPointerEnterLeaveDispatcher

>-  aWrapper->mLastOverFrame = DispatchMouseEvent(aMouseEvent, NS_MOUSE_ENTER_SYNTH,
>+  aWrapper->mLastOverFrame = DispatchMouseEvent(aMouseEvent, isPointer ? NS_POINTER_OVER : NS_MOUSE_ENTER_SYNTH,
overlong line


> nsEventStateManager::GetWrapperByEventID(WidgetMouseEvent* aEvent)
> {
>-  if (!mMouseEnterLeaveHelper) {
>-    mMouseEnterLeaveHelper = new OverOutElementsWrapper();
>-  }
>-  return mMouseEnterLeaveHelper.get();
>+  WidgetPointerEvent* pointer = aEvent->AsPointerEvent();
>+  if (!pointer) {
Could you MOZ_ASSERT here that the AsMouseEvent() is non-null.

>+    if (!mMouseEnterLeaveHelper) {
>+      mMouseEnterLeaveHelper = new OverOutElementsWrapper();
>+    }
>+    return mMouseEnterLeaveHelper.get();
Why you need .get() here?

>+  }
>+  nsRefPtr<OverOutElementsWrapper> helper;
>+  if (!mPointersEnterLeaveHelper.Get(pointer->pointerId, getter_AddRefs(helper))) {
>+    helper = new OverOutElementsWrapper();
>+    mPointersEnterLeaveHelper.Put(pointer->pointerId, helper);
>+  }
>+
>+  return helper.get();
Why you need .get() here?


This all looks surprisingly simple. Needs some tests.
I could take another look.

Comment 15

[Oleg Romashin (:romaxa)]

Attachment: Refactor Out/Over element helpers not functional change

Fixed review comments

Comment 16

[Oleg Romashin (MS)]

Attachment: Add support for Pointer events Enter/Leave over/out dispatching

Fixed review comments, fixed problem with anonymous content dispatching.

Comment 17

[Oleg Romashin (MS)]

Attachment: Pointer events Enter/Leave over/out tests

Added tests for pointer events, based on test_bug432698.html

Comment 18

[Oleg Romashin (MS)]

> >+  case NS_POINTER_CANCEL:
> >+  case NS_POINTER_UP: {
> >+    WidgetPointerEvent* pointerEvent = aEvent->AsPointerEvent();
> >+    // Remove helper if touch event get's released
> >+    if (pointerEvent->inputSource == nsIDOMMouseEvent::MOZ_SOURCE_TOUCH) {
> >+      mPointersEnterLeaveHelper.Remove(pointerEvent->pointerId);
> >+    }
> I don't understand this. Why do we special case touch?
> Or will we remove the relevant thing for mouse when legacy MOUSE_UP is
> handled?

For Mouse/Pen we have mouse events and those converted into Pointers with nsIDOMMouseEvent::MOZ_SOURCE_MOUSE/PEN, and we should not remove it, because we are tracking them all the time on MouseMove..

With Touch we may have bunch of PointerID's which are valid between POINTER_DOWN...POINTER_UP/CANCEL, so we want to destroy relevant helper when Pointer disappeared.

Comment 19

[Oleg Romashin (:romaxa)]

Try build with all tests
https://tbpl.mozilla.org/?tree=Try&rev=d2719e44338a

Comment 20

[Oleg Romashin (MS)]

Comment on attachment 8372640 [details] [diff] [review]
Pointer events Enter/Leave over/out tests


>+function runTests() {

Will add here: SpecialPowers.setBoolPref("dom.w3c_pointer_events.enabled", true);

>+  outer = document.getElementById("outertest");
....
>+  is(pointerleavecount, 7, "Unexpected pointerleave event count!");
>+

and SpecialPowers.clearUserPref("dom.w3c_pointer_events.enabled"); here

>+  SimpleTest.finish();
>+}

Comment 21

[Oleg Romashin (MS)]

Try build with SpecialPowers in tests
https://tbpl.mozilla.org/?tree=Try&rev=7785d927fb43

Comment 22

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

Comment on attachment 8372639 [details] [diff] [review]
Add support for Pointer events Enter/Leave over/out dispatching

>+  case NS_POINTER_CANCEL:
>+  case NS_POINTER_UP: {
>+    WidgetPointerEvent* pointerEvent = aEvent->AsPointerEvent();
>+    // Remove helper if touch event get's released
>+    if (pointerEvent->inputSource == nsIDOMMouseEvent::MOZ_SOURCE_TOUCH) {
>+      mPointersEnterLeaveHelper.Remove(pointerEvent->pointerId);
>+    }
This stuff needs still a comment why only touch is handled here.

>+  WidgetMouseEvent* event = nullptr;
nsAutoPtr<WidgetMouseEvent> event;


Though I wonder if some combination of Maybe<> and WidgetMouseEvent* would work better.
Less allocation at least, allocation is slow.
Maybe<WidgetPointerEvent> newPointerEvent;
Maybe<WidgetPointerEvent> newMouseEvent;
WidgetMouseEvent* event = nullptr;
and then construct(params...) the right Maybe and assign to event, but don't delete event ever.


>+  delete event;
So, remove this

>-  SetContentState(aContent, NS_EVENT_STATE_HOVER);
>+  if (!isPointer)
>+    SetContentState(aContent, NS_EVENT_STATE_HOVER);
if (expr) {
  stmt;
}

>+  case NS_POINTER_LEAVE:
>   case NS_MOUSE_EXIT:
>     {
>       // This is actually the window mouse exit event. We're not moving
>       // into any new element.
Want to fix the comment to not talk about mouse only.

Comment 23

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

Comment on attachment 8372640 [details] [diff] [review]
Pointer events Enter/Leave over/out tests

Why do you have nsDOMWindowUtils::SendPointerEvent and nsDOMWindowUtils::SendPointerEventCommon ?
Only SendPointerEvent seems to call SendPointerEventCommon, so you could merge them.


+                                aOptionalArgCount >= 4 ? aIsSynthesized : true);
I don't understand that.


>+NS_IMETHODIMP
>+nsDOMWindowUtils::SendPointerEvent(const nsAString& aType,
>+                                   float aX,
>+                                   float aY,
>+                                   int32_t aButton,
>+                                   int32_t aClickCount,
>+                                   int32_t aModifiers,
>+                                   bool aIgnoreRootScrollFrame,
>+                                   float aPressure,
>+                                   unsigned short aInputSourceArg,
>+                                   int32_t aPointerId,
>+                                   int32_t aWidth,
>+                                   int32_t aHeight,
>+                                   int32_t tiltX,
>+                                   int32_t tiltY,
>+                                   bool aIsPrimary,
>+                                   bool aIsSynthesized,
>+                                   uint8_t aOptionalArgCount,
>+                                   bool *aPreventDefault)
* goes with the type.

>+                                         bool aToWindow,
>+                                         bool *aPreventDefault,
ditto

>+                                         bool aIsSynthesized)
>+{
>+  if (!nsContentUtils::IsCallerChrome()) {
>+    return NS_ERROR_DOM_SECURITY_ERR;
>+  }
>+
>+  // get the widget to send the event to
>+  nsPoint offset;
>+  nsCOMPtr<nsIWidget> widget = GetWidget(&offset);
>+  if (!widget)
>+    return NS_ERROR_FAILURE;

{} with if

>+
>+  int32_t msg;
>+  if (aType.EqualsLiteral("pointerdown"))
>+    msg = NS_POINTER_DOWN;
>+  else if (aType.EqualsLiteral("pointerup"))
>+    msg = NS_POINTER_UP;
>+  else if (aType.EqualsLiteral("pointermove"))
>+    msg = NS_POINTER_MOVE;
>+  else if (aType.EqualsLiteral("pointerover"))
>+    msg = NS_POINTER_OVER;
>+  else if (aType.EqualsLiteral("pointerout"))
>+    msg = NS_POINTER_OUT;
>+  else
>+    return NS_ERROR_FAILURE;

And here and with else
Same also elsewhere.

>+                                    bool *aPreventDefault,
bool*

Given that test_bug967796.html copies lots of code from test_bug432698.html, could you please do a hg copy.
That would make reviewing a lot easier.


>+++ b/dom/interfaces/base/nsIDOMWindowUtils.idl
>@@ -262,16 +262,82 @@ interface nsIDOMWindowUtils : nsISupport
Update uuid of nsIDOMWindowUtils

Comment 24

[Oleg Romashin (MS)]

Attachment: Pointer events Enter/Leave over/out tests

Comment 25

[Oleg Romashin (MS)]

Attachment: Pointer events Enter/Leave over/out tests

aOptionalArgCount >= 4 ? aIsSynthesized : true - it should be 10, because we have 10 optional arguments now

Comment 26

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

Comment on attachment 8373757 [details] [diff] [review]
Pointer events Enter/Leave over/out tests


>+  int32_t msg;
>+  if (aType.EqualsLiteral("pointerdown")) {
>+    msg = NS_POINTER_DOWN;
>+  }
>+  else if (aType.EqualsLiteral("pointerup")) {

} else if (expr) {



>+  if (aToWindow) {
This patch hasn't been compiled.

>+++ b/dom/interfaces/base/nsIDOMWindowUtils.idl
update uuid

Comment 27

[Oleg Romashin (MS)]

Attachment: Pointer events Enter/Leave over/out tests

Fixed compile, updated uuid

Comment 28

[Oleg Romashin (MS)]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/4caebc3011d2
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/b8dfec3cca1c
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/37f194248784
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/594dda9c7a3a

Comment 29

[Oleg Romashin (:romaxa)]

Tests were backout due to issue with Special Powers  handling on Win platform.
https://hg.mozilla.org/integration/mozilla-inbound/rev/feadeb8a7cbb.
https://tbpl.mozilla.org/php/getParsedLog.php?id=34471422&tree=Mozilla-Inbound#error0

Comment 30

[Oleg Romashin (MS)]

Tests relanded back
https://hg.mozilla.org/integration/mozilla-inbound/rev/af35bd7bd9a8

Comment 31

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/b8dfec3cca1c
https://hg.mozilla.org/mozilla-central/rev/37f194248784
https://hg.mozilla.org/mozilla-central/rev/af35bd7bd9a8

Comment 32

[Jean-Yves Perrier [:teoli]]

I'm trying to document the changes here, but I don't really understand what this bug is changing: it looks to me that it adds the pointerenter and pointerleave events.

Is this correct and is this all what this bug is adding?

Comment 33

[Sebastian Zartner [:sebo]]

ni'ing Oleg regarding comment 32.

Sebastian

Comment 34

[Sebastian Zartner [:sebo]]

(In reply to Sebastian Zartner [:sebo] from comment #33)
> ni'ing Oleg regarding comment 32.

Ping, Oleg, can you help out here?

Sebastian

Comment 35

[Sebastian Zartner [:sebo]]

As Oleg doesn't respond, I'm forwarding the question to Olli.

Sebastian

Comment 36

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

Yes, this was adding support for pointerenter/leave. Note, pointerevents aren't enabled by default yet.

Comment 37

[Chris Mills [:cmills]]

Setting to dev-doc-complete, as we document these events. See https://developer.mozilla.org/en-US/docs/Web/API/Pointer_events#Event_types_and_Global_Event_Handlers