Implement Pointer Enter/Leave events support.

RESOLVED FIXED in mozilla30

Status

()

RESOLVED FIXED
5 years ago
a month ago

People

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

Tracking

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

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

Firefox Tracking Flags

(Not tracked)

Details

(URL)

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

Description

5 years ago
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
Blocks: 822898
(Assignee)

Comment 1

5 years ago
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.
(Assignee)

Comment 3

5 years ago
Created attachment 8370593 [details] [diff] [review]
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
Attachment #8370593 - Flags: feedback?(bugs)

Updated

5 years ago
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-
(Assignee)

Comment 5

5 years ago
Created attachment 8371066 [details] [diff] [review]
Pointer Enter Leave with Multi Pointer support.

Cleaned up things
Attachment #8370593 - Attachment is obsolete: true
Attachment #8371066 - Flags: review?(bugs)
(Assignee)

Comment 6

5 years ago
Created attachment 8371071 [details] [diff] [review]
Pointer Enter Leave Listeners detection
Attachment #8371071 - Flags: review?(bugs)
(Assignee)

Comment 7

5 years ago
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+
(Assignee)

Comment 8

5 years ago
Created attachment 8372050 [details] [diff] [review]
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.
Attachment #8372050 - Flags: review?(bugs)
(Assignee)

Comment 9

5 years ago
Created attachment 8372051 [details] [diff] [review]
Add support for Pointer events Enter/Leave over/out dispatching
Attachment #8372051 - Flags: review?(bugs)
(Assignee)

Comment 10

5 years ago
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+
(Assignee)

Comment 12

5 years ago
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
(Assignee)

Comment 13

5 years ago
> 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-
Created attachment 8372638 [details] [diff] [review]
Refactor Out/Over element helpers not functional change

Fixed review comments
Attachment #8372050 - Attachment is obsolete: true
Attachment #8372638 - Flags: review+
(Assignee)

Comment 16

5 years ago
Created attachment 8372639 [details] [diff] [review]
Add support for Pointer events Enter/Leave over/out dispatching

Fixed review comments, fixed problem with anonymous content dispatching.
Attachment #8372639 - Flags: review?(bugs)
(Assignee)

Comment 17

5 years ago
Created attachment 8372640 [details] [diff] [review]
Pointer events Enter/Leave over/out tests

Added tests for pointer events, based on test_bug432698.html
Attachment #8372051 - Attachment is obsolete: true
Attachment #8372640 - Flags: review?(bugs)
(Assignee)

Comment 18

5 years ago
> >+  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.
(Assignee)

Comment 20

5 years ago
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();
>+}
(Assignee)

Comment 21

5 years ago
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-
(Assignee)

Comment 24

5 years ago
Created attachment 8373755 [details] [diff] [review]
Pointer events Enter/Leave over/out tests
Attachment #8372640 - Attachment is obsolete: true
Attachment #8373755 - Flags: review?(bugs)
(Assignee)

Comment 25

5 years ago
Created attachment 8373757 [details] [diff] [review]
Pointer events Enter/Leave over/out tests

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

Comment 27

5 years ago
Created attachment 8373765 [details] [diff] [review]
Pointer events Enter/Leave over/out tests

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
Last Resolved: 5 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla30
(Assignee)

Updated

5 years ago
Depends on: 974138
Keywords: dev-doc-needed
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)
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
Keywords: dev-doc-needed → dev-doc-complete
You need to log in before you can comment on or make changes to this bug.