Closed Bug 864382 Opened 7 years ago Closed 7 years ago

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

Categories

(Firefox OS Graveyard :: General, defect)

defect
Not set

Tracking

(blocking-b2g:leo+, firefox22 wontfix, firefox23 wontfix, firefox24 fixed, b2g18+ fixed, b2g18-v1.0.0 wontfix, b2g18-v1.0.1 wontfix, b2g-v1.1hd fixed)

RESOLVED FIXED
1.1 QE2 (6jun)
blocking-b2g leo+
Tracking Status
firefox22 --- wontfix
firefox23 --- wontfix
firefox24 --- fixed
b2g18 + fixed
b2g18-v1.0.0 --- wontfix
b2g18-v1.0.1 --- wontfix
b2g-v1.1hd --- fixed

People

(Reporter: vingtetun, Assigned: vingtetun)

References

Details

(Whiteboard: [target:05/16] u=fx-os-user c=may-6-17 p=1)

Attachments

(4 files, 11 obsolete files)

8.67 KB, patch
Details | Diff | Splinter Review
331 bytes, patch
daleharvey
: review+
Details | Diff | Splinter Review
20.87 KB, patch
justin.lebar+bug
: review+
daleharvey
: feedback+
Details | Diff | Splinter Review
20.83 KB, patch
Details | Diff | Splinter Review
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.
Attached patch Patch (obsolete) — Splinter Review
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.
Attachment #740338 - Flags: review?(fabrice)
Attached patch Gaia Patch (obsolete) — Splinter Review
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.
Attachment #740344 - Flags: review?(dflanagan)
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.
Attachment #740344 - Flags: review?(dflanagan) → review-
(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 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?
Attachment #740338 - Flags: review?(fabrice) → review+
(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.
Blocking since user may miss some clicks...
blocking-b2g: --- → leo?
blocking-b2g: leo? → ---
tracking-b2g18: --- → +
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.
blocking-b2g: --- → tef?
Vivien, what is the impact for the end-users of this bug?
Flags: needinfo?(21)
Whiteboard: [tef-triage]
(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...
Flags: needinfo?(21)
I don't think we should block on this for 1.0.1
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.
blocking-b2g: tef? → tef+
Whiteboard: [tef-triage]
Whiteboard: u=fx-os-user c=may-6-17 p=0
Summary: :active pseudo class should be removed more often → When the platform decides _not_ to send the click event, the :active pseudo class should be removed
Whiteboard: u=fx-os-user c=may-6-17 p=0
Whiteboard: u=fx-os-user c=may-6-17 p=0
Whiteboard: u=fx-os-user c=may-6-17 p=0 → u=fx-os-user c=may-6-17 p=1
(In reply to lsblakk@mozilla.com 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.
Attached patch Patch (obsolete) — Splinter Review
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.
Attachment #740338 - Attachment is obsolete: true
Attachment #740344 - Attachment is obsolete: true
Attachment #746548 - Flags: review?(amarchesini)
Whiteboard: u=fx-os-user c=may-6-17 p=1 → [status: awaiting review] u=fx-os-user c=may-6-17 p=1
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;
Attachment #746548 - Flags: review?(justin.lebar+bug)
Attachment #746548 - Flags: review?(amarchesini) → feedback+
(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).
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.
Whiteboard: [status: awaiting review] u=fx-os-user c=may-6-17 p=1 → [status: land today, or unblock] u=fx-os-user c=may-6-17 p=1
I'm /think/ that returning false from one of these IPC methods will kill the child process.  That's probably not what you mean...
> 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.
Attachment #746548 - Flags: review?(justin.lebar+bug) → review-
Assignee: 21 → justin.lebar+bug
(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.
>> +  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...
(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 ?
I guess I should have looked closer at how the CPP code works there. I'm too used to JS. Sorry.
Attached patch Changes atop Vivien's patch. (obsolete) — Splinter Review
Vivien, I don't see you on IRC, but you're clearly lurking in the weeds here...
Attachment #748162 - Attachment is obsolete: true
Attached patch Roll-up patch, v1 (obsolete) — Splinter Review
Attachment #746548 - Attachment is obsolete: true
Attachment #748159 - Attachment is obsolete: true
Attachment #748164 - Flags: review?(bent.mozilla)
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.
Attachment #748164 - Flags: review?(bent.mozilla) → review-
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.
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.
For posterity, here are the pieces of bug 647216 that we need for b2g18 in order to tell whether the event was 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.
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.
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.
Attachment #748164 - Attachment is obsolete: true
Attachment #748261 - Attachment is obsolete: true
Attachment #748264 - Attachment is obsolete: true
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.
> 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...
(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.
(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 :)
> 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.
Attached patch Patch (obsolete) — Splinter Review
Justin let me know what do you think of the patch. If you r+ it I will add a test (hint!) :)
Attachment #748794 - Flags: review?(justin.lebar+bug)
Attached patch Gaia partSplinter Review
In order to work the patch needs the bits from #c30 (for the nsIDOMWindowUtils part) as well as a little Gaia patch.
Attachment #748323 - Attachment is obsolete: true
Assignee: justin.lebar+bug → 21
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.  :)
Attachment #748794 - Flags: review?(justin.lebar+bug)
Whiteboard: [status: land today, or unblock] u=fx-os-user c=may-6-17 p=1 → [target:05/16] u=fx-os-user c=may-6-17 p=1
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.
Attachment #748794 - Attachment is obsolete: true
Attachment #749774 - Flags: review?(justin.lebar+bug)
Attachment #749774 - Flags: feedback?(dale)
And to add more shame on the stack I also attached the wrong patch. Here the correct one.
Attachment #749774 - Attachment is obsolete: true
Attachment #749774 - Flags: review?(justin.lebar+bug)
Attachment #749774 - Flags: feedback?(dale)
Attachment #749776 - Flags: review?(justin.lebar+bug)
Attachment #749776 - Flags: feedback?(dale)
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
Attachment #749776 - Flags: feedback?(dale) → feedback+
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.
Attachment #749776 - Flags: review?(justin.lebar+bug) → review+
Whiteboard: [target:05/16] u=fx-os-user c=may-6-17 p=1 → [status: needs landing][target:05/16] u=fx-os-user c=may-6-17 p=1
Comment on attachment 748796 [details] [diff] [review]
Gaia part

I probably need dale's stamp here too.
Attachment #748796 - Flags: review?(dale)
Whiteboard: [status: needs landing][target:05/16] u=fx-os-user c=may-6-17 p=1 → [status: needs review][target:05/16] u=fx-os-user c=may-6-17 p=1
Whiteboard: [status: needs review][target:05/16] u=fx-os-user c=may-6-17 p=1 → [status: needs landing][target:05/16] u=fx-os-user c=may-6-17 p=1
Attachment #748796 - Flags: review?(dale) → review+
(In reply to Alex Keybl [:akeybl] from comment #13)
> (In reply to lsblakk@mozilla.com 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).
Whiteboard: [status: needs landing][target:05/16] u=fx-os-user c=may-6-17 p=1 → [NO_UPLIFT][status: needs landing][target:05/16] u=fx-os-user c=may-6-17 p=1
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?
Flags: needinfo?(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.
blocking-b2g: tef+ → leo+
Flags: needinfo?(akeybl)
Whiteboard: [NO_UPLIFT][status: needs landing][target:05/16] u=fx-os-user c=may-6-17 p=1 → [status: needs landing][target:05/16] u=fx-os-user c=may-6-17 p=1
(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.
https://hg.mozilla.org/projects/birch/rev/ec2f1b9a9726

I have disabled the test for Android.
Status: NEW → ASSIGNED
OS: Linux → All
Hardware: x86_64 → All
Summary: When the platform decides _not_ to send the click event, the :active pseudo class should be removed → Send a click event after a contextmenu event if the later has not been caught
I have not disable the test on Android sorry. Cross-bug comments.
https://hg.mozilla.org/mozilla-central/rev/ec2f1b9a9726
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
This has quite a few conflicts on b2g18.
Whiteboard: [status: needs landing][target:05/16] u=fx-os-user c=may-6-17 p=1 → [status: needs branch-specific patch][target:05/16] u=fx-os-user c=may-6-17 p=1
https://hg.mozilla.org/releases/mozilla-b2g18/rev/ae27a4f1696d
Whiteboard: [status: needs branch-specific patch][target:05/16] u=fx-os-user c=may-6-17 p=1 → [target:05/16] u=fx-os-user c=may-6-17 p=1
Flags: needinfo?(21)
Flags: in-moztrap-
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.
Attachment #748261 - Attachment is obsolete: false
Justin cherry pick + this patch should make it for b1g-18. I'm unsure of the process to land the cherry pick part?
Attachment #758176 - Attachment is obsolete: true
Flags: needinfo?(21)
> 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.)
Why was the test test_browserElement_inproc_ErrorSecurity.html disabled?
Flags: needinfo?(21)
(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.
Flags: needinfo?(21)
Thank you, I'm experiencing a problem with certificates and was wondering if I could disable my test on Android (mine is intermittently failing).
You need to log in before you can comment on or make changes to this bug.