941995 - Remove 300ms touch > mouse events delay for double-tap zoom on "responsive" pages

Status: RESOLVED FIXED

(Core :: Panning and Zooming, defect)

Description

[Tetsuharu OHZEKI [:tetsuharu] (UTC+9)]

Chromium for Android removes this mouse event delay:
* https://groups.google.com/a/chromium.org/d/topic/blink-dev/8evES7o-QTY/discussion
* https://code.google.com/p/chromium/issues/detail?id=169642.


Of course, this is related to the ux of browser. However, I think this might be web compatibility issue.

Comment 1

[Matt Brubeck (:mbrubeck)]

Chromium did this by disabling the "double-tap zoom" gesture to zoom in to an element.   They disable the feature only on "responsive" or "mobile-optimized" pages, where the viewport width is equal to or less than the device width (for example, any page with <meta name="viewport" content="width=device-width">).  The gesture and the delay are still enabled on pages with a viewport wider than the device, for example "desktop" pages viewed on a handheld device.

I agree that we should follow Chrome's lead here.  The Firefox OS team would also like to avoid introducing this delay to Firefox OS apps, so this would help them a lot (bug 944082).

Comment 2

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to Matt Brubeck (:mbrubeck) from comment #1)

> I agree that we should follow Chrome's lead here.

Sounds like a pln. Let's make sure this is fixed in the platform APZC and the Java APZC systems. We don't know how long it will take to convert Fennec to use the platform APZC system and I don't want to block this change on doing the conversion.

Comment 3

[Binyamin]

It is removed from Chrome 32.

Comment 4

[Wesley Johnston (:wesj)]

This does worry me a bit, since its potentially site breaking. I wonder if we should at least include an escape mechanism, so that it is possible for a site to be device-width and still support double tap zoom if they want to.

Comment 5

[Tetsuharu OHZEKI [:tetsuharu] (UTC+9)]

(In reply to Wesley Johnston (:wesj) from comment #4)
> This does worry me a bit, since its potentially site breaking. I wonder if
> we should at least include an escape mechanism, so that it is possible for a
> site to be device-width and still support double tap zoom if they want to.

I think:
* Existing sites are developed under delaying 300ms, this means we can assume that they care it as asynchronous. Thus this changing to clip it from 300ms to x~0ms would not break the backward compatibility.
* We have a pinch zoom behavior to zoom-in instead of double tap one. It might be no problem.

Comment 6

[Wesley Johnston (:wesj)]

(In reply to Tetsuharu OHZEKI [UTC+9] from comment #5)
> I think:
> * Existing sites are developed under delaying 300ms, this means we can

I'm more worried that they're designed to believe that double tap zoom exists. A site might currently even override pinch zoom (i.e. call prevent default on the second touch on a screen) assuming that double tap was available, and might even advertise that to users ("Double tap here to see more info!") That sort of behavior isn't available with this change.

Its a valid use case, which is why I said it might be nice to support an opt out. i.e. I just don't want to see us throwing any more roadblocks in front of mobile web devs.

I wasn't aware that there was a huge demand for enabling zooming, but disabling the click delay on sites (native apps don't seem to need to pinch zoom...). If I'd known, my preferred to solution to this would have been adding a "disable-doubletap-zoom" to the metaviewport tag. But given the press raised, I would guess that's out of the question now.

Comment 7

[Tetsuharu OHZEKI [:tetsuharu] (UTC+9)]

(In reply to Wesley Johnston (:wesj) from comment #6)
> I'm more worried that they're designed to believe that double tap zoom
> exists. A site might currently even override pinch zoom (i.e. call prevent
> default on the second touch on a screen) assuming that double tap was
> available, and might even advertise that to users ("Double tap here to see
> more info!") That sort of behavior isn't available with this change.

I seem that such cases may be other things which is not changed by this bug.

This bug would remove the delay for dispatch "click" event, like Chromium (http://updates.html5rocks.com/2013/12/300ms-tap-delay-gone-away). If the website using "click" event, there are 300ms delaying without the viewport is specified "user-scalable=no". So this change will  effect "click" event mainly. If the website uses "touchend" event, there is no delay at all.

If the website overrides "double tap zooming", it would use touch events, wouldn't use "click" event. Even if it uses "click" event, I think it will be broken, soon or late. (In the first place, I doubt we can override "double-tap-zoom" behavior on current browsers' implementations.)

Comment 8

[Wesley Johnston (:wesj)]

(In reply to Tetsuharu OHZEKI [UTC+9] from comment #7)

> If the website using "click" event, there are 300ms delaying without the viewport
> is specified "user-scalable=no". So this change will  effect "click" event
> mainly. If the website uses "touchend" event, there is no delay at all.

touch and click events are inter-related. You can listen to touches and clicks, and, in the most common use case here, sites often depend on link clicks to work even if they have touch listeners present.

Anyway, I'm not arguing against doing this. Just simplifying things a bit. Rather than relying on certain (magical and unrelated) conditions to be me in the meta-viewport to trigger this, we just default to disabling double-tap zoom on ALL pages that have a viewport tag, Instead, give them a tag they can add if they want to enable it. doubletap-zoom-enabled=true?

We'd essentially be saying:
a.) Add a tag to the viewport that specifies whether double tap is enabled or not. If the flag is not specified it defaults to false. If user-scalable=no, it defaults to false. This is the breaking change from old behavior.

b.) If a page doesn't have a viewport tag, we apply a default that looks something like: width=980,doubletap-zoom-enabled=true (i.e. we're not applying the 'default' in this case, which is a bit strange, but probably matches expected behavior)

Comment 9

[Tetsuharu OHZEKI [:tetsuharu] (UTC+9)]

(In reply to Wesley Johnston (:wesj) from comment #8)
> Anyway, I'm not arguing against doing this. Just simplifying things a bit.
> Rather than relying on certain (magical and unrelated) conditions to be me
> in the meta-viewport to trigger this, we just default to disabling
> double-tap zoom on ALL pages that have a viewport tag, Instead, give them a
> tag they can add if they want to enable it. doubletap-zoom-enabled=true?


Umm, I don't think we should introduce the new viewport option... It's not good for the future and interoperability.

We can try to remove 300ms delaying for "click" event on real web in Nightly/Aurora channel. For this bug, my thought is that we should introduce some workaround like your proposal if we need something to care after trying it on Nightly/Aurora.

Comment 10

[Brendan Eich [:brendan]]

See http://updates.html5rocks.com/2013/12/300ms-tap-delay-gone-away.

/be

Comment 11

[Andreas Gal :gal]

Vivien, are we correctly disabling the delay for apps that aren't zoomable?

Comment 12

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

Yes, on both Fennec and B2G we are disabling the delay for anything with user-scalable=no.

Comment 13

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

So in the case where double-tap-to-zoom is disabled, and the user does a double-tap, should we be sending two click events to content?

Comment 14

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

Actually we don't do that now so with user-scalable=no so the answer is no. If that needs to be changed it should be a separate bug.

Comment 15

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

Attachment: Patch for Fennec

Comment 16

[Matt Brubeck (:mbrubeck)]

Comment on attachment 8355646 [details] [diff] [review]
Patch for Fennec

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

Looks good in general, but I think there's one minor bug:

::: mobile/android/chrome/content/browser.js
@@ +5995,5 @@
>  
> +    if (hasMetaViewport && (autoSize || width <= window.outerWidth)) {
> +      // see bug 941995. For pages where the viewport is device-width or less, disable
> +      // double-tap behaviour.
> +      allowDoubleTap = false;

Since this can change when the window size changes, it should be part of updateViewportSize.  (This is kind of a corner case; it only matters when there is a hard-coded width that is larger than the screen in one orientation and smaller in another.)

Comment 17

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

Attachment: Patch for Fennec (v2)

Updated patch for Fennec. I'm a little less confident about this one so I'll run it through try before requesting review.

https://tbpl.mozilla.org/?tree=Try&rev=da8ac448d5ae

Comment 18

[Wesley Johnston (:wesj)]

The developer relations team is running a survey with devs to see what they'd like here. Again, I really hate the "if you happen to meet some magic conditions this works" approach. I also don't know that it accurately reflects what people expect on tablets or touch desktop machines.

I don't really want to jump the gun and start doing things before they've had a chance to talk and come up with better solutions.

Comment 19

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

Fair enough. Is there an ETA or contact person on the survey? I'll hold off on anything further here until I hear otherwise.

Comment 20

[Matt Brubeck (:mbrubeck)]

In this thread, Chrome team members mention that they have made this change on their Beta channel only in order to gather feedback, and that there's a user setting to override it.  Perhaps we should also land this with a pref and turn it by default in pre-release builds only:
https://news.ycombinator.com/item?id=6900388

Paul Kinlan (Chrome developer relations) also offered in that thread to share "data/lessons" with us if it would be helpful.  Wes, could you pass that on to our devrel people who are working on this?

Comment 21

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

For comment 19.

Comment 23

[Wesley Johnston (:wesj)]

I've been talking to Christian Heilman, but I haven't heard anything for a bit. I pinged Robery Nyman this morning. If I don't hear back soon, I'm fine with landing this (but I'm still annoyed that we're putting an "API" on the web that we'd never accept for our own use).

Comment 24

[Paul Kinlan]

Hi,

Happy to help as much as I can.  The stable version of Chrome on Desktop has just reached M32 which means the Android version should not be too far behind which includes this feature for all of our Chrome on Android users.

I am keen to understand if there is any specific bits of data that you might need before you land this feature.  On the beta channel I haven't seen negative feedback from people actually using the browser and noticing that this causes issues for them.  Happy to track down any developer issues that might have come up, but there has been nothing major.

P

Comment 25

[Paul Kinlan]

Oh, just to add.  Reading the above comments, it might not be clear from our implementation but we only disable double tap to zoom if the viewport meta tag is present *and* the content fits in the viewport.  If the content forces the width to be wider than the viewport even by 1px (i.e, an image is not sized correctly) then our logic reverts back to the 300ms delay.

Comment 26

[Andreas Gal :gal]

Just land it. We won't know until its in the field. This is worth taking a risk with.

Comment 27

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

(In reply to Paul Kinlan from comment #25)
> Oh, just to add.  Reading the above comments, it might not be clear from our
> implementation but we only disable double tap to zoom if the viewport meta
> tag is present *and* the content fits in the viewport.  If the content
> forces the width to be wider than the viewport even by 1px (i.e, an image is
> not sized correctly) then our logic reverts back to the 300ms delay.

This contradicts what Matt said in comment 1 and what I find at [1]. The commit message says that the viewport-width has to be <= screen width, whereas you're saying the content width has to be <= viewport width. It's possible to have a viewport meta tag that says "width=device-width" but for content to be wider than that (this usually results in horizontal scrolling). In such a case is double-tap enabled or disabled?

[1] https://code.google.com/p/chromium/issues/detail?id=169642#c35

Comment 28

[Paul Kinlan]

We seem to use the term screen in multiple contexts in that issue.  What you said I said is what is implemented, which I also think is similar to Comment #1.

I have produced some test cases which I think demonstrate most of the constraints.

When viewport configuration is width=device-width, if there is horizontal scrolling double-tap to zoom is ON. [http://jsbin.com/UxeBEbo/latest] - the content is bigger than the viewport.
When viewport configuration width=X when X>device-width double-tap to zoom is ON. [http://jsbin.com/aqerEfu/latest]

When viewport configuration width=X when X<device-width double-tap to zoom is OFF [http://jsbin.com/AnEyiqo/latest]
When viewport configuration width=device-width and content fits in viewport double-tap to zoom is OFF

Comment 29

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

(In reply to Paul Kinlan from comment #28)
> When viewport configuration is width=device-width, if there is horizontal
> scrolling double-tap to zoom is ON. [http://jsbin.com/UxeBEbo/latest] - the
> content is bigger than the viewport.
> 
> When viewport configuration width=device-width and content fits in viewport
> double-tap to zoom is OFF

Was there a particular reason you chose to do it this way? I mean, if you're going to use horizontal scrollability as the deciding factor in one case, why not do it for all cases? Or none of them?

Comment 30

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

Attachment: Patch for B2G

Here's the equivalent patch for B2G. I haven't tested it yet though. Both patches may need changing to take into account the page size as Paul is describing above.

Comment 31

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

https://tbpl.mozilla.org/?tree=Try&rev=bfe412995db9

Comment 32

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

For comment 29.

Comment 33

[Wesley Johnston (:wesj)]

We're moving forward with a hacks post on this, but I think we should land this in the mean time. Thanks for waiting kats.

Comment 34

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

I keep meaning to get back to this but I'm going to continue drowning in B2G/APZC issues for the next while, so unassigning. The current state here is that the patches are ready to land if we want to use the CSS viewport width as the determining factor, but the patches need updating if we want to use the page width as the determining factor. I was waiting for Paul to provide more info as to why they chose the latter. I think it makes more sense to use the former (which is why I assumed it was done that way and wrote the patches to match).

Comment 35

[hexalys]

(In reply to Paul Kinlan from comment #28)

> When viewport configuration is width=device-width, if there is horizontal
> scrolling double-tap to zoom is ON. [http://jsbin.com/UxeBEbo/latest] - the
> content is bigger than the viewport.
> When viewport configuration width=X when X>device-width double-tap to zoom
> is ON. [http://jsbin.com/aqerEfu/latest]

Is there a current way to preserve the delay without affecting the viewport width?
For example, should viewport configuration be set at width=320.1 (if that's even possible)

I am not sure if it takes decimals, but if so, that would seems like convenient way to preserve the delay for those who want to, without the off-1px caveat.

Comment 36

[Paul Kinlan]

All.  Sorry for the delay, I didn't see the replies in my mail.

@themacmaster - you can set it to a value that is not device width and it should preserve the delay.  I would say though that this feels like the user should be in control of and not the developer.  

@kats - sorry for the delay.  The reasoning was for horizontal scrolability is that this is first sign that something does not fit the viewport.  i.e, it looks similar to when a user has naturally zoomed in.  If the viewport has horizontal scrolling we are making the assumption that the user would pinch-zoom-out and if they do this then they are more likely to double tap to zoom-in again, so we disable it for the page.

Comment 37

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

(In reply to Paul Kinlan from comment #28)
> I have produced some test cases which I think demonstrate most of the
> constraints.
> 
> When viewport configuration is width=device-width, if there is horizontal
> scrolling double-tap to zoom is ON. [http://jsbin.com/UxeBEbo/latest] - the
> content is bigger than the viewport.
> When viewport configuration width=X when X>device-width double-tap to zoom
> is ON. [http://jsbin.com/aqerEfu/latest]
> 
> When viewport configuration width=X when X<device-width double-tap to zoom
> is OFF [http://jsbin.com/AnEyiqo/latest]
> When viewport configuration width=device-width and content fits in viewport
> double-tap to zoom is OFF

I just tested the behaviour on the latest Chrome for Android, and frankly the behaviour makes no real sense to me, and is inconsistent to boot. Here is my test page:
http://people.mozilla.org/~kgupta/tmp/doubletap.html

I'm testing on a Galaxy Nexus which has a 720px screen width and a devicePixelRatio of 2, so "device-width" should be 360px. I set a meta-viewport that says "width=300" so this should fall under your "X<device-width" case and always have double-tap to zoom as off. When the page first loads, sure, doubletap is off. When I click on the button to make the page content wider though, sometimes doubletap becomes enabled. At first I thought it was based on the width I set it to, and bisected that width value down to the 399px/400px boundary (i.e. setting the div's width to 399px would leave double-tap off, but setting it to 400px would turn it on). However this magic value made no sense to me and when I tried to reproduce the results they went away. I still think there's a correlation between the width and whether or not double-tap goes on, but for values in the range 300-400 it's basically a coin flip as to whether I'll get double-tap. There must be something else interacting with this behaviour, and if I couldn't figure it out it's unlikely that web developers will be able to either.

(In reply to Paul Kinlan from comment #36)
> @kats - sorry for the delay.  The reasoning was for horizontal scrolability
> is that this is first sign that something does not fit the viewport.  i.e,
> it looks similar to when a user has naturally zoomed in.  If the viewport
> has horizontal scrolling we are making the assumption that the user would
> pinch-zoom-out and if they do this then they are more likely to double tap
> to zoom-in again, so we disable it for the page.

This reasoning doesn't make sense to me. Specifically, it doesn't explain why there is a difference in these two cases:

1) width=device-width, with content that is wider and forces horizontal-scrollability
2) width<device-width, with content that is wider and forces horizontal-scrollability

According to your description in comment 28 the first will have double-tap behaviour and the second will not. This is what I was trying to verify when I ran into the problem I described above. It may be that I misunderstood what you were saying in comment 28 entirely, but even so I don't buy the reasoning you claim in comment 36. I still think it's simpler to just enable or disable double-tap based on the viewport width (as specified by the meta-viewport) and I'm going to go with that.

Comment 38

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

Dusted off the patches and re-pushed to try:

https://tbpl.mozilla.org/?tree=Try&rev=c4834decd3b3

Comment 39

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

Attachment: Patch for B2G (rebased)

Comment 40

[Matt Brubeck (:mbrubeck)]

Comment on attachment 8380749 [details] [diff] [review]
Patch for B2G (rebased)

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

r=mbrubeck, with some optional stylistic suggestions below.

::: gfx/layers/ipc/AsyncPanZoomController.cpp
@@ +1965,5 @@
> +  // In addition to looking at the zoom constraints, which comes from the meta
> +  // viewport tag, disallow zooming if we are overflow:hidden in either direction.
> +  ReentrantMonitorAutoEnter lock(mMonitor);
> +  return mZoomConstraints.mAllowDoubleTap
> +      && !(mFrameMetrics.GetDisableScrollingX() || mFrameMetrics.GetDisableScrollingY());

To reduce duplication, should this just be "return AllowZoom() && mZoomConstrains.mAllowDoubleTap;"?  (It is functionally equivalent as long as !AllowZoom implies !AllowDoubleTap.)

::: widget/windows/winrt/APZController.cpp
@@ +291,5 @@
>    if (aOutConstraints) {
>      // Until we support the meta-viewport tag properly allow zooming
>      // from 1/4 to 4x by default.
>      aOutConstraints->mAllowZoom = true;
> +    aOutConstraints->mAllowDoubleTap = true;

"false" would be more accurate here, since we never support double-tap on Metro currently.  (It doesn't make any practical difference, since we also don't use APZC's tap detection, but it might be less confusing.  Also a comment along these lines might help future readers.)

Comment 41

[Botond Ballo [:botond]]

Comment on attachment 8380749 [details] [diff] [review]
Patch for B2G (rebased)

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

::: content/base/src/nsDocument.cpp
@@ +7476,5 @@
>                (docId.Find("WML") != -1))
>            {
>              // We're making an assumption that the docType can't change here
>              mViewportType = DisplayWidthHeight;
> +            return nsViewportInfo(aDisplaySize, /*allowZoom*/true, /*allowDoubleTap*/false);

Why false here?

@@ +7485,5 @@
>        nsAutoString handheldFriendly;
>        GetHeaderData(nsGkAtoms::handheldFriendly, handheldFriendly);
>        if (handheldFriendly.EqualsLiteral("true")) {
>          mViewportType = DisplayWidthHeight;
> +        return nsViewportInfo(aDisplaySize, /*allowZoom*/true, /*allowDoubleTap*/false);

And here?

Comment 42

[Wesley Johnston (:wesj)]

Comment on attachment 8355671 [details] [diff] [review]
Patch for Fennec (v2)

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

Nits.

::: mobile/android/base/gfx/JavaPanZoomController.java
@@ +1390,5 @@
>      }
>  
>      @Override
>      public boolean onDoubleTap(MotionEvent motionEvent) {
> +        if (mTarget.getZoomConstraints().getAllowDoubleTap()) {

Long term we actually need to dispatch a DOM "doubleclick" event here (but not zoom, so this patch wouldn't change at all). Maybe naming this AllowDoubleTapZoom would be clearer.

::: mobile/android/chrome/content/browser.js
@@ +3994,3 @@
>      this.sendViewportMetadata();
> +
> +    this.updateViewportSize(gScreenWidth, aInitialLoad);

Is order important here. Can you add a note why if it is?

@@ +4117,5 @@
>      this.userScrollPos.y = win.scrollY;
>  
>      this.sendViewportUpdate();
>  
> +    if (metadata.allowZoom && !Services.prefs.getBoolPref("browser.ui.zoom.force-user-scalable")) {

Ouch. We should cache this pref too...

@@ +4122,5 @@
> +      // If the CSS viewport is narrower than the screen (i.e. width <= device-width)
> +      // then we disable double-tap-to-zoom behaviour.
> +      var oldAllowDoubleTap = metadata.allowDoubleTap;
> +      var newAllowDoubleTap = (viewportW > screenW / window.devicePixelRatio);
> +      if (oldAllowDoubleTap != newAllowDoubleTap) {

!==

@@ +5955,5 @@
>      // WebKit allows 0, "no", or "false" for viewport-user-scalable.
>      // Note: NaN != NaN. Therefore if minScale and maxScale are undefined the clause has no effect.
>      let allowZoomStr = windowUtils.getDocumentMetadata("viewport-user-scalable");
>      let allowZoom = !/^(0|no|false)$/.test(allowZoomStr) && (minScale != maxScale);
> +    // Double-tap should always be disabled if allowZoom is disabled. So we initialize

Add a blank line here so that this is a bit more readable. Different bug, but it looks (at a glance) like we could move this regex to not even run if the conditions below are true. We should cache these regex's if they're going to run on every page load as well...

@@ +5958,5 @@
>      let allowZoom = !/^(0|no|false)$/.test(allowZoomStr) && (minScale != maxScale);
> +    // Double-tap should always be disabled if allowZoom is disabled. So we initialize
> +    // allowDoubleTap to the same value as allowZoom and have additional conditions to
> +    // disable it in updateViewportSize.
> +    let allowDoubleTap = allowZoom;

It doesn't look like its worth making this temp variable even?

Comment 43

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

(In reply to Matt Brubeck (:mbrubeck) from comment #40)
> To reduce duplication, should this just be "return AllowZoom() &&
> mZoomConstrains.mAllowDoubleTap;"?  (It is functionally equivalent as long
> as !AllowZoom implies !AllowDoubleTap.)

Makes sense, will do.

> > +    aOutConstraints->mAllowDoubleTap = true;
> 
> "false" would be more accurate here, since we never support double-tap on
> Metro currently.  (It doesn't make any practical difference, since we also
> don't use APZC's tap detection, but it might be less confusing.  Also a
> comment along these lines might help future readers.)

Yeah I wasn't too sure about that. I'll change it to false.

(In reply to Botond Ballo [:botond] from comment #41)
> >              mViewportType = DisplayWidthHeight;
> > +            return nsViewportInfo(aDisplaySize, /*allowZoom*/true, /*allowDoubleTap*/false);
> 
> Why false here?
> 
> >          mViewportType = DisplayWidthHeight;
> > +        return nsViewportInfo(aDisplaySize, /*allowZoom*/true, /*allowDoubleTap*/false);
> 
> And here?

In both of these cases we assume "width=device-width" for the meta-viewport tag, and therefore we want to disable double-tap-to-zoom.

(In reply to Wesley Johnston (:wesj) from comment #42)
> >      public boolean onDoubleTap(MotionEvent motionEvent) {
> > +        if (mTarget.getZoomConstraints().getAllowDoubleTap()) {
> 
> Long term we actually need to dispatch a DOM "doubleclick" event here (but
> not zoom, so this patch wouldn't change at all). Maybe naming this
> AllowDoubleTapZoom would be clearer.

Makes sense, I can make that change throughout then.

> > +
> > +    this.updateViewportSize(gScreenWidth, aInitialLoad);
> 
> Is order important here. Can you add a note why if it is?

Yeah I'll add a comment.

> >      this.sendViewportUpdate();
> >  
> > +    if (metadata.allowZoom && !Services.prefs.getBoolPref("browser.ui.zoom.force-user-scalable")) {
> 
> Ouch. We should cache this pref too...

Maybe.. I'm not sure this gets hit often enough to matter though.

> > +      var newAllowDoubleTap = (viewportW > screenW / window.devicePixelRatio);
> > +      if (oldAllowDoubleTap != newAllowDoubleTap) {
> 
> !==

Will change.

> >      let allowZoomStr = windowUtils.getDocumentMetadata("viewport-user-scalable");
> >      let allowZoom = !/^(0|no|false)$/.test(allowZoomStr) && (minScale != maxScale);
> > +    // Double-tap should always be disabled if allowZoom is disabled. So we initialize
> 
> Add a blank line here so that this is a bit more readable.

Ok.

> > +    // Double-tap should always be disabled if allowZoom is disabled. So we initialize
> > +    // allowDoubleTap to the same value as allowZoom and have additional conditions to
> > +    // disable it in updateViewportSize.
> > +    let allowDoubleTap = allowZoom;
> 
> It doesn't look like its worth making this temp variable even?

I could probably get rid of it but it's a nice place to hang that comment.

Comment 44

[Botond Ballo [:botond]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #43)
> (In reply to Botond Ballo [:botond] from comment #41)
> > >              mViewportType = DisplayWidthHeight;
> > > +            return nsViewportInfo(aDisplaySize, /*allowZoom*/true, /*allowDoubleTap*/false);
> > 
> > Why false here?
> > 
> > >          mViewportType = DisplayWidthHeight;
> > > +        return nsViewportInfo(aDisplaySize, /*allowZoom*/true, /*allowDoubleTap*/false);
> > 
> > And here?
> 
> In both of these cases we assume "width=device-width" for the meta-viewport
> tag, and therefore we want to disable double-tap-to-zoom.

Can we add a comment in this function (not necessarily in these spots) that describes under what conditions we enable double-tap-to-zoom?

r=me with that comment

Comment 45

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

Added the comment and landed:

https://hg.mozilla.org/integration/mozilla-inbound/rev/c0f13ab0740e
https://hg.mozilla.org/integration/mozilla-inbound/rev/ff2a1d3d39f2

Comment 46

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/c0f13ab0740e
https://hg.mozilla.org/mozilla-central/rev/ff2a1d3d39f2

Comment 48

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

This caused a Talos regression on Android, I filed bug 976563 for it.

Comment 49

[Aaron Train [:aaronmt]]

Please see fallout bug 981636 where basic zooming functionality has regressed on Android.

Comment 50

[Kohei Yoshino [:kohei]]

Added this to the site compat doc. Feel free to edit the section.
https://developer.mozilla.org/en-US/Firefox/Releases/30/Site_Compatibility

Comment 51

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

Thanks! I made a couple of minor changes for clarification there.

Comment 52

[Kohei Yoshino [:kohei]]

Thank you kats!