454456 - Tweak zoom on pageload to favor quality

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: Panning/Zooming, defect)

Description

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

We currently zoom on pageload so webpages fill the screen better. However, some situations, where the zoom change is very small, the loss in rendering quality is not worth the webpage size change.

We should limit the zoom level so we don't scale for very small zoom changes.

Comment 1

[Brad Lassey [:blassey] (use needinfo?)]

can this be closed out?

Comment 2

[Matt Brubeck (:mbrubeck)]

We currently round the zoom level to the nearest .0001 (1/kBrowserViewZoomLevelPrecision).  For example, a zoom level of 1.01538 gets rounded to 1.0154.  We don't give special treatment to zoom levels near 1.0 or any other special values.  We should probably at least round zoom levels to 1.0 if they are within a certain range, like 0.9 < zoom < 1.1.

Comment 3

[Matt Brubeck (:mbrubeck)]

Another problem: If the page-fit zoom is, say, 0.902, then using the zoom in/out buttons will change it to 0.802 or 1.002.  Zoom in/out should perhaps round to the nearest 0.1.

Comment 4

[Matt Brubeck (:mbrubeck)]

Attachment: patch to tweak zoom near 1.0

Force pageZoomLevel values near 1.0 (plus or minus .05) to be exactly 1.0.

Also, make zoomin and zoomout commands round the new level to the nearest 0.1.

Comment 5

[Benjamin Stover (:stechz)]

Comment on attachment 434904 [details] [diff] [review]
patch to tweak zoom near 1.0

>+  /**
>+   * Force zoom levels "near" 1 to exactly 1.
>+   */

Nit: If function comment is one line, put it on one line as so: /** Function comment */

>   pageZoomLevel: function pageZoomLevel(visibleRect, browserW, browserH) {
>-    return BrowserView.Util.clampZoomLevel(visibleRect.width / browserW);
>+    let zl = BrowserView.Util.clampZoomLevel(visibleRect.width / browserW);
>+    return BrowserView.Util.adjustZoomLevel(zl);
>   },

>+  /**
>+   * Zoom one step in (negative) or out (positive).
>+   */

Great comment btw, the relation to +/- and in/out is useful.

>   zoom: function zoom(aDirection) {
>     let bv = this._browserView;
>-    let zoomLevel = bv.getZoomLevel() + (aDirection > 0 ? -0.1 : 0.1);
>+    let zoomLevel = bv.getZoomLevel() + kBrowserViewZoomLevelIncrement * (aDirection > 0 ? -1 : 1);
>+    // Round to the nearest whole increment, so we don't end up at values like 1.004.
>+    let rounded = Math.round(zoomLevel / kBrowserViewZoomLevelIncrement) * kBrowserViewZoomLevelIncrement;
>     let center = this.getVisibleRect().center().map(bv.viewportToBrowser);
>-    this.setVisibleRect(this._getZoomRectForPoint(center.x, center.y, zoomLevel));
>+    this.setVisibleRect(this._getZoomRectForPoint(center.x, center.y, rounded));
>   },

Hmm, it seems like you're clamping the zoom levels inline and with your own constant.  Is this intentional?  clampZoomLevel does the same thing:

>   clampZoomLevel: function clampZoomLevel(zl) {
>     let bounded = Math.min(Math.max(kBrowserViewZoomLevelMin, zl), kBrowserViewZoomLevelMax);
>     let rounded = Math.round(bounded * kBrowserViewZoomLevelPrecision) / kBrowserViewZoomLevelPrecision;
>     return (rounded) ? rounded : 1.0;
>   },
> 

I'd argue bumping up our existing constant instead of adding a new one and doing it in zoom, unless I'm missing something.

One other thing: personally I'd bump up the fuzzy-1.0 constant to a .2 tolerance as this keeps websites up to 960px rendering at the beautiful ZL of 1.0.

Comment 6

[Matt Brubeck (:mbrubeck)]

Attachment: patch v2

(In reply to comment #5)
> Hmm, it seems like you're clamping the zoom levels inline and with your own
> constant.  Is this intentional?  clampZoomLevel does the same thing: [...]
> 
> I'd argue bumping up our existing constant instead of adding a new one and
> doing it in zoom, unless I'm missing something.

I did also try changing clampZoomLevel's "precision" constant, but that causes pageZoomLevel to be less precise even when not near 1.0.  That causes pages to frequently be mis-fit with no quality benefit.

Instead of rounding all values to the nearest increment, we could just have Browser.zoom call BrowserView.Util.adjustZoomLevel.  Then it would only adjust levels near 1.  This is probably best since we may get rid of the linear zoom increment for bug 554979.  Okay, done.

> One other thing: personally I'd bump up the fuzzy-1.0 constant to a .2
> tolerance as this keeps websites up to 960px rendering at the beautiful ZL of
> 1.0.

Sounds good.  If I'm calculating this right, it means that pages with widths from 667px to 1000px (pageZoomLevel of 1.2 to 0.8) will no longer be zoomed to fit on an 800px display.  That's a lot of pages!  Well, might as well try it.

Here's a revised patch with all the above changes.

Comment 7

[Benjamin Stover (:stechz)]

Comment on attachment 434923 [details] [diff] [review]
patch v2

>+  /**
>+   * Zoom one step in (negative) or out (positive).
>+   */

One-line this comment too.

>   zoom: function zoom(aDirection) {
>     let bv = this._browserView;
>     let zoomLevel = bv.getZoomLevel() + (aDirection > 0 ? -0.1 : 0.1);
>+    let adjusted = BrowserView.Util.adjustZoomLevel(zoomLevel);
>     let center = this.getVisibleRect().center().map(bv.viewportToBrowser);
>-    this.setVisibleRect(this._getZoomRectForPoint(center.x, center.y, zoomLevel));
>+    this.setVisibleRect(this._getZoomRectForPoint(center.x, center.y, adjusted));
>   },

Much better.

Comment 8

[Matt Brubeck (:mbrubeck)]

Attachment: patch v3 [r=stechz,r=mfinkle]

Comment 9

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

Working pretty well on the pages I visited. Minimal horz panning occurred, but keeping zoom=1 was worth it.

pushed:
http://hg.mozilla.org/mobile-browser/rev/267c87f3460e

Comment 10

[Aakash Desai [:aakashd]]

verified FIXED On builds using www.scu.edu/ecampus :

Mozilla/5.0 (X11; U; Linux armv7l; Nokia N900; en-US; rv:1.9.2.3pre) Gecko/20100329 Namoroka/3.6.3pre Fennec/1.1a2pre

and

Mozilla/5.0 (X11; U; Linux armv7l; Nokia N900; en-US; rv:1.9.3a4pre) Gecko/20100329 Namoroka/3.7a4pre Fennec/1.1a2pre


litmus testcase https://litmus.mozilla.org/show_test.cgi?id=11635 created to regression test this bug.