Implement Pointer Enter/Leave events support.

RESOLVED FIXED in mozilla30

Status

()

defect
RESOLVED FIXED
6 years ago
9 months ago

People

(Reporter: oleg.romashin, Assigned: oleg.romashin)

Tracking

(Blocks 1 bug, {dev-doc-complete})

unspecified
mozilla30
x86_64
Windows 8.1
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

()

Attachments

(4 attachments, 7 obsolete attachments)

7.96 KB, patch
smaug
: review+
Details | Diff | Splinter Review
22.84 KB, patch
romaxa
: review+
Details | Diff | Splinter Review
21.20 KB, patch
smaug
: review+
Details | Diff | Splinter Review
26.37 KB, patch
smaug
: review+
Details | Diff | Splinter Review
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
Simple test which allow to see over/out events when pointer moved on top of red and blue box
Hopefully mouseenter/leave dispatcher can be used here too. And we probably want similar optimization
to dispatch the events only if there are listeners.
Still dirty, but kind of work with multi pointers/targets.
Would like to hear if this way would be appropriate for Cycle collector
Attachment #8370593 - Flags: feedback?(bugs)
Status: UNCONFIRMED → NEW
Ever confirmed: true
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.
Attachment #8370593 - Flags: feedback?(bugs) → feedback-
Cleaned up things
Attachment #8370593 - Attachment is obsolete: true
Attachment #8371066 - Flags: review?(bugs)
Attachment #8371071 - Flags: review?(bugs)
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
Attachment #8371066 - Attachment is obsolete: true
Attachment #8371066 - Flags: review?(bugs)
Attachment #8371071 - Flags: review?(bugs) → review+
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.
Attachment #8372050 - Flags: review?(bugs)
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 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.
Attachment #8372050 - Flags: review?(bugs) → review+
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
> 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 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.
Attachment #8372051 - Flags: review?(bugs) → review-
Fixed review comments
Attachment #8372050 - Attachment is obsolete: true
Attachment #8372638 - Flags: review+
Fixed review comments, fixed problem with anonymous content dispatching.
Attachment #8372639 - Flags: review?(bugs)
Added tests for pointer events, based on test_bug432698.html
Attachment #8372051 - Attachment is obsolete: true
Attachment #8372640 - Flags: review?(bugs)
> >+  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 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();
>+}
Try build with SpecialPowers in tests
https://tbpl.mozilla.org/?tree=Try&rev=7785d927fb43
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.
Attachment #8372639 - Flags: review?(bugs) → review+
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
Attachment #8372640 - Flags: review?(bugs) → review-
Attachment #8372640 - Attachment is obsolete: true
Attachment #8373755 - Flags: review?(bugs)
aOptionalArgCount >= 4 ? aIsSynthesized : true - it should be 10, because we have 10 optional arguments now
Attachment #8373755 - Attachment is obsolete: true
Attachment #8373755 - Flags: review?(bugs)
Attachment #8373757 - Flags: review?(bugs)
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
Attachment #8373757 - Flags: review?(bugs)
Fixed compile, updated uuid
Attachment #8373757 - Attachment is obsolete: true
Attachment #8373765 - Flags: review?(bugs)
Attachment #8373765 - Flags: review?(bugs) → review+
https://hg.mozilla.org/mozilla-central/rev/b8dfec3cca1c
https://hg.mozilla.org/mozilla-central/rev/37f194248784
https://hg.mozilla.org/mozilla-central/rev/af35bd7bd9a8
Assignee: nobody → oleg.romashin
Status: NEW → RESOLVED
Closed: 6 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla30
Depends on: 974138
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?
ni'ing Oleg regarding comment 32.

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

Ping, Oleg, can you help out here?

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

Sebastian
Flags: needinfo?(oleg.romashin) → needinfo?(bugs)
Yes, this was adding support for pointerenter/leave. Note, pointerevents aren't enabled by default yet.
Flags: needinfo?(bugs)
You need to log in before you can comment on or make changes to this bug.