Closed Bug 1021804 Opened 5 years ago Closed 5 years ago

Long press on news story links invoke context menu while at the same time load the page

Categories

(Firefox for Android :: General, defect)

29 Branch
ARM
Android
defect
Not set

Tracking

()

VERIFIED FIXED
Firefox 34
Tracking Status
firefox34 --- verified
firefox35 --- verified
firefox36 --- verified
firefox37 --- verified

People

(Reporter: chaaban.saad, Assigned: capella)

References

Details

Attachments

(1 file, 7 obsolete files)

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.9; rv:29.0) Gecko/20100101 Firefox/29.0 (Beta/Release)
Build ID: 20140506152807

Steps to reproduce:

First, this problem especially only happens in Pages from the Kinja Network (Lifehacker, gizmodo etc...)

i long clicked (kept my finger on the touchscreen) an article/link


Actual results:

opened the menu and load the link in the background.


Expected results:

just opening the menu and ask me what to do.
OS: Mac OS X → Android
Hardware: x86 → ARM
Might be an website issue. Mike do you  have some time to check what is happening here?
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: needinfo?(miket)
I see this on Lifehacker, long-tapping on a page story URL invokes the context-menu while also loading it at the same time in the foreground.

Need to inspect what the click listeners are doing, likely we're just honouring what they want.
Summary: long press a link open it instead of the menu → Long press on news story links invoke context menu while at the same time load the page
The troublesome code is here, in Main-en-US-9d95185cfabd902f24db3c4d6b7ad3c0.js:

FastClick.prototype.onTouchEnd = function(a) {
  var b, c, d, e, f, g = this.targetElement;
  if (!this.trackingClick) return !0;
  if (a.timeStamp - this.lastClickTime < this.tapDelay) return this.cancelNextClick = !0,
  !0;
  if (this.cancelNextClick = !1, this.lastClickTime = a.timeStamp, c = this.trackingClickStart,
  this.trackingClick = !1, this.trackingClickStart = 0, deviceIsIOSWithBadTarget && (f = a.changedTouches[0],
  g = document.elementFromPoint(f.pageX - window.pageXOffset, f.pageY - window.pageYOffset) || g,
  g.fastClickScrollParent = this.targetElement.fastClickScrollParent), d = g.tagName.toLowerCase(),
  "label" === d) {
      if (b = this.findControl(g)) {
          if (this.focus(g), deviceIsAndroid) return !1;
          g = b;
      }
  } else if (this.needsFocus(g)) return a.timeStamp - c > 100 || deviceIsIOS && window.top !== window && "input" === d ? (this.targetElement = null,
  !1) : (this.focus(g), this.sendClick(g, a), deviceIsIOS && "select" === d || (this.targetElement = null,
  a.preventDefault()), !1);
  return deviceIsIOS && !deviceIsIOS4 && (e = g.fastClickScrollParent, e && e.fastClickLastScrollTop !== e.scrollTop) ? !0 : (this.needsClick(g) || (a.preventDefault(), this.sendClick(g, a)), !1);
}

The most relevant part being: 

return deviceIsIOS && !deviceIsIOS4 && (e = g.fastClickScrollParent, e && e.fastClickLastScrollTop !== e.scrollTop) ? !0 : (this.needsClick(g) || (a.preventDefault(), this.sendClick(g, a)), !1);

If you're an iOS (not v4) device, return true. Otherwise, preventDefault, dispatch a synthetic click event, and return false. We're falling into the second condition here.

So there's some UA sniffing going on, but my question is, should touchEnd fire when a finger lets go of the screen after a context menu is opened? If we didn't fire touchEnd in this case, we wouldn't see the bug. 

(I'm guessing this is the difference with Chrome Mobile but I'll need to make a test case.)
Yep, see https://miketaylr.com/bzla/context.html. If you long press to open the context menu, as soon as you life your finger you get a "touchend" event. This does not happen in Chrome Mobile.

Kats, is this intentional on our behalf? Or something we might consider changing for compat?
We could fire touch-cancel events (iff we fire a context menu event...) Does Chrome do that?
Chrome just opens a context menu and does not visit the link automatically.
OK, I added listeners for touchcancel and contextmenu events to https://miketaylr.com/bzla/context.html:

iOS Safari: fires touchcancel
Chrome Mobile: fires contextmenu -> touchcancel
FxA Nightly: fires contextemenu -> touchend
AFAIK we should be able to change this. I won't have time to do this for the forseeable future though, too busy with B2G stuff.
Flags: needinfo?(bugmail.mozilla)
well dang, i thought this was fixed somewhere along the way. This happens for websites like The Guardian and The Washington Post for my GS3 but not my N7.
Attached patch bug1021804.diff (obsolete) — Splinter Review
This is basically your suggestion from comment #5 ... it fixs my GS3 w/o regressing my N7, and w/o mucking up pages that weren't exhibiting this issue to begin with.

As in comment #7 we used to fire contextemenu -> touchend
Now we actually fire contextemenu -> touchcancel -> touchend
Attachment #8474492 - Flags: review?(wjohnston)
This peaked my interest a bit. This isn't probably the right place for this fix. If we're going to cancel touch events, we should really cancel them. Follow me through the winding bowels of touch events....

When the events come in they start in the LayerView. We run them through some options for handlers including sending them to the panZoomController:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/gfx/LayerView.java#222

the panZoomController there is the man who will decide if this is a long tap or not here:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/gfx/JavaPanZoomController.java#1345

so what we really want to do is send a touchcancel event from right there. You might try calling
MotionEvent e = MotionEvent.obtain(event);
e.setAction(MotionEvent.ACTION_CANCEL);
getLayerViewSomehow().onTouchEvent(e);

right there. It should come through here and cancel... everything... so that JavaPanZoomController is in a good state and then get sent to Gecko. It WON'T stop Android from firing the next MOVE event for this finger though. When we receive that, I expect JavaPanZoomController to get a bit mad, but it will be interesting to see what breaks.

Alternatively, maybe you can convert the current event to a cancel one and send it on its way, and then set a flag that tells the JavaPanZoomController to consume all touch events for this touch "session".... (i.e. return true so that the LayerView doesn't send them to Gecko).

kats might have better ideas. There are other cases where we'd like to do that as well (for instance, whenever we start panning or zooming, we should also fire a cancel event to the page and stop sending touches for that finger).
Flags: needinfo?(bugmail.mozilla)
Hm, so this is kind of interesting. As Wes says, the events go through something like this:

LayerView -> PanZoomController -> Gecko -> browser.js -> content

and at each level here we want the events to be "consistent". Mark's patch inserts the touchcancel at the browser.js -> content level, but other touchmove/touchend events might still come through, in which case it would be the responsibility of browser.js to eat those events and make sure content never sees them (or to somehow magically ensure that content can deal with inconsistent events).

Wes' suggestion is to inject a cancel at the LayerView -> PZC level, but again Android will keep sending us ACTION_MOVE and ACTION_UP events after that and it would be the LayerView responsible for filtering those out, or to ensure that the PZC code can deal with inconsistent events. To be honest, the PZC code might be able to but I'm not sure if it can - it certainly wasn't designed with that in mind.

My suggestion is to inject a cancel at the PZC -> Gecko level, because the Gecko code is in fact designed to deal with an inconsistent event stream. The code in nsPresShell and EventStateManager will ignore touch events for touch points that have already been cancelled or lifted, and that code should be robust enough to handle this case. So what I would suggest is to create a cancel event like how wes described above and send it via GeckoAppShell.sendEventToGecko right after sending the Gesture:LongPress message in JavaPanZoomController.

This approach also has the advantage that you don't need extra plumbing to get a hold of the LayerView in the JavaPanZoomController code. As far as the PanZoomController code is concerned it will go on as before, and browser.js/content should see no further touch events after the cancel.
Flags: needinfo?(bugmail.mozilla)
Attached patch bug1021804v1.diff (obsolete) — Splinter Review
Thank you both for taking the time to do those write-ups! I appreciate knowing more about how Gecko/Android handle Motion/Pointer/Touch/(Pen?) etc. events :-D

This seems to do the trick, (solving the original, no regressions I can see, etc). And bonus points it does avoid us sending |touchend|.

Note, instead of sending contextmenu -> touchcancel events, the code as suggested / attached sends touchcancel -> contextmenu events, based on testing with mike's test provided in comment #4.

Quick-changing the "Gesture:LongPress" to occur after the cancelEvent doesn't seem to affect this outcome.
Assignee: nobody → markcapella
Attachment #8474492 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #8474492 - Flags: review?(wjohnston)
Attachment #8475028 - Flags: review?(wjohnston)
Attachment #8475028 - Flags: review?(bugmail.mozilla)
Attachment #8475028 - Flags: review?(bugmail.mozilla) → review+
Comment on attachment 8475028 [details] [diff] [review]
bug1021804v1.diff

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

Sorry, I was thinking about this a bit more. Its possible that there's nothing at the point you tapped to show a context menu on. With this code, we'll cancel touches for no reason if you touch in a whitespace for awhile. I wonder if we instead should return something from JS if that happens, and then use that to fire this event. i.e.
http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/browser.js#2378

That's where your original patch was though :( I think your original would have needed to use DOMWindowUtils.sendTouchEvent in order to travel the code path's kats is talking about (i.e. prevent all future mousemoves from this finger). But I think a slightly cleaner version might actually be to just use our new fancy GeckoRequest to send a message from Java and expect a response from JS:

http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/util/GeckoRequest.java#12
http://mxr.mozilla.org/mozilla-central/source/mobile/android/modules/Messaging.jsm#47

Of course, there's also text selection and other things to worry about in browser.js. Opinions?
Attachment #8475028 - Flags: review+ → review-
I agree the original patch only cancelled in the block where we've realized we're triggering contextmenu, and this one triggers regardless of that outcome.

Enhancing this patch using the new GeckoRequest (I'm reading this: http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/GeckoAppShell.java#414) to conditionally canceltouch in java on response back from js seems like a good thing.

I didn't catch the textselection implications.
Bug convergence ... nice :-)
Attachment #8475028 - Flags: review?(wjohnston)
Attached patch bug1021804v2.diff (obsolete) — Splinter Review
Moving back from Bug 1055548 ... This indeed is cleaner with your suggestions.

I left the pt param as an nsIntPoint while passing to OnContextmenuEvent() as I need it defined that way to retrieve the target element using FindWindowForPoint().

Then I just did a quick conversion in OnContextmenuEvent() for the population of the new WidgetMouseEvent.refPoint.

fyi(?) I don't see where the contextmenu messages "before-build-contextmenu" and "context-menu-not-shown" are actually consumed. Obsolete code? Planned-for-future? Or did my google-foo and MXR-foo let me down?
Attachment #8475028 - Attachment is obsolete: true
Attachment #8477939 - Flags: review?(bugmail.mozilla)
Comment on attachment 8477939 [details] [diff] [review]
bug1021804v2.diff

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

This is very close! A few more simplifications below, but you can carry the r+ assuming there's no problem implementing those. Also - the JS changes look fine to me but I don't know that code too well so wesj should definitely look at that bit if nothing else.

::: mobile/android/base/GeckoEvent.java
@@ +465,5 @@
> +     * The keepInViewCoordinates parameter can be set to false to convert from the Java
> +     * coordinate system (device pixels relative to the LayerView) to a coordinate system
> +     * relative to gecko's coordinate system (CSS pixels relative to gecko scroll position).
> +     */
> +    public static GeckoEvent createLongPressEvent(MotionEvent m, boolean keepInViewCoordinates) {

You can get rid of keepInViewCoordinates on this. I only added it on the MOTION_EVENT event in order to send the events directly to native APZ, but we won't be doing that here.

@@ +467,5 @@
> +     * relative to gecko's coordinate system (CSS pixels relative to gecko scroll position).
> +     */
> +    public static GeckoEvent createLongPressEvent(MotionEvent m, boolean keepInViewCoordinates) {
> +        GeckoEvent event = GeckoEvent.get(NativeGeckoEvent.LONG_PRESS);
> +        event.initLongPressEvent(m, keepInViewCoordinates);

You can just call initMotionEvent(m, false) here. No need for initLongPressEvent which basically duplicates the code from initMotionEvent. If you want, you can add an assert or log or something here if the motion event's action is not DOWN but it doesn't really matter because in the C++ code we only need the pointer data anyway.

::: widget/android/nsAppShell.cpp
@@ +397,5 @@
>  
> +    // Long-press event starts contextmenu processing.
> +    case AndroidGeckoEvent::LONG_PRESS: {
> +        nsCOMPtr<nsIObserverService> obsServ = mozilla::services::GetObserverService();
> +        obsServ->NotifyObservers(nullptr, "before-build-contextmenu", nullptr);

This works, but it seems like we could also put this observer service stuff in the handler in nsWindow.cpp (or move the stuff from nsWindow.cpp here). That way it would all be together. It seems kind of arbitrary to dispatch the before-build-contextmenu from one file and do the rest of the processing in the other file. I would prefer doing it all in nsWindow because then you wouldn't even need to touch this file as the default case handler for this switch statement sends the event to nsWindow already.

::: widget/android/nsWindow.cpp
@@ +863,5 @@
>  
> +        // LongPress events mostly trigger contextmenu options, but can also lead to
> +        // textSelection processing.
> +        case AndroidGeckoEvent::LONG_PRESS: {
> +            nsIntPoint pt(0,0);

Don't need the (0,0) as nsIntPoint will initialize to 0,0 by default.

@@ +1033,5 @@
> +
> +    nsEventStatus contextMenuStatus;
> +    DispatchEvent(&contextMenuEvent, contextMenuStatus);
> +
> +    // If the contextenu event was consumed (preventDefault issued), we follow with a

s/contextenu/contextmenu/
Attachment #8477939 - Flags: review?(bugmail.mozilla) → review+
The part in nsWindow where I upscale the touch-points isn't quite right.

// The long-press touch-point is in CSS coordinates. It needs to be scaled to Layout.
...
nsWindow *target = win->FindWindowForPoint(pt);
if (target) { }

... is often producing null targets for long-clicks on images, noticeably when not zoomed.

I'll spent some more time digging/following the various points where we upscale/downscale "pt"'s.
status...

   One issue I have on my N7 is obvious when long-tapping the right edge of a full screen .jpg.

   Basically, the zoom factors we depend on from Java in GeckoLayerCLient.convertViewPointToLayerPoint(), |viewportMetrics.zoomFactor| and |geckoViewport.zoomFactor| report number range from 1.875 and up depending on zoom.

   In nsWindow.cpp I'm using |win->GetDefaultScale()| to do reverse calculate CSSToLayoutDeviceScale, and GetDefaultScale() always reports 2.000 (getDisplayMetrics().density).

   So obviously (x / 1.875 * 2) will re-construct x too large and appear to push touchevents off the screen.
Look at other code that calls FindWindowFromPoint. It looks like it just uses the CSS pixels and clamps it to the widget bounds.
For the FindWindowForPoint() ... Yes I saw that ... but >ouch<.

Also, currently, we send the longpress event's (x,y) on an observable broadcast and gecko is receiving those as larger than 600 in aData.

When sending actual events, the clientX/clientY between .cpp and .js gets further scaled down by half, which is why I'm upscaling in nsWindow first. Upscaling from >600 to >1200 annoys things, as the event never makes it to gecko, getting lost somewhere between c++ and .js.

So I can't provide the specific x/y that browser.js was previously receiving via JSON without additional mucking around that I haven't decided on yet :(

Am I making this too complicated?
Not sure I fully understand. It may help to read this blog post on the coordinate systems:

https://staktrace.com/spout/entry.php?id=800

The old Gesture:LongPress event contained coordinates in CSS pixels relative to gecko's scroll offset. (That is what convertViewPointToLayerPoint ouptuts, despite what the name implies). The LONG_PRESS AndroidGeckoEvent that your patch adds does the same thing. Events dispatched into gecko (i.e. WidgetMouseEvent and WidgetTouchEvent) require LayoutDevicePixel values. The WidgetTouchEvent generated by ae.MakeTouchEvent already is converted from CSSPixel space to LayoutDevicePixel space already. The WidgetMouseEvent that you use to dispatch the contextmenu also uses LayoutDevicePixel because you are multiplying in the widget scale manually. So all those bits are fine and should work as expected.

The only thing that should need fixing is the call to FindWindowForPoint. To be honest I think nsWindow::mBounds are probably in screen pixels but I'm not 100% sure of that. It looks like all the other bits of code pass in CSS pixels, clamped to gAndroidBounds, probably because that's the only thing that works. If we had the original view coordinates from Java we could probably use that without clamping but we don't have that in the C++ code. For now I think it's better to just copy that clamping stuff into your patch, and then later we can try to clean it up and document what coordinate mBounds and FindWindowForPoint are in.
Ok, let's do it then :) I'm working in a new area with gfx, so I may just be being overly cautious to be sure I'm correct. ( I may be worrying about edge cases that don't realistically exist :-D )

I'll ask you to r+ once again to make sure after all that, I tidied up properly, and here's where we rope wesj into it for his r+ on the gecko bits.
Attached patch bug1021804v2.diff (obsolete) — Splinter Review
Attachment #8477939 - Attachment is obsolete: true
Attachment #8478153 - Flags: review?(wjohnston)
Attachment #8478153 - Flags: review?(bugmail.mozilla)
Comment on attachment 8478153 [details] [diff] [review]
bug1021804v2.diff

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

::: mobile/android/base/GeckoEvent.java
@@ +424,5 @@
> +    /**
> +     * Creates a GeckoEvent that contains the data from the LongPressEvent.
> +     * The keepInViewCoordinates parameter can be set to false to convert from the Java
> +     * coordinate system (device pixels relative to the LayerView) to a coordinate system
> +     * relative to gecko's coordinate system (CSS pixels relative to gecko scroll position).

Remove stuff about keepInViewCoordinates; just say it dispatches a long-press event in CSS pixels relative to gecko's scroll position.

::: widget/android/nsWindow.cpp
@@ +878,5 @@
> +            // The long-press touch-point is in CSS coordinates. It needs to be scaled to Layout.
> +            CSSToLayoutDeviceScale scale = win ?
> +                win->GetDefaultScale() : CSSToLayoutDeviceScale(1.0);
> +            pt.x = pt.x * scale.scale;
> +            pt.y = pt.y * scale.scale;

So.. what you're doing here is taking the point, scaling it, clamping it, using it for FindWindowForPoint, and then dispatching it in the contextmenu event. What you should be doing is:
point -> clamp -> FindWindowForPoint
point -> scale -> contextmenu event

That is, the point passed to FindWindowForPoint should be clamped but not scaled, and the point sent in the contextmenu event should be scaled and not clamped.

I would recommend removing the scaling bit from here and also removing the nsIntPoint argument to OnContextmenuEvent...

@@ +1034,5 @@
> +
> +    // Send the contextmenu event.
> +    WidgetMouseEvent contextMenuEvent(true, NS_CONTEXTMENU, this,
> +                                      WidgetMouseEvent::eReal, WidgetMouseEvent::eNormal);
> +    contextMenuEvent.refPoint = LayoutDeviceIntPoint(pt.x, pt.y);

... and here, pull out the point again from ae->Points() and do the scaling by the widget scale.
Attachment #8478153 - Flags: review?(bugmail.mozilla) → review-
Attached patch bug1021804v_KATS_WORKS.diff (obsolete) — Splinter Review
Ok, holding back r? from wesj until I get your approval first :)
Attachment #8478153 - Attachment is obsolete: true
Attachment #8478153 - Flags: review?(wjohnston)
Attachment #8478429 - Flags: review?(bugmail.mozilla)
Comment on attachment 8478429 [details] [diff] [review]
bug1021804v_KATS_WORKS.diff

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

This works but we can do a teeny bit better with more self-documenting code :)

Also keep in mind that GetDefaultScale is not always the same as GetDefaultScaleInternal, because setting a pref can change the return value of GetDefaultScale but it won't affect GetDefaultScaleInternal.

::: widget/android/nsWindow.cpp
@@ +1025,5 @@
> +nsWindow::OnContextmenuEvent(AndroidGeckoEvent *ae)
> +{
> +    nsRefPtr<nsWindow> kungFuDeathGrip(this);
> +
> +    nsIntPoint pt;

Make this a CSSPoint

@@ +1034,5 @@
> +
> +    // The long-press touch-point is in CSS coordinates. It needs to be scaled to Layout.
> +    double scale = GetDefaultScaleInternal();
> +    pt.x = pt.x * scale;
> +    pt.y = pt.y * scale;

Remove these four lines (see below for what replaces them)

@@ +1039,5 @@
> +
> +    // Send the contextmenu event.
> +    WidgetMouseEvent contextMenuEvent(true, NS_CONTEXTMENU, this,
> +                                      WidgetMouseEvent::eReal, WidgetMouseEvent::eNormal);
> +    contextMenuEvent.refPoint = LayoutDeviceIntPoint(pt.x, pt.y);

contextMenuEvent.refPoint = LayoutDeviceIntPoint(pt * GetDefaultScale());
Attachment #8478429 - Flags: review?(bugmail.mozilla) → review+
Attached patch bug1021804v_KATS_WORKS.diff (obsolete) — Splinter Review
I couldn't build with LayoutDeviceIntPoint(pt * GetDefaultScale()) where pt is a CSSPoint.

I strined my c++ reading through the templated typedefs and the closest I can get is the attached.
Attachment #8478429 - Attachment is obsolete: true
Oh my bad, sorry! What you have is fine, but what I should have said was mozilla::gfx::RoundedToInt(pt * GetDefaultScale()). [1]

[1] http://mxr.mozilla.org/mozilla-central/source/gfx/2d/Point.h?rev=d02d8790e5c5#100
Attached patch bug1021804v_KATS_WORKS.diff (obsolete) — Splinter Review
Awesome! I tightened that up, and once again we're over to wesj for r? on the gecko bits  :-D
Attachment #8478523 - Attachment is obsolete: true
Attachment #8478548 - Flags: review?(wjohnston)
Comment on attachment 8478548 [details] [diff] [review]
bug1021804v_KATS_WORKS.diff

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

This looks pretty good, but I still want the frontend to have a say in whether we send the event to Gecko.

::: mobile/android/chrome/content/browser.js
@@ +2043,5 @@
>      DEFAULT_HTML5_ORDER: -1, // Sort order for HTML5 context menu items
>  
>      init: function() {
> +      BrowserApp.deck.addEventListener("contextmenu", this.sendToContent.bind(this), true);
> +      BrowserApp.deck.addEventListener("contextmenu", this.show.bind(this), false);

Hmm.. I would actually rather we just moved everything into show. The reason it was the way it was before was that we avoided sending the contextmenu event if no contextmenu was going to be shown. This alters that, and removes the need to handle anything beforehand.

I'm not sure what I think of that. Is it ok for us to fire a contextmenu event if we're not showing a contextmenu? You could avoid this change pretty easily I think though. Below this where you have:

if (this.shouldShow()) {
  // do nothing
}

you can actually return something to java. i.e.  in onLongPress in java you would so something like:

sendRequestToGecko(new GeckoRequest("Gesture:LongPress", "{x:x, y:y}") {
  @Override
  public void onResponse(NativeJSObject nativeJSObject) {
    if (nativeJSObject.optBoolean("hasMenu")) {
      GeckoEvent e = GeckoEvent.createLongPressEvent(motionEvent);
      GeckoAppShell.sendEventToGecko(e);
    }
  }
});

In JS you would register here with:

RequestService.addListener((event) => {
  this.setupMenu(event);
  return { hasMenu: this.shouldShow() };
}, "Gesture:LongPress");
Attachment #8478548 - Flags: review?(wjohnston) → review-
(In reply to Wesley Johnston (:wesj) from comment #32)
> I'm not sure what I think of that. Is it ok for us to fire a contextmenu
> event if we're not showing a contextmenu?

Yes, I think so. B2G does it.
I know a bug was filed on us for doing it long ago. But B2G's experience suggests it was just QA filing, not sites depending on anything. I'm fine with doing things this way, but we should still move all of the sendToContent code into show.
fyi - I'm not sure how this plays into things then, but I'd been tracking down a mochitest failure that shows up when I pushed the patch to Try for yucks. I had to add this guard to resolve it:

    sendToContent: function(event) {
      // Android Long-tap contextmenu events provide clientX/Y data. This is not provided
      // by mochitest: test_browserElement_inproc_ContextmenuEvents.html.
      if (!event.clientX || !event.clientY) {
        return;
      }

Since we'd now be listening for all contextmenu events, and previously only when we expected them / generated them to ourselves.

Now that that's resolved, I'll review the above and catch up to the new thoughts under discussion.
Attached patch bug1021804.diffSplinter Review
In response to wes, I'm guessing this is how you'd like the methods combined ...

This seems to work for me locally (no run through Try yet) ... though I wonder why we were trapping the event on the way down (useCapture) previously?

Also, without suggesting it as in-scope of this patch, do we still need to hold a weak-ref to the target internal to browser.js, versus passing it around internally tied to the event?
Attachment #8478548 - Attachment is obsolete: true
Attachment #8481299 - Flags: review?(wjohnston)
Comment on attachment 8481299 [details] [diff] [review]
bug1021804.diff

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

Assuming try is happy, we can try this. I want to keep our target code here. Since we do our own target redirection in JS, I don't trust that the event target sent to us from Gecko will match what is highlighted on the screen.

::: mobile/android/chrome/content/browser.js
@@ +2397,5 @@
> +        BrowserEventHandler._cancelTapHighlight();
> +
> +        // Consume / preventDefault the event, and show the contextmenu.
> +        if (!event.defaultPrevented) {
> +          event.preventDefault();

Why do we need to do this?
Attachment #8481299 - Flags: review?(wjohnston) → review+
(In reply to Wesley Johnston (:wesj) from comment #37)
> Assuming try is happy, we can try this. I want to keep our target code here.
> Since we do our own target redirection in JS, I don't trust that the event
> target sent to us from Gecko will match what is highlighted on the screen.

We should get rid of that code, btw. There is code in Gecko that does that now. (layout/base/PositionedEventTargeting.cpp)
re: Why do we need to do this?
... It was carried over from previous code where we issued the contextmenu event out and back to our own listener ... 

https://tbpl.mozilla.org/?tree=Try&rev=26fde9299f33
https://hg.mozilla.org/mozilla-central/rev/070491691b4e
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 34
Long pressing on links seems broken on latest nightly. Regression from this?
Flags: needinfo?(markcapella)
I'm afraid the short answer is going to be yes. I notice problems with longtaps positioned to the right and bottom of the screen, particularly when "gecko-zoomed" (not sure that's the term) such as what you'd see when a bugzilla page is displayed.

I'm reviewing what I was looking at previously in comment #20. I thought our subsequent modifications had fixed that, though maybe we lost something around comment #26 when we discussed:

...
So.. what you're doing here is taking the point, scaling it, clamping it, using it for FindWindowForPoint, and then dispatching it in the contextmenu event. What you should be doing is:
point -> clamp -> FindWindowForPoint
point -> scale -> contextmenu event
...
Flags: needinfo?(markcapella)
Hm, my problems seem to have gone away after a restart. To me it seems like maybe we get stuck in a bad state somehow and then it stops working until the browser is restarted. I haven't seen anything that seems restricted to the right/bottom part of the screen.
I haven't noticed what you're seeing. But, I can repro something like this ... if I long press the miketaylr link (in comment #4) here:
https://www.dropbox.com/s/pxhrkog9tjlug1e/linkOnTop.png?dl=0

All is well ... a brief log shows ... onContextmenuEvent() fires the scaled-up event, and browser.js receives it.
nsWindow::OnGlobalAndroidEvent(LONG_PRESS) Starts ...
  nsWindow::OnContextmenuEvent() Starts ...
  nsWindow::OnContextmenuEvent()           geckoPoint: (X=325, Y=42)
  nsWindow::OnContextmenuEvent()    scaled geckoPoint: (X=650, Y=84)

  nsWindow::OnContextmenuEvent()    SENDS MOUSE EVENT ...
    browser: show() Starts ...
    browser: show()    clientX/Y:       [325 , 42 ]
    browser: show() Finishes ...
  nsWindow::OnContextmenuEvent()    CANCELS TOUCH EVENT ...

  nsWindow::OnContextmenuEvent() Finishes ...
nsWindow::OnGlobalAndroidEvent(LONG_PRESS) Finishes ...


With the screen and link scrolled down as here:
https://www.dropbox.com/s/hrss9hx6t3bpmla/linkAtBottom.png?dl=0

My log shows the event being sent and never received in browser.js:
nsWindow::OnGlobalAndroidEvent(LONG_PRESS) Starts ...
  nsWindow::OnContextmenuEvent() Starts ...

  nsWindow::OnContextmenuEvent()           geckoPoint: (X=351, Y=1350)
  nsWindow::OnContextmenuEvent()    scaled geckoPoint: (X=702, Y=2700)

  nsWindow::OnContextmenuEvent()    SENDS MOUSE EVENT ...

  nsWindow::OnContextmenuEvent() Finishes ...
nsWindow::OnGlobalAndroidEvent(LONG_PRESS) Finishes ...
Depends on: 1062307
See Also: → 1100932
Long pressing on the link from comment 7:
Firefox for Android 37.0a1 (2014-12-15):fires contextmenu -> touchcancel
Firefox for Android 36.0a2 (2014-12-15):fires contextmenu -> touchcancel
Firefox for Android 35.b1 (2014-12-15):fires contextmenu -> touchcancel
Firefox for Android 34 (2014-12-15):fires contextmenu -> touchcancel, so I will mark this as Verified fixed.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.