Closed Bug 1303957 Opened 3 years ago Closed 2 years ago

Add support for PointerEvent.getCoalescedEvents()

Categories

(Core :: DOM: Core & HTML, defect, P1)

50 Branch
defect

Tracking

()

RESOLVED FIXED
mozilla59
Tracking Status
firefox57 --- wontfix
firefox59 --- fixed

People

(Reporter: smaug, Assigned: stone)

References

(Depends on 1 open bug, Blocks 1 open bug)

Details

Attachments

(3 files, 9 obsolete files)

1.76 KB, text/html
Details
14.65 KB, patch
smaug
: review+
Details | Diff | Splinter Review
6.36 KB, patch
stone
: review+
Details | Diff | Splinter Review
Priority: -- → P3
Assignee: nobody → sshih
Blocks: pointerevent
Depends on: 1404273
Attachment #8915780 - Flags: review?(bugs)
Comment on attachment 8915780 [details] [diff] [review]
Add support for PointerEvent.getCoalescedEvents.

>+JSObject*
>+PointerEvent::WrapObjectInternal(JSContext* aCx,
>+                                 JS::Handle<JSObject*> aGivenProto)
>+{
>+  return PointerEventBinding::Wrap(aCx, this, aGivenProto);
>+}
I have no idea why this needs to be non-inline. But should cause much harm.




>+  if (aType.EqualsLiteral("pointermove") &&
I filed https://github.com/w3c/pointerevents/issues/223 since the spec is super weird.
But I guess we can do this for now.


>+PointerEvent::GetCoalescedEvents(nsTArray<RefPtr<PointerEvent>>& aPointerEvents)
>+{
>+  // getCoalescedEvents always returns a sequence which contains at least one
>+  // coalesced event for pointermove. For other types of pointer events, it
>+  // returns an empty list.
>+  if (mEvent->mMessage != ePointerMove) {
>+    return;
>+  }
>+
>+  if (!mCoalescedEvents) {
>+    WidgetPointerEvent* widgetEvent = mEvent->AsPointerEvent();
>+    if (widgetEvent && widgetEvent->mCoalescedWidgetEvents) {
>+      mCoalescedEvents = MakeUnique<nsTArray<RefPtr<PointerEvent>>>();
>+      for (const UniquePtr<WidgetPointerEvent>& event :
>+             widgetEvent->mCoalescedWidgetEvents->mEvents) {
>+        mCoalescedEvents->AppendElement(NS_NewDOMPointerEvent(nullptr, nullptr,
>+                                                              event.get()));
>+      }
>+    }
>+  }
>+  if (mCoalescedEvents) {
>+    LayoutDeviceIntPoint lastRefPoint = mEvent->mLastRefPoint;
>+    aPointerEvents.Clear();
>+    for (RefPtr<PointerEvent>& pointerEvent : *mCoalescedEvents) {
>+      // We don't dispatch these widget events so that their mLastRefPoint are
>+      // not updated. Update them as their previous event's mRefPoint.
>+      pointerEvent->mEvent->mLastRefPoint = lastRefPoint;
Does some spec define this?


>+      lastRefPoint = pointerEvent->mEvent->mRefPoint;
Hmm, I don't understand this. Please explain.

>+++ b/dom/webidl/PointerEvent.webidl
>@@ -20,22 +20,27 @@ interface PointerEvent : MouseEvent
>   readonly attribute float tangentialPressure;
>   readonly attribute long tiltX;
>   readonly attribute long tiltY;
>   readonly attribute long twist;
>   readonly attribute DOMString pointerType;
>   readonly attribute boolean isPrimary;
> };
> 
>+partial interface PointerEvent {
>+  [Pref="dom.w3c_pointer_events.enabled"]
>+  sequence<PointerEvent> getCoalescedEvents();
>+};

You could just put this to the PointerEvent interface.
>+class WidgetPointerEvent;
>+class WidgetPointerEventHolder final : public nsISupports
>+{
>+public:
>+  nsTArray<UniquePtr<WidgetPointerEvent>> mEvents;
>+  NS_DECL_ISUPPORTS
>+
>+private:
>+  virtual ~WidgetPointerEventHolder() {}
>+};
I don't understand the need for this, and I definitely don't understand why this need to inherit nsISupports.
If you need refcounting, just use the macro to define refcounting inline.
NS_INLINE_DECL_REFCOUNTING

This is missing the cycle collection stuff.
mCoalescedEvents needs to be cycle collected at least.
Attachment #8915780 - Flags: review?(bugs) → review-
(In reply to Olli Pettay [:smaug] from comment #2)
> Comment on attachment 8915780 [details] [diff] [review]
> Add support for PointerEvent.getCoalescedEvents.
> 
> >+JSObject*
> >+PointerEvent::WrapObjectInternal(JSContext* aCx,
> >+                                 JS::Handle<JSObject*> aGivenProto)
> >+{
> >+  return PointerEventBinding::Wrap(aCx, this, aGivenProto);
> >+}
> I have no idea why this needs to be non-inline. But should cause much harm.
> 
That's because I added 'sequence<PointerEvent> getCoalescedEvents();' in PointerEvent.webidl, which induce PointerEventBinding.h to include PointerEvent.h and causes a circular include. Still thinking how to fix it.
We want this before shipping Pointer events, so setting P1.
Priority: P3 → P1
Attachment #8915780 - Attachment is obsolete: true
(In reply to Ming-Chou Shih [:stone] from comment #5)
> Created attachment 8919138 [details] [diff] [review]
> Add support for PointerEvent.getCoalescedEvents.

Followed the discussions in https://github.com/w3c/pointerevents/issues/223 to revise the patch.
(In reply to Ming-Chou Shih [:stone] from comment #3)
> (In reply to Olli Pettay [:smaug] from comment #2)
> > Comment on attachment 8915780 [details] [diff] [review]
> > Add support for PointerEvent.getCoalescedEvents.
> > 
> > >+JSObject*
> > >+PointerEvent::WrapObjectInternal(JSContext* aCx,
> > >+                                 JS::Handle<JSObject*> aGivenProto)
> > >+{
> > >+  return PointerEventBinding::Wrap(aCx, this, aGivenProto);
> > >+}
> > I have no idea why this needs to be non-inline. But should cause much harm.
> > 
> That's because I added 'sequence<PointerEvent> getCoalescedEvents();' in
> PointerEvent.webidl, which induce PointerEventBinding.h to include
> PointerEvent.h and causes a circular include. Still thinking how to fix it.

Have no idea how to fix it since PointerEventBinding.h is generated.
Attachment #8919138 - Attachment is obsolete: true
Attachment #8920471 - Flags: review?(bugs)
Comment on attachment 8920471 [details] [diff] [review]
Add support for PointerEvent.getCoalescedEvents.

> 
>+  if (!aParam.mCoalescedEvents.IsEmpty()) {
>+    e->mCoalescedEvents = MakeUnique<nsTArray<RefPtr<PointerEvent>>>();
Why are we using UniquePtr for mCoalescedEvents?
Couldn't it be just nsTArray<RefPtr<PointerEvent>

>+
>+NS_IMPL_CYCLE_COLLECTION_UNLINK_BEGIN_INHERITED(PointerEvent, MouseEvent)
>+  tmp->mCoalescedEvents = nullptr;
then this would be just normal NS_IMPL_CYCLE_COLLECTION_UNLINK(mCoalescedEvents)

>+NS_IMPL_CYCLE_COLLECTION_UNLINK_END
>+
>+NS_IMPL_CYCLE_COLLECTION_TRAVERSE_BEGIN_INHERITED(PointerEvent, MouseEvent)
>+  if (tmp->mCoalescedEvents) {
>+    for (RefPtr<PointerEvent>& event : *tmp->mCoalescedEvents.get()) {
>+      ImplCycleCollectionTraverse(
>+        cb, event, "mozilla::dom::PointerEvent.mCoalescedEvents");
>+    }
>+  }
And this would be NS_IMPL_CYCLE_COLLECTION_TRAVERSE(mCoalescedEvents)


>+PointerEvent::GetCoalescedEvents(nsTArray<RefPtr<PointerEvent>>& aPointerEvents)
>+{
>+  if (mShouldAppendWidgetEventsToCoalescedEvents) {
Why we need this check? Why we need the bool member variable at all?
Shouldn't we just check here that if mCoalescedEvents is empty and idgetEvent->mCoalescedWidgetEvents->mEvents
isn't empty, then populate mCoalescedEvents.


Also, do we need to ensure mCoalescedEvents is populated when we DuplicatePrivateData is called, so that getCoalescedEvents returns the right values after event dispatch?

"This API always returns at least one coalesced event for pointermove events and an empty list for other types of PointerEvents."
I filed https://github.com/w3c/pointerevents/issues/224
Attachment #8920471 - Flags: review?(bugs) → review-
Attachment #8920471 - Attachment is obsolete: true
Attachment #8924508 - Attachment is obsolete: true
Attachment #8924509 - Flags: review?(bugs)
Comment on attachment 8924509 [details] [diff] [review]
Add support for PointerEvent.getCoalescedEvents.

>+PointerEvent::GetCoalescedEvents(nsTArray<RefPtr<PointerEvent>>& aPointerEvents)
>+{
>+  WidgetPointerEvent* widgetEvent = mEvent->AsPointerEvent();
>+  if (mCoalescedEvents.IsEmpty() && widgetEvent &&
>+      widgetEvent->mCoalescedWidgetEvents &&
>+      !widgetEvent->mCoalescedWidgetEvents->mEvents.IsEmpty()) {
>+    for (const UniquePtr<WidgetPointerEvent>& event :
>+         widgetEvent->mCoalescedWidgetEvents->mEvents) {
>+      mCoalescedEvents.AppendElement(NS_NewDOMPointerEvent(nullptr, nullptr,
>+                                                           event.get()));
So pointer to event is passed to the new DOM event, but nothing guarantees the current DOM event on which 
widgetEvent lives outlives coalesced events, meaning that it is possible that mEvent on those coalesced events may end up pointing
to a deleted object eventually.

I don't see this patch dealing with DuplicatePrivateData

Does something guarantee each browser dispatched pointermove has non-empty coalesced events array?
Attachment #8924509 - Flags: review?(bugs) → review-
(In reply to Olli Pettay [:smaug] from comment #12)
> Comment on attachment 8924509 [details] [diff] [review]
> Add support for PointerEvent.getCoalescedEvents.
> 
> >+PointerEvent::GetCoalescedEvents(nsTArray<RefPtr<PointerEvent>>& aPointerEvents)
> >+{
> >+  WidgetPointerEvent* widgetEvent = mEvent->AsPointerEvent();
> >+  if (mCoalescedEvents.IsEmpty() && widgetEvent &&
> >+      widgetEvent->mCoalescedWidgetEvents &&
> >+      !widgetEvent->mCoalescedWidgetEvents->mEvents.IsEmpty()) {
> >+    for (const UniquePtr<WidgetPointerEvent>& event :
> >+         widgetEvent->mCoalescedWidgetEvents->mEvents) {
> >+      mCoalescedEvents.AppendElement(NS_NewDOMPointerEvent(nullptr, nullptr,
> >+                                                           event.get()));
> So pointer to event is passed to the new DOM event, but nothing guarantees
> the current DOM event on which 
> widgetEvent lives outlives coalesced events, meaning that it is possible
> that mEvent on those coalesced events may end up pointing
> to a deleted object eventually.
I think the coalesced widget events are held by the dispatched widget event. The created DOM events should live during the lifetime of the dispatched DOM event of the dispatch widget event.

> I don't see this patch dealing with DuplicatePrivateData
DuplicatePrivateData calls widgetEvent's Duplicate function, which calls AssignPointerHelperData.

> Does something guarantee each browser dispatched pointermove has non-empty
> coalesced events array?
When coalescing widgetEvent in CoalescedMouseData::Coalesce, the widgetEvent is cloned and added in the coalesced widget events.
JS can take a ref to the coalesced events, so nothing controls how long they will live.

Does something guarantee we always coalesce pointermoves?
And about DuplicatePrivateData, what guarantees the same coalesced pointer events are returned during event dispatch and afterwards?
Hmm, but the spec is unclear here. Should it return always the same objects or new objects?
Attachment #8924509 - Attachment is obsolete: true
(In reply to Olli Pettay [:smaug] from comment #14)
> JS can take a ref to the coalesced events, so nothing controls how long they
> will live.
Revise the patch to call DuplicatePrivateData on each dom event we added to the array of coalesced pointer events.

> Does something guarantee we always coalesce pointermoves?
Revise the patch to only add mousemove widget events to the array of coalesced widget events.

(In reply to Olli Pettay [:smaug] from comment #15)
> And about DuplicatePrivateData, what guarantees the same coalesced pointer
> events are returned during event dispatch and afterwards?
> Hmm, but the spec is unclear here. Should it return always the same objects
> or new objects?
We keep the coalesced pointer events in the dispatch pointer event. The array of coalesced pointer events is only initialized when its empty. So we won't overwrite it when dispatching and afterwards.
Attachment #8924850 - Flags: review?(bugs)
Comment on attachment 8924850 [details] [diff] [review]
Add support for PointerEvent.getCoalescedEvents.

>+void
>+PointerEvent::GetCoalescedEvents(nsTArray<RefPtr<PointerEvent>>& aPointerEvents)
>+{
>+  WidgetPointerEvent* widgetEvent = mEvent->AsPointerEvent();
>+  if (mCoalescedEvents.IsEmpty() && widgetEvent &&
>+      widgetEvent->mCoalescedWidgetEvents &&
>+      !widgetEvent->mCoalescedWidgetEvents->mEvents.IsEmpty()) {
>+    for (const UniquePtr<WidgetPointerEvent>& event :
>+         widgetEvent->mCoalescedWidgetEvents->mEvents) {
>+      RefPtr<PointerEvent> domEvent =
>+        NS_NewDOMPointerEvent(nullptr, nullptr, event.get());
>+
>+      // JS could hold reference to dom events. We have to ask dom event to
>+      // duplicate its private data to avoid the widget event is destroyed.
>+      domEvent->DuplicatePrivateData();
This looks wrong. DuplicatePrivateData() is called when needed, but DuplicatePrivateData call needs to actually created the coalesced events if needed.
Sorry if I was unclear. So this DuplicatePrivateData() can be removed.

>+  void AssignPointerHelperData(const WidgetPointerHelper& aEvent,
>+                               bool aCopyCoalescedEvents = false)
>   {
>     pointerId = aEvent.pointerId;
>     tiltX = aEvent.tiltX;
>     tiltY = aEvent.tiltY;
>     twist = aEvent.twist;
>     tangentialPressure = aEvent.tangentialPressure;
>     convertToPointer = aEvent.convertToPointer;
>+    if (aCopyCoalescedEvents) {
>+      mCoalescedWidgetEvents = aEvent.mCoalescedWidgetEvents;
>+    }
And looks like this does the thing.


>+class WidgetPointerEventHolder final
>+{
>+public:
>+  nsTArray<UniquePtr<WidgetPointerEvent>> mEvents;
I don't understand this. Why we need UniquePtr here? That causes extra malloc/free.
Why nsTArray<WidgetPointerEvent> doesn't work? I think it should work.
Attachment #8924850 - Flags: review?(bugs) → review-
Attachment #8924850 - Attachment is obsolete: true
Attachment #8925849 - Flags: review?(bugs)
Comment on attachment 8925849 [details] [diff] [review]
Add support for PointerEvent.getCoalescedEvents.

>+PointerEvent::GetCoalescedEvents(nsTArray<RefPtr<PointerEvent>>& aPointerEvents)
>+{
>+  WidgetPointerEvent* widgetEvent = mEvent->AsPointerEvent();
>+  if (mCoalescedEvents.IsEmpty() && widgetEvent &&
>+      widgetEvent->mCoalescedWidgetEvents &&
>+      !widgetEvent->mCoalescedWidgetEvents->mEvents.IsEmpty()) {
>+    for (WidgetPointerEvent& event :
>+         widgetEvent->mCoalescedWidgetEvents->mEvents) {
>+      RefPtr<PointerEvent> domEvent =
>+        NS_NewDOMPointerEvent(nullptr, nullptr, &event);
>+
>+      // JS could hold reference to dom events. We have to ask dom event to
>+      // duplicate its private data to avoid the widget event is destroyed.
>+      domEvent->DuplicatePrivateData();
>+      mCoalescedEvents.AppendElement(domEvent);
>+    }
>+  }
>+
>+  for (RefPtr<PointerEvent>& pointerEvent : mCoalescedEvents) {
>+    // The event target should be the same as the dispatched event's target.
>+    pointerEvent->mEvent->mTarget = mEvent->mTarget;
>+    aPointerEvents.AppendElement(pointerEvent);
>+  }
Don't we want to set the target only when initially creating DOM event, not always.
Just in case JS re-dispatches the event, target of coalesced events shouldn't change, if I read the spec right.
So, pointerEvent->mEvent->mTarget = mEvent->mTarget; should be somewhere after NS_NewDOMPointerEvent.


Do we have tests that coalesced events actually have some sane values for the properties?


>+  if (aEvent.mMessage == eMouseMove &&
>+      PointerEventHandler::IsPointerEventEnabled()) {
>+    // PointerEvent::getCoalescedEvents is only applied to pointermove events.
>+    if (!mCoalescedInputEvent->mCoalescedWidgetEvents) {
>+      mCoalescedInputEvent->mCoalescedWidgetEvents =
>+        new WidgetPointerEventHolder();
>+    }
>+    // Append current event in mCoalescedWidgetEvents. We use them to generate
>+    // DOM events when content calls PointerEvent::getCoalescedEvents.
>+    WidgetMouseEvent* event = mCoalescedInputEvent->mCoalescedWidgetEvents
>+                                ->mEvents.AppendElement(aEvent);
WidgetMouseEvent? Make it WidgetPointerEvent, since that is the concrete class
Attachment #8925849 - Flags: review?(bugs) → review+
Attached file dispatch_ev_twice.html
(In reply to Olli Pettay [:smaug] from comment #21)
> Comment on attachment 8925849 [details] [diff] [review]
> Add support for PointerEvent.getCoalescedEvents.
> >+  for (RefPtr<PointerEvent>& pointerEvent : mCoalescedEvents) {
> >+    // The event target should be the same as the dispatched event's target.
> >+    pointerEvent->mEvent->mTarget = mEvent->mTarget;
> >+    aPointerEvents.AppendElement(pointerEvent);
> >+  }
> Don't we want to set the target only when initially creating DOM event, not
> always.
> Just in case JS re-dispatches the event, target of coalesced events
> shouldn't change, if I read the spec right.
> So, pointerEvent->mEvent->mTarget = mEvent->mTarget; should be somewhere
> after NS_NewDOMPointerEvent.
Spec says "it should still be the same as the dispatched event's target". I thought the target of the coalesced events should be the same as the target of the dispatched event. Tested dispatch_ev_twice.html with chrome and the target of coalesced events changed when dispatching the same event to different targets.

> 
> Do we have tests that coalesced events actually have some sane values for
> the properties?
We have a web platform test and I could create a mochitest for this.
Attachment #8925849 - Attachment is obsolete: true
Attachment #8926305 - Flags: review?(bugs)
(In reply to Ming-Chou Shih [:stone] from comment #23)

> Spec says "it should still be the same as the dispatched event's target". I
> thought the target of the coalesced events should be the same as the target
> of the dispatched event. Tested dispatch_ev_twice.html with chrome and the
> target of coalesced events changed when dispatching the same event to
> different targets.
That sounds like a bug in Chrome, I'd say.
"When the event is created by the user agent" sure, but once you use JS to redispatch the event, it becomes like it had been created from JS. Please file a spec bug.
Comment on attachment 8926305 [details] [diff] [review]
Part2: Setup necessary attributes to calculate offsetX/offsetY and add a test with synthesized widget events.

>+++ b/dom/events/PointerEvent.cpp
>@@ -214,16 +214,22 @@ PointerEvent::GetCoalescedEvents(nsTArra
>     for (WidgetPointerEvent& event :
>          widgetEvent->mCoalescedWidgetEvents->mEvents) {
>       RefPtr<PointerEvent> domEvent =
>         NS_NewDOMPointerEvent(nullptr, nullptr, &event);
> 
>       // JS could hold reference to dom events. We have to ask dom event to
>       // duplicate its private data to avoid the widget event is destroyed.
>       domEvent->DuplicatePrivateData();
>+
>+      // The dom event is derived from an OS generated widget event. Setup
>+      // mWidget and mPresContext since they are necessary to calculate
>+      // offsetX / offsetY.
>+      domEvent->mEvent->AsGUIEvent()->mWidget = widgetEvent->mWidget;
>+      domEvent->mPresContext = mPresContext;
Don't you want this all before DuplicatePrivateData() call so that it works properly
https://searchfox.org/mozilla-central/rev/fe75164c55321e011d9d13b6d05c1e00d0a640be/dom/events/UIEvent.cpp#362
Attachment #8926305 - Flags: review?(bugs) → review+
(In reply to Olli Pettay [:smaug] from comment #26)
> (In reply to Ming-Chou Shih [:stone] from comment #23)
> 
> > Spec says "it should still be the same as the dispatched event's target". I
> > thought the target of the coalesced events should be the same as the target
> > of the dispatched event. Tested dispatch_ev_twice.html with chrome and the
> > target of coalesced events changed when dispatching the same event to
> > different targets.
> That sounds like a bug in Chrome, I'd say.
> "When the event is created by the user agent" sure, but once you use JS to
> redispatch the event, it becomes like it had been created from JS. Please
> file a spec bug.

Created https://github.com/w3c/pointerevents/issues/229
Revise the patch to set event target only when it's null.
Attachment #8926299 - Attachment is obsolete: true
Comment on attachment 8935617 [details] [diff] [review]
Part1: Add support for PointerEvent.getCoalescedEvents.

Request review again to make sure everything is as expected.
Attachment #8935617 - Flags: review?(bugs)
Comment on attachment 8935617 [details] [diff] [review]
Part1: Add support for PointerEvent.getCoalescedEvents.

>+  if (mEvent->mTarget && mCoalescedEvents.Length() > 0 &&
>+      !mCoalescedEvents[0]->mEvent->mTarget) {
>+    // Only set event target when it's null.
Why the first event is special?
This is not what the spec issue was talking about

>+    for (RefPtr<PointerEvent>& pointerEvent : mCoalescedEvents) {
>+      pointerEvent->mEvent->mTarget = mEvent->mTarget;
I think we'd need to check that target is null each time here.
Attachment #8935617 - Flags: review?(bugs) → review+
Pushed by sshih@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a91a06d7c3f3
Part1: Add support for PointerEvent.getCoalescedEvents. r=smaug.
https://hg.mozilla.org/integration/mozilla-inbound/rev/a0a5bec2bb3f
Part2: Setup necessary attributes to calculate offsetX/offsetY and add a test with synthesized widget events. r=smaug.
Duplicate of this bug: 1425221
The requests to synthesize native mouse move may be coalesced by OS. Tweak the test case a little bit and submit a try https://treeherder.mozilla.org/#/jobs?repo=try&revision=ce92c59b14bac8a5a261e9051e7547034282c915
Pushed by sshih@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/0152c7d3651e
Part1: Add support for PointerEvent.getCoalescedEvents. r=smaug.
https://hg.mozilla.org/integration/mozilla-inbound/rev/767cb7462ef8
Part2: Setup necessary attributes to calculate offsetX/offsetY and add a test with synthesized widget events. r=smaug.
https://hg.mozilla.org/mozilla-central/rev/0152c7d3651e
https://hg.mozilla.org/mozilla-central/rev/767cb7462ef8
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
Depends on: 1426527
Depends on: 1457859
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.