Bug 856497 - Fix dynamic viewport sizing for margin changes. r=kats

The viewport wasn't being recalculated when the viewport margins changed, and
was also subject to some rounding errors. Round off the values before
comparing them (as they're screen pixels), and make sure to update the viewport
when the margins change. Margins were also not correctly being altered when
in overscroll, which could cause bottom-aligned fixed position content to be
incorrectly offset.

mobile/android/base/BrowserApp.java

@@ -592,6 +592,11 @@ abstract public class BrowserApp extends GeckoApp
 
         mBrowserToolbar.updateBackButton(false);
         mBrowserToolbar.updateForwardButton(false);
+
+        // Reset mToolbarHeight before setting margins so we force the
+        // Viewport:FixedMarginsChanged message to be sent again now that
+        // Gecko has loaded.
+        mToolbarHeight = 0;
         ((BrowserToolbarLayout)mBrowserToolbar.getLayout()).refreshMargins();
 
         mDoorHangerPopup.setAnchor(mBrowserToolbar.mFavicon);

mobile/android/base/gfx/GeckoLayerClient.java

@@ -259,57 +259,51 @@ public class GeckoLayerClient implements LayerView.Listener, PanZoomTarget
         });
     }
 
-    private boolean adjustFixedLayerMarginsForOverscroll(ImmutableViewportMetrics metrics, RectF adjustedMargins) {
+    private void adjustFixedLayerMarginsForOverscroll(ImmutableViewportMetrics metrics, RectF adjustedMargins) {
         // When the page is in overscroll, we want that to 'eat into' the fixed
         // margin on that side of the viewport. This is because overscroll
         // equates to extra visible area and we use the fixed margins to stop
         // fixed position elements from being obscured by chrome.
-        // In this situation, we also want to do the opposite adjustment to the
+        // When we're overscrolled, we also want to make sure that the fixed
-        // other end of the axis, so that when overscroll is cancelled out by
+        // content on the non-overscroll side isn't obscured by the edge of the
-        // the margin area, the opposite side isn't pushed out of the viewport.
+        // window, so when adjusting one side of a margin, we apply the opposite
-        boolean changed = false;
+        // adjustment to the other side of the margin.
         adjustedMargins.left = metrics.fixedLayerMarginLeft;
         adjustedMargins.top = metrics.fixedLayerMarginTop;
         adjustedMargins.right = metrics.fixedLayerMarginRight;
         adjustedMargins.bottom = metrics.fixedLayerMarginBottom;
 
-        if (metrics.getPageWidth() > metrics.getWidthWithoutMargins()) {
+        // The maximum margins are determined by the scrollable area of the page.
-            // Adjust for left overscroll
+        float maxMarginWidth = Math.max(0, metrics.getPageWidth() - metrics.getWidthWithoutMargins());
-            if (metrics.viewportRectLeft < metrics.pageRectLeft && metrics.fixedLayerMarginLeft > 0) {
+        float maxMarginHeight = Math.max(0, metrics.getPageHeight() - metrics.getHeightWithoutMargins());
-                adjustedMargins.left = Math.max(0, metrics.fixedLayerMarginLeft
-                                                   - (metrics.pageRectLeft - metrics.viewportRectLeft));
-                adjustedMargins.right += metrics.fixedLayerMarginLeft - adjustedMargins.left;
-                changed = true;
-            }
 
-            // Adjust for right overscroll
+        // Adjust for left overscroll
-            if (metrics.viewportRectRight < metrics.pageRectRight && metrics.fixedLayerMarginRight > 0) {
+        float leftOverscroll = metrics.pageRectLeft - metrics.viewportRectLeft;
-                adjustedMargins.right = Math.max(0, metrics.fixedLayerMarginRight
+        if (leftOverscroll > 0) {
-                                                   - (metrics.pageRectRight - metrics.viewportRectRight));
+            adjustedMargins.left = FloatUtils.clamp(adjustedMargins.left - leftOverscroll, 0, maxMarginWidth);
-                adjustedMargins.left += metrics.fixedLayerMarginRight - adjustedMargins.right;
+            adjustedMargins.right = FloatUtils.clamp(adjustedMargins.right + leftOverscroll, 0, maxMarginWidth - adjustedMargins.left);
-                changed = true;
-            }
         }
 
-        if (metrics.getPageHeight() > metrics.getHeightWithoutMargins()) {
+        // Adjust for right overscroll
-            // Adjust for top overscroll
+        float rightOverscroll = metrics.viewportRectRight - metrics.pageRectRight;
-            if (metrics.viewportRectTop < metrics.pageRectTop && metrics.fixedLayerMarginTop > 0) {
+        if (rightOverscroll > 0) {
-                adjustedMargins.top = Math.max(0, metrics.fixedLayerMarginTop
+            adjustedMargins.right = FloatUtils.clamp(adjustedMargins.right - rightOverscroll, 0, maxMarginWidth);
-                                                   - (metrics.pageRectTop - metrics.viewportRectTop));
+            adjustedMargins.left = FloatUtils.clamp(adjustedMargins.left + rightOverscroll, 0, maxMarginWidth - adjustedMargins.right);
-                adjustedMargins.bottom += metrics.fixedLayerMarginTop - adjustedMargins.top;
-                changed = true;
-            }
-
-            // Adjust for bottom overscroll
-            if (metrics.viewportRectBottom < metrics.pageRectBottom && metrics.fixedLayerMarginBottom > 0) {
-                adjustedMargins.bottom = Math.max(0, metrics.fixedLayerMarginBottom
-                                                   - (metrics.pageRectBottom - metrics.viewportRectBottom));
-                adjustedMargins.top += metrics.fixedLayerMarginBottom - adjustedMargins.bottom;
-                changed = true;
-            }
         }
 
-        return changed;
+        // Adjust for top overscroll
+        float topOverscroll = metrics.pageRectTop - metrics.viewportRectTop;
+        if (topOverscroll > 0) {
+            adjustedMargins.top = FloatUtils.clamp(adjustedMargins.top - topOverscroll, 0, maxMarginHeight);
+            adjustedMargins.bottom = FloatUtils.clamp(adjustedMargins.bottom + topOverscroll, 0, maxMarginHeight - adjustedMargins.top);
+        }
+
+        // Adjust for bottom overscroll
+        float bottomOverscroll = metrics.viewportRectBottom - metrics.pageRectBottom;
+        if (bottomOverscroll > 0) {
+            adjustedMargins.bottom = FloatUtils.clamp(adjustedMargins.bottom - bottomOverscroll, 0, maxMarginHeight);
+            adjustedMargins.top = FloatUtils.clamp(adjustedMargins.top + bottomOverscroll, 0, maxMarginHeight - adjustedMargins.bottom);
+        }
     }
 
     private void adjustViewport(DisplayPortMetrics displayPort) {
@@ -317,11 +311,10 @@ public class GeckoLayerClient implements LayerView.Listener, PanZoomTarget
         ImmutableViewportMetrics clampedMetrics = metrics.clamp();
 
         RectF fixedLayerMargins = new RectF();
-        if (adjustFixedLayerMarginsForOverscroll(metrics, fixedLayerMargins)) {
+        adjustFixedLayerMarginsForOverscroll(metrics, fixedLayerMargins);
-            clampedMetrics = clampedMetrics.setFixedLayerMargins(
+        clampedMetrics = clampedMetrics.setFixedLayerMargins(
-                fixedLayerMargins.left, fixedLayerMargins.top,
+            fixedLayerMargins.left, fixedLayerMargins.top,
-                fixedLayerMargins.right, fixedLayerMargins.bottom);
+            fixedLayerMargins.right, fixedLayerMargins.bottom);
-        }
 
         if (displayPort == null) {
             displayPort = DisplayPortCalculator.calculate(metrics, mPanZoomController.getVelocityVector());

mobile/android/base/util/FloatUtils.java

@@ -7,6 +7,8 @@ package org.mozilla.gecko.util;
 
 import android.graphics.PointF;
 
+import java.lang.IllegalArgumentException;
+
 public final class FloatUtils {
     private FloatUtils() {}
 
@@ -26,4 +28,16 @@ public final class FloatUtils {
     public static float interpolate(float from, float to, float t) {
         return from + (to - from) * t;
     }
+
+    /**
+     * Returns 'value', clamped so that it isn't any lower than 'low', and it
+     * isn't any higher than 'high'.
+     */
+    public static float clamp(float value, float low, float high) {
+        if (high < low) {
+          throw new IllegalArgumentException(
+              "clamp called with invalid parameters (" + high + " < " + low + ")" );
+        }
+        return Math.max(low, Math.min(high, value));
+    }
 }

mobile/android/chrome/content/browser.js

@@ -1311,6 +1311,7 @@ var BrowserApp = {
 
       case "Viewport:FixedMarginsChanged":
         gViewportMargins = JSON.parse(aData);
+        this.selectedTab.updateViewportSize(gScreenWidth);
         break;
 
       case "nsPref:changed":
@@ -2952,13 +2953,13 @@ Tab.prototype = {
     // within the screen size, so remeasure when the page size remains within
     // the threshold of screen + margins, in case it's sizing itself relative
     // to the viewport.
-    if (!this.updatingViewportForPageSizeChange) {
+    if (!this.updatingViewportForPageSizeChange && aPageSizeUpdate) {
       this.updatingViewportForPageSizeChange = true;
-      if (((viewport.pageBottom - viewport.pageTop
+      if (((Math.round(viewport.pageBottom - viewport.pageTop)
-              < gScreenHeight + gViewportMargins.top + gViewportMargins.bottom)
+              <= gScreenHeight + gViewportMargins.top + gViewportMargins.bottom)
              != this.viewportExcludesVerticalMargins) ||
-          ((viewport.pageRight - viewport.pageLeft
+          ((Math.round(viewport.pageRight - viewport.pageLeft)
-              < gScreenWidth + gViewportMargins.left + gViewportMargins.right)
+              <= gScreenWidth + gViewportMargins.left + gViewportMargins.right)
              != this.viewportExcludesHorizontalMargins)) {
         this.updateViewportSize(gScreenWidth);
       }