Closed Bug 1062307 Opened 5 years ago Closed 5 years ago

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

Categories

(Firefox for Android :: General, defect)

ARM
Android
defect
Not set

Tracking

()

VERIFIED FIXED
Firefox 35
Tracking Status
firefox33 --- unaffected
firefox34 + verified
firefox35 --- verified
fennec 34+ ---

People

(Reporter: ioana.chiorean, Assigned: kats)

References

Details

(Keywords: regression, reproducible)

Attachments

(1 file, 2 obsolete files)

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
Flags: needinfo?(ioana.chiorean)
Might be bug 1021804 ...
tracking-fennec: --- → ?
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
Flags: needinfo?(ioana.chiorean)
Summary: Context menu not triggered on long tap on links [nexus 4] → Regression: No context menu action is invoked on long-tapping some links
Blocks: 1021804
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)
cc: to kats the discussion over to here.
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
Assignee: nobody → markcapella
tracking-fennec: ? → 34+
Attached patch bug1062307.diff (obsolete) — Splinter Review
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.
Attachment #8484526 - Flags: review?(bugmail.mozilla)
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!
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
Attached patch Patch (obsolete) — Splinter Review
This seems to do the job.
Assignee: markcapella → bugmail.mozilla
Attachment #8484526 - Attachment is obsolete: true
Attachment #8484526 - Flags: review?(bugmail.mozilla)
Attachment #8485033 - Flags: review?(markcapella)
Attachment #8485033 - Flags: review?(bugs)
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 :)
Attachment #8485033 - Flags: review?(markcapella) → review+
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 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.
Attachment #8485033 - Flags: review?(bugs) → review+
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.
Attached patch Patch v2Splinter Review
Updated to fix indentation and stuff. Carrying r+, will flag checkin-needed since inbound is closed.
Attachment #8485033 - Attachment is obsolete: true
Attachment #8485246 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/d6d25702fb28
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 35
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
Attachment #8485246 - Flags: approval-mozilla-aurora?
Flags: qe-verify+
Status: RESOLVED → VERIFIED
Comment on attachment 8485246 [details] [diff] [review]
Patch v2

Aurora+
Attachment #8485246 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Verified as fixed in
Build: Firefox for Android 34.0a2 (2014-09-17)
Device: Nexus 4 Android 4.4.4
Remove qe-verify flag based on comment 20
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.