1250614 - Repeated zooming in bug on mobile Wikipedia site

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

Tested on my 6P, today's Nightly, mobile Wikipedia article.

STR:
---
 1) Visit "Today's featured article" on Wikipedia
 2) Tap on "Search Wikipedia"
 3) Tap on white area under search bar to dismiss keyboard
 4) Tap on "Search Wikipedia" again
 5) See zoom in
 6) Repeat step 2 onwards, see repeated zoom in.

Expected:
---
 - Do not zoom in at all since this was designed for mobile sites, it doesn't need it

Comment 1

[Randall Barker [:rbarker]]

I think what me might want to do is not zoom beyond 1.0? That way we will zoom in on desktop pages that are zoomed out to fit but will only center elements if the page has no zoom? I've tested this and I think it will give the desired behavior. I'll post the patch after I get it cleaned up.

Comment 2

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

I do feel like this might be the best behavior, and it's an easy change.

Comment 3

[Randall Barker [:rbarker]]

Attachment: 0001-Bug-1250614-Repeated-zooming-in-bug-on-mobile-Wikipedia-site-16022315-15b0003.patch

Comment 4

[Botond Ballo [:botond]]

Comment on attachment 8722749 [details] [diff] [review]
0001-Bug-1250614-Repeated-zooming-in-bug-on-mobile-Wikipedia-site-16022315-15b0003.patch

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

::: gfx/layers/apz/src/APZCTreeManager.h
@@ +563,5 @@
>    gfx::TreeLog mApzcTreeLog;
>  
>    static float sDPI;
> +
> +  CSSToLayoutDeviceScale mDefaultScale;

I don't think there's a need to add a new property here - I would use FrameMetrics::GetDevPixelsPerCSSPixel(). Numerically it should be the same value on mobile (and if it's not, I think conceptually the FrameMetrics value is more correct).

::: gfx/layers/apz/src/AsyncPanZoomController.cpp
@@ +3546,5 @@
>        targetZoom = currentZoom;
> +    } else if(aFlags & ONLY_ZOOM_TO_DEFAULT_SCALE) {
> +      CSSToParentLayerScale defaultScale = ViewTargetAs<ParentLayerPixel, LayoutDevicePixel, CSSPixel>(
> +        GetApzcTreeManager()->GetDefaultScale(),
> +        PixelCastJustification::LayoutDeviceIsParentLayerForRCDRSF);

Using this justification is misleading; it says that the LayoutDevice pixels of the chrome widget (used to populate initial event coordinates and such) is the same as the ParentLayer pixels of the RCD-RSF. Here, however, we are converting the LayoutDevice pixels of the RCD-RSF to its ParentLayer pixels.

I think what we mean to say here is, we want to zoom to a level where the CSSToLayoutDeviceScale makes up the *entire* zoom, i.e. the remaining components of the zoom are 1. I would write that as:

  CSSToParentLayerScale zoomAtDefaultScale = mFrameMetrics.GetDevPixelsPerCSSPixel()
                                           * LayoutDeviceToParentLayerScale(1.0);

Comment 5

[Randall Barker [:rbarker]]

Attachment: 0001-Bug-1250614-Repeated-zooming-in-bug-on-mobile-Wikipedia-site-16022410-9bb62bb.patch

Address review comments.

Comment 6

[Botond Ballo [:botond]]

Comment on attachment 8723142 [details] [diff] [review]
0001-Bug-1250614-Repeated-zooming-in-bug-on-mobile-Wikipedia-site-16022410-9bb62bb.patch

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

Thanks!

::: gfx/layers/apz/src/APZCTreeManager.h
@@ +40,5 @@
>    DOUBLE_TAP_ZOOM =    1 << 3,
>    UNKNOWN =            1 << 4
>  };
>  
>  enum ZoomToRectBehavior : uint32_t {

Had I reviewed the patch that added this enum, I would have asked for it to be an enum class, and to reside in APZUtils.h. Guess it's too late for that now :)

::: gfx/layers/apz/src/AsyncPanZoomController.cpp
@@ +3548,5 @@
> +      CSSToParentLayerScale zoomAtDefaultScale =
> +        mFrameMetrics.GetDevPixelsPerCSSPixel() * LayoutDeviceToParentLayerScale(1.0);
> +      if (targetZoom.scale > zoomAtDefaultScale.scale) {
> +        // Only change the zoom if we are less than the default zoom
> +        if (currentZoom.scale < zoomAtDefaultScale.scale) {

This can also be written as:

  targetZoom.scale = std::max(zoomAtDefaultScale.scale, currentZoom.scale);

Comment 7

[Botond Ballo [:botond]]

Comment on attachment 8723142 [details] [diff] [review]
0001-Bug-1250614-Repeated-zooming-in-bug-on-mobile-Wikipedia-site-16022410-9bb62bb.patch

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

Bugzilla's being weird.

Comment 8

[Randall Barker [:rbarker]]

(In reply to Botond Ballo [:botond] from comment #6)
> Comment on attachment 8723142 [details] [diff] [review]
> 0001-Bug-1250614-Repeated-zooming-in-bug-on-mobile-Wikipedia-site-16022410-
> 9bb62bb.patch
> 
> Review of attachment 8723142 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Thanks!
> 
> ::: gfx/layers/apz/src/APZCTreeManager.h
> @@ +40,5 @@
> >    DOUBLE_TAP_ZOOM =    1 << 3,
> >    UNKNOWN =            1 << 4
> >  };
> >  
> >  enum ZoomToRectBehavior : uint32_t {
> 
> Had I reviewed the patch that added this enum, I would have asked for it to
> be an enum class, and to reside in APZUtils.h. Guess it's too late for that
> now :)
> 

This was done in Bug 1231517. :kats said he wanted it in APZCTreeManager.h so I went for consistency with the enum already there.

> ::: gfx/layers/apz/src/AsyncPanZoomController.cpp
> @@ +3548,5 @@
> > +      CSSToParentLayerScale zoomAtDefaultScale =
> > +        mFrameMetrics.GetDevPixelsPerCSSPixel() * LayoutDeviceToParentLayerScale(1.0);
> > +      if (targetZoom.scale > zoomAtDefaultScale.scale) {
> > +        // Only change the zoom if we are less than the default zoom
> > +        if (currentZoom.scale < zoomAtDefaultScale.scale) {
> 
> This can also be written as:
> 
>   targetZoom.scale = std::max(zoomAtDefaultScale.scale, currentZoom.scale);

My only push back on this is that it relies on knowing implementation details of LayoutDeviceToParentLayerScale and assumes scale will be the only field it will ever contain. I realize that is probably true but I've been burned by assumptions like that in the past.

Comment 9

[Pulsebot]

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

Comment 10

[Carsten Book [:Tomcat]]

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

Comment 11

[Alex Caziuc]

Tested using:
Device: Huawei Honor (Android 5.1.1)
Build: Firefox for Android Beta - 47.0b6

Comment 12

[Alex Caziuc]

Since this is verified I'll mark it as verified fixed.