Closed Bug 942754 Opened 6 years ago Closed 6 years ago

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

Categories

(Core :: Panning and Zooming, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla28
Tracking Status
firefox28 --- fixed

People

(Reporter: vingtetun, Assigned: botond)

References

Details

Attachments

(2 files, 3 obsolete files)

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.
In order to reproduce just open the settings app and double click somewhere.
Just tried this on a device with latest gecko and apz enabled and did not see the issue in settings app.
Oh, interesting, I do see it in Calendar :)
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.
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.
Assignee: nobody → botond
Attached patch bug942754.patch (obsolete) — Splinter Review
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
Attachment #8340563 - Flags: review?(chrislord.net)
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 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).
Attachment #8340563 - Flags: review?(chrislord.net) → review+
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.
Attached patch bug942754.patch (obsolete) — Splinter Review
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
Attachment #8340563 - Attachment is obsolete: true
Attachment #8341327 - Flags: review?(bugmail.mozilla)
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()
Attachment #8341327 - Flags: review?(bugmail.mozilla) → review-
Attached patch bug942754.patch (obsolete) — Splinter Review
(In reply to Kartikaya Gupta (email:kats@mozilla.com) 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?
Attachment #8341327 - Attachment is obsolete: true
Attachment #8342014 - Flags: review?(bugmail.mozilla)
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
Attachment #8342014 - Flags: review?(bugmail.mozilla) → review+
(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 :)
Attached patch bug942754.patchSplinter Review
Updated patch to address latest review comments. Carrying r+.
Attachment #8342014 - Attachment is obsolete: true
Attachment #8342490 - Flags: review+
(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.
https://hg.mozilla.org/mozilla-central/rev/72d6f1c45d09
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla28
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).
Flags: needinfo?(21)
Keywords: verifyme
(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.
Flags: needinfo?(21)
Attached file 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.
You need to log in before you can comment on or make changes to this bug.