942754 - Double-Tap to zoom is not disabled even with user-scalable=no

Status: RESOLVED FIXED

(Core :: Panning and Zooming, defect)

Description

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

With APZ turned on and  <meta name="viewport" content="width=device-width, user-scalable=no, initial-scale=1"> double-tap to zoom is not disabled.

I guess we can workaround that by changing the rule to <meta name="viewport" content="width=device-width, user-scalable=no, initial-scale=1, maximum-scale=1"> but it sounds bad since I guess that the double tap will still be fired and in this case if the user clicks quickly into a game or an app then a click will be missed.

Comment 1

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

In order to reproduce just open the settings app and double click somewhere.

Comment 2

[Kevin Grandon :kgrandon]

Just tried this on a device with latest gecko and apz enabled and did not see the issue in settings app.

Comment 3

[Kevin Grandon :kgrandon]

Oh, interesting, I do see it in Calendar :)

Comment 4

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

It seems like mAllowZoom is correctly set to false in http://mxr.mozilla.org/mozilla-central/source/gfx/layers/ipc/AsyncPanZoomController.cpp#1583 but is read as true in http://mxr.mozilla.org/mozilla-central/source/gfx/layers/ipc/AsyncPanZoomController.cpp#801

Comment 5

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

A 100% reproducible way of doing it is to:
  - Populate Gaia with some data (calls, messages, contacts, ..), go into the gaia directory and do |make reference-workload-medium| with the device connected

  - Open the dialer application
  - Go to the call log (bottom left tab)
  - double tap on the little cat image (miaou) in one of the call log row

Expected result:
 - the row is clicked

Actual result:
 - the cat image zoom in, even with user-scalable=no.

Comment 6

[Botond Ballo [:botond]]

I looked into this a bit.

There are two APZCs when viewing the call log page: a root APZC for the entire app, and an APZC for the list of log entries that actually scroll. We correctly propagate the effects of user-scalable=no to the root APZC and set mAllowZoom = false on it, but the APZC that handles the double tap is the inner one, and that has mAllowZoom = true.

Comment 7

[Botond Ballo [:botond]]

Related: https://bugzilla.mozilla.org/show_bug.cgi?id=911349

Comment 8

[Botond Ballo [:botond]]

Attachment: bug942754.patch

This patch solves the problem by vetoing a request from APZC to B2G's GeckoContentController to handle a double tap if the root's zoom constraints specify allowZoom = false.

I have considered other solutions like setting mAllowZoom = false for child APZCs, but that would have caused the opposite problem where double-tapping would never work for a subframe, even if user-scalable=yes. We could have set mAllowZoom to true or false for the entire tree of APZCs when setting it on the root, but this approach seemed easier.

Try run: https://tbpl.mozilla.org/?tree=Try&rev=35fd14b7b1a6

Comment 9

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

My understanding of the code path for events with APZ could be wrong, but it is not too late to check that into RenderFrameParent.cpp instead of http://mxr.mozilla.org/mozilla-central/source/gfx/layers/ipc/GestureEventListener.cpp#332 ?

I would be happy to be wrong and learn more about the event dispatching order in that case!

I just want to make sure that single tap does not suffer a delay of 300ms from http://mxr.mozilla.org/mozilla-central/source/gfx/layers/ipc/GestureEventListener.cpp#27 when the app has explicitly asked to disable double tab with user-scalable=no.

Comment 10

[Chris Lord [:cwiiis]]

Comment on attachment 8340563 [details] [diff] [review]
bug942754.patch

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

LGTM. Check out comment #9 though, I'm kind of assuming that this event is more raw than that and disabling it makes no difference to event delivery (though hopefully taps aren't delayed when it is disabled).

Comment 11

[Botond Ballo [:botond]]

Vivien is right, my patch disables double-tap to zoom but it does not get rid of the 300 ms timeout to handle a single tap.

Investigating a bit more, it looks like the 300 ms timeout issue was solved in bug 922896 by having AsyncPanZoomController::OnSingleTapUp() call GeckoContentController::HandleSingleTap() if mAllozZoom = false.

So, it seems the alternative approach I described in comment #8 (setting mAllowZoom to true or false for the entire tree of APZCs when setting it on the root) will solve both problems.

Comment 12

[Botond Ballo [:botond]]

Attachment: bug942754.patch

After thinking about this some more, I realized that setting zoom constraints for the entire tree of APZCs when setting them for the root is not enough because new APZCs would still be created with the default zoom constraints which have mAllowZoom = true.

Instead, we can initialize an APZC's zoom constraints based on those of its parent. The attached patch implements that.

Try push: https://tbpl.mozilla.org/?tree=Try&rev=5be6d2cea05c

Comment 13

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

Comment on attachment 8341327 [details] [diff] [review]
bug942754.patch

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

This doesn't handle the case where APZCTreeManager::UpdateZoomConstraints is called after the tree is built (e.g. if script on the page changes the meta viewport tag). This can result in the zoom constraints getting out of sync. I guess you'll also want to throw away calls to APZCTM::UpdateZoomConstraints that refer to an APZC that is !IsRootForLayersId()

Comment 14

[Botond Ballo [:botond]]

Attachment: bug942754.patch

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #13)
> This doesn't handle the case where APZCTreeManager::UpdateZoomConstraints is
> called after the tree is built (e.g. if script on the page changes the meta
> viewport tag). This can result in the zoom constraints getting out of sync.
> I guess you'll also want to throw away calls to
> APZCTM::UpdateZoomConstraints that refer to an APZC that is
> !IsRootForLayersId()

Good points, thanks! Updated patch accordingly.

A couple of design musings, not particularly intended to be acted upon before this patch lands:

  - After we introduce a ZoomConstraints structure (bug 915985)
    would it make sense for each APZC to hold an nsRefPtr<ZoomConstraints>
    and have non-root (-for-layers-id) APZCs refer to the ZoomConstraints
    of the root?

  - We've been adding a new recursive function for every operations that
    needs to traverse the APZC tree. Would it make sense to encapsulate
    this into a visitor or something?

Comment 15

[Botond Ballo [:botond]]

Try push: https://tbpl.mozilla.org/?tree=Try&rev=77ea7689ed9d

Comment 16

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

Comment on attachment 8342014 [details] [diff] [review]
bug942754.patch

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

r=me with comments addressed

::: gfx/layers/composite/APZCTreeManager.cpp
@@ +595,5 @@
>  {
>    nsRefPtr<AsyncPanZoomController> apzc = GetTargetAPZC(aGuid);
> +  // For a given layers id, non-root APZCs inherit the zoom constraints
> +  // of their root.
> +  if (apzc && apzc->IsRootForLayersId()){

nit: space before {

@@ +611,5 @@
> +  mTreeLock.AssertCurrentThreadOwns();
> +
> +  aApzc->UpdateZoomConstraints(aAllowZoom, aMinScale, aMaxScale);
> +  for (AsyncPanZoomController* child = aApzc->GetLastChild(); child; child = child->GetPrevSibling())
> +    UpdateZoomConstraintsRecursively(child, aAllowZoom, aMinScale, aMaxScale);

You should abort the recursion if you hit an apzc that has a different layers id, because as-is this code will recurse from the root all the way down into subtrees which are supposed to have their own constraints. You should be able to accomplish this easily if you add a "if (!child->IsRootForLayersId())" inside the body of the for loop.

Also, braces on the for loop.

@@ +614,5 @@
> +  for (AsyncPanZoomController* child = aApzc->GetLastChild(); child; child = child->GetPrevSibling())
> +    UpdateZoomConstraintsRecursively(child, aAllowZoom, aMinScale, aMaxScale);
> +}
> +
> +

remove one of these blank lines

Comment 17

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

(In reply to Botond Ballo [:botond] from comment #14)
> A couple of design musings, not particularly intended to be acted upon
> before this patch lands:
> 
>   - After we introduce a ZoomConstraints structure (bug 915985)
>     would it make sense for each APZC to hold an nsRefPtr<ZoomConstraints>
>     and have non-root (-for-layers-id) APZCs refer to the ZoomConstraints
>     of the root?
> 
>   - We've been adding a new recursive function for every operations that
>     needs to traverse the APZC tree. Would it make sense to encapsulate
>     this into a visitor or something?

Yeah, both of these would be nice to have. The first one more so than the second, I think, as it results in code deletion which is always nice :)

Comment 18

[Botond Ballo [:botond]]

Attachment: bug942754.patch

Updated patch to address latest review comments. Carrying r+.

Comment 19

[Botond Ballo [:botond]]

(In reply to Botond Ballo [:botond] from comment #15)
> Try push: https://tbpl.mozilla.org/?tree=Try&rev=77ea7689ed9d

Looks clean. Will land when trees open.

Comment 20

[Botond Ballo [:botond]]

https://hg.mozilla.org/integration/b2g-inbound/rev/72d6f1c45d09

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/72d6f1c45d09

Comment 22

[Manuela Muntean [Away]]

Besides the STR for Firefox OS in comment 5, could someone provide some guidance in order to reproduce and verify this on desktop Firefox (Metro mode).

Comment 23

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

(In reply to Manuela Muntean [:Manuela] [QA] from comment #22)
> Besides the STR for Firefox OS in comment 5, could someone provide some
> guidance in order to reproduce and verify this on desktop Firefox (Metro
> mode).

You probably need to create a web page that contains:
<html>
  <head>
    <meta name="viewport" content="width=device-width, user-scalable=no, initial-scale=1">
  </head>

  <body>
   <div style="width: 10000px; height: 10000px; background-color: red;">
     <p>Hello world!</p>
   </div>
  </body>
</html>

And try to double tap on the Hello world text to see if it is zoomed or not.

Comment 24

[Manuela Muntean [Away]]

Attachment: testcase.html

With the attached testcase (offered in comment 23) and the Nightly from 2013-11-25 on a Dell XPS 12 ultrabook with Windows 8 64-bit, when I double tap on the text, it doesn't zoom in.