1062307 - Regression: No context menu action is invoked on long-tapping some links

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Ioana Chiorean]

Device: Nexus 4 (Android 4.4) 
Aurora 34.0a2 and Nightly 35.0a1 

Steps:
- Go to cnn.com or nightly.mozilla.org
- Long tap on links

Expected results:
- Context menu is triggered.

Actual results:
- No context menu is triggered. 

Note:
- not reproducible on other phones in the office

Comment 1

[Mark Capella [:capella]]

Might be bug 1021804 ...

Comment 2

[Aaron Train [:aaronmt]]

This is reproducible on CNN with their small links. I couldn't reproduce this anywhere else, e.g, theverge.com, digg.com, neowin.net with their mobile sites.

Last good revision: eed9fe35a00d (2014-08-30)
First bad revision: 1db35d2c9a2f (2014-08-31)

Pushlog:
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=eed9fe35a00d&tochange=1db35d2c9a2f

Comment 3

[Aaron Train [:aaronmt]]

I tested the above on my Galaxy S5 (4.4.4) using the 'BREAKING NEWS' news story link on the top of CNN.com using Nightly (09/03)

Comment 4

[Mark Capella [:capella]]

cc: to kats the discussion over to here.

Comment 5

[Kartikaya Gupta (email:[e-mail])]

Hm, based on this discussion and the discussion on bug 1021804 I think we're missing some flags on the contextmenu event. See for example the B2G code at [1]. In particular, we should set event.ignoreRootScrollFrame=true and also set the inputSource to MOZ_SOURCE_TOUCH. The former will prevent Gecko from clamping the event to the CSS viewport and allow targeting items in the bottom/right areas of the screen when zoomed out. The latter will engage event fluffing and so make it easier to hit smaller links.

[1] http://mxr.mozilla.org/mozilla-central/source/dom/ipc/TabChild.cpp?rev=52b4f9600a14#594

Comment 6

[Mark Capella [:capella]]

Attachment: bug1062307.diff

I thought for sure that was the answer, but it doesn't work. So I spent today following the event dispatch logic from:
   nsWindow::OnContextmenuEvent() ->
   nsView::HandleEvent() ->
   nsPresShell::ReleasePointerCaptureCaller::::HandleEvent() ->
   PositionedEventTargeting::FindFrameTargetedByInputEvent() ->
   nsLayoutUtils::GetFramesForArea()

And trying to figure out why that last routine kept insisting on NOT finding a target to return :-(

I swore this wasn't a problem in testing, and I finally just backed out the last modification to the original patch we made in bug 1021804 comment #26
re: |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:|

... And the stupid thing works again ...

This solves the issues I've been seeing, and also the cnn.com issue reported here in comment #2.

Comment 7

[Kartikaya Gupta (email:[e-mail])]

I'm not really satisfied with this patch - I don't like the fact that we're sending a clamped point for the contextmenu. Either the clamping is doing nothing, in which case this should be equivalent to your original patch, or it is doing something, in which case it feels wrong. I'll try to reproduce and investigate tomorrow or over the weekend. Sorry about the delay!

Comment 8

[Kartikaya Gupta (email:[e-mail])]

So the problem appears to be that when we dispatch the contextmenu event it gets targeted to the wrong frame, because it doesn't execute the code at [1]. The contextmenu event in this case is triggered by a touch event, but the contextmenu event itself is a mouse event, so the touch events all execute that code but the contextmenu event does not. I have an idea on how to fix it, trying it out now...

[1] http://mxr.mozilla.org/mozilla-central/source/layout/base/nsPresShell.cpp?rev=01a10676eef1#6919

Comment 9

[Kartikaya Gupta (email:[e-mail])]

Attachment: Patch

This seems to do the job.

Comment 10

[Mark Capella [:capella]]

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

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

Yep, that does it and makes more sense. I wasn't expecting the subtraction of WidgetToScreenOffset() but you're the expert in this area, and this was mostly new to me in any case :)

Comment 11

[Kartikaya Gupta (email:[e-mail])]

I'm not sure if the WidgetToScreenOffset() change makes any difference in this case, but I noticed it was missing while comparing the code in MakeTouchEvent so I figured I might as well add it.

Comment 12

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

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

>-    } else if (aEvent->mClass == eTouchEventClass) {
>+    } else if (aEvent->mClass == eTouchEventClass ||
>+        (aEvent->AsMouseEvent() && aEvent->AsMouseEvent()->inputSource == nsIDOMMouseEvent::MOZ_SOURCE_TOUCH)) {
Somewhat unusual indentation.

>     // Send the contextmenu event.
>     WidgetMouseEvent contextMenuEvent(true, NS_CONTEXTMENU, this,
>                                       WidgetMouseEvent::eReal, WidgetMouseEvent::eNormal);
>     contextMenuEvent.refPoint =
>-        LayoutDeviceIntPoint(RoundedToInt(pt * GetDefaultScale()));
>+        LayoutDeviceIntPoint(RoundedToInt(pt * GetDefaultScale()))
>+        - LayoutDeviceIntPoint::FromUntyped(WidgetToScreenOffset());
I would put - to the end of the previous line.


We really should get rid of that Android specific code path in Presshell.

Comment 13

[Mark Capella [:capella]]

fyi - I just noticed something that may or may not be related to this bug, while testing text selection cases ...

On this page http://adamsteinbaugh.com/2014/09/02/ares-rights-adopts-matroyshka-doll-approach-to-censorious-dmca-takedown-notices/ while scrolled way down and testing long-tapping on my GS3 in the tiny little "comments" seems to fail.

For other "normal" text on the page it seems to be fine.

Comment 14

[Kartikaya Gupta (email:[e-mail])]

Attachment: Patch v2

Updated to fix indentation and stuff. Carrying r+, will flag checkin-needed since inbound is closed.

Comment 15

[Kartikaya Gupta (email:[e-mail])]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d6d25702fb28

Comment 16

[Carsten Book [:Tomcat]]

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

Comment 17

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8485246 [details] [diff] [review]
Patch v2

Approval Request Comment
[Feature/regressing bug #]: bug 1021804
[User impact if declined]: long-pressing on parts of the page often does nothing
[Describe test coverage new/current, TBPL]: no automated testing, manual only
[Risks and why]: pretty low-risk as the patch is small. most of it is fennec-only, the other bit might affect B2G but I would imagine that it should make things more correct there too.
[String/UUID change made/needed]: none

Comment 18

[Lawrence Mandel [:lmandel] (use needinfo)]

Comment on attachment 8485246 [details] [diff] [review]
Patch v2

Aurora+

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/9bc2db4fd695

Comment 20

[Cristina Madaras, QA [:CristinaM]]

Verified as fixed in
Build: Firefox for Android 34.0a2 (2014-09-17)
Device: Nexus 4 Android 4.4.4

Comment 21

[Stefan Deiac]

Remove qe-verify flag based on comment 20