WebUI NTP: fix background image selection issues

The background iframe was not loading correctly on initial load and
after an image was selected following the customize dialog closure. To
fix this, only set the path of the background image iframe if the path
is not empty and is different. Also do not reset the background
selection unless the theme changes.

The logo was not updating when the customize dialog background selection
changed.

Bug: 1032328
Change-Id: I99304d80e7149439f57c02c9192af98af5deb4de
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2145554
Commit-Queue: Esmael Elmoslimany <aee@chromium.org>
Reviewed-by: Tibor Goldschwendt <tiborg@chromium.org>
Cr-Commit-Position: refs/heads/master@{#758243}

chrome/browser/resources/new_tab_page/app.html

@@ -162,9 +162,8 @@
         --ntp-theme-text-color: [[rgbOrInherit_(theme_.shortcutTextColor)]];
         --ntp-theme-shortcut-background-color:
               [[rgbOrInherit_(theme_.shortcutBackgroundColor)]];
-        --ntp-logo-color: [[rgbOrInherit_(theme_.logoColor)]];">
-  <ntp-untrusted-iframe id="backgroundImage" path="[[backgroundImagePath_]]"
-      hidden="[[!showBackgroundImage_]]">
+        --ntp-logo-color: [[rgbOrInherit_(logoColor_)]];">
+  <ntp-untrusted-iframe id="backgroundImage" hidden="[[!showBackgroundImage_]]">
   </ntp-untrusted-iframe>
   <div id="backgroundGradient" hidden="[[!showBackgroundImage_]]"></div>
   <div id="content">

chrome/browser/resources/new_tab_page/app.js

@@ -18,7 +18,7 @@ import {html, PolymerElement} from 'chrome://resources/polymer/v3_0/polymer/poly
 
 import {BrowserProxy} from './browser_proxy.js';
 import {BackgroundSelection, BackgroundSelectionType} from './customize_dialog.js';
-import {skColorToRgb} from './utils.js';
+import {hexColorToSkColor, skColorToRgb} from './utils.js';
 
 class AppElement extends PolymerElement {
   static get is() {
@@ -44,7 +44,10 @@ class AppElement extends PolymerElement {
       },
 
       /** @private {!newTabPage.mojom.Theme} */
-      theme_: Object,
+      theme_: {
+        type: Object,
+        observer: 'updateBackgroundImagePath_',
+      },
 
       /** @private */
       showCustomizeDialog_: Boolean,
@@ -63,6 +66,7 @@ class AppElement extends PolymerElement {
       backgroundSelection_: {
         type: Object,
         value: () => ({type: BackgroundSelectionType.NO_SELECTION}),
+        observer: 'updateBackgroundImagePath_',
       },
 
       /** @private */
@@ -86,21 +90,21 @@ class AppElement extends PolymerElement {
             backgroundSelection_)`,
       },
 
-      /** @private */
-      backgroundImagePath_: {
-        computed: 'computeBackgroundImagePath_(theme_, backgroundSelection_)',
-        type: String,
-      },
-
       /** @private */
       doodleAllowed_: {
         computed: 'computeDoodleAllowed_(showBackgroundImage_, theme_)',
         type: Boolean,
       },
 
+      /** @private */
+      logoColor_: {
+        type: String,
+        computed: 'computeLogoColor_(theme_, backgroundSelection_)',
+      },
+
       /** @private */
       singleColoredLogo_: {
-        computed: 'computeSingleColoredLogo_(theme_)',
+        computed: 'computeSingleColoredLogo_(theme_, backgroundSelection_)',
         type: Boolean,
       },
     };
@@ -247,22 +251,43 @@ class AppElement extends PolymerElement {
   }
 
   /**
-   * @return {string}
+   * Set the #backgroundImage |path| only when different and non-empty. Reset
+   * the customize dialog background selection if the dialog is closed.
+   *
+   * The ntp-untrusted-iframe |path| is set directly. When using a data binding
+   * instead, the quick updates to the |path| result in iframe loading an error
+   * page.
    * @private
    */
-  computeBackgroundImagePath_() {
+  updateBackgroundImagePath_() {
+    // The |backgroundSelection_| is retained after the dialog commits the
+    // change to the theme. Since |backgroundSelection_| has precendence over
+    // the theme background, the |backgroundSelection_| needs to be reset when
+    // the theme is updated. This is only necessary when the dialog is closed.
+    // If the dialog is open, it will either commit the |backgroundSelection_|
+    // or reset |backgroundSelection_| on cancel.
+    if (!this.showCustomizeDialog_ &&
+        this.backgroundSelection_.type !==
+            BackgroundSelectionType.NO_SELECTION) {
+      this.backgroundSelection_ = {type: BackgroundSelectionType.NO_SELECTION};
+    }
+    let path;
     switch (this.backgroundSelection_.type) {
       case BackgroundSelectionType.NO_SELECTION:
-        return this.theme_ && this.theme_.backgroundImageUrl ?
-            `background_image?${this.theme_.backgroundImageUrl.url}` :
-            '';
+        path = this.theme_ && this.theme_.backgroundImageUrl &&
+            `background_image?${this.theme_.backgroundImageUrl.url}`;
+        break;
       case BackgroundSelectionType.IMAGE:
-        return `background_image?${
-            this.backgroundSelection_.image.imageUrl.url}`;
+        path =
+            `background_image?${this.backgroundSelection_.image.imageUrl.url}`;
+        break;
       case BackgroundSelectionType.NO_BACKGROUND:
       case BackgroundSelectionType.DAILY_REFRESH:
       default:
-        return '';
+        path = '';
+    }
+    if (path && this.$.backgroundImage.path !== path) {
+      this.$.backgroundImage.path = path;
     }
   }
 
@@ -276,12 +301,38 @@ class AppElement extends PolymerElement {
          this.theme_.type === newTabPage.mojom.ThemeType.DEFAULT);
   }
 
+  /**
+   * @return {skia.mojom.SkColor}
+   * @private
+   */
+  computeLogoColor_() {
+    switch (this.backgroundSelection_.type) {
+      case BackgroundSelectionType.NO_SELECTION:
+        return this.theme_ && this.theme_.logoColor || null;
+      case BackgroundSelectionType.IMAGE:
+        return hexColorToSkColor('#ffffff');
+      case BackgroundSelectionType.NO_BACKGROUND:
+      case BackgroundSelectionType.DAILY_REFRESH:
+      default:
+        return null;
+    }
+  }
+
   /**
    * @return {boolean}
    * @private
    */
   computeSingleColoredLogo_() {
-    return this.theme_ && !!this.theme_.logoColor;
+    switch (this.backgroundSelection_.type) {
+      case BackgroundSelectionType.NO_SELECTION:
+        return this.theme_ && !!this.theme_.logoColor;
+      case BackgroundSelectionType.IMAGE:
+        return true;
+      case BackgroundSelectionType.NO_BACKGROUND:
+      case BackgroundSelectionType.DAILY_REFRESH:
+      default:
+        return false;
+    }
   }
 
   /**

chrome/browser/resources/new_tab_page/customize_dialog.js

@@ -61,7 +61,6 @@ class CustomizeDialogElement extends PolymerElement {
        */
       backgroundSelection: {
         type: Object,
-        value: () => ({type: BackgroundSelectionType.NO_SELECTION}),
         notify: true,
       },
 
@@ -101,12 +100,12 @@ class CustomizeDialogElement extends PolymerElement {
     this.pageHandler_ = BrowserProxy.getInstance().handler;
     /** @private {!Array<!IntersectionObserver>} */
     this.intersectionObservers_ = [];
+    this.backgroundSelection = {type: BackgroundSelectionType.NO_SELECTION};
   }
 
   /** @override */
   disconnectedCallback() {
     super.disconnectedCallback();
-    this.backgroundSelection = {type: BackgroundSelectionType.NO_SELECTION};
     this.intersectionObservers_.forEach(observer => {
       observer.disconnect();
     });
@@ -133,7 +132,16 @@ class CustomizeDialogElement extends PolymerElement {
     this.$.dialog.cancel();
   }
 
-  /** @private */
+  /**
+   * The |backgroundSelection| is used in ntp-app to preview the image and has
+   * precedence over the theme background setting. |backgroundSelection| is not
+   * reset because it takes time for the theme to update, and after the update
+   * the theme and |backgroundSelection| are the same. By not resetting the
+   * value here, ntp-app can reset it if needed (other theme update). This
+   * prevents a flicker between |backgroundSelection| and the previous theme
+   * background setting.
+   * @private
+   */
   onDoneClick_() {
     this.pageHandler_.confirmThemeChanges();
     this.shadowRoot.querySelector('ntp-customize-shortcuts').apply();
@@ -151,7 +159,6 @@ class CustomizeDialogElement extends PolymerElement {
         this.pageHandler_.setDailyRefreshCollectionId(
             assert(this.backgroundSelection.dailyRefreshCollectionId));
     }
-    this.backgroundSelection = {type: BackgroundSelectionType.NO_SELECTION};
     this.$.dialog.close();
   }
 

chrome/test/data/webui/new_tab_page/app_test.js

@@ -222,4 +222,38 @@ suite('NewTabPageAppTest', () => {
         'background_image?https://example.com/image.png',
         app.$.backgroundImage.path);
   });
+
+  test('theme update when dialog closed clears selection', async () => {
+    const theme = createTheme();
+    theme.backgroundImageUrl = {url: 'https://example.com/image.png'};
+    testProxy.callbackRouterRemote.setTheme(theme);
+    await testProxy.callbackRouterRemote.$.flushForTesting();
+    assertEquals(
+        'background_image?https://example.com/image.png',
+        app.$.backgroundImage.path);
+    app.$.customizeButton.click();
+    await flushTasks();
+    const dialog = app.shadowRoot.querySelector('ntp-customize-dialog');
+    dialog.backgroundSelection = {
+      type: BackgroundSelectionType.IMAGE,
+      image: {
+        attribution1: '1',
+        attribution2: '2',
+        attributionUrl: {url: 'https://example.com'},
+        imageUrl: {url: 'https://example.com/other.png'},
+      },
+    };
+    assertEquals(
+        'background_image?https://example.com/other.png',
+        app.$.backgroundImage.path);
+    dialog.dispatchEvent(new Event('close'));
+    assertEquals(
+        'background_image?https://example.com/other.png',
+        app.$.backgroundImage.path);
+    testProxy.callbackRouterRemote.setTheme(theme);
+    await testProxy.callbackRouterRemote.$.flushForTesting();
+    assertEquals(
+        'background_image?https://example.com/image.png',
+        app.$.backgroundImage.path);
+  });
 });

chrome/test/data/webui/new_tab_page/customize_dialog_test.js

@@ -145,6 +145,22 @@ suite('NewTabPageCustomizeDialogTest', () => {
       assertFalse(customizeDialog.$.refreshToggle.checked);
     });
 
+    test('clicking cancel', () => {
+      customizeDialog.backgroundSelection = {
+        type: BackgroundSelectionType.IMAGE,
+        image: {
+          attribution1: '1',
+          attribution2: '2',
+          attributionUrl: {url: 'https://example.com'},
+          imageUrl: {url: 'https://example.com/other.png'}
+        },
+      };
+      customizeDialog.shadowRoot.querySelector('.cancel-button').click();
+      assertDeepEquals(
+          {type: BackgroundSelectionType.NO_SELECTION},
+          customizeDialog.backgroundSelection);
+    });
+
     suite('clicking done', () => {
       function done() {
         customizeDialog.shadowRoot.querySelector('.action-button').click();
@@ -168,7 +184,15 @@ suite('NewTabPageCustomizeDialogTest', () => {
         assertEquals('https://example.com', attributionUrl.url);
         assertEquals('https://example.com/other.png', imageUrl.url);
         assertDeepEquals(
-            {type: BackgroundSelectionType.NO_SELECTION},
+            {
+              type: BackgroundSelectionType.IMAGE,
+              image: {
+                attribution1: '1',
+                attribution2: '2',
+                attributionUrl: {url: 'https://example.com'},
+                imageUrl: {url: 'https://example.com/other.png'}
+              },
+            },
             customizeDialog.backgroundSelection);
       });
 
@@ -180,7 +204,10 @@ suite('NewTabPageCustomizeDialogTest', () => {
             'abstract',
             await testProxy.handler.whenCalled('setDailyRefreshCollectionId'));
         assertDeepEquals(
-            {type: BackgroundSelectionType.NO_SELECTION},
+            {
+              type: BackgroundSelectionType.DAILY_REFRESH,
+              dailyRefreshCollectionId: 'abstract',
+            },
             customizeDialog.backgroundSelection);
       });
 
@@ -189,19 +216,16 @@ suite('NewTabPageCustomizeDialogTest', () => {
         customizeDialog.$.refreshToggle.click();
         done();
         await testProxy.handler.whenCalled('setNoBackgroundImage');
-        assertDeepEquals(
-            {type: BackgroundSelectionType.NO_SELECTION},
-            customizeDialog.backgroundSelection);
       });
 
       test('set no background', async () => {
         customizeDialog.backgroundSelection = {
-          type: BackgroundSelectionType.NO_BACKGROUND
+          type: BackgroundSelectionType.NO_BACKGROUND,
         };
         done();
         await testProxy.handler.whenCalled('setNoBackgroundImage');
         assertDeepEquals(
-            {type: BackgroundSelectionType.NO_SELECTION},
+            {type: BackgroundSelectionType.NO_BACKGROUND},
             customizeDialog.backgroundSelection);
       });
     });