[b4p/Settings] Display removal dialog

This CL starts displaying the dialog from crrev.com/c/2248180. Moreover,
RemovePasswordBehavior is deprecated. The more elaborate logic of
removing different copies of a password can live directly in the dialog,
while PasswordsListHandler can use simple logic for the case when a
single copy must be removed. In particular, it felt strange that
PasswordListItem has this Polymer behavior but never used any of its
functionalities (it was PasswordsListHandler that used it instead).

We also use the occasion to fix an aspect of crrev.com/c/2235850. If
the entry is moved, there's no element to reset the focus to, so the
focusWithoutInk() call is removed. The same is done for removal dialog.

Bug: 1049141
Change-Id: I06fe35b8bd14ee27873b6a56849e755c02cbc1bb
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2246157
Commit-Queue: Victor Vianna <victorvianna@google.com>
Reviewed-by: Esmael Elmoslimany <aee@chromium.org>
Reviewed-by: Friedrich [CET] <fhorschig@chromium.org>
Cr-Commit-Position: refs/heads/master@{#779757}

chrome/browser/resources/settings/autofill_page/BUILD.gn

@@ -38,7 +38,6 @@ js_type_check("closure_compile_module") {
     ":passwords_section",
     ":payments_list",
     ":payments_section",
-    ":remove_password_behavior",
     ":show_password_behavior",
     ":upi_id_list_entry",
   ]
@@ -196,7 +195,6 @@ js_library("password_remove_confirmation_dialog") {
 js_library("password_list_item") {
   deps = [
     ":blocking_request_manager",
-    ":remove_password_behavior",
     ":show_password_behavior",
     "//third_party/polymer/v3_0/components-chromium/polymer:polymer_bundled",
     "//ui/webui/resources/js:load_time_data.m",
@@ -211,7 +209,6 @@ js_library("passwords_list_handler") {
     ":password_manager_proxy",
     ":password_move_to_account_dialog",
     ":password_remove_dialog",
-    ":remove_password_behavior",
     "//third_party/polymer/v3_0/components-chromium/polymer:polymer_bundled",
     "//ui/webui/resources/cr_elements/cr_action_menu:cr_action_menu.m",
     "//ui/webui/resources/cr_elements/cr_toast:cr_toast_manager.m",
@@ -353,13 +350,6 @@ js_library("merge_exceptions_store_copies_behavior") {
   ]
 }
 
-js_library("remove_password_behavior") {
-  deps = [
-    ":multi_store_password_ui_entry",
-    ":password_manager_proxy",
-  ]
-}
-
 js_library("upi_id_list_entry") {
   deps = [
     "//third_party/polymer/v3_0/components-chromium/polymer:polymer_bundled",

chrome/browser/resources/settings/autofill_page/password_list_item.js

@@ -19,7 +19,6 @@ import {html, Polymer} from 'chrome://resources/polymer/v3_0/polymer/polymer_bun
 import {loadTimeData} from '../i18n_setup.js';
 
 import {MultiStorePasswordUiEntry} from './multi_store_password_ui_entry.js';
-import {RemovePasswordBehavior} from './remove_password_behavior.js';
 import {ShowPasswordBehavior} from './show_password_behavior.js';
 
 Polymer({
@@ -30,7 +29,6 @@ Polymer({
   behaviors: [
     FocusRowBehavior,
     ShowPasswordBehavior,
-    RemovePasswordBehavior,
   ],
 
   /**

chrome/browser/resources/settings/autofill_page/passwords_list_handler.html

@@ -47,6 +47,13 @@
       </password-move-to-account-dialog>
     </template>
 
+    <template is="dom-if" if="[[showPasswordRemoveDialog_]]" restamp>
+      <password-remove-dialog id="passwordRemoveDialog"
+          duplicated-password="[[activePassword.entry]]"
+          on-close="onPasswordRemoveDialogClosed_">
+      </password-remove-dialog>
+    </template>
+
     <cr-toast-manager duration="5000">
       <cr-button aria-label="$i18n{undoDescription}"
           on-click="onUndoButtonClick_">$i18n{undoRemovePassword}</cr-button>

chrome/browser/resources/settings/autofill_page/passwords_list_handler.js

@@ -10,6 +10,7 @@
 
 import './password_edit_dialog.js';
 import './password_move_to_account_dialog.js';
+import './password_remove_dialog.js';
 import './password_list_item.js';
 import 'chrome://resources/cr_elements/shared_style_css.m.js';
 import './password_edit_dialog.js';
@@ -19,12 +20,14 @@ import {assert} from 'chrome://resources/js/assert.m.js';
 import {focusWithoutInk} from 'chrome://resources/js/cr/ui/focus_without_ink.m.js';
 import {I18nBehavior} from 'chrome://resources/js/i18n_behavior.m.js';
 import {html, Polymer} from 'chrome://resources/polymer/v3_0/polymer/polymer_bundled.min.js';
+
 import {loadTimeData} from '../i18n_setup.js';
+
 // <if expr="chromeos">
 import {BlockingRequestManager} from './blocking_request_manager.js';
 // </if>
 import {PasswordManagerImpl} from './password_manager_proxy.js';
-import {RemovalResult} from './remove_password_behavior.js';
+import {PasswordRemoveDialogPasswordsRemovedEvent} from './password_remove_dialog.js';
 
 Polymer({
   is: 'passwords-list-handler',
@@ -80,6 +83,9 @@ Polymer({
     /** @private */
     showPasswordMoveToAccountDialog_: {type: Boolean, value: false},
 
+    /** @private */
+    showPasswordRemoveDialog_: {type: Boolean, value: false},
+
     /**
      * The element to return focus to, when the currently active dialog is
      * closed.
@@ -95,6 +101,8 @@ Polymer({
 
   listeners: {
     'password-menu-tap': 'onPasswordMenuTap_',
+    'password-remove-dialog-passwords-removed':
+        'onPasswordRemoveDialogPasswordsRemoved_',
   },
 
   /** @override */
@@ -177,26 +185,43 @@ Polymer({
   },
 
   /**
-   * Fires an event that should delete the saved password.
+   * Handler for the remove option in the overflow menu. If the password only
+   * exists in one location, deletes it directly. Otherwise, opens the remove
+   * dialog to allow choosing from which locations to remove.
    * @private
    */
   onMenuRemovePasswordTap_() {
-    // TODO(crbug.com/1049141): Open removal dialog if password is present in
-    // both locations. Add tests for when no password is selected in the dialog.
-    /** @type {!RemovalResult} */
-    const result = this.activePassword.requestRemove();
-
-    if (!result.removedFromDevice && !result.removedFromAccount) {
-      this.$.menu.close();
-      this.activePassword = null;
+    this.$.menu.close();
+
+    if (this.activePassword.entry.isPresentOnDevice() &&
+        this.activePassword.entry.isPresentInAccount()) {
+      this.showPasswordRemoveDialog_ = true;
       return;
     }
 
+    const idToRemove = this.activePassword.entry.isPresentInAccount() ?
+        this.activePassword.entry.accountId :
+        this.activePassword.entry.deviceId;
+    PasswordManagerImpl.getInstance().removeSavedPassword(idToRemove);
+    this.displayRemovalNotification_(
+        this.activePassword.entry.isPresentInAccount(),
+        this.activePassword.entry.isPresentOnDevice());
+    this.activePassword = null;
+  },
+
+  /**
+   * At least one of |removedFromAccount| or |removedFromDevice| must be true.
+   * @param {boolean} removedFromAccount
+   * @param {boolean} removedFromDevice
+   * @private
+   */
+  displayRemovalNotification_(removedFromAccount, removedFromDevice) {
+    assert(removedFromAccount || removedFromDevice);
     let text = this.i18n('passwordDeleted');
     if (this.shouldShowStorageDetails) {
-      if (result.removedFromAccount && result.removedFromDevice) {
+      if (removedFromAccount && removedFromDevice) {
         text = this.i18n('passwordDeletedFromAccountAndDevice');
-      } else if (result.removedFromAccount) {
+      } else if (removedFromAccount) {
         text = this.i18n('passwordDeletedFromAccount');
       } else {
         text = this.i18n('passwordDeletedFromDevice');
@@ -204,9 +229,14 @@ Polymer({
     }
     getToastManager().show(text);
     this.fire('iron-announce', {text: this.i18n('undoDescription')});
+  },
 
-    this.$.menu.close();
-    this.activePassword = null;
+  /**
+   * @param {PasswordRemoveDialogPasswordsRemovedEvent} event
+   */
+  onPasswordRemoveDialogPasswordsRemoved_(event) {
+    this.displayRemovalNotification_(
+        event.detail.removedFromAccount, event.detail.removedFromDevice);
   },
 
   /**
@@ -233,9 +263,21 @@ Polymer({
    */
   onPasswordMoveToAccountDialogClosed_() {
     this.showPasswordMoveToAccountDialog_ = false;
-    focusWithoutInk(assert(this.activeDialogAnchor_));
+    this.activePassword = null;
+
+    // The entry possibly disappeared, so don't reset the focus.
     this.activeDialogAnchor_ = null;
+  },
+
+  /**
+   * @private
+   */
+  onPasswordRemoveDialogClosed_() {
+    this.showPasswordRemoveDialog_ = false;
     this.activePassword = null;
+
+    // A removal possibly happened, so don't reset the focus.
+    this.activeDialogAnchor_ = null;
   },
 
 });

chrome/browser/resources/settings/autofill_page/passwords_section.html

@@ -223,8 +223,6 @@
             class="cr-separators list-with-header"
             scroll-target="[[subpageScrollTarget]]" risk-selection>
           <template>
-            <!-- The entry property is shared by ShowPasswordBehavior and
-            RemovePasswordBehavior. -->
             <password-list-item entry="[[item]]"
                 tabindex$="[[tabIndex]]" focus-row-index="[[index]]"
 <if expr="chromeos">

chrome/browser/resources/settings/autofill_page/remove_password_behavior.js

-// Copyright 2020 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.
-
-import {MultiStorePasswordUiEntry} from './multi_store_password_ui_entry.js';
-import {PasswordManagerImpl} from './password_manager_proxy.js';
-
-/**
- * This behavior bundles functionality required to remove a user password.
- *
- * @polymerBehavior
- */
-export const RemovePasswordBehavior = {
-
-  properties: {
-    /**
-     * The password that will be removed.
-     * @type {!MultiStorePasswordUiEntry}
-     */
-    entry: Object,
-  },
-
-  /**
-   * Removes all existing versions of the password.
-   * @return {!RemovalResult}
-   */
-  // TODO(crbug.com/1049141): Expose APIs in the behavior to allow selecting
-  // which versions to remove. Update the comment on this method then.
-  requestRemove() {
-    /** @type {!RemovalResult} */
-    const result = {
-      removedFromAccount: false,
-      removedFromDevice: false,
-    };
-
-    /** @type {!Array<number>} */
-    const idsToRemove = [];
-    if (this.entry.isPresentInAccount()) {
-      result.removedFromAccount = true;
-      idsToRemove.push(this.entry.accountId);
-    }
-    if (this.entry.isPresentOnDevice()) {
-      result.removedFromDevice = true;
-      idsToRemove.push(this.entry.deviceId);
-    }
-
-    if (idsToRemove.length) {
-      PasswordManagerImpl.getInstance().removeSavedPasswords(idsToRemove);
-    }
-
-    return result;
-  },
-};
-
-
-/**
- * Used to inform which password versions were removed.
- * @typedef {{
- *   removedFromAccount: boolean,
- *   removedFromDevice: boolean,
- * }}
- */
-export let RemovalResult;

chrome/browser/resources/settings/settings_resources_v3.grdp

@@ -174,10 +174,6 @@
            file="${root_gen_dir}/chrome/browser/resources/settings/autofill_page/payments_section.js"
            use_base_dir="false"
            compress="false" type="BINDATA" />
-  <include name="IDR_SETTINGS_AUTOFILL_PAGE_REMOVE_PASSWORD_BEHAVIOR_JS"
-           file="autofill_page/remove_password_behavior.js"
-           compress="false" type="BINDATA"
-           preprocess="true" />
   <include name="IDR_SETTINGS_AUTOFILL_PAGE_SHOW_PASSWORD_BEHAVIOR_JS"
            file="autofill_page/show_password_behavior.js"
            compress="false" type="BINDATA"

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

@@ -143,6 +143,22 @@ function exceptionsListContainsUrl(exceptionList, url) {
   return false;
 }
 
+/**
+ * Simulates user who is eligible and opted-in for account storage. Should be
+ * called after the PasswordsSection element is created. The load time value for
+ * enableAccountStorage must be overridden separately.
+ * @param {TestPasswordManagerProxy} passwordManager
+ */
+function simulateAccountStorageUser(passwordManager) {
+  simulateSyncStatus({signedIn: false});
+  simulateStoredAccounts([{
+    fullName: 'john doe',
+    givenName: 'john',
+    email: 'john@gmail.com',
+  }]);
+  passwordManager.setIsOptedInForAccountStorageAndNotify(true);
+}
+
 suite('PasswordsSection', function() {
   /** @type {TestPasswordManagerProxy} */
   let passwordManager = null;
@@ -480,9 +496,9 @@ suite('PasswordsSection', function() {
     firstNode.$$('#passwordMenu').click();
     passwordsSection.$.passwordsListHandler.$.menuRemovePassword.click();
 
-    const ids = await passwordManager.whenCalled('removeSavedPasswords');
+    const id = await passwordManager.whenCalled('removeSavedPassword');
     // Verify that the expected value was passed to the proxy.
-    assertDeepEquals([firstPassword.id], ids);
+    assertEquals(firstPassword.id, id);
     assertEquals(
         passwordsSection.i18n('passwordDeleted'),
         getToastManager().$.content.textContent);
@@ -1237,54 +1253,28 @@ suite('PasswordsSection', function() {
           passwordsSection.$.devicePasswordsLinkLabel.innerText);
     });
 
-    // Test verifies that the notification displayed after removing a password
-    // informs whether the password was stored on the device, in the account or
-    // in both, for users in account storage mode.
+    // Test verifies that, for account-scoped password storage users, removing
+    // a password stored in a single location indicates the location in the
+    // toast manager message.
     test(
-        'passwordDeletionMessageSpecifiesStoreIfAccountStorageIsEnabled',
+        'passwordRemovalMessageSpecifiesStoreForAccountStorageUsers',
         function() {
-          // Feature flag enabled.
           loadTimeData.overrideValues({enableAccountStorage: true});
 
-          // Create array with one account-only password, one device-only, and
-          // two duplicates that will be merged.
+          const passwordList = [
+            createPasswordEntry(
+                {username: 'account', id: 0, fromAccountStore: true}),
+            createPasswordEntry(
+                {username: 'local', id: 1, fromAccountStore: false}),
+          ];
           const passwordsSection = elementFactory.createPasswordsSection(
-              passwordManager,
-              [
-                createPasswordEntry(
-                    {username: 'account', id: 0, fromAccountStore: true}),
-                createPasswordEntry(
-                    {username: 'local', id: 1, fromAccountStore: false}),
-                createPasswordEntry({
-                  username: 'both',
-                  frontendId: 2,
-                  id: 21,
-                  fromAccountStore: false
-                }),
-                createPasswordEntry({
-                  username: 'both',
-                  frontendId: 2,
-                  id: 22,
-                  fromAccountStore: true
-                }),
-              ],
-              []);
-
-          // Setup user in account storage mode: sync disabled, user signed in
-          // and opted in.
-          simulateSyncStatus({signedIn: false});
-          simulateStoredAccounts([{
-            fullName: 'john doe',
-            givenName: 'john',
-            email: 'john@gmail.com',
-          }]);
-          passwordManager.setIsOptedInForAccountStorageAndNotify(true);
+              passwordManager, passwordList, []);
 
-          const passwordListItems =
-              passwordsSection.root.querySelectorAll('password-list-item');
+          simulateAccountStorageUser(passwordManager);
 
           // No removal actually happens, so all passwords keep their position.
-          // The merged entry comes last.
+          const passwordListItems =
+              passwordsSection.root.querySelectorAll('password-list-item');
           passwordListItems[0].$$('#passwordMenu').click();
           passwordsSection.$.passwordsListHandler.$.menuRemovePassword.click();
           assertEquals(
@@ -1296,16 +1286,86 @@ suite('PasswordsSection', function() {
           assertEquals(
               passwordsSection.i18n('passwordDeletedFromDevice'),
               getToastManager().$.content.textContent);
+        });
 
-          // TODO(crbug.com/1049141): Update the test when the removal dialog
-          // is introduced.
-          passwordListItems[2].$$('#passwordMenu').click();
-          passwordsSection.$.passwordsListHandler.$.menuRemovePassword.click();
-          assertEquals(
-              passwordsSection.i18n('passwordDeletedFromAccountAndDevice'),
-              getToastManager().$.content.textContent);
+    // Test verifies that if the user attempts to remove a password stored
+    // both on the device and in the account, the PasswordRemoveDialog shows up.
+    // Clicking the button in the dialog then removes both versions of the
+    // password.
+    test('verifyPasswordRemoveDialogRemoveBothCopies', async function() {
+      loadTimeData.overrideValues({enableAccountStorage: true});
 
-        });
+      const accountCopy =
+          createPasswordEntry({frontendId: 42, id: 0, fromAccountStore: true});
+      const deviceCopy =
+          createPasswordEntry({frontendId: 42, id: 1, fromAccountStore: false});
+      const passwordsSection = elementFactory.createPasswordsSection(
+          passwordManager, [accountCopy, deviceCopy], []);
+
+      simulateAccountStorageUser(passwordManager);
+
+      // At first the dialog is not shown.
+      assertTrue(
+          !passwordsSection.$.passwordsListHandler.$$('#passwordRemoveDialog'));
+
+      // Clicking remove in the overflow menu shows the dialog.
+      getFirstPasswordListItem(passwordsSection).$$('#passwordMenu').click();
+      passwordsSection.$.passwordsListHandler.$.menuRemovePassword.click();
+      flush();
+      const removeDialog =
+          passwordsSection.$.passwordsListHandler.$$('#passwordRemoveDialog');
+      assertTrue(!!removeDialog);
+
+      // Both checkboxes are selected by default. Confirming removes from both
+      // account and device.
+      assertTrue(
+          removeDialog.$.removeFromAccountCheckbox.checked &&
+          removeDialog.$.removeFromDeviceCheckbox.checked);
+      removeDialog.$.removeButton.click();
+      const removedIds =
+          await passwordManager.whenCalled('removeSavedPasswords');
+      assertTrue(removedIds.includes(accountCopy.id));
+      assertTrue(removedIds.includes(deviceCopy.id));
+    });
+
+    // Test verifies that if the user attempts to remove a password stored
+    // both on the device and in the account, the PasswordRemoveDialog shows up.
+    // The user then chooses to remove only of the copies.
+    test('verifyPasswordRemoveDialogRemoveSingleCopy', async function() {
+      loadTimeData.overrideValues({enableAccountStorage: true});
+
+      const accountCopy =
+          createPasswordEntry({frontendId: 42, id: 0, fromAccountStore: true});
+      const deviceCopy =
+          createPasswordEntry({frontendId: 42, id: 1, fromAccountStore: false});
+      const passwordsSection = elementFactory.createPasswordsSection(
+          passwordManager, [accountCopy, deviceCopy], []);
+
+      simulateAccountStorageUser(passwordManager);
+
+      // At first the dialog is not shown.
+      assertTrue(
+          !passwordsSection.$.passwordsListHandler.$$('#passwordRemoveDialog'));
+
+      // Clicking remove in the overflow menu shows the dialog.
+      getFirstPasswordListItem(passwordsSection).$$('#passwordMenu').click();
+      passwordsSection.$.passwordsListHandler.$.menuRemovePassword.click();
+      flush();
+      const removeDialog =
+          passwordsSection.$.passwordsListHandler.$$('#passwordRemoveDialog');
+      assertTrue(!!removeDialog);
+
+      // Uncheck the account checkboxes then confirm. Only the device copy is
+      // removed.
+      removeDialog.$.removeFromAccountCheckbox.click();
+      assertTrue(
+          !removeDialog.$.removeFromAccountCheckbox.checked &&
+          removeDialog.$.removeFromDeviceCheckbox.checked);
+      removeDialog.$.removeButton.click();
+      const removedIds =
+          await passwordManager.whenCalled('removeSavedPasswords');
+      assertTrue(removedIds.includes(deviceCopy.id));
+    });
   }
 
   // The export dialog is dismissable.