Closed Bug 454456 Opened 11 years ago Closed 10 years ago

Tweak zoom on pageload to favor quality

Categories

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

x86
macOS
defect
Not set

Tracking

(Not tracked)

VERIFIED FIXED
fennec1.0b1

People

(Reporter: mfinkle, Assigned: mbrubeck)

Details

Attachments

(1 file, 2 obsolete files)

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.
Flags: wanted-fennec1.0+
Target Milestone: --- → Fennec A3
can this be closed out?
Component: General → Panning/Zooming
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.
Assignee: nobody → 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.
Attached patch patch to tweak zoom near 1.0 (obsolete) — Splinter Review
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.
Attachment #434904 - Flags: review?(webapps)
Status: NEW → ASSIGNED
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.
Attachment #434904 - Flags: review?(webapps) → review-
Attached patch patch v2 (obsolete) — Splinter Review
(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.
Attachment #434904 - Attachment is obsolete: true
Attachment #434923 - Flags: review?(webapps)
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.
Attachment #434923 - Flags: review?(webapps) → review+
Attachment #434923 - Attachment is obsolete: true
Keywords: checkin-needed
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
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Flags: in-litmus?
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.
Status: RESOLVED → VERIFIED
Flags: in-litmus? → in-litmus+
You need to log in before you can comment on or make changes to this bug.