Closed Bug 1250614 Opened 4 years ago Closed 4 years ago

Repeated zooming in bug on mobile Wikipedia site

Categories

(Firefox for Android :: General, defect)

All
Android
defect
Not set

Tracking

()

VERIFIED FIXED
Firefox 47
Tracking Status
firefox47 --- verified

People

(Reporter: antlam, Assigned: rbarker)

Details

Attachments

(1 file, 1 obsolete file)

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
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.
I do feel like this might be the best behavior, and it's an easy change.
Assignee: nobody → rbarker
Attachment #8722749 - Flags: review?(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);
Attachment #8722749 - Flags: review?(botond)
Attachment #8723142 - Flags: review?(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);
Attachment #8723142 - Flags: review+
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.
Attachment #8723142 - Flags: review?(botond)
(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.
https://hg.mozilla.org/mozilla-central/rev/df0a1af46ff0
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 47
Tested using:
Device: Huawei Honor (Android 5.1.1)
Build: Firefox for Android Beta - 47.0b6
Since this is verified I'll mark it as verified fixed.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.