Bug 1194895 - New tab page has unnecessary (or marginally-necessary) vertical scrollbar at various sizes. [r=emtwo]

browser/base/content/newtab/grid.js

@@ -9,6 +9,7 @@
  */
 const GRID_BOTTOM_EXTRA = 7; // title's line-height extends 7px past the margin
 const GRID_WIDTH_EXTRA = 1; // provide 1px buffer to allow for rounding error
+const SPONSORED_TAG_BUFFER = 2; // 2px buffer to clip off top of sponsored tag
 
 /**
  * This singleton represents the grid that contains all sites.
@@ -175,6 +176,15 @@ let gGrid = {
     this._siteFragment.appendChild(site);
   },
 
+  /**
+   * Test a tile at a given position for being pinned or history
+   * @param position Position in sites array
+   */
+  _isHistoricalTile: function Grid_isHistoricalTile(aPos) {
+    let site = this.sites[aPos];
+    return site && (site.isPinned() || site.link && site.link.type == "history");
+  },
+
   /**
    * Make sure the correct number of rows and columns are visible
    */
@@ -195,9 +205,50 @@ let gGrid = {
         parseFloat(getComputedStyle(refCell).marginBottom);
       this._cellWidth = refCell.offsetWidth + this._cellMargin;
     }
-    this._node.style.height = this._computeHeight() + "px";
-    this._node.style.maxHeight = this._node.style.height;
+
+    let searchContainer = document.querySelector("#newtab-search-container");
+    // Save search-container margin height
+    if (this._searchContainerMargin  === undefined) {
+      this._searchContainerMargin = parseFloat(getComputedStyle(searchContainer).marginBottom) +
+                                    parseFloat(getComputedStyle(searchContainer).marginTop);
+    }
+
+    // Find the number of rows we can place into view port
+    let availHeight = document.documentElement.clientHeight - this._cellMargin -
+                      searchContainer.offsetHeight - this._searchContainerMargin;
+    let visibleRows = Math.floor(availHeight / this._cellHeight);
+
+    // Find the number of columns that fit into view port
+    let maxGridWidth = gGridPrefs.gridColumns * this._cellWidth + GRID_WIDTH_EXTRA;
+    // available width is current grid width, but no greater than maxGridWidth
+    let availWidth = Math.min(document.querySelector("#newtab-grid").clientWidth,
+                              maxGridWidth);
+    // finally get the number of columns we can fit into view port
+    let gridColumns =  Math.floor(availWidth / this._cellWidth);
+    // walk sites backwords until a pinned or history tile is found or visibleRows reached
+    let tileIndex = Math.min(gGridPrefs.gridRows * gridColumns, this.sites.length) - 1;
+    while (tileIndex >= visibleRows * gridColumns) {
+      if (this._isHistoricalTile(tileIndex)) {
+        break;
+      }
+      tileIndex --;
+    }
+
+    // Compute the actual number of grid rows we will display (potentially
+    // with a scroll bar). tileIndex now points to a historical tile with
+    // heighest index or to the last index of the visible row, if none found
+    // Dividing tileIndex by number of tiles in a column gives the rows
+    let gridRows = Math.floor(tileIndex / gridColumns) + 1;
+
+    // we need to set grid width, for otherwise the scrollbar may shrink
+    // the grid when shown and cause grid layout to be different from
+    // what being computed above. This, in turn, may cause scrollbar shown
+    // for directory tiles, and introduce jitter when grid width is aligned
+    // exactly on the column boundary
+    this._node.style.width = gridColumns * this._cellWidth + "px";
     this._node.style.maxWidth = gGridPrefs.gridColumns * this._cellWidth +
                                 GRID_WIDTH_EXTRA + "px";
+    this._node.style.height = this._computeHeight() + "px";
+    this._node.style.maxHeight = this._computeHeight(gridRows) - SPONSORED_TAG_BUFFER + "px";
   }
 };

browser/base/content/test/newtab/browser.ini

@@ -49,3 +49,4 @@ support-files =
 [browser_newtab_update.js]
 [browser_newtab_bug1145428.js]
 [browser_newtab_bug1178586.js]
+[browser_newtab_bug1194895.js]

browser/base/content/test/newtab/browser_newtab_bug1194895.js

@@ -0,0 +1,141 @@
+/* Any copyright is dedicated to the Public Domain.
+   http://creativecommons.org/publicdomain/zero/1.0/ */
+
+const PRELOAD_PREF = "browser.newtab.preload";
+const PREF_NEWTAB_COLUMNS = "browser.newtabpage.columns";
+const PREF_NEWTAB_ROWS = "browser.newtabpage.rows";
+
+function populateDirectoryTiles() {
+  let directoryTiles = [];
+  let i = 0;
+  while (i++ < 14) {
+    directoryTiles.push({
+      directoryId: i,
+      url: "http://example" + i + ".com/",
+      enhancedImageURI: "data:image/png;base64,helloWORLD",
+      title: "dirtitle" + i,
+      type: "affiliate"
+    });
+  }
+  return directoryTiles;
+}
+
+gDirectorySource = "data:application/json," + JSON.stringify({
+  "directory": populateDirectoryTiles()
+});
+
+
+function runTests() {
+  let origEnhanced = NewTabUtils.allPages.enhanced;
+  let origCompareLinks = NewTabUtils.links.compareLinks;
+  registerCleanupFunction(() => {
+    Services.prefs.clearUserPref(PRELOAD_PREF);
+    Services.prefs.clearUserPref(PREF_NEWTAB_ROWS);
+    Services.prefs.clearUserPref(PREF_NEWTAB_COLUMNS);
+    NewTabUtils.allPages.enhanced = origEnhanced;
+    NewTabUtils.links.compareLinks = origCompareLinks;
+  });
+
+  // turn off preload to ensure grid updates on every setLinks
+  Services.prefs.setBoolPref(PRELOAD_PREF, false);
+  // set newtab to have three columns only
+  Services.prefs.setIntPref(PREF_NEWTAB_COLUMNS, 3);
+  Services.prefs.setIntPref(PREF_NEWTAB_ROWS, 5);
+
+  yield addNewTabPageTab();
+  yield customizeNewTabPage("enhanced"); // Toggle enhanced off
+
+  // Testing history tiles
+
+  // two rows of tiles should always fit on any screen
+  yield setLinks("0,1,2,3,4,5");
+  yield addNewTabPageTab();
+
+  // should do not see scrollbar since tiles fit into visible space
+  checkGrid("0,1,2,3,4,5");
+  ok(!hasScrollbar(), "no scrollbar");
+
+  // add enough tiles to cause extra two rows and observe scrollbar
+  yield setLinks("0,1,2,3,4,5,6,7,8,9");
+  yield addNewTabPageTab();
+  checkGrid("0,1,2,3,4,5,6,7,8,9");
+  ok(hasScrollbar(), "document has scrollbar");
+
+  // pin the last tile to make it stay at the bottom of the newtab
+  pinCell(9);
+  // block first 6 tiles, which should not remove the scroll bar
+  // since the last tile is pinned in the nineth position
+  for (let i = 0; i < 6; i++) {
+    yield blockCell(0);
+  }
+  yield addNewTabPageTab();
+  checkGrid("6,7,8,,,,,,,9p");
+  ok(hasScrollbar(), "document has scrollbar when tile is pinned to the last row");
+
+  // unpin the site: this will move tile up and make scrollbar disappear
+  yield unpinCell(9);
+  yield addNewTabPageTab();
+  checkGrid("6,7,8,9");
+  ok(!hasScrollbar(), "no scrollbar when bottom row tile is unpinned");
+
+  // reset everything to clean slate
+  NewTabUtils.restore();
+
+  // Testing directory tiles
+  yield customizeNewTabPage("enhanced"); // Toggle enhanced on
+
+  // setup page with no history tiles to test directory only display
+  yield setLinks([]);
+  yield addNewTabPageTab();
+  ok(!hasScrollbar(), "no scrollbar for directory tiles");
+
+  // introduce one history tile - it should occupy the last
+  // available slot at the bottom of newtab and cause scrollbar
+  yield setLinks("41");
+  yield addNewTabPageTab();
+  ok(hasScrollbar(), "adding low frecency history site causes scrollbar");
+
+  // set PREF_NEWTAB_ROWS to 4, that should clip off the history tile
+  // and remove scroll bar
+  Services.prefs.setIntPref(PREF_NEWTAB_ROWS, 4);
+  yield addNewTabPageTab();
+  ok(!hasScrollbar(), "no scrollbar if history tiles falls past max rows");
+
+  // restore max rows and watch scrollbar re-appear
+  Services.prefs.setIntPref(PREF_NEWTAB_ROWS, 5);
+  yield addNewTabPageTab();
+  ok(hasScrollbar(), "scrollbar is back when max rows allow for bottom history tile");
+
+  // block that history tile, and watch scrollbar disappear
+  yield blockCell(14);
+  yield addNewTabPageTab();
+  ok(!hasScrollbar(), "no scrollbar after bottom history tiles is blocked");
+
+  // Test well-populated user history - newtab has highly-frecent history sites
+  // redefine compareLinks to always choose history tiles first
+  NewTabUtils.links.compareLinks = function (aLink1, aLink2) {
+    if (aLink1.type == aLink2.type) {
+      return aLink2.frecency - aLink1.frecency ||
+             aLink2.lastVisitDate - aLink1.lastVisitDate;
+    }
+    else {
+      if (aLink2.type == "history") {
+        return 1;
+      }
+      else {
+        return -1;
+      }
+    }
+  };
+
+  // add a row of history tiles, directory tiles will be clipped off, hence no scrollbar
+  yield setLinks("31,32,33");
+  yield addNewTabPageTab();
+  ok(!hasScrollbar(), "no scrollbar when directory tiles follow history tiles");
+
+  // fill first four rows with history tiles and observer scrollbar
+  yield setLinks("30,31,32,33,34,35,36,37,38,39");
+  yield addNewTabPageTab();
+  ok(hasScrollbar(), "scrollbar appears when history tiles need extra row");
+
+}

browser/base/content/test/newtab/head.js

@@ -785,3 +785,11 @@ function customizeNewTabPage(aTheme) {
 
   promise.then(TestRunner.next);
 }
+
+/**
+ * Reports presence of a scrollbar
+ */
+function hasScrollbar() {
+  let docElement = getContentDocument().documentElement;
+  return docElement.scrollHeight > docElement.clientHeight;
+}