Bug 1546123 - Correct size calculation of theme previews r=mstriemer
☠☠ backed out by 2f3479842f91 ☠ ☠
authorRob Wu <rob@robwu.nl>
Mon, 06 May 2019 10:41:36 +0000
changeset 531523 c372247f4432fe0244a26e91d2bc28429ae67ab6
parent 531522 56f4b00581076b591e83e69dbb20735f15b5a2ee
child 531524 b3f31aa87d0d11690a169368e39c8f07edf4ea88
push id11265
push userffxbld-merge
push dateMon, 13 May 2019 10:53:39 +0000
treeherdermozilla-beta@77e0fe8dbdd3 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersmstriemer
bugs1546123
milestone68.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1546123 - Correct size calculation of theme previews r=mstriemer Differential Revision: https://phabricator.services.mozilla.com/D29482
toolkit/mozapps/extensions/content/aboutaddons.css
toolkit/mozapps/extensions/content/aboutaddons.js
toolkit/mozapps/extensions/test/browser/browser_html_detail_view.js
--- a/toolkit/mozapps/extensions/content/aboutaddons.css
+++ b/toolkit/mozapps/extensions/content/aboutaddons.css
@@ -37,20 +37,22 @@
 }
 
 addon-list .addon.card {
   -moz-user-select: none;
 }
 
 /* Theme preview image. */
 .card-heading-image {
+  /* If the width, height or aspect ratio changes, don't forget to update the
+   * getScreenshotUrlForAddon function in aboutaddons.js */
   width: var(--section-width);
-  /* This is a magic number for the aspect ratio we get from AMO. */
-  height: 89px;
-  object-fit: cover;
+  /* Adjust height so that the image preserves the aspect ratio from AMO.
+   * For details, see https://bugzilla.mozilla.org/show_bug.cgi?id=1546123 */
+  height: calc(var(--section-width) * 92 / 680);
 }
 
 .card-heading-icon {
   flex-shrink: 0;
   width: var(--addon-icon-size);
   height: var(--addon-icon-size);
   margin-inline-end: 16px;
   -moz-context-properties: fill;
--- a/toolkit/mozapps/extensions/content/aboutaddons.js
+++ b/toolkit/mozapps/extensions/content/aboutaddons.js
@@ -149,16 +149,40 @@ function nl2br(text) {
     }
     frag.appendChild(new Text(part));
     hasAppended = true;
   }
   return frag;
 }
 
 /**
+ * Select the screeenshot to display above an add-on card.
+ *
+ * @param {AddonWrapper|DiscoAddonWrapper} addon
+ * @returns {string|null}
+ *          The URL of the best fitting screenshot, if any.
+ */
+function getScreenshotUrlForAddon(addon) {
+  let {screenshots} = addon;
+  if (!screenshots || !screenshots.length) {
+    return null;
+  }
+
+  // The image size is defined at .card-heading-image in aboutaddons.css, and
+  // is based on the aspect ratio for a 680x92 image. Use the image if possible,
+  // and otherwise fall back to the first image and hope for the best.
+  let screenshot = screenshots.find(s => s.width === 680 && s.height === 92);
+  if (!screenshot) {
+    console.warn(`Did not find screenshot with desired size for ${addon.id}.`);
+    screenshot = screenshots[0];
+  }
+  return screenshot.url;
+}
+
+/**
  * Adds UTM parameters to a given URL, if it is an AMO URL.
  *
  * @param {string} contentAttribute
  *        Identifies the part of the UI with which the link is associated.
  * @param {string} url
  * @returns {string}
  *          The url with UTM parameters if it is an AMO URL.
  *          Otherwise the url in unmodified form.
@@ -801,48 +825,16 @@ class AddonCard extends HTMLElement {
     if (install && install.state == AddonManager.STATE_AVAILABLE) {
       this.updateInstall = install;
     }
     if (this.children.length > 0) {
       this.render();
     }
   }
 
-  /**
-   * Determine which screenshot fits best into the given img element. The img
-   * should have a width and height set on it.
-   *
-   * @param {HTMLImageElement} img The <img> the screenshot is being set on.
-   */
-  screenshotForImg(img) {
-    let {addon} = this;
-    if (addon.screenshots && addon.screenshots[0]) {
-      let {width, height} = getComputedStyle(img);
-      let sectionWidth = parseInt(width, 10);
-      let sectionHeight = parseInt(height, 10);
-      let screenshots = addon.screenshots
-        // Only check screenshots with a width and height.
-        .filter(s => s.width && s.height)
-        // Sort the screenshots based how close their dimensions are to the
-        // requested size.
-        .sort((a, b) => {
-          let aCloseness =
-            Math.abs((a.width - sectionWidth) * (a.height - sectionHeight));
-          let bCloseness =
-            Math.abs((b.width - sectionWidth) * (b.height - sectionHeight));
-          if (aCloseness == bCloseness) {
-            return 0;
-          }
-          return aCloseness < bCloseness ? -1 : 1;
-        });
-      return screenshots[0];
-    }
-    return null;
-  }
-
   async handleEvent(e) {
     let {addon} = this;
     let action = e.target.getAttribute("action");
 
     if (e.type == "click") {
       switch (action) {
         case "toggle-disabled":
           if (addon.userDisabled) {
@@ -1022,19 +1014,19 @@ class AddonCard extends HTMLElement {
       icon = AddonManager.getPreferredIconURL(addon, 32, window);
     }
     card.querySelector(".addon-icon").src = icon;
 
     // Update the theme preview.
     let preview = card.querySelector(".card-heading-image");
     preview.hidden = true;
     if (addon.type == "theme") {
-      let screenshot = this.screenshotForImg(preview);
-      if (screenshot) {
-        preview.src = screenshot.url;
+      let screenshotUrl = getScreenshotUrlForAddon(addon);
+      if (screenshotUrl) {
+        preview.src = screenshotUrl;
         preview.hidden = false;
       }
     }
 
     // Update the name.
     let name = card.querySelector(".addon-name");
     if (addon.isActive) {
       name.textContent = addon.name;
@@ -1186,20 +1178,19 @@ class RecommendedAddonCard extends HTMLE
       card.querySelector(".addon-icon").src =
         AddonManager.getPreferredIconURL(addon, 32, window);
     }
 
     // Set the theme preview.
     let preview = card.querySelector(".card-heading-image");
     preview.hidden = true;
     if (addon.type == "theme") {
-      let screenshot =
-        AddonCard.prototype.screenshotForImg.call({addon}, preview);
-      if (screenshot) {
-        preview.src = screenshot.url;
+      let screenshotUrl = getScreenshotUrlForAddon(addon);
+      if (screenshotUrl) {
+        preview.src = screenshotUrl;
         preview.hidden = false;
       }
     }
 
     // Set the name.
     card.querySelector(".disco-addon-name").textContent = addon.name;
 
     // Set the author name and link to AMO.
--- a/toolkit/mozapps/extensions/test/browser/browser_html_detail_view.js
+++ b/toolkit/mozapps/extensions/test/browser/browser_html_detail_view.js
@@ -450,32 +450,32 @@ add_task(async function testStaticTheme(
   let card = getAddonCard(doc, "theme1@mochi.test");
   ok(!card.hasAttribute("expanded"), "The list card is not expanded");
 
   // Make sure the preview is set.
   let preview = card.querySelector(".card-heading-image");
   ok(preview, "There is a preview");
   is(preview.src, "http://example.com/preview.png", "The preview URL is set");
   is(preview.width, "664", "The width is set");
-  is(preview.height, "89", "The height is set");
+  is(preview.height, "90", "The height is set");
   is(preview.hidden, false, "The preview is visible");
 
   // Load the detail view.
   let loaded = waitForViewLoad(win);
   card.querySelector('[action="expand"]').click();
   await loaded;
 
   card = getAddonCard(doc, "theme1@mochi.test");
 
   // Make sure the preview is still set.
   preview = card.querySelector(".card-heading-image");
   ok(preview, "There is a preview");
   is(preview.src, "http://example.com/preview.png", "The preview URL is set");
   is(preview.width, "664", "The width is set");
-  is(preview.height, "89", "The height is set");
+  is(preview.height, "90", "The height is set");
   is(preview.hidden, false, "The preview is visible");
 
   let rows = Array.from(card.querySelectorAll(".addon-detail-row"));
 
   // Automatic updates.
   let row = rows.shift();
   checkLabel(row, "updates");