864382 - Send a click event after a contextmenu event if the later has not been caught

Status: RESOLVED FIXED

(Firefox OS Graveyard :: General, defect)

Description

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

The :active class is given by BrowserElementPanning.js but it is not removed when a contextmenu even is fired or when the target under the finger is not the same.

As a result the pseudo class is sometimes given to an element even if nothing will happen when the mouse/finger is release. That sounds bad.

Comment 1

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: Patch

The patch remove the class when the finger/mouse moves is not on the target anymore and also when a contextmenu event that will cancel the click is fired.

Comment 2

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: Gaia Patch

David it seems like mouse event shim manage their own hold item and fire a click even if a regular context menu event has fired.

I'm not sure of the right behavior right now but I would like to land the Gecko part and to not break anything I need this gaia part as well.

Comment 3

[David Flanagan [:djf]]

Comment on attachment 740344 [details] [diff] [review]
Gaia Patch

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

This patch seems heavy handed.  I'm willing to give r+ if you verify (with grep) that no app that uses the shim also uses context menus. And then add a big warning to the comment at the top of the file saying that the shim can not be used in apps that need to display context menus.

Or, I think there is a better fix possible.  If I'm understanding correctly, you're saying that if a contextmenu event is fired and preventDefault() is not called on it, then no click event should occur.  If that is right, I think it is pretty easy to register a non-capturing contextmenu listener on the window that sets emitclick to false.  That might be all it takes to do what you want.

Comment 4

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

(In reply to David Flanagan [:djf] from comment #3)
> Comment on attachment 740344 [details] [diff] [review]
> Gaia Patch
> 
> Review of attachment 740344 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This patch seems heavy handed.  I'm willing to give r+ if you verify (with
> grep) that no app that uses the shim also uses context menus. And then add a
> big warning to the comment at the top of the file saying that the shim can
> not be used in apps that need to display context menus.
> 
> Or, I think there is a better fix possible.  If I'm understanding correctly,
> you're saying that if a contextmenu event is fired and preventDefault() is
> not called on it, then no click event should occur.  If that is right, I
> think it is pretty easy to register a non-capturing contextmenu listener on
> the window that sets emitclick to false.  That might be all it takes to do
> what you want.

Tbh I need to dig in nsEventStateManager.cpp a bit more to give a real answer to this. Will do asap.

Comment 5

[[:fabrice] Fabrice Desré]

Comment on attachment 740338 [details] [diff] [review]
Patch

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

r=me with comment addressed

::: dom/browser-element/BrowserElementPanning.js
@@ +90,5 @@
> +        this._resetActive();
> +        this.dragging = false;
> +        this.detectingScrolling = false;
> +        delete this.primaryPointerId;
> +        this._activationTimer.cancel();

These lines are copy-pasted from the onTouchEnd() function. Can you refactor to not duplicate?

Comment 6

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

(In reply to Fabrice Desré [:fabrice] from comment #5)
> Comment on attachment 740338 [details] [diff] [review]
> Patch
> 
> Review of attachment 740338 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r=me with comment addressed
> 
> ::: dom/browser-element/BrowserElementPanning.js
> @@ +90,5 @@
> > +        this._resetActive();
> > +        this.dragging = false;
> > +        this.detectingScrolling = false;
> > +        delete this.primaryPointerId;
> > +        this._activationTimer.cancel();
> 
> These lines are copy-pasted from the onTouchEnd() function. Can you refactor
> to not duplicate?

Thanks for the review. I feel like I would like to try something else too. I wonder if I can fix the code so a click will still be dispatched if nobody has caught the contextmenu event. That could be better imho.

Comment 7

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Blocking since user may miss some clicks...

Comment 8

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Sorry Fabrice I don't see how we can block on UI stuffs and let the user miss clicks.... Let me ask for tef? this time.

Comment 9

[Daniel Coloma:dcoloma]

Vivien, what is the impact for the end-users of this bug?

Comment 10

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

(In reply to Daniel Coloma:dcoloma from comment #9)
> Vivien, what is the impact for the end-users of this bug?

Missed clicks.

Steps to reproduce:
 - Open the sms app
 - Hold the finger on one of the thread to open it
 - Wait a few seconds and then release the finger.

Actual result:
 - nothing happens

Expected result:
 - The thread list is opened...

Comment 11

[Daniel Coloma:dcoloma]

I don't think we should block on this for 1.0.1

Comment 12

[Lukas Blakk [:lsblakk] use ?needinfo]

It's tricky, and we don't want to open the doors to a bunch of late fixed, but at the same time we're handing out targets code we know won't work when touched.  This type of papercut is going to really harm our users' experience with v1.0.1 when the phone seems unresponsive in things like sms (what about dialer app?) -- so we support blocking on this issue.

Comment 13

[Alex Keybl [:akeybl]]

(In reply to [e-mail] from comment #12)
> It's tricky, and we don't want to open the doors to a bunch of late fixed,
> but at the same time we're handing out targets code we know won't work when
> touched.  This type of papercut is going to really harm our users'
> experience with v1.0.1 when the phone seems unresponsive in things like sms
> (what about dialer app?) -- so we support blocking on this issue.

We'll of course reconsider after 5/10 if this hasn't landed.

Comment 14

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: Patch

I completely change my approach. Instead of cancelling the :active class let's dispatch the click event if nobody caught it. That sounds the saner approach.

Comment 15

[Andrea Marchesini [:baku]]

Comment on attachment 746548 [details] [diff] [review]
Patch

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

I'm not peer of this component. The patch looks good but I cannot give you a r+

::: dom/ipc/TabChild.cpp
@@ +1355,5 @@
>                           const bool&     aIgnoreRootScrollFrame)
>  {
>    nsCOMPtr<nsIDOMWindowUtils> utils(GetDOMWindowUtils());
>    NS_ENSURE_TRUE(utils, true);
> +  return !utils->SendMouseEvent(aType, aX, aY, aButton, aClickCount, aModifiers,

I prefer:

if (NS_FAILED(...)) {
  NS_WARNING("failed to send a mouse event");
  return falase;
}

return true;

Comment 16

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

(In reply to Andrea Marchesini (:baku) from comment #15)
> Comment on attachment 746548 [details] [diff] [review]
> Patch
> 
> Review of attachment 746548 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I'm not peer of this component. The patch looks good but I cannot give you a
> r+
> 
> ::: dom/ipc/TabChild.cpp
> @@ +1355,5 @@
> >                           const bool&     aIgnoreRootScrollFrame)
> >  {
> >    nsCOMPtr<nsIDOMWindowUtils> utils(GetDOMWindowUtils());
> >    NS_ENSURE_TRUE(utils, true);
> > +  return !utils->SendMouseEvent(aType, aX, aY, aButton, aClickCount, aModifiers,
> 
> I prefer:
> 
> if (NS_FAILED(...)) {
>   NS_WARNING("failed to send a mouse event");
>   return falase;
> }
> 
> return true;

I can't use NS_FAILED since the return value is a boolean set whether or not the receiver of the event has called event.preventDefault() on it or not. But I can change the code style if needed :) (I'm waiting for Justin's input).

Comment 17

[Alex Keybl [:akeybl]]

This needs to land today or go unfixed in v1.0.1, so that this can get at least two partner test cycles before shipping.

Comment 18

[Justin Lebar (not reading bugmail)]

I'm /think/ that returning false from one of these IPC methods will kill the child process.  That's probably not what you mean...

Comment 19

[Justin Lebar (not reading bugmail)]

> I'm /think/ that returning false from one of these IPC methods will kill the child process.

Yeah, bent confirmed this on IRC.  So at least that's wrong here.

I'll fix up the patch, but would appreciate some help testing it.

Comment 20

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

(In reply to Justin Lebar [:jlebar] from comment #19)
> > I'm /think/ that returning false from one of these IPC methods will kill the child process.
> 
> Yeah, bent confirmed this on IRC.  So at least that's wrong here.
> 
> I'll fix up the patch, but would appreciate some help testing it.

In order to test it:
 - open the sms app
 - Hold the finger on one of the icon on the top right corner
 - Release the finger

Actual result:
 - Nothing happens

Expected result:
 - The button is clicked...

Hope it helps.

Comment 21

[Justin Lebar (not reading bugmail)]

>> +  return !utils->SendMouseEvent(aType, aX, aY, aButton, aClickCount, aModifiers,
>
> I can't use NS_FAILED since the return value is a boolean set whether or not the receiver of the 
> event has called event.preventDefault() on it or not.

I think SendMouseEvent returns an nsresult...

Comment 22

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

(In reply to Justin Lebar [:jlebar] from comment #21)
> >> +  return !utils->SendMouseEvent(aType, aX, aY, aButton, aClickCount, aModifiers,
> >
> > I can't use NS_FAILED since the return value is a boolean set whether or not the receiver of the 
> > event has called event.preventDefault() on it or not.
> 
> I think SendMouseEvent returns an nsresult...

Are we not calling http://mxr.mozilla.org/mozilla-central/source/dom/interfaces/base/nsIDOMWindowUtils.idl#249 ?

Comment 23

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

I guess I should have looked closer at how the CPP code works there. I'm too used to JS. Sorry.

Comment 24

[Justin Lebar (not reading bugmail)]

Attachment: Changes atop Vivien's patch.

Vivien, I don't see you on IRC, but you're clearly lurking in the weeds here...

Comment 25

[Justin Lebar (not reading bugmail)]

Attachment: When a TabChild sends a ContextMenu event that is not preventDefault()'ed, send a click event.

Comment 26

[Justin Lebar (not reading bugmail)]

Attachment: Roll-up patch, v1

Comment 27

[Justin Lebar (not reading bugmail)]

Comment on attachment 748164 [details] [diff] [review]
Roll-up patch, v1

bent points out that this is wrong on b2g18, where we don't have an outparam indicating whether or not we preventDefault()'ed.

I'll backport the relevant stuff.

Comment 28

[Justin Lebar (not reading bugmail)]

In the case of the SMS app, someone is calling preventDefault() on the contextmenu event, so a correct patch here would not send the click event.  I think the patch Vivien posted worked by always sending a click event after a contextmenu event.

It's coming from JS.  I'm working on getting a stack.

Comment 29

[Justin Lebar (not reading bugmail)]

And the winner is:

> 0 anonymous() ["chrome://global/content/BrowserElementChildPreload.js":461]
>     this = [object Object]

BrowserElementChild actually calls preventDefault() on every contextmenu event it gets.  It has to, because it doesn't know whether the parent process is going to do anything with the contextmenu event.

Given this, I don't know how to make this approach work.

Comment 30

[Justin Lebar (not reading bugmail)]

Attachment: Cherry-pick pieces of bug 647216 to b2g18 so that nsDOMWindowUtils::SendMouseEvent returns whether the event was PreventDefault()'ed.

For posterity, here are the pieces of bug 647216 that we need for b2g18 in order to tell whether the event was preventDefault()'ed.

Comment 31

[Justin Lebar (not reading bugmail)]

Attachment: Patch, v2: Send a click event after a contextmenu event iff the contextmenu event does not get preventDefault()'ed.

This doesn't fix the bug because BrowserElementChild() always calls preventDefault on the contextmenu event.  Indeed, I don't see how we have any other choice.

BrowserElementParent knows whether the contextmenu event was handled.  I suppose it could fire a click if it wasn't.

Comment 32

[Justin Lebar (not reading bugmail)]

I think we may be able to handle this the way Vivien intended, by synthesizing some mouse events in the BrowserElement code.  But I'm not so familiar with mouse and touch events to be comfortable crash landing this today, particularly not without smaug having a look.

I'm sorry if that means that this doesn't make it into 1.0.1.  I'll attach incomplete patches so an intrepid soul can try to pick this up.

Comment 33

[Justin Lebar (not reading bugmail)]

Attachment: Completely untested WIP: Send a click event when context menu event isn't handled.

This is the general idea.  The commented-out code in BrowserElementParent
works, except it simulates the click too early; we have to wait until the user
releases their finger.  I haven't tried to run the bit that waits; this is just
a sketch.

Comment 34

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

I have an updated patch that use a sendSyncMessage in BrowserElementChildPreload.js and still rely on TabChild.cpp to dispatch the click. I need to fix a few things and I will attach it here afterwards to see what you think about it Justin.

Comment 35

[Justin Lebar (not reading bugmail)]

> I have an updated patch that use a sendSyncMessage in BrowserElementChildPreload.js and still rely 
> on TabChild.cpp to dispatch the click.

Hm.  What's the advantage of doing this synchronously?  I guess it's less code.  I'm not sure if it's a good idea, though.  The embedder can cause the child to jank for an arbitrary amount of time (via its event handler).  Maybe that's fine.  I guess I can think about it...

Comment 36

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

(In reply to Justin Lebar [:jlebar] from comment #35)
> > I have an updated patch that use a sendSyncMessage in BrowserElementChildPreload.js and still rely 
> > on TabChild.cpp to dispatch the click.
> 
> Hm.  What's the advantage of doing this synchronously?  I guess it's less
> code.  I'm not sure if it's a good idea, though.  The embedder can cause the
> child to jank for an arbitrary amount of time (via its event handler). 
> Maybe that's fine.  I guess I can think about it...

Sounds cleaner to me (There is already too many code paths for events imho). With sync messages you can tell the children if the real contextmenu event has been handle or not. Also I've been told that sendSyncMessage are not bad when the direction is child -> parent.

Comment 37

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

(In reply to Vivien Nicolas (:vingtetun) (:21) from comment #36)
> Also I've been told that sendSyncMessage are not bad
> when the direction is child -> parent.

At least that was the opinion of people when they designed the messageManager API :)

Comment 38

[Justin Lebar (not reading bugmail)]

> Sounds cleaner to me (There is already too many code paths for events imho).

I agree it's likely cleaner.

> Also I've been told that sendSyncMessage are not bad when the direction is child -> parent.

I think "not bad" is relative.  It's not as bad as the parent waiting on a child!

We can try it and see if it's problematic; I think maybe I was prematurely optimizing.

Comment 39

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: Patch

Justin let me know what do you think of the patch. If you r+ it I will add a test (hint!) :)

Comment 40

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: Gaia part

In order to work the patch needs the bits from #c30 (for the nsIDOMWindowUtils part) as well as a little Gaia patch.

Comment 41

[Justin Lebar (not reading bugmail)]

Comment on attachment 748794 [details] [diff] [review]
Patch

>diff --git a/dom/browser-element/BrowserElementChildPreload.js b/dom/browser-element/BrowserElementChildPreload.js
>--- a/dom/browser-element/BrowserElementChildPreload.js
>+++ b/dom/browser-element/BrowserElementChildPreload.js

>@@ -483,17 +495,22 @@ BrowserElementChild.prototype = {
>     }
> 
>     if (ctxMenuId) {
>       var menu = e.target.ownerDocument.getElementById(ctxMenuId);
>       if (menu) {
>         menuData.contextmenu = this._buildMenuObj(menu, '');
>       }
>     }
>-    sendAsyncMsg('contextmenu', menuData);
>+
>+    // Call prevent default on the context menu event if it has been caught by
>+    // the parent process in order to prevent TabChild.cpp to fire fire a click 

Nit: s/prevent default/preventDefault()/
s/fire fire/fire/

But also s/prevent TabChild.cpp to fire a click/prevent TabChild.cpp from
firing a click./  This is a weird English thing: Some verbs take infinitives,
and some take gerunds, and of course there's no rule.  "Prevent" takes "from"
plus a gerund.

http://owl.english.purdue.edu/owl/resource/627/04/
http://www.writing.utoronto.ca/advice/english-as-a-second-language/gerunds
http://grammar.ccc.commnet.edu/grammar/verblist.htm <-- this one has "prevent from doing"

It would also make more sense to me if we explained what exactly the parent
process has to do to cause sendSyncMsg to return true/false.  So maybe:

  // The value returned by the contextmenu sync call is true iff the embedder
  // called preventDefault() on its contextmenu event.
  //
  // We call preventDefault() on our contextmenu event iff the embedder called
  // preventDefault() on /its/ contextmenu event.  This way, if the embedder
  // ignored the contextmenu event, TabChild will fire a click.

>+    if (sendSyncMsg('contextmenu', menuData)[0]) {
>+      e.preventDefault();
>+    }
>   },

>diff --git a/dom/browser-element/BrowserElementParent.jsm b/dom/browser-element/BrowserElementParent.jsm
>--- a/dom/browser-element/BrowserElementParent.jsm
>+++ b/dom/browser-element/BrowserElementParent.jsm

>@@ -288,28 +288,29 @@ BrowserElementParent.prototype = {
>     }
>   },
> 
>   _fireCtxMenuEvent: function(data) {
>     let detail = data.json;
>     let evtName = detail.msg_name;
> 
>     debug('fireCtxMenuEventFromMsg: ' + evtName + ' ' + detail);
>-    let evt = this._createEvent(evtName, detail);
>+    let evt = this._createEvent(evtName, detail, /* cancellable */ true);
> 
>     if (detail.contextmenu) {
>       var self = this;
>       defineAndExpose(evt.detail, 'contextMenuItemSelected', function(id) {
>         self._sendAsyncMsg('fire-ctx-callback', {menuitem: id});
>       });
>     }
>     // The embedder may have default actions on context menu events, so
>     // we fire a context menu event even if the child didn't define a
>     // custom context menu
>-    this._frameElement.dispatchEvent(evt);
>+    let handle = !this._frameElement.dispatchEvent(evt);
>+    return handle;

s/handle/defaultPrevented/ (if that's what this value means, anyway).

We should also prevent contextMenuItemSelected from doing anything if
preventDefault() was not called.  The patch I attached does this.  (We should
test this behavior, of course.  :)

Also, I wonder what we should do if they call contextMenuItemSelected but also
/don't/ call preventDefault.  I'd guess that should count as the same as
calling preventDefault().

>diff --git a/dom/ipc/TabChild.h b/dom/ipc/TabChild.h
>--- a/dom/ipc/TabChild.h
>+++ b/dom/ipc/TabChild.h

>@@ -294,16 +294,24 @@ public:
>     ScreenOrientation GetOrientation() { return mOrientation; }
> 
>     void SetBackgroundColor(const nscolor& aColor);
> 
>     void NotifyPainted();
> 
>     bool IsAsyncPanZoomEnabled();
> 
>+    bool SendMouseEvent(const nsString& aType,
>+                        const float&    aX,
>+                        const float&    aY,
>+                        const int32_t&  aButton,
>+                        const int32_t&  aClickCount,
>+                        const int32_t&  aModifiers,
>+                        const bool&     aIgnoreRootScrollFrame);

Please add a comment, at the very least explaining what the return value is.

>diff --git a/dom/ipc/TabChild.cpp b/dom/ipc/TabChild.cpp
>--- a/dom/ipc/TabChild.cpp
>+++ b/dom/ipc/TabChild.cpp

>@@ -1490,18 +1489,31 @@ TabChild::UpdateTapState(const nsTouchEv
>     NS_WARNING("Unknown touch event type");
>   }
> }
> 
> void
> TabChild::FireContextMenuEvent()
> {
>   MOZ_ASSERT(mTapHoldTimer && mActivePointerId >= 0);
>-  RecvHandleLongTap(mGestureDownPoint);
>-  CancelTapTracking();
>+  bool prevented = SendMouseEvent(NS_LITERAL_STRING("contextmenu"),
>+                                  mGestureDownPoint.x, mGestureDownPoint.y,
>+                                  2 /* Right button */,
>+                                  1 /* Click count */,
>+                                  0 /* Modifiers */,
>+                                  false /* Ignore root scroll frame */);
>+
>+  // If someone has called preventDefault() on the context menu event
>+  // do not fire a click event.

Before we used to do nothing if !mCx || !mTabChildGlobal.  But now we don't
check that.  I'm not sure if we need to.

Nit: s/prevented/defaultPrevented/
Nit: s/event/event,/

(The comma is not required for most short "if" clauses, but it is usually
necessary for longer ones.  "Short" depends on how it sounds, but is not
usually more than four or five words.)

Or even better would be to invert the sentence order and make the beginning
positive again: "Fire a click event if someone didn't call preventDefault() on
the context menu event."  This makes it clear that we're actively synthesizing
the click event, instead of simply preventing one from being fired.

>@@ -1975,16 +1987,34 @@ TabChild::NotifyPainted()
> }
> 
> bool
> TabChild::IsAsyncPanZoomEnabled()
> {
>     return mScrolling == ASYNC_PAN_ZOOM;
> }
> 
>+bool
>+TabChild::SendMouseEvent(const nsString& aType,
>+                         const float&    aX,
>+                         const float&    aY,
>+                         const int32_t&  aButton,
>+                         const int32_t&  aClickCount,
>+                         const int32_t&  aModifiers,
>+                         const bool&     aIgnoreRootScrollFrame)
>+{

Nit: This is a confusing name because SendMouseEvent could be an IPDL method
(cf PBrowserChild::SendEvent).

I'm not sure what to call it.  Maybe DispatchMouseEvent?

>+  nsCOMPtr<nsIDOMWindowUtils> utils(GetDOMWindowUtils());
>+  NS_ENSURE_TRUE(utils, true);
>+  
>+  bool prevented = false;
>+  utils->SendMouseEvent(aType, aX, aY, aButton, aClickCount, aModifiers,
>+                        aIgnoreRootScrollFrame, 0, 0, &prevented);
>+  return prevented;
>+}

You'll need part of my patch for b2g18, where the |prevented| arg doesn't
exist.

Also, nit: s/prevented/defaultPrevented/.

Thanks for volunteering to write a test!  (I was going to ask for one before I
saw your comment.  :)  We should check that the click event is /not/ fired when
we do display a context menu event, since that's a bug we had in an earlier
patch.

This isn't r+ only because I want to look at the BrowserElementParent changes.
The rest of this is just long-winded nits.  :)

Comment 42

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: Patch + (shame) only 1 additional test

Justin I address most of your review comments. I spent some times inside the test file (as you may notice by the little rewrite I did), but I end up adding only 1 test case. 

I got an hard time trying to fire the context menu on Firefox desktop with touch events. I'm pretty sure this is doable but I need to spend more time thinking on the interaction between nsEventStateManager -> Tabchild -> BrowserElementPanning and calling nsIDOMWindowUtils.sendTouchEvent. 

I will appreciate if I can land that as if and open a followup for the click part of the testcase. I'm pretty sure this work will end up by:
 - fixing https://bugzilla.mozilla.org/show_bug.cgi?id=766813 (same bug for regular mouse events).
 - fixing the tests in content/events/test/test_bug563329.html to show the changes of bug 766813 and to support TouchEvents.

But in any cases it seems like the tests for it should be part of the other test file.


Also as a side note during the rewrite of the tests for contextmenu I removed some of them that was looking duplicates to me as well as the onerror case that was not testing the contextmenu code path imho.

If you don't like the rewrite please let me know. If you like it I would appreciate to have dale's feedback to see if I have not removed any important part of the tests.

Comment 43

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: Patch + (shame) only 1 additional test (for real)

And to add more shame on the stack I also attached the wrong patch. Here the correct one.

Comment 44

[Dale Harvey (:daleharvey)]

Comment on attachment 749776 [details] [diff] [review]
Patch + (shame) only 1 additional test (for real)

Tests are much clearer imo and important paths are still tested

Comment 45

[Justin Lebar (not reading bugmail)]

Comment on attachment 749776 [details] [diff] [review]
Patch + (shame) only 1 additional test (for real)

>diff --git a/dom/ipc/TabChild.cpp b/dom/ipc/TabChild.cpp
>--- a/dom/ipc/TabChild.cpp
>+++ b/dom/ipc/TabChild.cpp

>+bool
>+TabChild::DispatchMouseEvent(const nsString& aType,
>+                             const float&    aX,
>+                             const float&    aY,
>+                             const int32_t&  aButton,
>+                             const int32_t&  aClickCount,
>+                             const int32_t&  aModifiers,
>+                             const bool&     aIgnoreRootScrollFrame)
>+{
>+  nsCOMPtr<nsIDOMWindowUtils> utils(GetDOMWindowUtils());
>+  NS_ENSURE_TRUE(utils, true);

Should we return false here?  Neither is good, but certainly in this case, the
page didn't explicitly preventDefault.

>+  bool defaultPrevented = false;
>+  utils->SendMouseEvent(aType, aX, aY, aButton, aClickCount, aModifiers,
>+                        aIgnoreRootScrollFrame, 0, 0, &defaultPrevented);
>+  return defaultPrevented;
>+}

I didn't look at the test, since Dale already did, and I have tef blockers of my own.  I appreciate that you did what you could.

Comment 46

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Comment on attachment 748796 [details] [diff] [review]
Gaia part

I probably need dale's stamp here too.

Comment 47

[Alex Keybl [:akeybl]]

(In reply to Alex Keybl [:akeybl] from comment #13)
> (In reply to [e-mail] from comment #12)
> > It's tricky, and we don't want to open the doors to a bunch of late fixed,
> > but at the same time we're handing out targets code we know won't work when
> > touched.  This type of papercut is going to really harm our users'
> > experience with v1.0.1 when the phone seems unresponsive in things like sms
> > (what about dialer app?) -- so we support blocking on this issue.
> 
> We'll of course reconsider after 5/10 if this hasn't landed.

If we don't know of any real user scenarios that we're running into this with, please leave [NO_UPLIFT] and bump to blocking-b2g:tef?. Otherwise, please remove [NO_UPLIFT] so that this goes up the trees.

We don't want to take any click event risk at this point without a known broken user scenario (STR).

Comment 48

[Justin Lebar (not reading bugmail)]

What do you mean by a "real user scenario"?

Vivien has simple testcase that could be hit by any user: Long-tap on a message in the SMS app, then let go.  He wants that to act as though you clicked on the item that you'd long-tapped on.

Is the "real user" burden that we have to have a scenario that a real user has actually hit and reported to us independently?

Comment 49

[Alex Keybl [:akeybl]]

Comment 20 qualifies as a fairly minor inconvenience and doesn't really break functionality. Comment 13 set the expectation that we would likely not block after 5/13. Let's fix for v1.1 at this point.

Comment 50

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

(In reply to Alex Keybl [:akeybl] from comment #49)
> Comment 20 qualifies as a fairly minor inconvenience and doesn't really
> break functionality. Comment 13 set the expectation that we would likely not
> block after 5/13. Let's fix for v1.1 at this point.

System wide clicks that are ignored: It could be considered minor inconvenience but it sounds like an inconsistent OS to me. Especially since it works sometimes because of some local code living in some apps.

I have been told since 3 years by UX designers that a phone is not always used in a test environment but in real life while you're walking or speaking with someone else for example. The attention of the user is only partially focused on the UI and user's response time can be longer. So I wonder if this issue is bigger than what it looks like in a testing env.

Also the risk for this patch is pretty minor too imho.

Comment 51

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

https://hg.mozilla.org/projects/birch/rev/ec2f1b9a9726

I have disabled the test for Android.

Comment 52

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

I have not disable the test on Android sorry. Cross-bug comments.

Comment 53

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

https://github.com/mozilla-b2g/gaia/commit/296c8434d7f49966b67e2bce9dc173658d4fa367

Comment 54

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/ec2f1b9a9726

Comment 55

[Ryan VanderMeulen [:RyanVM]]

This has quite a few conflicts on b2g18.

Comment 56

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: b2g-18 patch

Comment 57

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g18/rev/ae27a4f1696d

Comment 58

[Ryan VanderMeulen [:RyanVM]]

Backed out for bustage.
https://hg.mozilla.org/releases/mozilla-b2g18/rev/09dc1ae3b1b5

https://tbpl.mozilla.org/php/getParsedLog.php?id=23811633&tree=Mozilla-B2g18
https://tbpl.mozilla.org/php/getParsedLog.php?id=23811833&tree=Mozilla-B2g18

Comment 59

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Comment on attachment 748261 [details] [diff] [review]
Cherry-pick pieces of bug 647216 to b2g18 so that nsDOMWindowUtils::SendMouseEvent returns whether the event was PreventDefault()'ed.

Argh. I think I forgot to said that this patch is needed for the other patch to work.

Comment 60

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: b2g-18 patch rebased on top of the dependency.

Justin cherry pick + this patch should make it for b1g-18. I'm unsure of the process to land the cherry pick part?

Comment 61

[Justin Lebar (not reading bugmail)]

> I'm unsure of the process to land the cherry pick part?

Just get leo+ and then the sheriffs can figure this out (with your help, if necessary).  (There's no more approval-b2g18.)

Comment 62

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g18/rev/dc1cc9ef32eb
https://hg.mozilla.org/releases/mozilla-b2g18/rev/eeed221e5b11

Comment 63

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g18_v1_1_0_hd/rev/dc1cc9ef32eb
https://hg.mozilla.org/releases/mozilla-b2g18_v1_1_0_hd/rev/eeed221e5b11

Comment 64

[Marco Castelluccio [:marco]]

Why was the test test_browserElement_inproc_ErrorSecurity.html disabled?

Comment 65

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

(In reply to Marco Castelluccio [:marco] from comment #64)
> Why was the test test_browserElement_inproc_ErrorSecurity.html disabled?

Right bug ?(In reply to Marco Castelluccio [:marco] from comment #64)
> Why was the test test_browserElement_inproc_ErrorSecurity.html disabled?

I didn't disabled it in this bug but in https://bugzilla.mozilla.org/show_bug.cgi?id=855543#c21. The comment here was a cross-post comment error.

Basically Justin told me that most mozbrowser tests fails on Android and it was fine to not worry about that yet and just disable it.

Comment 66

[Marco Castelluccio [:marco]]

Thank you, I'm experiencing a problem with certificates and was wondering if I could disable my test on Android (mine is intermittently failing).