WebUI Settings: fixed automatic scroll bug via noAutomaticCollapse flag

This CL fixes the bug in the security_page.html of the chrome privacy
settings with the automatic option collapsing while the confirmation
dialog pops up. An additional flag is added to the
collapse_radio_button.js which disables automatic collapsing.

Bug: 1096968
Change-Id: I0dcc56d9ab6f2649ad6e707edbbb631803685c05
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2294802Reviewed-by: Theodore Olsauskas-Warren <sauski@google.com>
Reviewed-by: Sean Harrison <harrisonsean@chromium.org>
Commit-Queue: Tymofii Chudakov <tchudakov@chromium.org>
Cr-Commit-Position: refs/heads/master@{#792191}

chrome/browser/resources/settings/privacy_page/collapse_radio_button.js

@@ -26,6 +26,11 @@ Polymer({
       value: false,
     },
 
+    noAutomaticCollapse: {
+      type: Boolean,
+      value: false,
+    },
+
     noCollapse: Boolean,
 
     label: String,
@@ -53,9 +58,30 @@ Polymer({
     'onPrefChanged_(pref.*)',
   ],
 
+  /**
+   * Tracks if this button was clicked but wasn't expanded.
+   * @private
+   */
+  pendingUpdateCollapsed_: false,
+
+  /**
+   * Updates the collapsed status of this radio button to reflect
+   * the user selection actions.
+   * @public
+   */
+  updateCollapsed() {
+    if (this.pendingUpdateCollapsed_) {
+      this.pendingUpdateCollapsed_ = false;
+      this.expanded = this.checked;
+    }
+  },
+
   /** @private */
   onCheckedChanged_() {
-    this.expanded = this.checked;
+    this.pendingUpdateCollapsed_ = true;
+    if (!this.noAutomaticCollapse) {
+      this.updateCollapsed();
+    }
   },
 
   /** @private */

chrome/browser/resources/settings/privacy_page/security_page.html

@@ -70,7 +70,8 @@
             pref="[[prefs.generated.safe_browsing]]"
             label="$i18n{safeBrowsingEnhanced}"
             sub-label="$i18n{safeBrowsingEnhancedDesc}"
-            hidden="[[!safeBrowsingEnhancedEnabled_]]">
+            hidden="[[!safeBrowsingEnhancedEnabled_]]"
+            no-automatic-collapse>
           <div slot="collapse">
             <div class="bullet-line">
               <iron-icon icon="cr:security"></iron-icon>
@@ -109,7 +110,8 @@
             pref="[[prefs.generated.safe_browsing]]"
             label="$i18n{safeBrowsingStandard}"
             sub-label="$i18n{safeBrowsingStandardDesc}"
-            info-opened="{{infoOpened_}}">
+            info-opened="{{infoOpened_}}"
+            no-automatic-collapse>
           <div slot="collapse">
             <div class="bullet-line">
               <iron-icon icon="cr:security"></iron-icon>
@@ -146,7 +148,7 @@
             label="$i18n{safeBrowsingNone}"
             sub-label="$i18n{safeBrowsingNoneDesc}">
         </settings-collapse-radio-button>
-      </cr-radio-group>
+      </settings-radio-group>
     </div>
     <div class="cr-row first">
       <h2>$i18n{advancedPageTitle}</h2>

chrome/browser/resources/settings/privacy_page/security_page.js

@@ -140,11 +140,29 @@ Polymer({
 
   /** @override */
   ready() {
+    // Expand initial pref value manually because automatic
+    // expanding is disabled.
+    const prefValue = this.getPref('generated.safe_browsing').value;
+    if (prefValue === SafeBrowsingSetting.ENHANCED) {
+      this.$.safeBrowsingEnhanced.expanded = true;
+    } else if (prefValue === SafeBrowsingSetting.STANDARD) {
+      this.$.safeBrowsingStandard.expanded = true;
+    }
     this.browserProxy_ = PrivacyPageBrowserProxyImpl.getInstance();
 
     this.metricsBrowserProxy_ = MetricsBrowserProxyImpl.getInstance();
   },
 
+  /**
+   * Updates the buttons' expanded status by propagating previous click
+   * events
+   * @private
+   */
+  updateCollapsedButtons_() {
+    this.$.safeBrowsingEnhanced.updateCollapsed();
+    this.$.safeBrowsingStandard.updateCollapsed();
+  },
+
   /**
    * Possibly displays the Safe Browsing disable dialog based on the users
    * selection.
@@ -156,6 +174,7 @@ Polymer({
     if (selected === SafeBrowsingSetting.DISABLED) {
       this.showDisableSafebrowsingDialog_ = true;
     } else {
+      this.updateCollapsedButtons_();
       this.$.safeBrowsingRadioGroup.sendPrefChange();
     }
   },
@@ -208,6 +227,7 @@ Polymer({
     if (/** @type {!SettingsDisableSafebrowsingDialogElement} */
         (this.$$('settings-disable-safebrowsing-dialog')).wasConfirmed()) {
       this.$.safeBrowsingRadioGroup.sendPrefChange();
+      this.updateCollapsedButtons_();
     } else {
       this.$.safeBrowsingRadioGroup.resetToPrefValue();
     }

chrome/test/data/webui/settings/collapse_radio_button_tests.js

@@ -42,6 +42,40 @@ suite('CrCollapseRadioButton', function() {
     assertFalse(collapse.opened);
   });
 
+  // Button should remain closed when noAutomaticCollapse flag is set.
+  test('closedWhenInitiallyClosedAndNoAutomaticCollapse', function() {
+    const collapse = collapseRadioButton.$$('iron-collapse');
+    collapseRadioButton.checked = false;
+    flush();
+    assertFalse(collapse.opened);
+
+    collapseRadioButton.noAutomaticCollapse = true;
+    collapseRadioButton.checked = true;
+    flush();
+    assertFalse(collapse.opened);
+
+    collapseRadioButton.updateCollapsed();
+    flush();
+    assertTrue(collapse.opened);
+  });
+
+  // Button should remain opened when noAutomaticCollapse flag is set.
+  test('openedWhenInitiallyOpenedAndNoAutomaticCollapse', function() {
+    const collapse = collapseRadioButton.$$('iron-collapse');
+    collapseRadioButton.checked = true;
+    flush();
+    assertTrue(collapse.opened);
+
+    collapseRadioButton.noAutomaticCollapse = true;
+    collapseRadioButton.checked = false;
+    flush();
+    assertTrue(collapse.opened);
+
+    collapseRadioButton.updateCollapsed();
+    flush();
+    assertFalse(collapse.opened);
+  });
+
   // When the button is not selected clicking the expand icon should still
   // open the iron collapse.
   test('openOnExpandHit', function() {
@@ -66,6 +100,40 @@ suite('CrCollapseRadioButton', function() {
     assertFalse(collapse.opened);
   });
 
+  // When the noAutomaticCollapse flag if set, the expand arrow should expand
+  // the radio button immediately.
+  test('openOnExpandHitWhenNoAutomaticCollapse', function() {
+    const collapse = collapseRadioButton.$$('iron-collapse');
+    collapseRadioButton.checked = false;
+    flush();
+    assertFalse(collapse.opened);
+
+    collapseRadioButton.noAutomaticCollapse = true;
+    flush();
+    assertFalse(collapse.opened);
+
+    collapseRadioButton.$$('cr-expand-button').click();
+    flush();
+    assertTrue(collapse.opened);
+  });
+
+  // When the noAutomaticCollapse flag if set, the expand arrow should collapse
+  // the radio button immediately.
+  test('closeOnExpandHitWhenSelectedWhenNoAutomaticCollapse', function() {
+    const collapse = collapseRadioButton.$$('iron-collapse');
+    collapseRadioButton.checked = true;
+    flush();
+    assertTrue(collapse.opened);
+
+    collapseRadioButton.noAutomaticCollapse = true;
+    flush();
+    assertTrue(collapse.opened);
+
+    collapseRadioButton.$$('cr-expand-button').click();
+    flush();
+    assertFalse(collapse.opened);
+  });
+
   test('expansionHiddenWhenNoCollapseSet', function() {
     assertTrue(isChildVisible(collapseRadioButton, 'cr-expand-button'));
     assertTrue(isChildVisible(collapseRadioButton, '.separator'));

chrome/test/data/webui/settings/privacy_page_test.js

@@ -7,6 +7,7 @@ import {isMac, isWindows, webUIListenerCallback} from 'chrome://resources/js/cr.
 import {loadTimeData} from 'chrome://resources/js/load_time_data.m.js';
 import {flush} from 'chrome://resources/polymer/v3_0/polymer/polymer_bundled.min.js';
 import {ClearBrowsingDataBrowserProxyImpl, CookieControlsMode, SiteSettingsPrefsBrowserProxyImpl} from 'chrome://settings/lazy_load.js';
+import {SafeBrowsingSetting} from 'chrome://settings/lazy_load.js';
 import {HatsBrowserProxyImpl, MetricsBrowserProxyImpl, PrivacyElementInteractions, PrivacyPageBrowserProxyImpl, Router, routes, SecureDnsMode, SyncBrowserProxyImpl} from 'chrome://settings/settings.js';
 import {TestClearBrowsingDataBrowserProxy} from 'chrome://test/settings/test_clear_browsing_data_browser_proxy.js';
 import {TestHatsBrowserProxy} from 'chrome://test/settings/test_hats_browser_proxy.js';
@@ -212,6 +213,27 @@ suite('HappinessTrackingSurveys', function() {
     HatsBrowserProxyImpl.instance_ = testHatsBrowserProxy;
     PolymerTest.clearBody();
     page = document.createElement('settings-privacy-page');
+    // Initialize the privacy page pref. Security page manually expands
+    // the initially selected safe browsing option so the pref object
+    // needs to be defined.
+    page.prefs = {
+      generated: {
+        safe_browsing: {
+          type: chrome.settingsPrivate.PrefType.NUMBER,
+          value: SafeBrowsingSetting.STANDARD,
+        },
+        cookie_session_only: {value: false},
+        cookie_primary_setting:
+            {type: chrome.settingsPrivate.PrefType.NUMBER, value: 0},
+      },
+      profile: {password_manager_leak_detection: {value: false}},
+      dns_over_https:
+          {mode: {value: SecureDnsMode.AUTOMATIC}, templates: {value: ''}},
+      signin: {
+        allowed_on_next_startup:
+            {type: chrome.settingsPrivate.PrefType.BOOLEAN, value: true}
+      },
+    };
     document.body.appendChild(page);
     return flushTasks();
   });

chrome/test/data/webui/settings/security_page_test.js

@@ -69,6 +69,8 @@ suite('CrSettingsSecurityPageTestWithEnhanced', function() {
           {mode: {value: SecureDnsMode.AUTOMATIC}, templates: {value: ''}},
     };
     document.body.appendChild(page);
+    page.$$('#safeBrowsingEnhanced').updateCollapsed();
+    page.$$('#safeBrowsingStandard').updateCollapsed();
     flush();
   });
 
@@ -83,6 +85,12 @@ suite('CrSettingsSecurityPageTestWithEnhanced', function() {
     });
   }
 
+  // Initially specified pref option should be expanded
+  test('SafeBrowsingRadio_InitialPrefOptionIsExpanded', function() {
+    assertFalse(page.$$('#safeBrowsingEnhanced').expanded);
+    assertTrue(page.$$('#safeBrowsingStandard').expanded);
+  });
+
   test('LogManageCerfificatesClick', async function() {
     page.$$('#manageCertificates').click();
     const result =
@@ -131,6 +139,66 @@ suite('CrSettingsSecurityPageTestWithEnhanced', function() {
     assertTrue(safeBrowsingReportingToggle.checked);
   });
 
+  test(
+      'SafeBrowsingRadio_ManuallyExpandedRemainExpandedOnRepeatSelection',
+      function() {
+        page.$$('#safeBrowsingStandard').click();
+        flush();
+        assertEquals(
+            SafeBrowsingSetting.STANDARD,
+            page.prefs.generated.safe_browsing.value);
+        assertTrue(page.$$('#safeBrowsingStandard').expanded);
+        assertFalse(page.$$('#safeBrowsingEnhanced').expanded);
+
+        // Expanding another radio button should not collapse already expanded
+        // option.
+        page.$$('#safeBrowsingEnhanced').$$('cr-expand-button').click();
+        flush();
+        assertTrue(page.$$('#safeBrowsingStandard').expanded);
+        assertTrue(page.$$('#safeBrowsingEnhanced').expanded);
+
+        // Clicking on already selected button should not collapse manually
+        // expanded option.
+        page.$$('#safeBrowsingStandard').click();
+        flush();
+        assertTrue(page.$$('#safeBrowsingStandard').expanded);
+        assertTrue(page.$$('#safeBrowsingEnhanced').expanded);
+      });
+
+  test(
+      'SafeBrowsingRadio_ManuallyExpandedRemainExpandedOnSelectedChanged',
+      async function() {
+        page.$$('#safeBrowsingStandard').click();
+        flush();
+        assertEquals(
+            SafeBrowsingSetting.STANDARD,
+            page.prefs.generated.safe_browsing.value);
+
+        page.$$('#safeBrowsingEnhanced').$$('cr-expand-button').click();
+        flush();
+        assertTrue(page.$$('#safeBrowsingStandard').expanded);
+        assertTrue(page.$$('#safeBrowsingEnhanced').expanded);
+
+        page.$$('#safeBrowsingDisabled').click();
+        flush();
+
+        // Previously selected option must remain opened.
+        assertTrue(page.$$('#safeBrowsingStandard').expanded);
+        assertTrue(page.$$('#safeBrowsingEnhanced').expanded);
+
+        page.$$('settings-disable-safebrowsing-dialog')
+            .$$('.action-button')
+            .click();
+        flush();
+
+        // Wait for onDisableSafebrowsingDialogClose_ to finish.
+        await flushTasks();
+
+        // The deselected option should become collapsed.
+        assertFalse(page.$$('#safeBrowsingStandard').expanded);
+        assertTrue(page.$$('#safeBrowsingEnhanced').expanded);
+      });
+
   test('DisableSafebrowsingDialog_Confirm', async function() {
     page.$$('#safeBrowsingStandard').click();
     assertEquals(
@@ -140,6 +208,9 @@ suite('CrSettingsSecurityPageTestWithEnhanced', function() {
     page.$$('#safeBrowsingDisabled').click();
     flush();
 
+    // Previously selected option must remain opened.
+    assertTrue(page.$$('#safeBrowsingStandard').expanded);
+
     page.$$('settings-disable-safebrowsing-dialog')
         .$$('.action-button')
         .click();
@@ -166,6 +237,9 @@ suite('CrSettingsSecurityPageTestWithEnhanced', function() {
     page.$$('#safeBrowsingDisabled').click();
     flush();
 
+    // Previously selected option must remain opened.
+    assertTrue(page.$$('#safeBrowsingEnhanced').expanded);
+
     page.$$('settings-disable-safebrowsing-dialog')
         .$$('.cancel-button')
         .click();
@@ -192,6 +266,9 @@ suite('CrSettingsSecurityPageTestWithEnhanced', function() {
     page.$$('#safeBrowsingDisabled').click();
     flush();
 
+    // Previously selected option must remain opened.
+    assertTrue(page.$$('#safeBrowsingStandard').expanded);
+
     page.$$('settings-disable-safebrowsing-dialog')
         .$$('.cancel-button')
         .click();
@@ -240,6 +317,9 @@ suite('CrSettingsSecurityPageTestWithEnhanced', function() {
     page.$$('#safeBrowsingDisabled').click();
     flush();
 
+    // Previously selected option must remain opened.
+    assertTrue(page.$$('#safeBrowsingStandard').expanded);
+
     page.$$('settings-disable-safebrowsing-dialog')
         .$$('.action-button')
         .click();
@@ -259,6 +339,9 @@ suite('CrSettingsSecurityPageTestWithEnhanced', function() {
     page.$$('#safeBrowsingDisabled').click();
     flush();
 
+    // Previously selected option must remain opened.
+    assertTrue(page.$$('#safeBrowsingStandard').expanded);
+
     page.$$('settings-disable-safebrowsing-dialog')
         .$$('.action-button')
         .click();
@@ -291,6 +374,9 @@ suite('CrSettingsSecurityPageTestWithEnhanced', function() {
     page.$$('#safeBrowsingDisabled').click();
     flush();
 
+    // Previously selected option must remain opened.
+    assertTrue(page.$$('#safeBrowsingStandard').expanded);
+
     page.$$('settings-disable-safebrowsing-dialog')
         .$$('.action-button')
         .click();