1233056 - Long tapping on a link will select a different link from the page

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Flaviu Cos]

Environment: 
Device:  (Android Asus Transformer Pad (Android 4.2.1);
Build: Aurora 45.0a2 (2015-12-15);

Steps to reproduce:
1. Go to http://people.mozilla.com/~nhirata/html_tp/listlinks.html
2. Long tap on a link.

Expected result:
The tapped link is highlighted and the context menu is triggered.

Actual result:
The long tap action is made on a different position on screen (different link is selected).

Notes:
This issue is intermittent.

Comment 1

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

I wonder if this could be related to the discussion in bug 1222234 comment 10.

Flaviu, can you reproduce this on a wide range of devices, or just the one?

Comment 2

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Or maybe related to bug 1232893.

Comment 3

[Flaviu Cos]

Ran into similar behavior on Google search results using Nexus 4 (Android 5.0.1).

Comment 4

[James Willcox (:snorp) ([e-mail]) (he/him)]

This is on Aurora 45, so probably not APZ related. Kats, any ideas?

Comment 6

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

Not off the top of my head. It's probably an accidental regression from the APZ work but getting a window would be helpful.

Comment 7

[Matt Brubeck (:mbrubeck)]

From bisecting mozilla-central with "ac_add_options --disable-android-apz":

The first bad revision is:
changeset:   276168:73246a388339
user:        Botond Ballo <[e-mail]>
date:        Fri Nov 27 21:39:07 2015 -0500
summary:     Bug 1228597 - Remove the MOZ_SINGLE_PROCESS_APZ define. r=tn

Comment 8

[Matt Brubeck (:mbrubeck)]

Direct link to the commit: https://hg.mozilla.org/mozilla-central/rev/73246a388339

Comment 9

[Timothy Nikkel (:tnikkel)]

Hmm, did that land before or after we enabled APZ for fennec?

Comment 10

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

It landed after we enabled APZ for fennec on nightly

Comment 11

[Timothy Nikkel (:tnikkel)]

Then shouldn't removing the ifdefs not change anything?

Comment 12

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

For the APZ-enabled case, yes. For the APZ-disabled case, there's a bunch of code (previously guarded by MOZ_SINGLE_PROCESS_APZ) that is now getting run. And that's causing a regression, when APZ is disabled.

Comment 13

[Randall Barker [:rbarker]]

Attachment: 0001-Bug-1233056-Long-tapping-on-a-link-will-select-a-different-link-from-the-page-15123117-55ff145.patch

Comment 14

[Randall Barker [:rbarker]]

Try with JPZ enabled: https://treeherder.mozilla.org/#/jobs?repo=try&revision=1adab20378a1

Comment 16

[Randall Barker [:rbarker]]

Judging by that try, I missed something.

Comment 17

[Randall Barker [:rbarker]]

Attachment: 0001-Bug-1233056-Long-tapping-on-a-link-will-select-a-different-link-from-the-page-16010409-2a9f79e.patch

Comment 18

[Randall Barker [:rbarker]]

Try again: https://treeherder.mozilla.org/#/jobs?repo=try&revision=2ae66fe843d7

Comment 19

[Randall Barker [:rbarker]]

Attachment: 0001-Bug-1233056-Long-tapping-on-a-link-will-select-a-different-link-from-the-page-16010409-45228fc.patch

Fix typo.

Comment 20

[Randall Barker [:rbarker]]

Attachment: 0001-Bug-1233056-Long-tapping-on-a-link-will-select-a-different-link-from-the-page-16010416-00bd839.patch

Address comments.

Comment 21

[Randall Barker [:rbarker]]

Attachment: 0001-Bug-1233056-Long-tapping-on-a-link-will-select-a-different-link-from-the-page-16010417-04bfe9b.patch

Added missing comment.

Comment 22

[Randall Barker [:rbarker]]

With APZ disabled as it is on Aurora: 
https://treeherder.mozilla.org/#/jobs?repo=try&revision=b0e4d20a853b

With APZ enabled as it is on nightly:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=5cc6299394a9

While there is some orange, None of the failures looks to be related to this patch.

Comment 23

[Timothy Nikkel (:tnikkel)]

Comment on attachment 8703917 [details] [diff] [review]
0001-Bug-1233056-Long-tapping-on-a-link-will-select-a-different-link-from-the-page-16010417-04bfe9b.patch

> void
> nsDisplayResolution::HitTest(nsDisplayListBuilder* aBuilder,
>                              const nsRect& aRect,
>                              HitTestState* aState,
>                              nsTArray<nsIFrame*> *aOutFrames)
> {
>+#if !defined(MOZ_WIDGET_ANDROID) || defined(MOZ_ANDROID_APZ)
>   nsIPresShell* presShell = mFrame->PresContext()->PresShell();
>   nsRect rect = aRect.RemoveResolution(presShell->ScaleToResolution() ? presShell->GetResolution () : 1.0f);
>   mList.HitTest(aBuilder, rect, aState, aOutFrames);
>+#else
>+  mList.HitTest(aBuilder, aRect, aState, aOutFrames);
>+#endif
> }

I think the |#if !defined(MOZ_WIDGET_ANDROID) || defined(MOZ_ANDROID_APZ)| code should be correct everywhere. Do you know if this hunk is needed? Can we land without this change?

Comment 24

[Pulsebot]

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

Comment 25

[Randall Barker [:rbarker]]

Attachment: 0001-Bug-1233056-Long-tapping-on-a-link-will-select-a-different-link-from-the-page-r-tnikkel-16010820-8a839aa.patch

Patch pushed to try.

Comment 26

[Randall Barker [:rbarker]]

Comment on attachment 8705938 [details] [diff] [review]
0001-Bug-1233056-Long-tapping-on-a-link-will-select-a-different-link-from-the-page-r-tnikkel-16010820-8a839aa.patch

Approval Request Comment
[Feature/regressing bug #]: Bug 1233056
[User impact if declined]: Long tapping on a link will select the wrong link in Fennec
[Describe test coverage new/current, TreeHerder]: Normal test coverage on try won't catch this because Aurora is still using the Java Pan Zoom controller (which is what was broken) while nightly is using the C++APZ which the patch does not affect. I did a try runs with both JPZ and C++APZ and did not find any failures related to this patch.
[Risks and why]: Risk should be low since this patch is restoring fennec to how it used to work before the breakage.
[String/UUID change made/needed]: None.

Comment 27

[Phil Ringnalda (:philor)]

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

Comment 28

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8705938 [details] [diff] [review]
0001-Bug-1233056-Long-tapping-on-a-link-will-select-a-different-link-from-the-page-r-tnikkel-16010820-8a839aa.patch

Fix a regression, early in the 45 cycle, taking it.

Comment 29

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/42fba734e845

Comment 30

[Ryan VanderMeulen [:RyanVM]]

Confirmed that the link from c0 and my issues with Reddit are fixed in today's Aurora nightly :)