Bug 1194895 - New tab page has unnecessary (or marginally-necessary) vertical scrollbar at various sizes. r=emtwo, a=sledru
authorMaxim Zhilyaev <mzhilyaev@mozilla.com>
Sun, 30 Aug 2015 14:34:00 -0400
changeset 289027 63e48f89ff5caca7c4f99d6909d16038f5ac0cea
parent 289026 8f5c669a887e5111dbce9884145fe278e3e1afad
child 289028 753d9e05c8e820c4e91ba2373a321012b27c54ef
push id5067
push userraliiev@mozilla.com
push dateMon, 21 Sep 2015 14:04:52 +0000
treeherdermozilla-beta@14221ffe5b2f [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersemtwo, sledru
bugs1194895
milestone42.0a2
Bug 1194895 - New tab page has unnecessary (or marginally-necessary) vertical scrollbar at various sizes. r=emtwo, a=sledru
browser/base/content/newtab/grid.js
browser/base/content/test/newtab/browser.ini
browser/base/content/test/newtab/browser_newtab_bug1194895.js
browser/base/content/test/newtab/browser_newtab_bug991111.js
browser/base/content/test/newtab/browser_newtab_bug998387.js
browser/base/content/test/newtab/browser_newtab_focus.js
browser/base/content/test/newtab/head.js
--- a/browser/base/content/newtab/grid.js
+++ b/browser/base/content/newtab/grid.js
@@ -4,16 +4,17 @@
  * You can obtain one at http://mozilla.org/MPL/2.0/. */
 #endif
 
 /**
  * Define various fixed dimensions
  */
 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.
  */
 let gGrid = {
   /**
    * The DOM node of the grid.
    */
@@ -171,16 +172,25 @@ let gGrid = {
       '       class="newtab-control newtab-control-block"/>' +
       '<span class="newtab-suggested"/>';
 
     this._siteFragment = document.createDocumentFragment();
     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
    */
   _resizeGrid: function Grid_resizeGrid() {
     // If we're somehow called before the page has finished loading,
     // let's bail out to avoid caching zero heights and widths.
     // We'll be called again when DOMContentLoaded fires.
     // Same goes for the grid if that's not ready yet.
     if (!this.isDocumentLoaded || !this._ready) {
@@ -191,17 +201,54 @@ let gGrid = {
     if (this._cellMargin === undefined) {
       let refCell = document.querySelector(".newtab-cell");
       this._cellMargin = parseFloat(getComputedStyle(refCell).marginTop);
       this._cellHeight = refCell.offsetHeight + this._cellMargin +
         parseFloat(getComputedStyle(refCell).marginBottom);
       this._cellWidth = refCell.offsetWidth + this._cellMargin;
     }
 
-    let availSpace = document.documentElement.clientHeight - this._cellMargin -
-                     document.querySelector("#newtab-search-container").offsetHeight;
-    let visibleRows = Math.floor(availSpace / this._cellHeight);
-    this._node.style.height = this._computeHeight() + "px";
-    this._node.style.maxHeight = this._computeHeight(visibleRows) + "px";
+    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";
   }
 };
--- a/browser/base/content/test/newtab/browser.ini
+++ b/browser/base/content/test/newtab/browser.ini
@@ -44,8 +44,9 @@ support-files =
   ../general/searchSuggestionEngine.xml
   ../general/searchSuggestionEngine.sjs
 [browser_newtab_sponsored_icon_click.js]
 [browser_newtab_undo.js]
 [browser_newtab_unpin.js]
 [browser_newtab_update.js]
 [browser_newtab_bug1145428.js]
 [browser_newtab_bug1178586.js]
+[browser_newtab_bug1194895.js]
new file mode 100644
--- /dev/null
+++ b/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");
+
+}
--- a/browser/base/content/test/newtab/browser_newtab_bug991111.js
+++ b/browser/base/content/test/newtab/browser_newtab_bug991111.js
@@ -1,19 +1,26 @@
 /* Any copyright is dedicated to the Public Domain.
    http://creativecommons.org/publicdomain/zero/1.0/ */
 
+const PREF_NEWTAB_ROWS = "browser.newtabpage.rows";
+
 function runTests() {
+  // set max rows to 1, to avoid scroll events by clicking middle button
+  Services.prefs.setIntPref(PREF_NEWTAB_ROWS, 1);
   yield setLinks("-1");
   yield addNewTabPageTab();
+  // we need a second newtab to honor max rows
+  yield addNewTabPageTab();
 
   // Remember if the click handler was triggered
   let cell = getCell(0);
   let clicked = false;
   cell.site.onClick = e => {
     clicked = true;
     executeSoon(TestRunner.next);
   };
 
   // Send a middle-click and make sure it happened
   yield EventUtils.synthesizeMouseAtCenter(cell.node, {button: 1}, getContentWindow());
   ok(clicked, "middle click triggered click listener");
+  Services.prefs.clearUserPref(PREF_NEWTAB_ROWS);
 }
--- a/browser/base/content/test/newtab/browser_newtab_bug998387.js
+++ b/browser/base/content/test/newtab/browser_newtab_bug998387.js
@@ -1,14 +1,20 @@
 /* Any copyright is dedicated to the Public Domain.
    http://creativecommons.org/publicdomain/zero/1.0/ */
 
+const PREF_NEWTAB_ROWS = "browser.newtabpage.rows";
+
 function runTests() {
+  // set max rows to 1, to avoid scroll events by clicking middle button
+  Services.prefs.setIntPref(PREF_NEWTAB_ROWS, 1);
   yield setLinks("0");
   yield addNewTabPageTab();
+  // we need a second newtab to honor max rows
+  yield addNewTabPageTab();
 
   // Remember if the click handler was triggered
   let {site} = getCell(0);
   let origOnClick = site.onClick;
   let clicked = false;
   site.onClick = e => {
     origOnClick.call(site, e);
     clicked = true;
@@ -17,9 +23,10 @@ function runTests() {
 
   // Send a middle-click and make sure it happened
   let block = getContentDocument().querySelector(".newtab-control-block");
   yield EventUtils.synthesizeMouseAtCenter(block, {button: 1}, getContentWindow());
   ok(clicked, "middle click triggered click listener");
 
   // Make sure the cell didn't actually get blocked
   checkGrid("0");
+  Services.prefs.clearUserPref(PREF_NEWTAB_ROWS);
 }
--- a/browser/base/content/test/newtab/browser_newtab_focus.js
+++ b/browser/base/content/test/newtab/browser_newtab_focus.js
@@ -5,17 +5,18 @@
  * These tests make sure that focusing the 'New Tage Page' works as expected.
  */
 function runTests() {
   // Handle the OSX full keyboard access setting
   Services.prefs.setIntPref("accessibility.tabfocus", 7);
 
   // Focus count in new tab page.
   // 30 = 9 * 3 + 3 = 9 sites, each with link, pin and remove buttons; search
-  // bar; search button; and toggle button.
+  // bar; search button; and toggle button. Additionaly there may or may not be
+  // a scroll bar caused by fix to 1180387, which will eat an extra focus
   let FOCUS_COUNT = 30;
 
   // Create a new tab page.
   yield setLinks("0,1,2,3,4,5,6,7,8");
   setPinnedLinks("");
 
   yield addNewTabPageTab();
   gURLBar.focus();
@@ -37,17 +38,19 @@ function runTests() {
 function countFocus(aExpectedCount) {
   let focusCount = 0;
   let contentDoc = getContentDocument();
 
   window.addEventListener("focus", function onFocus() {
     let focusedElement = document.commandDispatcher.focusedElement;
     if (focusedElement && focusedElement.classList.contains("urlbar-input")) {
       window.removeEventListener("focus", onFocus, true);
-      is(focusCount, aExpectedCount, "Validate focus count in the new tab page.");
+      // account for a potential presence of a scroll bar
+      ok(focusCount == aExpectedCount || focusCount == (aExpectedCount + 1),
+         "Validate focus count in the new tab page.");
       executeSoon(TestRunner.next);
     } else {
       if (focusedElement && focusedElement.ownerDocument == contentDoc &&
           focusedElement instanceof HTMLElement) {
         focusCount++;
       }
       document.commandDispatcher.advanceFocus();
     }
--- a/browser/base/content/test/newtab/head.js
+++ b/browser/base/content/test/newtab/head.js
@@ -582,17 +582,18 @@ function createExternalDropIframe() {
   let doc = getContentDocument();
   let iframe = doc.createElement("iframe");
   iframe.setAttribute("src", url);
   iframe.style.width = "50px";
   iframe.style.height = "50px";
   iframe.style.position = "absolute";
   iframe.style.zIndex = 50;
 
-  let margin = doc.getElementById("newtab-margin-top");
+  // the frame has to be attached to a visible element
+  let margin = doc.getElementById("newtab-search-container");
   margin.appendChild(iframe);
 
   iframe.addEventListener("load", function onLoad() {
     iframe.removeEventListener("load", onLoad);
     executeSoon(() => deferred.resolve(iframe));
   });
 
   return deferred.promise;
@@ -779,8 +780,16 @@ function customizeNewTabPage(aTheme) {
 
     let closed = panelOpened(false);
     customizeButton.click();
     yield closed;
   });
 
   promise.then(TestRunner.next);
 }
+
+/**
+ * Reports presence of a scrollbar
+ */
+function hasScrollbar() {
+  let docElement = getContentDocument().documentElement;
+  return docElement.scrollHeight > docElement.clientHeight;
+}