Repeated zooming in bug on mobile Wikipedia site

VERIFIED FIXED in Firefox 47

Status

()

Firefox for Android
General
VERIFIED FIXED
2 years ago
a year ago

People

(Reporter: antlam, Assigned: rbarker)

Tracking

unspecified
Firefox 47
All
Android
Points:
---

Firefox Tracking Flags

(firefox47 verified)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

2 years ago
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
(Assignee)

Comment 1

2 years ago
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)

Updated

2 years ago
Assignee: nobody → rbarker
(Assignee)

Comment 3

2 years ago
Created attachment 8722749 [details] [diff] [review]
0001-Bug-1250614-Repeated-zooming-in-bug-on-mobile-Wikipedia-site-16022315-15b0003.patch
(Assignee)

Updated

2 years ago
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)
(Assignee)

Comment 5

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

Address review comments.
Attachment #8722749 - Attachment is obsolete: true
(Assignee)

Updated

2 years ago
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)
(Assignee)

Comment 8

2 years ago
(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

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

Comment 10

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/df0a1af46ff0
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox47: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 47

Comment 11

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

Updated

a year ago
status-firefox47: fixed → verified

Comment 12

a year ago
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.