1120566 - [Browser][Pinch Zoom] Tap events do not occur in the correct location when the browser window is zoomed in.

Status: VERIFIED FIXED

(Core :: Panning and Zooming, defect)

Description

[Martin Shuman [:Marty]]

Attachment: logcat-browser-zoom.txt

Description:
If the user performs a zooms in with the reverse-pinch gesture, the hit boxes for links in the browser window will not be aligned with the browser itself.  This results in the user tapping on links with nothing happening, or the browser opening a different link.

Most times I've seen this issue, the actual tap event seems to be occurring below, and to the right of where I am tapping.

Note: Tap events ARE placed properly in a non-zoomed browser window

Repro Steps:
1) Update a Flame device to BuildID: 20150112010228
2) Enable WiFi or Data and connect to a network
3) Open the Browser app
4) Perform a Google search for 'Mozilla' in the URL bar.
5) Zoom in on the browser window using the reverse-pinch gesture.
6) Attempt to tap on the search results in the zoomed window.
  
Actual:
Tap events do not occur in the correct locations, causing the user to miss links, or accidentally hit the wrong links.
  
Expected: 
Tap events always occur in the correct location.
  
Environmental Variables:
Device: Flame KK
BuildID: 20150112010228
Gaia: f5e481d4caf9ffa561720a6fc9cf521a28bd8439
Gecko: bb8d6034f5f2
Gonk: a814b2e2dfdda7140cb3a357617dc4fbb1435e76
Version: 37.0a1 (KK)
Firmware: V18D-1
User Agent: Mozilla/5.0 (Mobile; rv:37.0) Gecko/37.0 Firefox/37.0
  
Notes: This issue occurs on the v18D, V18D-1, and v188-1 bases
  
Repro frequency: 7/8
See attached: video clip(URL), logcat

-------------------------------------------------------

This issue does NOT occur on Flame 2.1
Tap events occur in the correct location when zoomed in the Browser window.

Environmental Variables:
Device: Flame 2.1
BuildID: 20150112001215
Gaia: 1975241ac29f723479e6c60b2bf74ebed54da91a
Gecko: 0863fe4b75c3
Gonk: a814b2e2dfdda7140cb3a357617dc4fbb1435e76
Version: 34.0 (2.1)
Firmware: V18D
User Agent: Mozilla/5.0 (Mobile; rv:34.0) Gecko/34.0 Firefox/34.0

Comment 1

[Peter Bylenga [:PBylenga]]

Nominating to block, functional regression of a core feature causing a bad UX.

Requesting a window.

Comment 2

[Parul Mathur [:pragmatic] - out of office - please ni? [:bajaj] instead]

Adding to our daily smoke test suite, as per discussion with :gwagner. Bug is now a smoketest blocker.

Comment 3

[Jayme Mercado [:JMercado] (Inactive 3/4/2016)]

A regression window cannot be found for this issue.  The touch events are off on all builds in the Flame KK Branch of Central, but does not occur in the JB branch of Central.  This issue also does not occur on 2.1 of Flame KK.

Earliest Flame KK Central build:
Environmental Variables:
Device: Flame 2.2
BuildID: 20150107152659
Gaia: de59e0c3614dd0061881fe284e9f2d74fa0d1d5d
Gecko: 8703c1895505
Version: 35.0a1 (2.2)
Firmware Version: v18D-1
User Agent: Mozilla/5.0 (Mobile; rv:35.0) Gecko/35.0 Firefox/35.0

Flame JB central Build
Environmental Variables:
Device: Flame 2.2
BuildID: 20140919051551
Gaia: d170091ba1b5597b05f37fb259f6b8eb02568798
Gecko: dadafedc0760
Version: 35.0a1 (2.2) 
Firmware Version: v123
User Agent: Mozilla/5.0 (Mobile; rv:35.0) Gecko/35.0 Firefox/35.0

Comment 4

[Gregor Wagner [:gwagner]]

Botond, any idea whats going on here?

Comment 5

[Botond Ballo [:botond]]

This appears to be a regression from bug 1076241.

In that bug, we moved a transform from being part of APZC::GetCurrentAsyncTransform() (where it was called the "non-transient async transform"), to being part of the layer's GetTransform() (via the post-scale [1]).

As a result, this transform is no longer un-applied by APZCTreeManager::GetApzcToGeckoTransform(), even though it's still technically a compositor-side transform, meaning that Gecko doesn't know about it and so it should be unapplied before passing an event to Gecko.

It's not immediately clear to me how to fix this; I guess we'd need GetApzcToGeckoTransform() to know about it somehow, but I'm not sure how given that in that function we walk a chain of APZCs, and the point of bug 1076241 was to dis-associate that transform from APZCs.

[1] http://mxr.mozilla.org/mozilla-central/source/gfx/layers/composite/ContainerLayerComposite.h?rev=42272b7f8e48#95

Comment 6

[Milan Sreckovic [:milan] (needinfo for best results)]

So... is this an APZ issue, should we change the bug product/component?

Comment 7

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

(In reply to Botond Ballo [:botond] from comment #5)
> As a result, this transform is no longer un-applied by
> APZCTreeManager::GetApzcToGeckoTransform(), even though it's still
> technically a compositor-side transform, meaning that Gecko doesn't know
> about it and so it should be unapplied before passing an event to Gecko.

Two questions:
1) If GetApzcToGeckoTransform is "wrong" now, does this impact untransforming regular touch events (in ReceiveInputEvent, as opposed to the HandleSingleTap notification) as well?
2) If regular touch events are fine but HandleSingleTap et al are busted, then maybe we can fix it on the gecko side in the callback-transform code?

Comment 8

[Botond Ballo [:botond]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #7)
> Two questions:
> 1) If GetApzcToGeckoTransform is "wrong" now, does this impact
> untransforming regular touch events (in ReceiveInputEvent, as opposed to the
> HandleSingleTap notification) as well?

I believe it's wrong in both cases (regular touch events and taps).

> 2) If regular touch events are fine but HandleSingleTap et al are busted,
> then maybe we can fix it on the gecko side in the callback-transform code?

Looking at the relevant code, it looks like the callback-transform is applied to points in both touch events and taps, so by adjusting the callback-transform we should be able to catch both cases.

Comment 9

[Botond Ballo [:botond]]

Attachment: bug1120566.patch

Seems to fix the problem locally.

Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=483807b49f67

Comment 10

[Botond Ballo [:botond]]

I discussed the solution approach in my patch with :kats. It's not the prettiest, because it puts the burden of adjusting for a transform applied by the commpositor, into implementations of GeckoContentController. I believe it would be nicer to perform the adjustment on the compositor side (ideally in APZCTreeManager::GetApzcToGeckoTransform()), but figuring out how to do that correctly will require some more investigation.

In the meantime, this approach is good enough for fixing the smoketest, so let's land it. Marking as checkin-needed since the tree is closed.

Comment 11

[Botond Ballo [:botond]]

(In reply to Botond Ballo [:botond] from comment #10)
> I believe it would be nicer to perform the adjustment on the compositor side
> (ideally in APZCTreeManager::GetApzcToGeckoTransform()), but figuring out
> how to do that correctly will require some more investigation.

Filed bug 1122804 so we don't forget about this.

Comment 12

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/3e38ec9ed49a

Comment 14

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/3e38ec9ed49a

Comment 15

[Botond Ballo [:botond]]

Comment on attachment 8550429 [details] [diff] [review]
bug1120566.patch

NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 
  Bug 1076241.

User impact if declined: 
  Tapping when zoomed results in the wrong 

Testing completed: 
  Tested locally. Landed on m-c 2015-01-17.

Risk to taking this patch (and alternatives if risky): 
  The are of the code being touched here is fairly tricky,
  but I believe we understand the issue being solved here
  relatively well, and the patch solves a serious smoketest
  regression.

String or UUID changes made by this patch:
  None.

Comment 16

[Martin Shuman [:Marty]]

This issue is verified fixed on Flame 3.0
Tap events are correctly aligned in a zoomed browser window.

Environmental Variables:
Device: Flame 3.0 Master (319MB)(Full Flash)
BuildID: 20150120010227
Gaia: a5c5ac093814a80b0627514c3bd5f9e96c096a4b
Gecko: c1c6840d9255
Gonk: a814b2e2dfdda7140cb3a357617dc4fbb1435e76
Version: 38.0a1 (3.0 Master)
Firmware: V18D-1
User Agent: Mozilla/5.0 (Mobile; rv:38.0) Gecko/38.0 Firefox/38.0

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/7762046d3296

Comment 18

[Alissa（leave from Mozilla）]

This issue had been successfully verified on Flame 2.2:
Reproducing rate: 0/5
Device: Flame 2.2[Verified]
Build ID               20150409002503
Gaia Revision          ea735c21bfb0d78333213ff0376fce1eac89ead6
Gaia Date              2015-04-07 20:58:15
Gecko Revision         https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/0efd5cdbe224
Gecko Version          37.0
Device Name            flame
Firmware(Release)      4.4.2
Firmware(Incremental)  eng.cltbld.20150409.041814
Firmware Date          Thu Apr  9 04:18:23 EDT 2015
Bootloader             L1TC000118D0