Bug 1546123 - Correct size calculation of theme previews r=mstriemer
authorRob Wu <rob@robwu.nl>
Wed, 08 May 2019 23:36:31 +0000
changeset 532041 134e12b67bba8cb1921aff84ddab311c4305f8a1
parent 532040 043d273f14ca4ff406c4d6674701ecfa51ecbd9d
child 532042 912b44cef0bcf2ddf29778ee2f5d8586933095af
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
@@ -52,20 +52,22 @@ addon-list message-bar-stack.pending-uni
 }
 
 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
@@ -158,16 +158,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.
@@ -844,48 +868,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) {
@@ -1076,19 +1068,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;
@@ -1240,20 +1232,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
@@ -472,32 +472,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");