Closed Bug 822898 Opened 11 years ago Closed 1 year ago

[meta] Implement pointer events

Categories

(Core :: DOM: Events, enhancement, P3)

enhancement

Tracking

()

RESOLVED FIXED

People

(Reporter: smaug, Unassigned)

References

(Blocks 1 open bug, )

Details

(Keywords: dev-doc-needed, meta)

Attachments

(6 files, 24 obsolete files)

25.88 KB, patch
smaug
: review+
Details | Diff | Splinter Review
12.63 KB, patch
smaug
: review+
Details | Diff | Splinter Review
784 bytes, patch
ehsan.akhgari
: review+
Details | Diff | Splinter Review
14.59 KB, patch
smaug
: review+
Details | Diff | Splinter Review
11.29 KB, patch
Details | Diff | Splinter Review
1.26 KB, patch
smaug
: review+
Details | Diff | Splinter Review
The API is still very my in flux, but we should start implementing it soon.
cc: Matt Brubeck for input.
For what input ? :)

We should implement pointer events, and the spec is getting more stable.
Yes, I agree.  We are about to publish a second Last Call Working Draft to address comments from our first LCWD; the time to start implementing is now (especially if we want any feedback we discover during implementation to influence the final Recommendation).
Attached patch WIP Implementation for Pointer events (obsolete) — — Splinter Review
Here is the initial implementation for Pointer Events.
Current WIP is partial implementation which has:
1) Definition of PointerEvent type in content/dom/webidl/widget derived from MouseEvent
2) Basic Implementation of Content EventManager tricks for out/over enter/leave part.
   Mainly copied from MouseEvent implementation, and currently not merged with Mouse functions
   Ideally need to merge NotifyMouse/Out/Over and Enter/Leave dispatchers and share them between Mouse and Pointer events
3) May/HavePointerEvents implementation for content/dom
4) Declaration for pointerEnabled maxTouchPoints attributes without real implementation
5) setPointerCapture - without implementation (need to connect/share it to mouse event setCapture API)
6) Dummy stub for TabChild stuff... TBD
7) Quick wrapper in nsView which convert Mouse events and TouchEvents into Pointer events when page have any pointer listeners.
8) Basic declaration of touch-action, need some help in order to implement it properly, or point to existing Css property with similar functionality
9) Basic unit test for pointer events

All changes attached in one blob, but I will split logical parts into separate patches to make reviewing process easier.

Need some advice on license headers format... (not sure what is the right way to attach 3-rd party copyright contributions)

Basically it would be nice to overlook on patch and figure out if we can do better integration between Mouse/Pointer events. Any other comments are welcome.
Attachment #763013 - Flags: feedback?(wjohnston)
Attachment #763013 - Flags: feedback?(mbrubeck)
Attachment #763013 - Flags: feedback?(bugs)
Assignee: nobody → oleg.romashin
OS: Linux → All
Hardware: x86 → All
Attached patch Pointer Events Definition. part 1 (obsolete) — — Splinter Review
Attached patch Pointer Events Tests (Quick WIP). part 2 (obsolete) — — Splinter Review
Attached patch Pointer Events. Navigator Props. part 3 (obsolete) — — Splinter Review
Attached patch Pointer Events. Set Capture wip. part 5 (obsolete) — — Splinter Review
Will pointer events improve the responsiveness of a pen in FF?
I've tried drawing on HTML5 canvas on a Windows 8 tablet and a Galaxy Note 10.1 both with a Wacom digitizer and the responsiveness is bad compared to IE10 on Windows 8 and native apps on the Galaxy Note.
Pointer events will have a bit more cheap processing code due to no TouchPoints list... but I believe in your case that problem related to something else, possibly Graphics...
I don't think its a Graphics issue because on Win 8, for example, IE10 works great. You can see the bug details here including a test page.
https://bugzilla.mozilla.org/show_bug.cgi?id=827247
Depends on: 795567
Comment on attachment 763013 [details] [diff] [review]
WIP Implementation for Pointer events

>+++ b/content/base/src/nsTreeSanitizer.cpp
>@@ -1,9 +1,10 @@
> /* -*- Mode: C++; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
>+/* Portions Copyright (c) Microsoft Open Technologies, Inc. All Rights Reserved. */
> /* vim: set sw=2 ts=2 et tw=80: */
> /* This Source Code Form is subject to the terms of the Mozilla Public
>  * License, v. 2.0. If a copy of the MPL was not distributed with this
>  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> 
> #include "mozilla/Util.h"
> 
> #include "nsTreeSanitizer.h"
>@@ -496,16 +497,17 @@ nsIAtom** const kAttributesSVG[] = {
>   &nsGkAtoms::overflow, // overflow
>   // panose-1
>   &nsGkAtoms::path, // path
>   &nsGkAtoms::pathLength, // pathLength
>   &nsGkAtoms::patternContentUnits, // patternContentUnits
>   &nsGkAtoms::patternTransform, // patternTransform
>   &nsGkAtoms::patternUnits, // patternUnits
>   &nsGkAtoms::pointer_events, // pointer-events XXX is this safe?
>+  &nsGkAtoms::touch_action, // touch action
Why we need this?

>+nsDOMPointerEvent::nsDOMPointerEvent(mozilla::dom::EventTarget* aOwner,
>+                                     nsPresContext* aPresContext,
>+                                     nsPointerEvent* aEvent)
>+  : nsDOMMouseEvent(aOwner, aPresContext, aEvent ? aEvent : new nsPointerEvent(false, 0, nullptr))
>+{
>+  NS_ASSERTION(mEvent->eventStructType == NS_POINTER_EVENT, "event type mismatch NS_POINTER_EVENT");
>+
>+  if (aEvent) {
>+    mEventIsInternal = false;
>+  } else {
>+    mEventIsInternal = true;
>+    mEvent->time = PR_Now();
>+    mEvent->refPoint.x = mEvent->refPoint.y = 0;
>+    static_cast<nsMouseEvent*>(mEvent)->inputSource = nsIDOMMouseEvent::MOZ_SOURCE_UNKNOWN;
>+  }
>+  SetIsDOMBinding();
All events use now Webidl bindings so no need for this.

>+already_AddRefed<nsDOMPointerEvent>
>+nsDOMPointerEvent::Constructor(const mozilla::dom::GlobalObject& aGlobal,
>+                                 const nsAString& aType,
>+                                 const mozilla::dom::PointerEventInit& aParam,
>+                                 mozilla::ErrorResult& aRv)
align params

>+ConvertStringToPointerType(const nsAString& aPointerTypeArg)
>+{
>+  if (aPointerTypeArg.EqualsLiteral("mouse"))
>+    return nsIDOMMouseEvent::MOZ_SOURCE_MOUSE;
>+  if (aPointerTypeArg.EqualsLiteral("pen"))
>+    return nsIDOMMouseEvent::MOZ_SOURCE_PEN;
>+  if (aPointerTypeArg.EqualsLiteral("touch"))
>+    return nsIDOMMouseEvent::MOZ_SOURCE_TOUCH;
>+
>+  return nsIDOMMouseEvent::MOZ_SOURCE_UNKNOWN;
>+}
Use {} with if.


>+nsDOMPointerEvent::InitPointerEvent(const nsAString& typeArg,
>+                                    bool canBubbleArg,
>+                                    bool cancelableArg,
>+                                    nsIDOMWindow* viewArg,
>+                                    int32_t detailArg,
>+                                    int32_t screenXArg,
>+                                    int32_t screenYArg,
>+                                    int32_t clientXArg,
>+                                    int32_t clientYArg,
>+                                    bool ctrlKeyArg,
>+                                    bool altKeyArg,
>+                                    bool shiftKeyArg,
>+                                    bool metaKeyArg,
>+                                    uint16_t buttonArg,
>+                                    nsIDOMEventTarget* relatedTargetArg,
>+                                    int32_t pointerIdArg,
>+                                    int32_t widthArg,
>+                                    int32_t heightArg,
>+                                    float pressureArg,
>+                                    int32_t tiltXArg,
>+                                    int32_t tiltYArg,
>+                                    const nsAString& pointerTypeArg,
>+                                    bool isPrimaryArg)
parameter names should be aParamName


>+  void InitPointerEvent(const nsAString& typeArg,
>+                        bool canBubbleArg,
>+                        bool cancelableArg,
>+                        nsIDOMWindow* viewArg,
>+                        int32_t detailArg,
>+                        int32_t screenXArg,
>+                        int32_t screenYArg,
>+                        int32_t clientXArg,
>+                        int32_t clientYArg,
>+                        bool ctrlKeyArg,
>+                        bool altKeyArg,
>+                        bool shiftKeyArg,
>+                        bool metaKeyArg,
>+                        uint16_t buttonArg,
>+                        mozilla::dom::EventTarget* relatedTargetArg,
>+                        int32_t pointerIdArg,
>+                        int32_t widthArg,
>+                        int32_t heightArg,
>+                        float pressureArg,
>+                        int32_t tiltXArg,
>+                        int32_t tiltYArg,
>+                        const nsAString& pointerTypeArg,
>+                        bool isPrimaryArg,
>+                        mozilla::ErrorResult& aRv)
Please use the form aParamName for parameter


>+  if (aEventType.LowerCaseEqualsLiteral("pointerevent"))
>+    return NS_NewDOMPointerEvent(aDOMEvent, aOwner, aPresContext, nullptr);
We don't want this for new events types.
New types should be creatable only using new EventType(...)

>+  } else if (aTypeAtom == nsGkAtoms::onpointerdown ||
>+             aTypeAtom == nsGkAtoms::onpointerup ||
>+             aTypeAtom == nsGkAtoms::onpointermove ||
>+             aTypeAtom == nsGkAtoms::onpointercancel ||
>+             aTypeAtom == nsGkAtoms::onpointerover ||
>+             aTypeAtom == nsGkAtoms::onpointerout ||
>+             aTypeAtom == nsGkAtoms::onpointerenter ||
>+             aTypeAtom == nsGkAtoms::onpointerleave ||
>+             aTypeAtom == nsGkAtoms::ongotpointercapture ||
>+             aTypeAtom == nsGkAtoms::onlostpointercapture) {
Should be faster to compare using aType, similar to mutation events

>+    mMayHavePointerEventListener = true;
>+    nsPIDOMWindow* window = GetInnerWindowForTarget();
>+    // we don't want touchevent listeners added by scrollbars to flip this flag
pointerevents

>@@ -524,18 +527,20 @@ protected:
> 
>   uint32_t mMayHavePaintEventListener : 1;
>   uint32_t mMayHaveMutationListeners : 1;
>   uint32_t mMayHaveCapturingListeners : 1;
>   uint32_t mMayHaveSystemGroupListeners : 1;
>   uint32_t mMayHaveAudioAvailableEventListener : 1;
>   uint32_t mMayHaveTouchEventListener : 1;
>   uint32_t mMayHaveMouseEnterLeaveEventListener : 1;
>+  uint32_t mMayHavePointerEnterLeaveEventListener : 1;
>   uint32_t mClearingListeners : 1;
>   uint32_t mNoListenerForEvent : 24;
>+  uint32_t mMayHavePointerEventListener : 1;
so, you should change 24 to 22. I think that would still be enough for highest event number in nsGUIEvent.h
So for various cases in nsEventStateManager we'd need to think about what happen when both
pointer and mouse events are dispatched.
(since we need to keep supporting mouse events, and we should just add pointer events on top of them.)


>+class PointerEnterLeaveDispatcher
>+{
>+public:
>+  PointerEnterLeaveDispatcher(nsEventStateManager* aESM,
>+                            nsIContent* aTarget, nsIContent* aRelatedTarget,
>+                            nsGUIEvent* aEvent, uint32_t aType)
>+  : mESM(aESM), mEvent(aEvent), mType(aType)
>+  {
>+    nsPIDOMWindow* win =
>+      aTarget ? aTarget->OwnerDoc()->GetInnerWindow() : nullptr;
>+    if (win && win->HasPointerEnterLeaveEventListeners()) {
>+      mRelatedTarget = aRelatedTarget ?
>+        aRelatedTarget->FindFirstNonChromeOnlyAccessContent() : nullptr;
>+      nsINode* commonParent = nullptr;
>+      if (aTarget && aRelatedTarget) {
>+        commonParent =
>+          nsContentUtils::GetCommonAncestor(aTarget, aRelatedTarget);
>+      }
>+      nsIContent* current = aTarget;
>+      // Note, it is ok if commonParent is null!
>+      while (current && current != commonParent) {
>+        if (!current->ChromeOnlyAccess()) {
>+          mTargets.AppendObject(current);
>+        }
>+        // Pointerenter/leave is fired only on elements.
>+        current = current->GetParent();
>+      }
>+    }
>+  }
>+
>+  ~PointerEnterLeaveDispatcher()
>+  {
>+    if (mType == NS_POINTER_ENTER) {
>+      for (int32_t i = mTargets.Count() - 1; i >= 0; --i) {
>+        mESM->DispatchPointerEvent(mEvent, mType, mTargets[i], mRelatedTarget);
>+      }
>+    } else {
>+      for (int32_t i = 0; i < mTargets.Count(); ++i) {
>+        mESM->DispatchPointerEvent(mEvent, mType, mTargets[i], mRelatedTarget);
>+      }
>+    }
>+  }
>+
>+  nsEventStateManager*   mESM;
>+  nsCOMArray<nsIContent> mTargets;
>+  nsCOMPtr<nsIContent>   mRelatedTarget;
>+  nsGUIEvent*            mEvent;
>+  uint32_t               mType;
>+};
Would be good to have just one class for enter/leave handling and use the same for mouse and pointer events


>+nsEventStateManager::NotifyPointerOver(nsGUIEvent* aEvent, nsIContent* aContent)
>+{
>+  NS_ASSERTION(aContent, "Pointer must be over something");
>+
>+  if (mLastPointerOverElement == aContent)
>+    return;
>+
>+  // Before firing Pointerover, check for recursion
>+  if (aContent == mFirstPointerOverEventElement)
>+    return;
You're copying code, but still,
if (expr) {
  stmt;
}

>+nsEventStateManager::GeneratePointerEnterExit(nsGUIEvent* aEvent)
Could we try to use the same code for mouse and pointer events.
Having the same or almost the same will lead to bugs later when one is changed, but not the other.

>+function testPreventDefault() {
>+  let event = document.createEvent("PointerEvent");
We don't want this to work.
new PointerEvent("pointerdown", {bubbles: true...});

> NS_IMETHODIMP
>+Navigator::GetPointerEnabled(bool* aPointerEnabled)
>+{
>+  *aPointerEnabled = true;
>+  NS_WARNING("Need better pointer events detection");
Well, shouldn't we support mouse pretty much always, and touch often.
It is not actually clear why the spec has pointerEnabled.
>+    }
>+    else if (aEvent->eventStructType == NS_TOUCH_EVENT)
} else if (...) {

>+        if (pointerMessage)
>+        {
if (expr) {



So the main issues are how mouse and pointer events interact and how they are handled in ESM.
In general the code looks good, as I expected :)
Attachment #763013 - Flags: feedback?(bugs) → feedback-
> >+  &nsGkAtoms::touch_action, // touch action
> Why we need this?

that was experiments for touch action css property described in spec... will move it into another bug 795567

> >+  SetIsDOMBinding();
> All events use now Webidl bindings so no need for this.

Hmm, why it is still here then?
http://mxr.mozilla.org/mozilla-central/source/content/events/src/nsDOMDragEvent.cpp#28

> 
> > NS_IMETHODIMP
> >+Navigator::GetPointerEnabled(bool* aPointerEnabled)
> >+{
> >+  *aPointerEnabled = true;
> >+  NS_WARNING("Need better pointer events detection");
> Well, shouldn't we support mouse pretty much always, and touch often.
> It is not actually clear why the spec has pointerEnabled.

IIUC this is just support check, which would allow to not disable runtime js code if browser does not support pointer events
And thanks for detailed review, will fix as much as I can soon
(In reply to Oleg Romashin (MS) from comment #16)
> Hmm, why it is still here then?
> http://mxr.mozilla.org/mozilla-central/source/content/events/src/
> nsDOMDragEvent.cpp#28
Because the patch is actually still only in m-i and the merge to m-c hasn't happened yet.
Comment on attachment 763013 [details] [diff] [review]
WIP Implementation for Pointer events

Review of attachment 763013 [details] [diff] [review]:
-----------------------------------------------------------------

I'd like to hear gerv's opinion about the changes to license headers here.
Attachment #763013 - Flags: review?(gerv)
Attached patch Pointer Events Definition. part 1 (obsolete) — — Splinter Review
Fixed review comments
Attachment #764019 - Attachment is obsolete: true
Hi Oleg and everyone,

Microsoft (and any other contributor) retains the copyright to their contributions whether there is an explicit notice to that effect in the file or not. Therefore, while the MPL permits adding additional copyright notices, Mozilla discourages it as it clutters up the file header. Also, it leads to other people wanting to do it. :-) Since we switched to the MPL 2, I personally know of no instance where anyone has done it.

If there is some legal or corporate reason why such annotations absolutely must be present, please add them in the following style:

 /* -*- Mode: C++; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*-
  * vim: sw=2 ts=2 et :
  * This Source Code Form is subject to the terms of the Mozilla Public
  * License, v. 2.0. If a copy of the MPL was not distributed with this
  * file, You can obtain one at http://mozilla.org/MPL/2.0/. 
  *
  * Portions Copyright 2013 Microsoft Open Technologies, Inc. */

Please don't say "All Rights Reserved"; it's legal cargo cult text is very misleading, because it appears to contradict the MPL.

Gerv
Attachment #763013 - Flags: review?(gerv)
Comment on attachment 763013 [details] [diff] [review]
WIP Implementation for Pointer events

>diff --git a/content/events/src/nsDOMPointerEvent.cpp b/content/events/src/nsDOMPointerEvent.cpp
>new file mode 100644
>--- /dev/null
>+++ b/content/events/src/nsDOMPointerEvent.cpp
>@@ -0,0 +1,217 @@
>+/* -*- Mode: C++; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
>+/* Copyright (c) Microsoft Open Technologies, Inc. All Rights Reserved. */
>+/* This Source Code Form is subject to the terms of the Mozilla Public
>+ * License, v. 2.0. If a copy of the MPL was not distributed with this
>+ * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>+
>+#include "nsDOMPointerEvent.h"
>+
>+nsDOMPointerEvent::nsDOMPointerEvent(mozilla::dom::EventTarget* aOwner,
>+                                     nsPresContext* aPresContext,
>+                                     nsPointerEvent* aEvent)
>+  : nsDOMMouseEvent(aOwner, aPresContext, aEvent ? aEvent : new nsPointerEvent(false, 0, nullptr))
>+{

Why don't you use C++ name space |mozilla::dom| like DOM wheel event?
http://mxr.mozilla.org/mozilla-central/source/content/events/src/DOMWheelEvent.cpp

>diff --git a/widget/nsGUIEvent.h b/widget/nsGUIEvent.h
>--- a/widget/nsGUIEvent.h
>+++ b/widget/nsGUIEvent.h
>@@ -1014,16 +1029,71 @@ public:
>   reasonType   reason : 4;
>   contextType  context : 4;
>   exitType     exit;
> 
>   /// The number of mouse clicks
>   uint32_t     clickCount;
> };
> 
>+class nsPointerEvent : public nsMouseEvent

Why don't you use |mozilla::widget| like wheel event?

We couldn't use name space, but now we can use it.
> We couldn't use name space, but now we can use it.
Good point, it was a bit hard to notice wheel event which is seems only has namespace.
Not sure if this is the correct place to raise the question, but: if in the medium term the idea is for the browser to support both touch events (for compatibility with most touch-specific JS out in the wild today) and pointer events at the same time, how will the simulated mouse events come into play? It seems the two event models are fundamentally incompatible in this respect, with touch events firing all synthetic mouse events after touchend

touchstart > [touchmove]+ > touchend > mouseover > mousemove > mousedown > click

while pointer events dispatch them "inline" with the pointer ones

pointerover > mouseover > pointerdown > mousedown > pointermove > mousemove > pointerup > mouseup > pointerout > mouseout > click

Additionally, synthetic mouse events and click can be prevented in touch events model by doing a preventDefault, while pointer events make use of the touch-action CSS to suppress synthetic mouse events and explicitly do not allow click to be prevented.
One further complication is the difference in when the classic 300ms delay (unless suppressed) happens: before all the synthetic mouse events in touch, before the click in pointer

touchstart > [touchmove]+ > touchend > [300ms delay] > mouseover > mousemove > mousedown > click

vs

pointerover > mouseover > pointerdown > mousedown > pointermove > mousemove > pointerup > mouseup > pointerout > mouseout > [300ms delay] > click
Attached patch Pointer Events Definition. part 1 (obsolete) — — Splinter Review
Fixed most of review comments for original Pointer events definition
https://tbpl.mozilla.org/?tree=Try&rev=33b51446c85b
Attachment #763013 - Attachment is obsolete: true
Attachment #763013 - Flags: feedback?(wjohnston)
Attachment #763013 - Flags: feedback?(mbrubeck)
Attachment #766226 - Attachment is obsolete: true
Comment on attachment 767073 [details] [diff] [review]
Pointer Events Definition. part 1

I regret to say that "ns" prefix should not be used if the class is defined in non-global name space.
Attached patch Pointer Events Definition. part 1 (obsolete) — — Splinter Review
Attachment #767073 - Attachment is obsolete: true
Attached patch Pointer Event Listeners detection (obsolete) — — Splinter Review
Attachment #764022 - Attachment is obsolete: true
Attached patch Basic Handling of pointer Events (obsolete) — — Splinter Review
Attachment #764025 - Attachment is obsolete: true
Attached patch Enter/Leave version shared with mouse (obsolete) — — Splinter Review
Here is the version of Pointers enter/leave handling which is shared with mouse events, actually it is sharing such properties mFirstMouseOutEventElement, mFirstMouseOverEventElement mLastMouseOverElement
it does not work very well because it fail tests https://tbpl.mozilla.org/php/getParsedLog.php?id=25329363&tree=Try
and it seems we should create wrapper class for mFirstMouseOutEventElement, mFirstMouseOverEventElement mLastMouseOverElement
and track them for each pointer. Need to check how IE works here, and how touch events implemented for :hover
Attachment #764024 - Attachment is obsolete: true
Attached patch Pointer Events Definition. part 1 (obsolete) — — Splinter Review
Fixed some problems with constructors and tests
https://tbpl.mozilla.org/?tree=Try&rev=476d45dd41e8
Attachment #777589 - Flags: review?(bugs)
Attachment #776444 - Attachment is obsolete: true
Attached patch Pointer Events Basic Tests (obsolete) — — Splinter Review
Attachment #764020 - Attachment is obsolete: true
Attachment #777592 - Flags: review?(bugs)
Attachment #776445 - Flags: review?(bugs)
Comment on attachment 776448 [details] [diff] [review]
Basic Handling of pointer Events

This is basic patch which allow to handle simple events down/up/move.
Attachment #776448 - Flags: feedback?(bugs)
[23:33]	smaug	romaxa: also, it would be nice to have some information how IE behaves
[23:33]	smaug	especially how pointer events and mouse events work together there
[23:34]	smaug	and which mouse events actually use PointerEvent interface (which extends MouseEvent)
Comment on attachment 776448 [details] [diff] [review]
Basic Handling of pointer Events

Might be good to have some helper method for 
         (aEvent->eventStructType != NS_MOUSE_EVENT &&
          aEvent->eventStructType != NS_MOUSE_SCROLL_EVENT &&
          aEvent->eventStructType != NS_WHEEL_EVENT &&
          aEvent->eventStructType != NS_POINTER_EVENT &&
          aEvent->eventStructType != NS_DRAG_EVENT &&
          aEvent->eventStructType != NS_SIMPLE_GESTURE_EVENT)


For the FIXME, I guess we need to flag either mouseevent for pointerevent that
there will be other event coming which will trigger click.
Attachment #776448 - Flags: feedback?(bugs) → feedback+
Comment on attachment 777589 [details] [diff] [review]
Pointer Events Definition. part 1

remove DOMCI_DATA, and I think we don't need nsIDOMPointerEvent.
And you can remove all the relevant methods too. Webidl is enough.

Could you call the class PointerEvent, and use header PointerEvent.h.
Then you wouldn't need to add anything to Bindings.conf

update uuid of nsIInlineEventHandlers.

Would like to see a new patch.
Attachment #777589 - Flags: review?(bugs) → review-
Comment on attachment 776445 [details] [diff] [review]
Pointer Event Listeners detection

Hmm, it is not quite clear to me why we need the stuff in nsGlobalWindow.cpp.
Remove them?
For touch they were needed because Windows can dispatch only
gesture or touch events, not both.
I hope it doesn't have the same limitation with pointer events.

Please make sure to initialize various bool member variables.
Attachment #776445 - Flags: review?(bugs) → review-
Comment on attachment 777592 [details] [diff] [review]
Pointer Events Basic Tests

We will need some more tests where event dispatching goes via
DOMWindowUtils/Widget level code. But this is ok for the interface.

But, could you not use random. Perhaps use some pre-randomfied array of values?
That would make debugging later easier.
Attachment #777592 - Flags: review?(bugs) → review+
Attached patch Pointer Events Definition. part 1 (obsolete) — — Splinter Review
Fixed comments
Attachment #777589 - Attachment is obsolete: true
Attachment #786169 - Flags: review?(bugs)
Comment on attachment 786169 [details] [diff] [review]
Pointer Events Definition. part 1

># HG changeset patch
># Parent 05d3797276d3d8c7d10ddc66e95c99352c2e3cc9
># User Oleg Romashin <oleg.romashin@microsoft.com>
>
>Bug 822898 - Implement pointer events. Interface p1
>

>+NS_IMPL_ADDREF_INHERITED(PointerEvent, nsDOMMouseEvent)
>+NS_IMPL_RELEASE_INHERITED(PointerEvent, nsDOMMouseEvent)
>+
>+NS_INTERFACE_MAP_BEGIN(PointerEvent)
>+NS_INTERFACE_MAP_END_INHERITING(nsDOMMouseEvent)

I believe you can drop all these macros now if you remove
>+  NS_DECL_ISUPPORTS_INHERITED
>+
>+  // Forward to base class
>+  NS_FORWARD_TO_NSDOMMOUSEEVENT
from the .h


>+
>+//static
>+already_AddRefed<PointerEvent>
>+PointerEvent::Constructor(const mozilla::dom::GlobalObject& aGlobal,
>+                             const nsAString& aType,
>+                             const mozilla::dom::PointerEventInit& aParam,
>+                             mozilla::ErrorResult& aRv)
>+{
>+  nsCOMPtr<mozilla::dom::EventTarget> t = do_QueryInterface(aGlobal.Get());
>+  nsRefPtr<PointerEvent> e = new PointerEvent(t, nullptr, nullptr);
>+  bool trusted = e->Init(t);
>+  e->InitPointerEvent(aType, aParam.mBubbles, aParam.mCancelable,
>+                      aParam.mView, aParam.mDetail, aParam.mScreenX,
>+                      aParam.mScreenY, aParam.mClientX, aParam.mClientY,
>+                      aParam.mCtrlKey, aParam.mAltKey, aParam.mShiftKey,
>+                      aParam.mMetaKey, aParam.mButton, aParam.mRelatedTarget,
>+                      aParam.mPointerId, aParam.mWidth, aParam.mHeight,
>+                      aParam.mPressure, aParam.mTiltX, aParam.mTiltY,
>+                      aParam.mPointerType, aParam.mIsPrimary,
>+                      aRv);
>+
>+  static_cast<widget::PointerEvent*>(e->mEvent)->buttons = aParam.mButtons;
>+  e->SetTrusted(trusted);
>+  return e.forget();
>+}
>+PointerEvent::GetPointerType(nsAString& aPointerType)
>+{
>+  ConvertPointerTypeToString(static_cast<mozilla::widget::PointerEvent*>(mEvent)->inputSource, aPointerType);
is this overlong line?

>+}
>+
>+void
>+PointerEvent::InitPointerEvent(const nsAString& aType,
>+                               bool aCanBubble,
>+                               bool aCancelable,
>+                               nsIDOMWindow* aView,
>+                               int32_t aDetail,
>+                               int32_t aScreenX,
>+                               int32_t aScreenY,
>+                               int32_t aClientX,
>+                               int32_t aClientY,
>+                               bool aCtrlKey,
>+                               bool aAltKey,
>+                               bool aShiftKey,
>+                               bool aMetaKey,
>+                               uint16_t aButton,
>+                               EventTarget* aRelatedTarget,
>+                               int32_t aPointerId,
>+                               int32_t aWidth,
>+                               int32_t aHeight,
>+                               float aPressure,
>+                               int32_t aTiltX,
>+                               int32_t aTiltY,
>+                               const nsAString& aPointerType,
>+                               bool aIsPrimary,
>+                               mozilla::ErrorResult& aRv)
>+{
>+  aRv = nsDOMMouseEvent::InitMouseEvent(aType,
>+                                        aCanBubble,
>+                                        aCancelable,
>+                                        aView,
>+                                        aDetail,
>+                                        aScreenX,
>+                                        aScreenY,
>+                                        aClientX,
>+                                        aClientY,
>+                                        aCtrlKey,
>+                                        aAltKey,
>+                                        aShiftKey,
>+                                        aMetaKey,
>+                                        aButton,
>+                                        aRelatedTarget);
>+
You should return here if aRv.Failed()...
>+  mozilla::widget::PointerEvent* pointerEvent = static_cast<mozilla::widget::PointerEvent*>(mEvent);
>+  pointerEvent->pointerId = aPointerId;
>+  pointerEvent->width = aWidth;
>+  pointerEvent->height = aHeight;
>+  pointerEvent->pressure = aPressure;
>+  pointerEvent->tiltX = aTiltX;
>+  pointerEvent->tiltY = aTiltY;
>+  pointerEvent->inputSource = ConvertStringToPointerType(aPointerType);
>+  pointerEvent->isPrimary = aIsPrimary;
>+}
...but if you don't need to ever initialize these events (in C++), you could drop the whole
Init*Event method, and move the pointerEvent initialization to Constructor.


>+NS_NewPointerEvent(nsIDOMEvent** aInstancePtrResult,
>+                      mozilla::dom::EventTarget* aOwner,
>+                      nsPresContext* aPresContext,
>+                      class mozilla::widget::PointerEvent* aEvent);
Align params

>+++ b/dom/webidl/EventHandler.webidl
>@@ -121,16 +121,30 @@ interface GlobalEventHandlers {
>            [SetterThrows]
>            attribute EventHandler onsuspend;
>            [SetterThrows]
>            attribute EventHandler ontimeupdate;
>            [SetterThrows]
>            attribute EventHandler onvolumechange;
>            [SetterThrows]
>            attribute EventHandler onwaiting;
>+           [SetterThrows]
>+           attribute EventHandler onpointerdown;
>+           [SetterThrows]
>+           attribute EventHandler onpointerup;
>+           [SetterThrows]
>+           attribute EventHandler onpointermove;
>+           [SetterThrows]
>+           attribute EventHandler onpointerout;
>+           [SetterThrows]
>+           attribute EventHandler onpointerover;
>+           [SetterThrows]
>+           attribute EventHandler onpointerenter;
>+           [SetterThrows]
>+           attribute EventHandler onpointerleave;
I think we need PrefControlled for each pointerevent handler and for the interface too, so that
we can opt-in having pointer events activated, for now.

>+[Constructor(DOMString type, optional PointerEventInit eventInitDict)]
>+interface PointerEvent : MouseEvent
>+{
>+  readonly attribute long pointerId;
>+  readonly attribute long width;
>+  readonly attribute long height;
>+  readonly attribute float pressure;
>+  readonly attribute long tiltX;
>+  readonly attribute long tiltY;
>+  readonly attribute DOMString pointerType;
>+  readonly attribute boolean isPrimary;
>+
>+  [Throws]
>+  void initPointerEvent(DOMString typeArg,
>+                        boolean canBubbleArg,
>+                        boolean cancelableArg,
>+                        WindowProxy? viewArg,
>+                        long detailArg,
>+                        long screenXArg,
>+                        long screenYArg,
>+                        long clientXArg,
>+                        long clientYArg,
>+                        boolean ctrlKeyArg,
>+                        boolean altKeyArg,
>+                        boolean shiftKeyArg,
>+                        boolean metaKeyArg,
>+                        unsigned short buttonArg,
>+                        EventTarget? relatedTargetArg,
>+                        long pointerIdArg,
>+                        long widthArg,
>+                        long heightArg,
>+                        float presureArg,
>+                        long tiltXArg,
>+                        long tiltYArg,
>+                        DOMString pointerTypeArg,
>+                        boolean isPrimaryArg);
certainly remove initPointerEvent from the .idl. It is not there per spec.

Looking good. I wonder if I need to review this again...probably not.
Attachment #786169 - Flags: review?(bugs) → review+
Attached patch Pointer Events Definition. part 1 (obsolete) — — Splinter Review
Last comments addressed, added interface enabler by pref
dom.w3c_pointer_events.enabled
Attachment #786169 - Attachment is obsolete: true
Ok, one more iteration here, rebased to 0b4b1330fafa m-c
Basic interface definition, preference based
Attachment #831533 - Flags: review?(bugs)
Attachment #786330 - Attachment is obsolete: true
This patch is minimal part which allow to dispatch pointer events (up/down/move) via content EventDispatcher pipeline and convert WidgetPointerEvents properties into dom::PointerEvent structure.
Attachment #831534 - Flags: review?(bugs)
Attachment #776448 - Attachment is obsolete: true
It would be nice to get last two patches pushed in sooner, in order to avoid painful rebase-ing in future, also these two patches allow to create and dispatch basic pointer events and open doors for other spec parts implementation without huge patch queue in place
Attached patch Pointer Events Basic Tests (obsolete) — — Splinter Review
Small update to make test_all_synthetic_events.html test pass.
Attachment #777592 - Attachment is obsolete: true
Attachment #831644 - Flags: review?(bugs)
Comment on attachment 831644 [details] [diff] [review]
Pointer Events Basic Tests

Oh, sounds like we should define special Powers pref which enable PointerEvents while running dom/tests/mochitest/general/test_interfaces.html
Attachment #831644 - Attachment is obsolete: true
Attachment #831644 - Flags: review?(bugs)
Attachment #777592 - Attachment is obsolete: false
Attached patch Pointer Events Basic Tests (obsolete) — — Splinter Review
Use SpecialPowers in tests
Attachment #777592 - Attachment is obsolete: true
Attachment #831886 - Flags: review?
Attachment #831886 - Flags: review? → review?(bugs)
Comment on attachment 831886 [details] [diff] [review]
Pointer Events Basic Tests

could you add tests also for onfoo properties you're adding to EventHandler.webidl
Attachment #831886 - Flags: review?(bugs) → review-
Attachment #831533 - Flags: review?(bugs) → review+
Comment on attachment 831534 [details] [diff] [review]
b822898 p2 pointer events allow dispatch

No need for NS_ENSURE_TRUE(pointerEvent, NS_ERROR_OUT_OF_MEMORY);
new is infallible.
Attachment #831534 - Flags: review?(bugs) → review+
Attached patch Pointer Events Basic Tests (obsolete) — — Splinter Review
Added target.onfoo tests
Attachment #831886 - Attachment is obsolete: true
Attachment #8333990 - Flags: review?(bugs)
Comment on attachment 8333990 [details] [diff] [review]
Pointer Events Basic Tests

(We'll get more tests with touch-action.)
Attachment #8333990 - Flags: review?(bugs) → review+
Whiteboard: [leave-open]
Backed out in  https://hg.mozilla.org/integration/mozilla-inbound/rev/b159af3edf46 for mochitest-1 bustage on Windows: https://tbpl.mozilla.org/php/getParsedLog.php?id=30725617&tree=Mozilla-Inbound&full=1

15:07:45     INFO -  84701 INFO TEST-INFO | MEMORY STAT largestContiguousVMBlock after test: 589824000
15:07:45     INFO -  84702 INFO TEST-END | /tests/content/events/test/test_bug812744.html | finished in 67ms
15:07:45     INFO -  84703 INFO TEST-START | /tests/content/events/test/test_bug822898.html
15:07:45     INFO -  84704 ERROR TEST-UNEXPECTED-FAIL | /tests/content/events/test/test_bug822898.html | pointerdown property isn't performed
15:07:45     INFO -  84705 ERROR TEST-UNEXPECTED-FAIL | /tests/content/events/test/test_bug822898.html | pointerup property isn't performed
15:07:45     INFO -  84706 ERROR TEST-UNEXPECTED-FAIL | /tests/content/events/test/test_bug822898.html | pointermove property isn't performed
15:07:45     INFO -  84707 ERROR TEST-UNEXPECTED-FAIL | /tests/content/events/test/test_bug822898.html | pointerout property isn't performed
15:07:45     INFO -  84708 ERROR TEST-UNEXPECTED-FAIL | /tests/content/events/test/test_bug822898.html | pointerover property isn't performed
15:07:45     INFO -  84709 ERROR TEST-UNEXPECTED-FAIL | /tests/content/events/test/test_bug822898.html | pointerenter property isn't performed
15:07:45     INFO -  84710 ERROR TEST-UNEXPECTED-FAIL | /tests/content/events/test/test_bug822898.html | pointerleave property isn't performed
15:07:45     INFO -  84711 INFO TEST-PASS | /tests/content/events/test/test_bug822898.html | Correct detail
15:07:45     INFO -  84712 INFO TEST-PASS | /tests/content/events/test/test_bug822898.html | Correct screenX
15:07:45     INFO -  84713 INFO TEST-PASS | /tests/content/events/test/test_bug822898.html | Correct screenY
hmm this looks strange, looks like some problem with special powers in this case applying on .onfoo properties...
it works on Linux build, it works if I set "dom.w3c_pointer_events.enabled" -> true in all.js on windows https://tbpl.mozilla.org/?tree=Try&rev=083eb9504e0f

Cannot test windows build because it does not compile on my environment, so if anyone has any suggestions, please advice
The pref change will not be reflected to existing global objects. You will have to create a new global object (e.g. by inserting an iframe) to test the default-disabled interfaces or properties.
Attached patch Updated Pointer Events Basic Tests. (obsolete) — — Splinter Review
This looks much better
https://tbpl.mozilla.org/?tree=Try&rev=88042f6343bd
Attachment #8333990 - Attachment is obsolete: true
Attached patch Create Event problem workaround — — Splinter Review
Workaround for fixing this build failure on windows
https://tbpl.mozilla.org/php/getParsedLog.php?id=30843057&tree=Try#error0
Attachment #8335452 - Flags: review?(ehsan)
Attachment #8335452 - Flags: review?(ehsan) → review+
ctor tests moved into same file, so we have one tests file covered with pointer enabler preference, and fixing this problem
https://tbpl.mozilla.org/php/getParsedLog.php?id=30879131&tree=Try#error0
Load style and preference enabler changed in order to fix issues on B2G emulator
https://tbpl.mozilla.org/php/getParsedLog.php?id=30855965&tree=Mozilla-Inbound#error0
Attachment #8335097 - Attachment is obsolete: true
Attachment #8336521 - Flags: review?(bugs)
Comment on attachment 8336521 [details] [diff] [review]
Updated Pointer Events Basic Tests.

Try build with all tests, all platforms enabled:
https://tbpl.mozilla.org/?tree=Try&rev=1926b22e35e7
Attachment #8336521 - Flags: review?(bugs) → review+
Blocks: html5test
Depends on: 956644
Blocks: 960316
Added attributes to the navigator interface according to the http://www.w3.org/TR/pointerevents/#extensions-to-the-navigator-interface.

Intentionally omitted pointerEnabled attribute since it was removed recently (reference: https://www.w3.org/Bugs/Public/show_bug.cgi?id=22890).
Attachment #8362917 - Flags: review?(romaxa)
Comment on attachment 8362917 [details] [diff] [review]
Add maxTouchPoints attribute to the navigator interface

>+Navigator::MaxTouchPoints()
>+{
>+  nsCOMPtr<nsIWidget> widget = widget::WidgetUtils::DOMWindowToWidget(mWindow);

DOMWindowToWidget may return nullptr, I believe we don't want to have possibility crash here 

>+  return widget->GetMaxTouchPoints();
>+}
>+


> 
>+uint32_t nsBaseWidget::GetMaxTouchPoints() const
>+{
>+  return 0;

Do we have any specification on what should be default value here when MaxTouchPoints info retreival not supported by platform? 
should it be -1 - indicating as not implemented or 0?


>-  nsIWidget *self = static_cast<nsIWidget *>(aClosure);
>+  nsIWidget *self = static_cast<nsIWidget *>(aClosure);

do we really need this line change?
Attachment #8362917 - Flags: review?(romaxa) → review-
(In reply to Oleg Romashin (:romaxa) from comment #68)
> Comment on attachment 8362917 [details] [diff] [review]
> Add maxTouchPoints attribute to the navigator interface
> 
> >+Navigator::MaxTouchPoints()
> >+{
> >+  nsCOMPtr<nsIWidget> widget = widget::WidgetUtils::DOMWindowToWidget(mWindow);
> 
> DOMWindowToWidget may return nullptr, I believe we don't want to have
> possibility crash here 
> 
> >+  return widget->GetMaxTouchPoints();
> >+}
> >+
> 

Ok. In most places where DOMWindowToWidget is used we use assert that ensures that widget is not null. E.g. http://mxr.mozilla.org/mozilla-central/source/widget/xpwidgets/nsBaseFilePicker.cpp#134, http://mxr.mozilla.org/mozilla-central/source/dom/plugins/ipc/PluginHangUIParent.cpp#392. Should I use assert too or simple check for null is fine?

> > 
> >+uint32_t nsBaseWidget::GetMaxTouchPoints() const
> >+{
> >+  return 0;
> 
> Do we have any specification on what should be default value here when
> MaxTouchPoints info retreival not supported by platform? 
> should it be -1 - indicating as not implemented or 0?

There is no info about it in the spec so i just left 0 here. Is it Ok?

> >-  nsIWidget *self = static_cast<nsIWidget *>(aClosure);
> >+  nsIWidget *self = static_cast<nsIWidget *>(aClosure);
> 
> do we really need this line change?

Nope, will fix this nit.
> Ok. In most places where DOMWindowToWidget is used we use assert that
> ensures that widget is not null. E.g.

in addition we may want to do same as Navigator::AddIdleObserver
  if (!widget) {
    aRv.Throw(NS_ERROR_UNEXPECTED);
    return;
  }
Addressed core review comments.
Attachment #8362917 - Attachment is obsolete: true
Attachment #8365025 - Flags: review?(romaxa)
Attachment #8365025 - Flags: review?(romaxa)
Attachment #8365025 - Flags: review?(bugs)
Attachment #8365025 - Flags: review+
Comment on attachment 8365025 [details] [diff] [review]
Add maxTouchPoints attribute to the navigator interface. v2.

Update IID of nsIWidget.

maxTouchPoints is not supposed to Throw. Could we perhaps just return 0 in 
case we can't access widget.

How slow or fast 
MetroWidget::GetMaxTouchPoints() is ?
Attachment #8365025 - Flags: review?(bugs) → review+
Addressed review comments. Also carrying r+.
Attachment #8365025 - Attachment is obsolete: true
Attachment #8366612 - Flags: review+
Depends on: 850630
Depends on: 966961
Depends on: 967796
Depends on: 968148
With pointer events still a work-in-progress (and as a newbie to Mozilla development), I hope its not bad form to submit a bugfix for one of the comitted patches here rather than opening a new bug...

While hacking on a GTK implementation for #956644, I noticed issues reporting pen pressure. It looks like the return type of PointerEvent::Pressure was accidentally set to 'int32_t' instead of the expected 'float'.
Attachment #8371670 - Flags: review?(bugs)
Attachment #8371670 - Flags: review?(bugs) → review+
A test for that would be good.
Attachment #764023 - Attachment is obsolete: true
Attachment #764021 - Attachment is obsolete: true
Attachment #776451 - Attachment is obsolete: true
Attachment #776445 - Attachment is obsolete: true
Depends on: 970964
Depends on: 977695
Depends on: 979497
Depends on: 996493
Depends on: 987078
Depends on: 985511
See Also: → 1015600
bug 1015600 suggests we're shipping a somewhat half-baked pointer events implementation, which causes site problems..?
Recent (and sad) news is that Chrome team decided not to support Pointer Events:

https://code.google.com/p/chromium/issues/detail?id=162757#c64
Depends on: 1066237
Depends on: 1071554
Depends on: 1071553
https://bugzilla.mozilla.org/show_bug.cgi?id=1071554 and https://bugzilla.mozilla.org/show_bug.cgi?id=1071553 are about the CSS pointer-events property...which has nothing to do with the Pointer Events spec. Please remove the dependency.
No longer depends on: 1071554
No longer depends on: 1071553
Thanks, Patrick. I thought I was going crazy here...
no worries, the naming clash is confusing to many :)
Depends on: 1122211
Depends on: 1137555
Depends on: 1149861
Hello, as I couldn't find something to test pen pressure, I did this https://jsfiddle.net/kgkf5kfx/

When using a digitizer pen on Windows, it should display a number between 0 and 1000 when scribbling on the result window. It works correctly with IE11 but in FF (with flag enabled) it's always 0 or 500.
(In reply to kungfoobar from comment #89)
> When using a digitizer pen on Windows, it should display a number between 0
> and 1000 when scribbling on the result window. It works correctly with IE11
> but in FF (with flag enabled) it's always 0 or 500.
There is bug 1031362 about it.
Depends on: 1153130
Depends on: 1153135
Depends on: 1159206
Depends on: 1162009
Depends on: 1297248
Depends on: 1303704
Depends on: 1403055
Depends on: 1404255
Depends on: 1405909
Priority: -- → P3
Keywords: leave-open, meta
Whiteboard: [leave-open]
Summary: Implement pointer events → [meta] Implement pointer events
Alias: pointerevent
Depends on: 1521082
Depends on: 1524251
Type: defect → enhancement
Depends on: 1539497
Depends on: 1546043
Depends on: 1550461
Depends on: 1550462
Depends on: 1556694
Depends on: 1591250
Depends on: 1520785
Depends on: 1036817
Depends on: 1610659
Depends on: 1284758
Depends on: 1613410
Depends on: 1614347
Depends on: 1556240
No longer depends on: 1501744
Depends on: 1644307
No longer depends on: 1546043
Depends on: 1669729
Depends on: 1666851
Depends on: 1655239
Depends on: 1669673
Depends on: 1671657
Depends on: 1675846
Depends on: 1686390
No longer depends on: 1686390
No longer depends on: 1608938
Depends on: 1509710
No longer depends on: 1583519
No longer depends on: 1487509
No longer depends on: 1583480
No longer depends on: 1597664
Depends on: 1631377
No longer depends on: 1437102
No longer depends on: 1449660
No longer depends on: 1613410
Blocks: pointerevent
No longer depends on: 1556694
No longer depends on: 1324956
No longer depends on: 1305346
Depends on: 1702175
Depends on: 1695574
No longer depends on: 1284758
No longer depends on: 1631377
No longer depends on: 1315250
No longer depends on: 1449990
No longer depends on: 1484186
No longer depends on: 1524251
No longer depends on: 1644307
No longer depends on: 1504210
No longer depends on: 1556240
No longer depends on: 1610659
No longer depends on: 1666851
No longer depends on: 1550461
Depends on: 1721628

The bug assignee didn't login in Bugzilla in the last 7 months.
:hsinyi, could you have a look please?
For more information, please visit auto_nag documentation.

Assignee: oleg.romashin → nobody
Flags: needinfo?(htsai)
Flags: needinfo?(htsai)
Severity: normal → S3
No longer depends on: 1550462
No longer depends on: 1695574
No longer depends on: 1675846
No longer depends on: 1609529
No longer depends on: 1529904
Alias: pointerevent

Close this as we have shipped pointer events for a while. Pointer events bugs are now tracked in bug 1705364.

Status: NEW → RESOLVED
Closed: 1 year ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.