1303704 - [Pointer Event] Implement prevent default behavior of pointerdown

Status: RESOLVED FIXED

(Core :: DOM: Events, defect)

Description

[Ming-Chou Shih [:stone]]

Pointer events spec v2 [1] specifies that calling preventDefault in the listener of pointerdown will prevent UA firing subsequent compatibility mouse events.

[1] https://w3c.github.io/pointerevents/#pointer-event-types

Comment 1

[Ming-Chou Shih [:stone]]

Attachment: [Pointer Event] Implement prevent default behavior of pointerdown

Comment 2

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 8793202 [details] [diff] [review]
[Pointer Event] Implement prevent default behavior of pointerdown

>diff --git a/layout/base/nsIPresShell.h b/layout/base/nsIPresShell.h
>--- a/layout/base/nsIPresShell.h
>+++ b/layout/base/nsIPresShell.h
>@@ -1288,18 +1288,20 @@ public:
>   // Additionally keep information about primaryState of pointer event.
>   static nsClassHashtable<nsUint32HashKey, PointerCaptureInfo>* gPointerCaptureList;
> 
>   struct PointerInfo
>   {
>     bool      mActiveState;
>     uint16_t  mPointerType;
>     bool      mPrimaryState;
>+    bool      mDefaultPreventedByContent;
>     PointerInfo(bool aActiveState, uint16_t aPointerType, bool aPrimaryState) :
>-      mActiveState(aActiveState), mPointerType(aPointerType), mPrimaryState(aPrimaryState) {}
>+      mActiveState(aActiveState), mPointerType(aPointerType),
>+      mPrimaryState(aPrimaryState), mDefaultPreventedByContent(false) {}

I hope that you'd rewrite this constructor with modern our style such as:

PointerInfo(bool...)
  : mActiveState(aActiveState)
  , mPointerType(aPointerType)
  , ...
{
}

> nsresult
> PresShell::DispatchEventToDOM(WidgetEvent* aEvent,
>                               nsEventStatus* aStatus,
>                               nsPresShellEventCB* aEventCB)
> {
>+  // Prevent dispatching mouse events to DOM if user preventDefault on
>+  // pointerdown
>+  if (sPointerEventEnabled) {
>+    WidgetMouseEvent* mouseEvent = aEvent->AsMouseEvent();
>+    if (mouseEvent && mouseEvent->mFlags.mDefaultPreventedByContent) {
>+      return NS_OK;
>+    }
>+  }

I don't understand why you check only mDefaultPreventedByContent.

According to the document, when pointerdown is consumed by web contents, mousedown event should be fired in neither content nor chrome.

However, there is a problem.  When it's consumed by chrome, how should we behave?  At least, following mousedown event shouldn't be fired in chrome because it's odd for apps running in chrome if mousedown event is fired after a call of preventDefault() of pointerdown.

Smaug, do you have any idea? (I'm not sure if the order of pointer events and mouse events are reverted in the system group like wheel event, though.)

>+/*
>+ * This function handles the preventDefault behavior of pointerdown. When user
>+ * preventDefault on pointerdown, We have to prevent sebsequent mouse events
>+ * (except mouse transition events) and their default behaviors.
>+ *
>+ * We add mDefaultPreventedByContent flag in PointerInfo to represent the active
>+ * pointer's default behavior has been prevented. It's set to true when
>+ * user preventDefault on pointerdown
>+ */
>+static void
>+PostHandlePointerEventsPreventDefault(WidgetPointerEvent* aPointerEvent,
>+                                      WidgetGUIEvent* aMouseOrTouchEvent)
>+{
>+  if (aPointerEvent->mIsPrimary && aPointerEvent->mMessage == ePointerDown &&
>+      aPointerEvent->mFlags.mDefaultPreventedByContent) {

Please use "early return style".  So, if this condition is false, exit from this function. Then, you can reduce indent level of following code.

And you can use WidgetEvent::DefaultPrevented() and/or WidgetEvent::DefaultPreventedByContent().

>+    nsIPresShell::PointerInfo* pointerInfo = nullptr;
>+    nsIPresShell::gActivePointersIds->Get(aPointerEvent->pointerId, &pointerInfo);

nit: not an issue of this patch, though, I feel odd a member in an interface has "g" prefix. It should be "s"...

>+    if (pointerInfo) {

Same here, you can use early return style.

>+      aMouseOrTouchEvent->mFlags.mDefaultPrevented = true;
>+      aMouseOrTouchEvent->mFlags.mDefaultPreventedByContent = true;

Why don't you use WidgetEvent::PrentDefault()? And I think that you should set only mDefaultPrevented to true when aPointerEvent is consumed only by chrome, no?

>+      pointerInfo->mDefaultPreventedByContent = true;

So, we might need to store if it's prevented by content or chrome...

>+/*
>+ * This function handles the case when user had called preventDefault on the
>+ * active pointer. We set flag mDefaultPrevented and mDefaultPreventedByContent
>+ * to stop default behaviors and prevent firing the subsequent mouse events
>+ * (except mouse transition events)
>+ */
>+static void
>+PreHandlePointerEventsPreventDefault(WidgetPointerEvent* aPointerEvent,
>+                                     WidgetGUIEvent* aMouseOrTouchEvent)
>+{
>+  if (aPointerEvent->mIsPrimary && aPointerEvent->mMessage != ePointerDown) {

Same, you should use "early return style" to reduce redundant indent.

>+    nsIPresShell::PointerInfo* pointerInfo = nullptr;
>+    nsIPresShell::gActivePointersIds->Get(aPointerEvent->pointerId, &pointerInfo);
>+    if (pointerInfo && pointerInfo->mDefaultPreventedByContent) {

Same here. And you can use WidgetEvent::DefaultPrevented() and/or WidgetEvent::DefaultPreventedByContent().

>+      aMouseOrTouchEvent->mFlags.mDefaultPrevented = true;
>+      aMouseOrTouchEvent->mFlags.mDefaultPreventedByContent = true;

Why don't you use WidgetEvent::PreventDefault()? And I think that if aPointerEvent is consumed only by chrome, only mDefaultPrevented should be true.


>+      if (aPointerEvent->mMessage == ePointerUp) {
>+        pointerInfo->mDefaultPreventedByContent = false;
>+      }

I think that you can keep this |if| block without early return style because looks like other cases might need additional code in the future and enough small block.

BTW, I don't understand why you need to clear mDefaultPreventedByContent of the PointerInfo only for ePointerUp. You need to explain with comment.



I guess, for example, pointerdown is fired following order better:

1. pointerdown in the default group.
2. mousedown in the default group.
3. mousedown in the system group.
4. pointerdown in the system group.

See this document for the order of wheel event with legacy events: https://developer.mozilla.org/en-US/docs/Web/Events/wheel#The_event_order_with_legacy_mouse_scroll_events

How pointer events fired in current implementation?

Comment 3

[Ming-Chou Shih [:stone]]

(In reply to Masayuki Nakano [:masayuki] (Mozilla Japan) from comment #2)
> I don't understand why you check only mDefaultPreventedByContent.
You are right, prevent by chrome should be taken into consideration. Although the behaviors of prevent default by chrome are unclear to me and would like to learn more from you and Smaug.

> BTW, I don't understand why you need to clear mDefaultPreventedByContent of
> the PointerInfo only for ePointerUp. You need to explain with comment.
I think prevent default on pointerdown cancels firing compatible mouse events for the following pointer events until pointerup. For touch, the pointer info will be different for different pointerdown / pointerup session. But for mouse, the pointer info will be the same instance. So I clear the prevent default flag when pointerup. Is it make sense?

> How pointer events fired in current implementation?
Current implementation generates pointer events from mouse events or touch events in [1]. The order is
pointer events in the default group
pointer events in the system group
mouse or touch events in the default group
mouse or touch events in the system group

[1] http://searchfox.org/mozilla-central/source/layout/base/nsPresShell.cpp#7322

Comment 4

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

(In reply to Ming-Chou Shih [:stone] from comment #3)
> > How pointer events fired in current implementation?
> Current implementation generates pointer events from mouse events or touch
> events in [1]. The order is
> pointer events in the default group
> pointer events in the system group
> mouse or touch events in the default group
> mouse or touch events in the system group

FWIW, I think that is what I'd expect to happen.
mouse events are less legacy than our DOMMouseScroll/MozPixelScroll.

But, if dispatching mouse events right after default group of pointer events makes implementation simpler, go for it.


(I think I'll need to read the patch here.)

Comment 5

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to Ming-Chou Shih [:stone] from comment #3)
> (In reply to Masayuki Nakano [:masayuki] (Mozilla Japan) from comment #2)
> > I don't understand why you check only mDefaultPreventedByContent.
> You are right, prevent by chrome should be taken into consideration.
> Although the behaviors of prevent default by chrome are unclear to me and
> would like to learn more from you and Smaug.

When chrome script or a event listener implemented by C++ calls Event.preventDefault(), it's consumed for preventing another event listener to handle the event.  However, the fact shouldn't be exposed to web apps because Event.defaultPrevented should be set to true only when Event.preventDefault() is called in web apps.  Therefore, we have two bool members to indicated that preventDefault() was called in which context.

> > BTW, I don't understand why you need to clear mDefaultPreventedByContent of
> > the PointerInfo only for ePointerUp. You need to explain with comment.
> I think prevent default on pointerdown cancels firing compatible mouse
> events for the following pointer events until pointerup. For touch, the
> pointer info will be different for different pointerdown / pointerup
> session. But for mouse, the pointer info will be the same instance. So I
> clear the prevent default flag when pointerup. Is it make sense?

Okay, I read "11.2 Mapping for devices that support hover".
https://w3c.github.io/pointerevents/#dfn-tracking-the-effective-position-of-the-legacy-mouse-pointer

Looks like your patch implements the behavior correctly.  However, I don't like the name.  How about |mPreventMouseEvent|? That matches the name of this section.

I wonder, is really eMouseUp event guaranteed to be dispatched after eMouseDown in any OSes? I guess that it's almost safe because we manages mouse capture.  Is this creating another management code?

> > How pointer events fired in current implementation?
> Current implementation generates pointer events from mouse events or touch
> events in [1]. The order is
> pointer events in the default group
> pointer events in the system group
> mouse or touch events in the default group
> mouse or touch events in the system group
> 
> [1]
> http://searchfox.org/mozilla-central/source/layout/base/nsPresShell.cpp#7322

(In reply to Olli Pettay [:smaug] from comment #4)
> (In reply to Ming-Chou Shih [:stone] from comment #3)
> > > How pointer events fired in current implementation?
> > Current implementation generates pointer events from mouse events or touch
> > events in [1]. The order is
> > pointer events in the default group
> > pointer events in the system group
> > mouse or touch events in the default group
> > mouse or touch events in the system group
> 
> FWIW, I think that is what I'd expect to happen.
> mouse events are less legacy than our DOMMouseScroll/MozPixelScroll.
> 
> But, if dispatching mouse events right after default group of pointer events
> makes implementation simpler, go for it.

I agree.

Then, mouse events should be fired from nsPresShellEventCB::HandleEvent() like eWheel case.
https://dxr.mozilla.org/mozilla-central/source/layout/base/nsPresShell.cpp#491,502-516
When it receives ePointer*, it means that the event is fired only in the default group. Any default action should be handled in nsIFrame::HandleEvent() (called at the end of the method) or listeners in the system group.  So, if this change would cause some regressions, the default event handler must be in the default group.

Comment 6

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

Hmm, what default action should happen in nsIFrame::HandleEvent? I consider that legacy and 
system group handlers, PostHandleEvent in EventTarget and EventStateManager are the normal places for default handling.

Also, one option is to pass explicitly some other callback to EventDispatcher::Dispatch than
nsPresShellEventCB, if for some reason we for example don't have PresShell at all.


One thing I don't like with dispatching mouse events after default group of pointer events is that chrome code gets then system group listeners called in wrong order - first mouse then pointer, when it should be first pointer then mouse.

Comment 7

[Ming-Chou Shih [:stone]]

(In reply to Masayuki Nakano [:masayuki] (Mozilla Japan) from comment #2)
> According to the document, when pointerdown is consumed by web contents,
> mousedown event should be fired in neither content nor chrome.
> 
> However, there is a problem.  When it's consumed by chrome, how should we
> behave?  At least, following mousedown event shouldn't be fired in chrome
> because it's odd for apps running in chrome if mousedown event is fired
> after a call of preventDefault() of pointerdown.

Should we fire following mouse events to content when pointerdown is consumed by chrome?

Comment 8

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to Olli Pettay [:smaug] from comment #6)
> Hmm, what default action should happen in nsIFrame::HandleEvent?

IIRC, some elements implements default action in it.

> Also, one option is to pass explicitly some other callback to
> EventDispatcher::Dispatch than nsPresShellEventCB, if for some reason
> we for example don't have PresShell at all.

Is that possible scenario with PointerEvent?

> One thing I don't like with dispatching mouse events after default group of
> pointer events is that chrome code gets then system group listeners called
> in wrong order - first mouse then pointer, when it should be first pointer
> then mouse.

Yeah, with my idea, the order is reversed only in the system group.  However, you said when I implement wheel event, chrome handles should be able to check if the legacy events are consumed.

If we use same order even in the system group, should prevent to dispatch legacy mouse events when preceding pointerdown event is consumed in chrome? I don't think so because content never can listen mousedown event if there is such handler. On the other hand, if it's fired in content even if pointerdown is consumed by chrome, the state management becomes complicated and both pointerdown event in chrome and mousedown event in content could be handled. How do you think about this issue?

(In reply to Ming-Chou Shih [:stone] from comment #7)
> (In reply to Masayuki Nakano [:masayuki] (Mozilla Japan) from comment #2)
> > According to the document, when pointerdown is consumed by web contents,
> > mousedown event should be fired in neither content nor chrome.
> > 
> > However, there is a problem.  When it's consumed by chrome, how should we
> > behave?  At least, following mousedown event shouldn't be fired in chrome
> > because it's odd for apps running in chrome if mousedown event is fired
> > after a call of preventDefault() of pointerdown.
> 
> Should we fire following mouse events to content when pointerdown is
> consumed by chrome?

Difficult issue. See above paragraph of this comment.

Comment 9

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

(In reply to Masayuki Nakano [:masayuki] (Mozilla Japan) from comment #8)
> Yeah, with my idea, the order is reversed only in the system group. 
> However, you said when I implement wheel event, chrome handles should be
> able to check if the legacy events are consumed.
> 
> If we use same order even in the system group, should prevent to dispatch
> legacy mouse events when preceding pointerdown event is consumed in chrome?
> I don't think so because content never can listen mousedown event if there
> is such handler. On the other hand, if it's fired in content even if
> pointerdown is consumed by chrome, the state management becomes complicated
> and both pointerdown event in chrome and mousedown event in content could be
> handled. How do you think about this issue?
This is tricky yes. But as of now, chrome uses mouseevents only and that is where default handling happens.
 

>Yeah, with my idea, the order is reversed only in the system group.  However, you said when I implement wheel event, chrome handles >should be able to check if the legacy events are consumed.
Right, and here the legacy are mouse events,  which is the stuff that chrome uses, at least for now.
And we don't expect to be able to get rid of mouse events, so chrome using them is totally fine.

Comment 10

[Ming-Chou Shih [:stone]]

(In reply to Olli Pettay [:smaug] from comment #9)
> This is tricky yes. But as of now, chrome uses mouseevents only and that is
> where default handling happens.

Considering preventDefault on pointerdown suppresses firing subsequent mouse events, I think we shouldn't suppress firing mouse events to system group handlers / chrome because we still need those mouse events to trigger default behaviors. Or we have to handling pointer events in all handlers where we implemented the default behaviors by handling mouse events. Is my understanding correct?

Comment 11

[Ming-Chou Shih [:stone]]

Attachment: Bug 1303704 - [Pointer Event] Implement prevent default behavior of pointerdown (V3)

Comment 12

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

(In reply to Ming-Chou Shih [:stone] from comment #10)
> (In reply to Olli Pettay [:smaug] from comment #9)
> > This is tricky yes. But as of now, chrome uses mouseevents only and that is
> > where default handling happens.
> 
> Considering preventDefault on pointerdown suppresses firing subsequent mouse
> events, I think we shouldn't suppress firing mouse events to system group
> handlers / chrome because we still need those mouse events to trigger
> default behaviors. 
Why? If you don't have mouse events (because pointer events have been cancelled), why should there be any default
handling?

Comment 13

[Ming-Chou Shih [:stone]]

For example, when user click on link, a listener cancels the pointerdown event to suppress the following mouse events, should be trigger link in this case? I'm not quite sure if the prevent default on pointerdown should 'only' suppress compatible mouse events or it also includes stop default behaviors related to mouse events.

Comment 14

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

Why should the link the be triggered if there is no mouse event to trigger the link.

Please also test blink and Edge, the spec isn't actually super clear here.
But, default handling is bound to click event usually, and if there is no mousedown (or some key events), there shouldn't be click either.

Comment 15

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

oh, but the spec is odd here.

In a note it says
"In user agents that support firing click and/or contextmenu, calling preventDefault during a pointer event typically does not have an effect on whether click and/or contextmenu are fired or not. Because they are not compatibility mouse events, user agents typically fire click and contextmenu for all pointing devices, including pointers that are not primary pointers."

Comment 16

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

ah, right, I misremembered. cancelling mousedown/up doesn't prevent click.

Comment 17

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

... so, click should still be dispatched, since that isn't compatibility event.

Comment 18

[Ming-Chou Shih [:stone]]

Attachment: Bug 1303704 Part1: [Pointer Event] Implement prevent default behavior of pointerdown.

Review commit: https://reviewboard.mozilla.org/r/85726/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/85726/

Comment 19

[Ming-Chou Shih [:stone]]

Attachment: Bug 1303704 Part2: [Pointer Event] Enable prevent default on pointerdown related test cases.

Review commit: https://reviewboard.mozilla.org/r/85728/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/85728/

Comment 20

[Ming-Chou Shih [:stone]]

The part1 patch handles the prevent default behavior of pointerdown. It does not support prevent default by chrome because now we handling default behaviors with mouse events and we don't expect to be able to get rid of mouse events. So I only add PointerInfo::mPreventMouseEventByContent for prevent default by content. Considering we suppress the mouse events and the default behaviors caused by them, so I check the flag in the begining of PresShell::DispatchEventToDOM to avoid creating the event target chain.

Comment 21

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to Olli Pettay [:smaug] from comment #9)
> (In reply to Masayuki Nakano [:masayuki] (Mozilla Japan) from comment #8)
> > Yeah, with my idea, the order is reversed only in the system group. 
> > However, you said when I implement wheel event, chrome handles should be
> > able to check if the legacy events are consumed.
> > 
> > If we use same order even in the system group, should prevent to dispatch
> > legacy mouse events when preceding pointerdown event is consumed in chrome?
> > I don't think so because content never can listen mousedown event if there
> > is such handler. On the other hand, if it's fired in content even if
> > pointerdown is consumed by chrome, the state management becomes complicated
> > and both pointerdown event in chrome and mousedown event in content could be
> > handled. How do you think about this issue?
> This is tricky yes. But as of now, chrome uses mouseevents only and that is
> where default handling happens.
>  
> 
> >Yeah, with my idea, the order is reversed only in the system group.  However, you said when I implement wheel event, chrome handles >should be able to check if the legacy events are consumed.
> Right, and here the legacy are mouse events,  which is the stuff that chrome
> uses, at least for now.
> And we don't expect to be able to get rid of mouse events, so chrome using
> them is totally fine.

My point is NOT what chrome should use pointer events. I worry about that when it does make sense to use pointer events in chrome for supporting all pointing device types, chrome will need to call preventDefault() in the future. Then, what should happen for mouse events? I think there are 3 possible ideas:

1. mouse events shouldn't be canceled by preventDefault() by chrome
2. mouse events should be canceled with a call of preventDefault() even by chrome
3. mouse events should be canceled only on the nodes in chrome and/or in the system group

Perhaps, #3 makes sense, but the implementation would be complicated.

Ming-Chou Shih's patch takes #1. Is it okay for you?

Comment 22

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 8800964 [details]
Bug 1303704 Part2: [Pointer Event] Enable prevent default on pointerdown related test cases.

https://reviewboard.mozilla.org/r/85728/#review84978

Comment 23

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

But if chrome is effectively doing default handling using pointer events, web page calling prevent default on mouse events can't prevent that anymore. So how could chrome use pointer events for default handling?
Perhaps we should even add an assertion that chrome code shouldn't call preventDefault() on pointer events?

Comment 24

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to Olli Pettay [:smaug] from comment #23)
> But if chrome is effectively doing default handling using pointer events,
> web page calling prevent default on mouse events can't prevent that anymore.
> So how could chrome use pointer events for default handling?

Hmm, it's a good point.

> Perhaps we should even add an assertion that chrome code shouldn't call
> preventDefault() on pointer events?

I agree. Or shouldn't we stop dispatching pointer events in chrome and in the system group? I guess that some addons will try to use PointerEvents.  How do you think?

Comment 25

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

Interesting idea to not have system group handling for pointer events. That might be useful. And still assertion for chrome calling preventDefault. Perhaps start with assertion?
But, whatever we do there, should go to some other bug.

Comment 26

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Thanks. I'll review the patch when my brain becomes clear because I got a cold. Sorry for the delay.

Comment 27

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Sorry for the delay to review. I'll try to get a chance to review it this weekend.  If I cannot, I'll review it on next Monday.

Comment 28

[Ming-Chou Shih [:stone]]

(In reply to Masayuki Nakano [:masayuki] (Mozilla Japan) from comment #27)
> Sorry for the delay to review. I'll try to get a chance to review it this
> weekend.  If I cannot, I'll review it on next Monday.

Take your time and many thanks for the review.

Comment 29

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 8800963 [details]
Bug 1303704 Part1: [Pointer Event] Implement prevent default behavior of pointerdown.

https://reviewboard.mozilla.org/r/85726/#review88792

In conclusion, chrome should not call peventDefault() of pointer events nor handle them.

Therefore, WidgetEvent::PreventDefault() should have:
MOZ_RELEASE_ASSERT(!aCalledByDefaultHandler || mMessage != ePointerDown);
for now.

After fixing this bug, please file a bug to stop dispatching pointer events in the system group. It can be fixed easy (just skip in EventDispatcher).

::: layout/base/nsPresShell.cpp:6842
(Diff revision 1)
> +PostHandlePointerEventsPreventDefault(WidgetPointerEvent* aPointerEvent,
> +                                      WidgetGUIEvent* aMouseOrTouchEvent)
> +{
> +  if (!aPointerEvent->mIsPrimary || aPointerEvent->mMessage != ePointerDown ||
> +      !aPointerEvent->DefaultPreventedByContent()) {
> +    return;
> +  }
> +  nsIPresShell::PointerInfo* pointerInfo = nullptr;
> +  if (!sActivePointersIds->Get(aPointerEvent->pointerId, &pointerInfo) ||

Hmm, if there is no proper PointerInfo for new active pointer, don't you need to create it instead of giving up?

Note that mouse events may be synthesized by 3rd party's utils which might not cause eMouseEnterIntoWidget　event first.　So, I think that PresShell::UpdateActivePointerState() doesn't manage the state enough.
https://dxr.mozilla.org/mozilla-central/rev/e3279760cd977aac30bd9e8032d3ee71f55d2a67/layout/base/nsPresShell.cpp#6541

::: layout/base/nsPresShell.cpp:6872
(Diff revision 1)
> +PreHandlePointerEventsPreventDefault(WidgetPointerEvent* aPointerEvent,
> +                                     WidgetGUIEvent* aMouseOrTouchEvent)
> +{
> +  if (!aPointerEvent->mIsPrimary || aPointerEvent->mMessage == ePointerDown) {
> +    return;
> +  }
> +  nsIPresShell::PointerInfo* pointerInfo = nullptr;
> +  if (!sActivePointersIds->Get(aPointerEvent->pointerId, &pointerInfo) ||

I guess that this is fine different from PostHandlePointerEventsPreventDefault() because we don't handling pointer events for an active pointer in such case. So, nothing to do here.

I hope you add a comment here for explaining such reason.

::: layout/base/nsPresShell.cpp:6890
(Diff revision 1)
>  DispatchPointerFromMouseOrTouch(PresShell* aShell,
>                                  nsIFrame* aFrame,
>                                  WidgetGUIEvent* aEvent,
>                                  bool aDontRetargetEvents,
>                                  nsEventStatus* aStatus,

I wonder, cannot the caller stop dispatching mouse event if pointerdown is consumed, instead of DispatchEventToDOM()? Because PresShell::HandleEvent() and other helper methods do a lot after this call even if the event won't be dispatched.　But I think that this is not a scope of this bug, see below.

::: layout/base/nsPresShell.cpp:8389
(Diff revision 1)
>                                nsEventStatus* aStatus,
>                                nsPresShellEventCB* aEventCB)
>  {
> +  // Prevent dispatching mouse events to DOM if content preventDefault on
> +  // pointerdown
> +  if (sPointerEventEnabled && aEvent->mClass == eMouseEventClass &&

I think that instead of checking event class, you need to check event message. Anyway, you should create a method to WidgetMouseEvent, e.g., IsAllowedToDispatch() which checks the event message and defaultPrevented.

Additionally, this is not a scope of this bug, I feel odd that such stateful event dispatcher is in PresShell. It should be in EventStateManager or an independent class which owned by ESM because it's difficult to understand the code if it's included in a big class like PresShell. Finally, DispatchPointerFromMouseOrTouch() should be called by EventStateManager::PreHandleEvent(), isn't it? That must be the cause of the caller of DispatchPointerFromMouseOrTouch() not returning even if pointerdown is consumed.

Comment 30

[Ming-Chou Shih [:stone]]

Comment on attachment 8800963 [details]
Bug 1303704 Part1: [Pointer Event] Implement prevent default behavior of pointerdown.

https://reviewboard.mozilla.org/r/85726/#review88792

> Hmm, if there is no proper PointerInfo for new active pointer, don't you need to create it instead of giving up?
> 
> Note that mouse events may be synthesized by 3rd party's utils which might not cause eMouseEnterIntoWidget　event first.　So, I think that PresShell::UpdateActivePointerState() doesn't manage the state enough.
> https://dxr.mozilla.org/mozilla-central/rev/e3279760cd977aac30bd9e8032d3ee71f55d2a67/layout/base/nsPresShell.cpp#6541

I think there must be an active pointer info in sActivePointersIds in this case. It's becuase when generating and handling pointerdown, we add pointer info for the active pointer then this function is invoked. I'd add some comments and an assertion to ensure it if it's ok for you.

About "mouse events may be synthesized by 3rd party's utils", could you kindly give me some hints about them?

> I guess that this is fine different from PostHandlePointerEventsPreventDefault() because we don't handling pointer events for an active pointer in such case. So, nothing to do here.
> 
> I hope you add a comment here for explaining such reason.

Similar to PostHandlePointerEventsPreventDefault but we invoke it before handling pointer events except pointerdown. The active pointer info will be removed after handling the event. So I'd add some comments and an asserton to ensure it.

> I wonder, cannot the caller stop dispatching mouse event if pointerdown is consumed, instead of DispatchEventToDOM()? Because PresShell::HandleEvent() and other helper methods do a lot after this call even if the event won't be dispatched.　But I think that this is not a scope of this bug, see below.

This is because I thought the pointer events should trigger everything that mouse events triggered and preventDefault on pointerdown only stop dispatching mouse events to content. Is my understanding correct?

Also, spec [1] says preventDefault during a pointer event typically does not have an effect on whether click and/or contextmenu are fired or not. And we fire click event in [2]. So I only stop dispatching mouse events to content.
[1] https://w3c.github.io/pointerevents/#compatibility-mapping-with-mouse-events
[2] http://searchfox.org/mozilla-central/source/dom/events/EventStateManager.cpp#3157

If it's ok for you, I'd create another bug to track this part and handle it.

Comment 31

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 8800963 [details]
Bug 1303704 Part1: [Pointer Event] Implement prevent default behavior of pointerdown.

https://reviewboard.mozilla.org/r/85726/#review88792

> I think there must be an active pointer info in sActivePointersIds in this case. It's becuase when generating and handling pointerdown, we add pointer info for the active pointer then this function is invoked. I'd add some comments and an assertion to ensure it if it's ok for you.
> 
> About "mouse events may be synthesized by 3rd party's utils", could you kindly give me some hints about them?

Ah, I see.
https://dxr.mozilla.org/mozilla-central/rev/aea5b4c3d165dcde027b3b6551b146a56748e9be/layout/base/nsPresShell.cpp#6554,6557-6559

Then, using MOZ_ASSERT() is fine.

Note that at least on Windows, another application can post/send any message to other application. Therefore, we need to assume any unexpected input event order.

> This is because I thought the pointer events should trigger everything that mouse events triggered and preventDefault on pointerdown only stop dispatching mouse events to content. Is my understanding correct?
> 
> Also, spec [1] says preventDefault during a pointer event typically does not have an effect on whether click and/or contextmenu are fired or not. And we fire click event in [2]. So I only stop dispatching mouse events to content.
> [1] https://w3c.github.io/pointerevents/#compatibility-mapping-with-mouse-events
> [2] http://searchfox.org/mozilla-central/source/dom/events/EventStateManager.cpp#3157

Ah, I understand. So, as I said, we should move PointerEvent implementation into EventStateManager (even if it's implemented as separated class, it should be managed by ESM, for example, PreHandleEvent() and PostHandleEvent() must be good point to do them).

Thanks!

Comment 32

[Ming-Chou Shih [:stone]]

Attachment: Bug 1303704 Part3: [Pointer Event] Add test case to ensure preventDefault on pointerdown doesn't impact firing click event.

Review commit: https://reviewboard.mozilla.org/r/91836/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/91836/

Comment 33

[Ming-Chou Shih [:stone]]

Comment on attachment 8800963 [details]
Bug 1303704 Part1: [Pointer Event] Implement prevent default behavior of pointerdown.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/85726/diff/1-2/

Comment 34

[Ming-Chou Shih [:stone]]

Comment on attachment 8800964 [details]
Bug 1303704 Part2: [Pointer Event] Enable prevent default on pointerdown related test cases.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/85728/diff/1-2/

Comment 35

[Ming-Chou Shih [:stone]]

Comment on attachment 8800963 [details]
Bug 1303704 Part1: [Pointer Event] Implement prevent default behavior of pointerdown.

https://reviewboard.mozilla.org/r/85726/#review88792

> Ah, I see.
> https://dxr.mozilla.org/mozilla-central/rev/aea5b4c3d165dcde027b3b6551b146a56748e9be/layout/base/nsPresShell.cpp#6554,6557-6559
> 
> Then, using MOZ_ASSERT() is fine.
> 
> Note that at least on Windows, another application can post/send any message to other application. Therefore, we need to assume any unexpected input event order.

Considering PostHandlePointerEventsPreventDefault handles ePointerDown and there must be a PointerInfo in list for the active pointer , so I use assertion to check it.

> Similar to PostHandlePointerEventsPreventDefault but we invoke it before handling pointer events except pointerdown. The active pointer info will be removed after handling the event. So I'd add some comments and an asserton to ensure it.

Considering unexpected event order cases, I think PreHandlePointerEventsPreventDefault doesn't interest in them (we only interest in those pointer events after a defaultPrevented pointerdown) so just ingore them.

> Ah, I understand. So, as I said, we should move PointerEvent implementation into EventStateManager (even if it's implemented as separated class, it should be managed by ESM, for example, PreHandleEvent() and PostHandleEvent() must be good point to do them).

Created Bug 1316251 - [Pointer Event] Separate pointer event implementation from PresShell

> I think that instead of checking event class, you need to check event message. Anyway, you should create a method to WidgetMouseEvent, e.g., IsAllowedToDispatch() which checks the event message and defaultPrevented.

I found WidgetEvent::IsAllowedToDispatchDOMEvent might work for this case.

Comment 36

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 8800963 [details]
Bug 1303704 Part1: [Pointer Event] Implement prevent default behavior of pointerdown.

https://reviewboard.mozilla.org/r/85726/#review91934

::: layout/base/nsPresShell.cpp:6865
(Diff revision 2)
> +  if (!sActivePointersIds->Get(aPointerEvent->pointerId, &pointerInfo) ||
> +      !pointerInfo) {
> +    // We already added the PointerInfo for active pointer when
> +    // PresShell::HandleEvent handling pointerdown event.
> +    MOZ_ASSERT(false);

Hmm, not good.

Please use one of following code:

>#ifdef DEBUG
>  if () {
>    MOZ_CRASH("something message to explain the reason");
>  }
>#endif

or

>  MOZ_ASSERT(sActivePointersIds->Get(
>               aPointerEvent->pointerId,
>               &pointerInfo) && pointerInfo);

::: widget/BasicEvents.h:433
(Diff revision 2)
>    void StopPropagation() { mFlags.StopPropagation(); }
>    void StopImmediatePropagation() { mFlags.StopImmediatePropagation(); }
>    void StopCrossProcessForwarding() { mFlags.StopCrossProcessForwarding(); }
>    void PreventDefault(bool aCalledByDefaultHandler = true)
>    {
> +    MOZ_RELEASE_ASSERT(!aCalledByDefaultHandler || mMessage != ePointerDown);

Could you explain with the second argument of MOZ_ASSERT, like "Legacy mouse events shouldn't be prevented by ePointerDown event.".

Comment 37

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 8809283 [details]
Bug 1303704 Part3: [Pointer Event] Add test case to ensure preventDefault on pointerdown doesn't impact firing click event.

https://reviewboard.mozilla.org/r/91836/#review91942

::: dom/events/test/pointerevents/test_bug1303704.html:45
(Diff revision 1)
> +  synthesizeMouseAtCenter(link1, { type: "mousedown",
> +                                   inputSource: SpecialPowers.Ci.nsIDOMMouseEvent.MOZ_SOURCE_MOUSE });
> +  synthesizeMouseAtCenter(link1, { type: "mouseup",
> +                                   inputSource: SpecialPowers.Ci.nsIDOMMouseEvent.MOZ_SOURCE_MOUSE });

IIRC, eMouseMove isn't synthesized with this automatically. So, I guess you should synthesize it between "mousedown" and "mouseup".

Comment 38

[Ming-Chou Shih [:stone]]

Comment on attachment 8800963 [details]
Bug 1303704 Part1: [Pointer Event] Implement prevent default behavior of pointerdown.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/85726/diff/2-3/

Comment 39

[Ming-Chou Shih [:stone]]

Comment on attachment 8800964 [details]
Bug 1303704 Part2: [Pointer Event] Enable prevent default on pointerdown related test cases.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/85728/diff/2-3/

Comment 40

[Ming-Chou Shih [:stone]]

Comment on attachment 8809283 [details]
Bug 1303704 Part3: [Pointer Event] Add test case to ensure preventDefault on pointerdown doesn't impact firing click event.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/91836/diff/1-2/

Comment 41

[Ming-Chou Shih [:stone]]

(In reply to Masayuki Nakano [:masayuki] (Mozilla Japan) from comment #36)
> Hmm, not good.
> 
> Please use one of following code:
> 
> >#ifdef DEBUG
> >  if () {
> >    MOZ_CRASH("something message to explain the reason");
> >  }
> >#endif
> 
> or
> 
> >  MOZ_ASSERT(sActivePointersIds->Get(
> >               aPointerEvent->pointerId,
> >               &pointerInfo) && pointerInfo);
I can't wrap sActivePointersIds->Get(aPointerEvent->pointerId, &pointerInfo) in MOZ_ASSERT or #ifdef DEBUG since it will be removed in optimized build.

Comment 42

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Ah, pointerInfo is necessary with non-debug build. Sorry, please use:

#ifdef DEBUG
MOZ_CRASH()
#endif // #ifdef DEBUG

Comment 43

[Ming-Chou Shih [:stone]]

Comment on attachment 8800963 [details]
Bug 1303704 Part1: [Pointer Event] Implement prevent default behavior of pointerdown.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/85726/diff/3-4/

Comment 44

[Ming-Chou Shih [:stone]]

Comment on attachment 8800964 [details]
Bug 1303704 Part2: [Pointer Event] Enable prevent default on pointerdown related test cases.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/85728/diff/3-4/

Comment 45

[Ming-Chou Shih [:stone]]

Comment on attachment 8809283 [details]
Bug 1303704 Part3: [Pointer Event] Add test case to ensure preventDefault on pointerdown doesn't impact firing click event.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/91836/diff/2-3/

Comment 46

[Ming-Chou Shih [:stone]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=60e3e1e34ef726a033cfa3871829d8fdb502fa80

Comment 47

[Ming-Chou Shih [:stone]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=0ee2cb917303c6a179ab14b80245524785bcdeed

Comment 48

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e998dc13aa5a
Part1: [Pointer Event] Implement prevent default behavior of pointerdown. r=masayuki
https://hg.mozilla.org/integration/autoland/rev/7a61dcea0be3
Part2: [Pointer Event] Enable prevent default on pointerdown related test cases. r=masayuki
https://hg.mozilla.org/integration/autoland/rev/afa4e843b8e9
Part3: [Pointer Event] Add test case to ensure preventDefault on pointerdown doesn't impact firing click event. r=masayuki

Comment 49

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/e998dc13aa5a
https://hg.mozilla.org/mozilla-central/rev/7a61dcea0be3
https://hg.mozilla.org/mozilla-central/rev/afa4e843b8e9

Comment 50

[areya]

https://hg.mozilla.org/mozilla-central/rev/e998dc13aa5a
https://hg.mozilla.org/mozilla-central/rev/7a61dcea0be3
https://hg.mozilla.org/mozilla-central/rev/afa4e843b8e9