diff options
author | huangs@chromium.org <huangs@chromium.org@0039d316-1c4b-4281-b951-d872f2087c98> | 2014-08-01 23:46:56 +0000 |
---|---|---|
committer | huangs@chromium.org <huangs@chromium.org@0039d316-1c4b-4281-b951-d872f2087c98> | 2014-08-01 23:46:56 +0000 |
commit | 80cfc6cf9a7006ea1e7ae4a2cc99799bc0d85daf (patch) | |
tree | 705384fa19079c75690c07a26a4527d3980acb2e | |
parent | a579f97a55d027dd38187bc9a801e528e91a6500 (diff) | |
download | chromium_src-80cfc6cf9a7006ea1e7ae4a2cc99799bc0d85daf.zip chromium_src-80cfc6cf9a7006ea1e7ae4a2cc99799bc0d85daf.tar.gz chromium_src-80cfc6cf9a7006ea1e7ae4a2cc99799bc0d85daf.tar.bz2 |
Local NTP: prevent tiles from reloading on resize by moving them into single <div>.
Tiles in chrome-search://local-ntp/local-ntp.html used to be arranged in
2 rows, and get reshuffled when screen resize causes number of columns
to change. However, detaching and reattaching elements with <iframe>s
cause the <iframe>s to reload, resulting in flickering.
This CL places all tiles in a single <div>, so no reshuffling is needed.
Instead, if number of columns change we resize the container width (and
also hide tiles beyond row 2), and let HTML layout place handle the
proper wrapping.
On blacklisting, we cannot compare identities between old tiles and new,
so a reload is necessary, which leads to some flickering.
Also tested for <body dir="RTL">
Additional cleanups:
- To show all tiles only after everything is loaded: using a Barrier
counter instead of looping on every load, which was O(n^2).
- Broke apart onMostVisitedChange() and added more comments.
- Removed unused CSS for fakebox and tiles resizing (JS does resizing now).
- Using CSS visibility instead of hidden to show #mv-tiles, so it take
up space and prevent content beneath tiles from jumping up briefly.
- Refactoring the logic to hide tiles during load and show all at once when
everything loads (or timeout occurs). Using new class Barrier to do this.
BUG=399388
Review URL: https://codereview.chromium.org/412073002
git-svn-id: svn://svn.chromium.org/chrome/trunk/src@287120 0039d316-1c4b-4281-b951-d872f2087c98
-rw-r--r-- | chrome/browser/browser_resources.grd | 1 | ||||
-rw-r--r-- | chrome/browser/resources/local_ntp/local_ntp.css | 47 | ||||
-rw-r--r-- | chrome/browser/resources/local_ntp/local_ntp.js | 268 | ||||
-rw-r--r-- | chrome/browser/resources/local_ntp/local_ntp_util.js | 54 | ||||
-rw-r--r-- | chrome/browser/search/local_ntp_source.cc | 1 |
5 files changed, 202 insertions, 169 deletions
diff --git a/chrome/browser/browser_resources.grd b/chrome/browser/browser_resources.grd index b79ec4d..2a94210 100644 --- a/chrome/browser/browser_resources.grd +++ b/chrome/browser/browser_resources.grd @@ -205,6 +205,7 @@ <include name="IDR_LOCAL_NTP_HTML" file="resources\local_ntp\local_ntp.html" flattenhtml="true" allowexternalscript="true" type="BINDATA" /> <include name="IDR_LOCAL_NTP_CSS" file="resources\local_ntp\local_ntp.css" type="BINDATA" /> <include name="IDR_LOCAL_NTP_JS" file="resources\local_ntp\local_ntp.js" flattenhtml="true" type="BINDATA" /> + <include name="IDR_LOCAL_NTP_UTIL_JS" file="resources\local_ntp\local_ntp_util.js" flattenhtml="true" type="BINDATA" /> <include name="IDR_MOST_VISITED_IFRAME_CSS" file="resources\local_ntp\most_visited_iframe.css" type="BINDATA" /> <include name="IDR_MOST_VISITED_TITLE_HTML" file="resources\local_ntp\most_visited_title.html" type="BINDATA" /> <include name="IDR_MOST_VISITED_TITLE_CSS" file="resources\local_ntp\most_visited_title.css" type="BINDATA" /> diff --git a/chrome/browser/resources/local_ntp/local_ntp.css b/chrome/browser/resources/local_ntp/local_ntp.css index cdc6ae4..3a46fad 100644 --- a/chrome/browser/resources/local_ntp/local_ntp.css +++ b/chrome/browser/resources/local_ntp/local_ntp.css @@ -137,48 +137,13 @@ body.fakebox-focused #cursor { } #mv-tiles { - /* Use GPU compositing if available. */ - -webkit-transform: translate3d(0, 0, 0); - height: 260px; - overflow: hidden; - padding: 0 1em; - white-space: nowrap; - width: 304px; -} - -@media only screen and (min-width:660px) { - #fakebox { - width: 458px; - } - #mv-tiles { - width: 460px; - } -} - -@media only screen and (min-width:820px) { - #fakebox { - width: 618px; - } - #mv-tiles { - width: 620px; - } -} - -.mv-row { - margin-bottom: 50px; -} - -.mv-row:last-child { - margin-bottom: 0; -} - -.mv-tile:first-child { - -webkit-margin-start: -1px; + height: calc(2 * 138px); + line-height: 138px; + margin: 0; + position: relative; } .mv-tile { - -webkit-margin-start: 20px; - -webkit-transform: translate3d(0, 0, 0); -webkit-transition-duration: 200ms; -webkit-transition-property: -webkit-transform, margin, opacity, width; background: -webkit-linear-gradient(#f2f2f2, #e8e8e8); @@ -187,6 +152,10 @@ body.fakebox-focused #cursor { box-shadow: inset 0 2px 3px rgba(0, 0, 0, .09); display: inline-block; height: 83px; + margin-left: 10px; + margin-right: 10px; /* Total horizontal margins add to TILE_MARGIN. */ + position: relative; + vertical-align: top; width: 138px; } diff --git a/chrome/browser/resources/local_ntp/local_ntp.js b/chrome/browser/resources/local_ntp/local_ntp.js index fb8b873..d775540 100644 --- a/chrome/browser/resources/local_ntp/local_ntp.js +++ b/chrome/browser/resources/local_ntp/local_ntp.js @@ -14,6 +14,7 @@ */ function LocalNTP() { <include src="../../../../ui/webui/resources/js/assert.js"> +<include src="local_ntp_util.js"> <include src="window_disposition_util.js"> @@ -39,7 +40,6 @@ var CLASSES = { NON_GOOGLE_PAGE: 'non-google-page', PAGE: 'mv-page', // page tiles PAGE_READY: 'mv-page-ready', // page tile when ready - ROW: 'mv-row', // tile row RTL: 'rtl', // Right-to-left language text. THUMBNAIL: 'mv-thumb', THUMBNAIL_MASK: 'mv-mask', @@ -169,8 +169,8 @@ var numColumnsShown = 0; /** - * True if the user initiated the current most visited change and false - * otherwise. + * A flag to indicate Most Visited changed caused by user action. If true, then + * in onMostVisitedChange() tiles remain visible so no flickering occurs. * @type {boolean} */ var userInitiatedMostVisitedChange = false; @@ -213,11 +213,11 @@ var TILE_WIDTH = 140; /** - * Margin between tiles. Should be equal to mv-tile's -webkit-margin-start. + * Margin between tiles. Should be equal to mv-tile's total horizontal margin. * @private {number} * @const */ -var TILE_MARGIN_START = 20; +var TILE_MARGIN = 20; /** @type {number} @const */ @@ -298,14 +298,22 @@ var MOST_VISITED_PAINT_TIMEOUT_MSEC = 500; * pad out the section when not enough pages exist. * * @param {Element} elem The element for rendering the tile. + * @param {Element=} opt_titleElem The element for rendering the title. + * @param {Element=} opt_thumbnailElem The element for rendering the thumbnail. * @param {number=} opt_rid The RID for the corresponding Most Visited page. * Should only be left unspecified when creating a filler tile. * @constructor */ -function Tile(elem, opt_rid) { +function Tile(elem, opt_titleElem, opt_thumbnailElem, opt_rid) { /** @type {Element} */ this.elem = elem; + /** @type {Element|undefined} */ + this.titleElem = opt_titleElem; + + /** @type {Element|undefined} */ + this.thumbnailElem = opt_thumbnailElem; + /** @type {number|undefined} */ this.rid = opt_rid; } @@ -329,7 +337,9 @@ function onThemeChange() { document.body.classList.toggle(CLASSES.ALTERNATE_LOGO, info.alternateLogo); updateThemeAttribution(info.attributionUrl); setCustomThemeStyle(info); - renderTiles(); + + tilesContainer.innerHTML = ''; + renderAndShowTiles(); } @@ -432,68 +442,120 @@ function convertToRGBAColor(color) { * Handles a new set of Most Visited page data. */ function onMostVisitedChange() { - var pages = ntpApiHandle.mostVisited; - if (isBlacklisting) { - // Trigger the blacklist animation and re-render the tiles when it - // completes. + // Trigger the blacklist animation, which then triggers reloadAllTiles(). var lastBlacklistedTileElement = lastBlacklistedTile.elem; lastBlacklistedTileElement.addEventListener( 'webkitTransitionEnd', blacklistAnimationDone); lastBlacklistedTileElement.classList.add(CLASSES.BLACKLIST); - } else { - // Otherwise render the tiles using the new data without animation. - tiles = []; - for (var i = 0; i < MAX_NUM_TILES_TO_SHOW; ++i) { - tiles.push(createTile(pages[i], i)); - } - if (!userInitiatedMostVisitedChange) { - tilesContainer.hidden = true; - window.setTimeout(function() { - if (tilesContainer) { - tilesContainer.hidden = false; - } - }, MOST_VISITED_PAINT_TIMEOUT_MSEC); - } - renderTiles(); + reloadAllTiles(); } } /** - * Renders the current set of tiles. + * Handles the end of the blacklist animation by showing the notification and + * re-rendering the new set of tiles. */ -function renderTiles() { - var rows = tilesContainer.children; - for (var i = 0; i < rows.length; ++i) { - removeChildren(rows[i]); +function blacklistAnimationDone() { + showNotification(); + isBlacklisting = false; + tilesContainer.classList.remove(CLASSES.HIDE_BLACKLIST_BUTTON); + lastBlacklistedTile.elem.removeEventListener( + 'webkitTransitionEnd', blacklistAnimationDone); + // Need to call explicitly to re-render the tiles, since the initial + // onmostvisitedchange issued by the blacklist function only triggered + // the animation. + reloadAllTiles(); +} + + +/** + * Fetches new data, creates, and renders tiles. + */ +function reloadAllTiles() { + var pages = ntpApiHandle.mostVisited; + + tiles = []; + for (var i = 0; i < MAX_NUM_TILES_TO_SHOW; ++i) + tiles.push(createTile(pages[i], i)); + + tilesContainer.innerHTML = ''; + renderAndShowTiles(); +} + + +/** + * Binds onload events for a tile's internal <iframe> elements. + * @param {Tile} tile The main tile to bind events to. + * @param {Barrier} tileVisibilityBarrier A barrier to make tile visible the + * moment all tiles are loaded. + */ +function bindTileOnloadEvents(tile, tileVisibilityBarrier) { + if (tile.titleElem) { + tileVisibilityBarrier.add(); + tile.titleElem.onload = function() { + tile.titleElem.hidden = false; + tileVisibilityBarrier.remove(); + }; } - for (var i = 0, length = tiles.length; - i < Math.min(length, numColumnsShown * NUM_ROWS); ++i) { - rows[Math.floor(i / numColumnsShown)].appendChild(tiles[i].elem); + if (tile.thumbnailElem) { + tileVisibilityBarrier.add(); + tile.thumbnailElem.onload = function() { + tile.thumbnailElem.hidden = false; + tile.elem.classList.add(CLASSES.PAGE_READY); + tileVisibilityBarrier.remove(); + }; } } /** - * Shows most visited tiles if all child iframes are loaded, and hides them - * otherwise. + * Renders the current list of visible tiles to DOM, and hides tiles that are + * already in the DOM but should not be seen. */ -function updateMostVisitedVisibility() { - var iframes = tilesContainer.querySelectorAll('iframe'); - var ready = true; - for (var i = 0, numIframes = iframes.length; i < numIframes; i++) { - if (iframes[i].hidden) { - ready = false; - break; +function renderAndShowTiles() { + var numExisting = tilesContainer.querySelectorAll('.' + CLASSES.TILE).length; + // Only add visible tiles to the DOM, to avoid creating invisible tiles that + // produce meaningless impression metrics. However, if a tile becomes + // invisible then we leave it in DOM to prevent reload if it's shown again. + var numDesired = Math.min(tiles.length, numColumnsShown * NUM_ROWS); + + // If we need to render new tiles, manage the visibility to hide intermediate + // load states of the <iframe>s. + if (numExisting < numDesired) { + var tileVisibilityBarrier = new Barrier(function() { + tilesContainer.style.visibility = 'visible'; + }); + + if (!userInitiatedMostVisitedChange) { + // Make titleContainer invisible, but still taking up space. + // titleContainer becomes visible again (1) on timeout, or (2) when all + // tiles finish loading (using tileVisibilityBarrier). + tilesContainer.style.visibility = 'hidden'; + window.setTimeout(function() { + tileVisibilityBarrier.cancel(); + tilesContainer.style.visibility = 'visible'; + }, MOST_VISITED_PAINT_TIMEOUT_MSEC); } - } - if (ready) { - tilesContainer.hidden = false; userInitiatedMostVisitedChange = false; + + for (var i = numExisting; i < numDesired; ++i) { + bindTileOnloadEvents(tiles[i], tileVisibilityBarrier); + tilesContainer.appendChild(tiles[i].elem); + } } + + // Show only the desired tiles. Not using .hidden because it does not work for + // inline-block elements. + for (var i = 0; i < numDesired; ++i) + tiles[i].elem.style.display = 'inline-block'; + // If |numDesired| < |numExisting| then hide extra tiles (e.g., this occurs + // when window is downsized). + for (; i < numExisting; ++i) + tiles[i].elem.style.display = 'none'; } @@ -563,36 +625,25 @@ function createTile(page, position) { // // TODO(jered): Find and fix the root (probably Blink) bug. + // Keep this ID here. See comment above. + titleElement.id = 'title-' + rid; + titleElement.className = CLASSES.TITLE; + titleElement.hidden = true; titleElement.src = getMostVisitedIframeUrl( MOST_VISITED_TITLE_IFRAME, rid, MOST_VISITED_COLOR, MOST_VISITED_FONT_FAMILY, MOST_VISITED_FONT_SIZE, position); - - // Keep this id here. See comment above. - titleElement.id = 'title-' + rid; - titleElement.hidden = true; - titleElement.onload = function() { - titleElement.hidden = false; - updateMostVisitedVisibility(); - }; - titleElement.className = CLASSES.TITLE; tileElement.appendChild(titleElement); // The iframe which renders either a thumbnail or domain element. var thumbnailElement = document.createElement('iframe'); thumbnailElement.tabIndex = '-1'; + // Keep this ID here. See comment above. + thumbnailElement.id = 'thumb-' + rid; + thumbnailElement.className = CLASSES.THUMBNAIL; + thumbnailElement.hidden = true; thumbnailElement.src = getMostVisitedIframeUrl( MOST_VISITED_THUMBNAIL_IFRAME, rid, MOST_VISITED_COLOR, MOST_VISITED_FONT_FAMILY, MOST_VISITED_FONT_SIZE, position); - - // Keep this id here. See comment above. - thumbnailElement.id = 'thumb-' + rid; - thumbnailElement.hidden = true; - thumbnailElement.onload = function() { - thumbnailElement.hidden = false; - tileElement.classList.add(CLASSES.PAGE_READY); - updateMostVisitedVisibility(); - }; - thumbnailElement.className = CLASSES.THUMBNAIL; tileElement.appendChild(thumbnailElement); // A mask to darken the thumbnail on focus. @@ -612,11 +663,10 @@ function createTile(page, position) { // The page favicon, if any. var faviconUrl = page.faviconUrl; if (faviconUrl) { - var favicon = createAndAppendElement( - tileElement, 'div', CLASSES.FAVICON); + var favicon = createAndAppendElement(tileElement, 'div', CLASSES.FAVICON); favicon.style.backgroundImage = 'url(' + faviconUrl + ')'; } - return new Tile(tileElement, rid); + return new Tile(tileElement, titleElement, thumbnailElement, rid); } else { return new Tile(tileElement); } @@ -664,23 +714,6 @@ function hideNotification() { /** - * Handles the end of the blacklist animation by showing the notification and - * re-rendering the new set of tiles. - */ -function blacklistAnimationDone() { - showNotification(); - isBlacklisting = false; - tilesContainer.classList.remove(CLASSES.HIDE_BLACKLIST_BUTTON); - lastBlacklistedTile.elem.removeEventListener( - 'webkitTransitionEnd', blacklistAnimationDone); - // Need to call explicitly to re-render the tiles, since the initial - // onmostvisitedchange issued by the blacklist function only triggered - // the animation. - onMostVisitedChange(); -} - - -/** * Handles a click on the notification undo link by hiding the notification and * informing Chrome. */ @@ -705,39 +738,30 @@ function onRestoreAll() { /** - * Re-renders the tiles if the number of columns has changed. As a temporary - * fix for crbug/240510, updates the width of the fakebox and most visited tiles - * container. + * Resizes elements because the number of tile columns may need to change in + * response to resizing. Also shows or hides extra tiles tiles according to the + * new width of the page. */ function onResize() { // If innerWidth is zero, then use the maximum snap size. var innerWidth = window.innerWidth || 820; - - // These values should remain in sync with local_ntp.css. - // TODO(jeremycho): Delete once the root cause of crbug/240510 is resolved. - var setWidths = function(tilesContainerWidth) { + // Each tile has left and right margins that sum to TILE_MARGIN. + var tileRequiredWidth = TILE_WIDTH + TILE_MARGIN; + var availableWidth = innerWidth + TILE_MARGIN - MIN_TOTAL_HORIZONTAL_PADDING; + var newNumColumns = Math.floor(availableWidth / tileRequiredWidth); + if (newNumColumns < MIN_NUM_COLUMNS) + newNumColumns = MIN_NUM_COLUMNS; + else if (newNumColumns > MAX_NUM_COLUMNS) + newNumColumns = MAX_NUM_COLUMNS; + + if (numColumnsShown != newNumColumns) { + numColumnsShown = newNumColumns; + var tilesContainerWidth = numColumnsShown * tileRequiredWidth; tilesContainer.style.width = tilesContainerWidth + 'px'; - if (fakebox) - fakebox.style.width = (tilesContainerWidth - 2) + 'px'; - }; - if (innerWidth >= 820) - setWidths(620); - else if (innerWidth >= 660) - setWidths(460); - else - setWidths(300); - - var tileRequiredWidth = TILE_WIDTH + TILE_MARGIN_START; - // Adds margin-start to the available width to compensate the extra margin - // counted above for the first tile (which does not have a margin-start). - var availableWidth = innerWidth + TILE_MARGIN_START - - MIN_TOTAL_HORIZONTAL_PADDING; - var numColumnsToShow = Math.floor(availableWidth / tileRequiredWidth); - numColumnsToShow = Math.max(MIN_NUM_COLUMNS, - Math.min(MAX_NUM_COLUMNS, numColumnsToShow)); - if (numColumnsToShow != numColumnsShown) { - numColumnsShown = numColumnsToShow; - renderTiles(); + if (fakebox) // -2 to account for border. + fakebox.style.width = (tilesContainerWidth - TILE_MARGIN - 2) + 'px'; + // Render without clearing tiles. + renderAndShowTiles(); } } @@ -880,15 +904,6 @@ function removeNode(node) { /** - * Removes all the child nodes on a DOM node. - * @param {Node} node Node to remove children from. - */ -function removeChildren(node) { - node.innerHTML = ''; -} - - -/** * @param {!Element} element The element to register the handler for. * @param {number} keycode The keycode of the key to register. * @param {!Function} handler The key handler to register. @@ -924,12 +939,6 @@ function init() { attribution = $(IDS.ATTRIBUTION); ntpContents = $(IDS.NTP_CONTENTS); - for (var i = 0; i < NUM_ROWS; i++) { - var row = document.createElement('div'); - row.classList.add(CLASSES.ROW); - tilesContainer.appendChild(row); - } - if (configData.isGooglePage) { var logo = document.createElement('div'); logo.id = IDS.LOGO; @@ -939,7 +948,7 @@ function init() { fakebox.innerHTML = '<input id="' + IDS.FAKEBOX_INPUT + '" autocomplete="off" tabindex="-1" aria-hidden="true">' + - '<div id=cursor></div>'; + '<div id="cursor"></div>'; ntpContents.insertBefore(fakebox, ntpContents.firstChild); ntpContents.insertBefore(logo, ntpContents.firstChild); @@ -965,7 +974,6 @@ function init() { var notificationCloseButton = $(IDS.NOTIFICATION_CLOSE_BUTTON); notificationCloseButton.addEventListener('click', hideNotification); - userInitiatedMostVisitedChange = false; window.addEventListener('resize', onResize); onResize(); diff --git a/chrome/browser/resources/local_ntp/local_ntp_util.js b/chrome/browser/resources/local_ntp/local_ntp_util.js new file mode 100644 index 0000000..6b05579 --- /dev/null +++ b/chrome/browser/resources/local_ntp/local_ntp_util.js @@ -0,0 +1,54 @@ +// Copyright 2014 The Chromium Authors. All rights reserved. +// Use of this source code is governed by a BSD-style license that can be +// found in the LICENSE file. + + +/** + * @fileoverview Utilities for local_ntp.js. + */ + + +/** + * A counter with a callback that gets executed on the 1-to-0 transition. + * + * @param {function()} callback The callback to be executed. + * @constructor + */ +function Barrier(callback) { + /** @private {function()} */ + this.callback_ = callback; + + /** @private {number} */ + this.count_ = 0; + + /** @private {boolean} */ + this.isCancelled_ = false; +} + + +/** + * Increments count of the Barrier. + */ +Barrier.prototype.add = function() { + ++this.count_; +}; + + +/** + * Decrements count of the Barrier, and executes callback on 1-to-0 transition. + */ +Barrier.prototype.remove = function() { + if (this.count_ === 0) // Guards against underflow. + return; + --this.count_; + if (!this.isCancelled_ && this.count_ === 0) + this.callback_(); +}; + + +/** + * Deactivates the Barrier; the callback will never be executed. + */ +Barrier.prototype.cancel = function() { + this.isCancelled_ = true; +}; diff --git a/chrome/browser/search/local_ntp_source.cc b/chrome/browser/search/local_ntp_source.cc index bcda9f4..c310ec9 100644 --- a/chrome/browser/search/local_ntp_source.cc +++ b/chrome/browser/search/local_ntp_source.cc @@ -42,6 +42,7 @@ const struct Resource{ } kResources[] = { { "local-ntp.html", IDR_LOCAL_NTP_HTML, "text/html" }, { "local-ntp.js", IDR_LOCAL_NTP_JS, "application/javascript" }, + { "local-ntp-util.js", IDR_LOCAL_NTP_UTIL_JS, "application/javascript" }, { kConfigDataFilename, kLocalResource, "application/javascript" }, { "local-ntp.css", IDR_LOCAL_NTP_CSS, "text/css" }, { "images/close_2.png", IDR_CLOSE_2, "image/png" }, |