Closed
Bug 864382
Opened 12 years ago
Closed 11 years ago
Send a click event after a contextmenu event if the later has not been caught
Categories
(Firefox OS Graveyard :: General, defect)
Firefox OS Graveyard
General
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)
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)
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
daleharvey
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
justin.lebar+bug
:
review+
daleharvey
:
feedback+
|
Details | Diff | Splinter Review |
(deleted),
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.
Assignee | ||
Comment 1•12 years ago
|
||
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)
Assignee | ||
Comment 2•12 years ago
|
||
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 3•12 years ago
|
||
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-
Assignee | ||
Comment 4•12 years ago
|
||
(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•12 years ago
|
||
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+
Assignee | ||
Comment 6•12 years ago
|
||
(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.
Assignee | ||
Comment 7•12 years ago
|
||
Blocking since user may miss some clicks...
blocking-b2g: --- → leo?
Updated•12 years ago
|
blocking-b2g: leo? → ---
tracking-b2g18:
--- → +
Assignee | ||
Comment 8•12 years ago
|
||
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?
Comment 9•12 years ago
|
||
Vivien, what is the impact for the end-users of this bug?
Flags: needinfo?(21)
Whiteboard: [tef-triage]
Assignee | ||
Comment 10•12 years ago
|
||
(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)
Comment 11•12 years ago
|
||
I don't think we should block on this for 1.0.1
Comment 12•12 years ago
|
||
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+
Updated•12 years ago
|
Whiteboard: [tef-triage]
Updated•12 years ago
|
Whiteboard: u=fx-os-user c=may-6-17 p=0
Updated•12 years ago
|
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
Updated•12 years ago
|
Whiteboard: u=fx-os-user c=may-6-17 p=0
Updated•12 years ago
|
Whiteboard: u=fx-os-user c=may-6-17 p=0 → u=fx-os-user c=may-6-17 p=1
Comment 13•12 years ago
|
||
(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.
Assignee | ||
Comment 14•12 years ago
|
||
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)
Updated•12 years ago
|
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 15•12 years ago
|
||
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)
Updated•12 years ago
|
Attachment #746548 -
Flags: review?(amarchesini) → feedback+
Assignee | ||
Comment 16•12 years ago
|
||
(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•12 years ago
|
||
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
Comment 18•12 years ago
|
||
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•12 years ago
|
||
> 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.
Updated•12 years ago
|
Attachment #746548 -
Flags: review?(justin.lebar+bug) → review-
Updated•12 years ago
|
Assignee: 21 → justin.lebar+bug
Assignee | ||
Comment 20•12 years ago
|
||
(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•12 years ago
|
||
>> + 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...
Assignee | ||
Comment 22•12 years ago
|
||
(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 ?
Assignee | ||
Comment 23•12 years ago
|
||
I guess I should have looked closer at how the CPP code works there. I'm too used to JS. Sorry.
Comment 24•12 years ago
|
||
Vivien, I don't see you on IRC, but you're clearly lurking in the weeds here...
Comment 25•12 years ago
|
||
Updated•12 years ago
|
Attachment #748162 -
Attachment is obsolete: true
Comment 26•12 years ago
|
||
Attachment #746548 -
Attachment is obsolete: true
Attachment #748159 -
Attachment is obsolete: true
Attachment #748164 -
Flags: review?(bent.mozilla)
Comment 27•12 years ago
|
||
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-
Comment 28•12 years ago
|
||
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•12 years ago
|
||
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•12 years ago
|
||
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•12 years ago
|
||
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•12 years ago
|
||
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•12 years ago
|
||
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.
Updated•12 years ago
|
Attachment #748164 -
Attachment is obsolete: true
Updated•12 years ago
|
Attachment #748261 -
Attachment is obsolete: true
Updated•12 years ago
|
Attachment #748264 -
Attachment is obsolete: true
Assignee | ||
Comment 34•12 years ago
|
||
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•12 years ago
|
||
> 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...
Assignee | ||
Comment 36•12 years ago
|
||
(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.
Assignee | ||
Comment 37•12 years ago
|
||
(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•12 years ago
|
||
> 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.
Assignee | ||
Comment 39•12 years ago
|
||
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)
Assignee | ||
Comment 40•12 years ago
|
||
In order to work the patch needs the bits from #c30 (for the nsIDOMWindowUtils part) as well as a little Gaia patch.
Updated•12 years ago
|
Attachment #748323 -
Attachment is obsolete: true
Updated•12 years ago
|
Assignee: justin.lebar+bug → 21
Comment 41•12 years ago
|
||
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)
Updated•12 years ago
|
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
Assignee | ||
Comment 42•12 years ago
|
||
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)
Assignee | ||
Comment 43•12 years ago
|
||
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 44•12 years ago
|
||
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 45•12 years ago
|
||
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+
Updated•12 years ago
|
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
Assignee | ||
Comment 46•12 years ago
|
||
Comment on attachment 748796 [details] [diff] [review]
Gaia part
I probably need dale's stamp here too.
Attachment #748796 -
Flags: review?(dale)
Updated•12 years ago
|
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
Updated•12 years ago
|
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
Updated•12 years ago
|
Attachment #748796 -
Flags: review?(dale) → review+
Comment 47•12 years ago
|
||
(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
Comment 48•12 years ago
|
||
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?
Updated•12 years ago
|
Flags: needinfo?(akeybl)
Comment 49•12 years ago
|
||
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
Assignee | ||
Comment 50•12 years ago
|
||
(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.
Assignee | ||
Comment 51•11 years ago
|
||
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
Assignee | ||
Comment 52•11 years ago
|
||
I have not disable the test on Android sorry. Cross-bug comments.
Assignee | ||
Comment 53•11 years ago
|
||
Comment 54•11 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Comment 55•11 years ago
|
||
This has quite a few conflicts on b2g18.
Keywords: branch-patch-needed
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
Assignee | ||
Comment 56•11 years ago
|
||
Comment 57•11 years ago
|
||
status-b2g18:
--- → fixed
status-b2g18-v1.0.0:
--- → wontfix
status-b2g18-v1.0.1:
--- → wontfix
status-b2g-v1.1hd:
--- → affected
status-firefox22:
--- → wontfix
status-firefox23:
--- → wontfix
status-firefox24:
--- → fixed
Keywords: branch-patch-needed
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
Comment 58•11 years ago
|
||
Updated•11 years ago
|
Updated•11 years ago
|
Keywords: branch-patch-needed
Updated•11 years ago
|
Flags: needinfo?(21)
Updated•11 years ago
|
Flags: in-moztrap-
Assignee | ||
Comment 59•11 years ago
|
||
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
Assignee | ||
Comment 60•11 years ago
|
||
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)
Comment 61•11 years ago
|
||
> 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•11 years ago
|
||
Comment 63•11 years ago
|
||
Comment 64•11 years ago
|
||
Why was the test test_browserElement_inproc_ErrorSecurity.html disabled?
Updated•11 years ago
|
Flags: needinfo?(21)
Assignee | ||
Comment 65•11 years ago
|
||
(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)
Comment 66•11 years ago
|
||
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.
Description
•