Fix Behavior of Visible Passwords

Prior to this change visible passwords were bound to a specific row in
the list of passwords. This led to bugs when the list changed, e.g. when
adding or deleting a password, or changing the list of passwords with a
search filter.

This change fixes this by grouping the password and the UiEntry into a
newly introduced typedef.

Bug: 786312, 771126
Cq-Include-Trybots: master.tryserver.chromium.linux:closure_compilation
Change-Id: I25751d6a20b315290f2c52cc76946f85c413405f
Reviewed-on: https://chromium-review.googlesource.com/779179Reviewed-by: Hector Carmona <hcarmona@chromium.org>
Reviewed-by: Devlin <rdevlin.cronin@chromium.org>
Commit-Queue: Jan Wilken Dörrie <jdoerrie@chromium.org>
Cr-Commit-Position: refs/heads/master@{#520200}

chrome/browser/resources/settings/passwords_and_forms_page/password_edit_dialog.html

@@ -44,25 +44,25 @@
       <div slot="title">$i18n{passwordDetailsTitle}</div>
       <div slot="body">
         <paper-input id="websiteInput" label="$i18n{editPasswordWebsiteLabel}"
-            value="[[item.loginPair.urls.link]]" readonly always-float-label
-            on-tap="onReadonlyInputTap_">
+            value="[[item.entry.loginPair.urls.link]]" readonly
+            always-float-label on-tap="onReadonlyInputTap_">
         </paper-input>
         <paper-input id="usernameInput" label="$i18n{editPasswordUsernameLabel}"
-            value="[[item.loginPair.username]]" readonly always-float-label
-            on-tap="onReadonlyInputTap_">
+            value="[[item.entry.loginPair.username]]" readonly
+            always-float-label on-tap="onReadonlyInputTap_">
         </paper-input>
         <div id="passwordGroup">
           <paper-input id="passwordInput" always-float-label
               label="$i18n{editPasswordPasswordLabel}"
-              type="[[getPasswordInputType_(item, password)]]"
-              value="[[getPassword_(item, password)]]" readonly
+              type="[[getPasswordInputType_(item.password)]]"
+              value="[[getPassword_(item.password)]]" readonly
             on-tap="onReadonlyInputTap_">
           </paper-input>
           <button is="paper-icon-button-light" id="showPasswordButton"
-              class$="[[getIconClass_(item, password)]]"
-              hidden$="[[item.federationText]]"
+              class$="[[getIconClass_(item.password)]]"
+              hidden$="[[item.entry.federationText]]"
               on-tap="onShowPasswordButtonTap_"
-              title="[[showPasswordTitle_(password,
+              title="[[showPasswordTitle_(item.password,
                   '$i18nPolymer{hidePassword}','$i18nPolymer{showPassword}')]]">
           </button>
         </div>

chrome/browser/resources/settings/passwords_and_forms_page/password_list_item.html

@@ -31,37 +31,37 @@
     </style>
     <div class="list-item" focus-row-container>
       <div class="website-column no-min-width"
-          title="[[item.loginPair.urls.link]]">
+          title="[[item.entry.loginPair.urls.link]]">
         <a id="originUrl" target="_blank" class="no-min-width"
-            href="[[item.loginPair.urls.link]]"
+            href="[[item.entry.loginPair.urls.link]]"
             focus-row-control focus-type="originUrl">
           <span class="text-elide">
             <!-- This bdo tag is necessary to fix the display of domains
               starting with numbers. -->
-            <bdo dir="ltr">[[item.loginPair.urls.shown]]</bdo>
+            <bdo dir="ltr">[[item.entry.loginPair.urls.shown]]</bdo>
           </span>
         </a>
       </div>
       <div class="username-column text-elide"
-           id="username">[[item.loginPair.username]]</div>
+           id="username">[[item.entry.loginPair.username]]</div>
       <div class="password-column">
-        <template is="dom-if" if="[[!item.federationText]]">
+        <template is="dom-if" if="[[!item.entry.federationText]]">
           <input id="password" aria-label=$i18n{editPasswordPasswordLabel}
-              type="[[getPasswordInputType_(item, password)]]"
+              type="[[getPasswordInputType_(item.password)]]"
               on-tap="onReadonlyInputTap_" class="password-field" readonly
-              disabled$="[[!password]]"
-              value="[[getPassword_(item, password)]]">
+              disabled$="[[!item.password]]"
+              value="[[getPassword_(item.password)]]">
           <button is="paper-icon-button-light" id="showPasswordButton"
-              class$="[[getIconClass_(item, password)]]"
+              class$="[[getIconClass_(item.password)]]"
               on-tap="onShowPasswordButtonTap_"
-              title="[[showPasswordTitle_(password,
+              title="[[showPasswordTitle_(item.password,
                   '$i18nPolymer{hidePassword}','$i18nPolymer{showPassword}')]]"
               focus-row-control focus-type="showPassword">
           </button>
         </template>
-        <template is="dom-if" if="[[item.federationText]]">
+        <template is="dom-if" if="[[item.entry.federationText]]">
           <span class="password-field text-elide" id="federated">
-            [[item.federationText]]
+            [[item.entry.federationText]]
           </span>
         </template>
       </div>

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

@@ -27,6 +27,6 @@ Polymer({
    */
   onPasswordMenuTap_: function() {
     this.fire(
-        'password-menu-tap', {target: this.$.passwordMenu, item: this.item});
+        'password-menu-tap', {target: this.$.passwordMenu, listItem: this});
   },
 });

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

@@ -143,7 +143,7 @@
     </template>
     <template is="dom-if" if="[[showPasswordEditDialog_]]" restamp>
       <password-edit-dialog on-close="onPasswordEditDialogClosed_"
-          item="[[activePassword]]">
+          item="[[activePassword.item]]">
       </password-edit-dialog>
     </template>
     <cr-toast id="undoToast" duration="[[toastDuration_]]">

chrome/browser/resources/settings/passwords_and_forms_page/passwords_section.js

@@ -100,6 +100,9 @@ PasswordManager.ExceptionEntry;
 /** @typedef {chrome.passwordsPrivate.PlaintextPasswordEventParameters} */
 PasswordManager.PlaintextPasswordEvent;
 
+/** @typedef {{ entry: !PasswordManager.PasswordUiEntry, password: string }} */
+PasswordManager.UiEntryWithPassword;
+
 /**
  * Implementation that accesses the private API.
  * @implements {PasswordManager}
@@ -234,7 +237,7 @@ Polymer({
 
     /**
      * The model for any password related action menus or dialogs.
-     * @private {?chrome.passwordsPrivate.PasswordUiEntry}
+     * @private {?PasswordListItemElement}
      */
     activePassword: Object,
 
@@ -320,7 +323,12 @@ Polymer({
   attached: function() {
     // Create listener functions.
     var setSavedPasswordsListener = list => {
-      this.savedPasswords = list;
+      this.savedPasswords = list.map(entry => {
+        return {
+          entry: entry,
+          password: '',
+        };
+      });
     };
 
     var setPasswordExceptionsListener = list => {
@@ -377,12 +385,16 @@ Polymer({
     this.showPasswordEditDialog_ = false;
     cr.ui.focusWithoutInk(assert(this.activeDialogAnchor_));
     this.activeDialogAnchor_ = null;
+
+    // Trigger a re-evaluation of the activePassword as the visibility state of
+    // the password might have changed.
+    this.activePassword.notifyPath('item.password');
   },
 
   /**
-   * @param {!Array<!chrome.passwordsPrivate.PasswordUiEntry>} savedPasswords
+   * @param {!Array<!PasswordManager.UiEntryWithPassword>} savedPasswords
    * @param {string} filter
-   * @return {!Array<!chrome.passwordsPrivate.PasswordUiEntry>}
+   * @return {!Array<!PasswordManager.UiEntryWithPassword>}
    * @private
    */
   getFilteredPasswords_: function(savedPasswords, filter) {
@@ -390,7 +402,7 @@ Polymer({
       return savedPasswords;
 
     return savedPasswords.filter(p => {
-      return [p.loginPair.urls.shown, p.loginPair.username].some(
+      return [p.entry.loginPair.urls.shown, p.entry.loginPair.username].some(
           term => term.toLowerCase().includes(filter.toLowerCase()));
     });
   },
@@ -411,7 +423,8 @@ Polymer({
    * @private
    */
   onMenuRemovePasswordTap_: function() {
-    this.passwordManager_.removeSavedPassword(this.activePassword.index);
+    this.passwordManager_.removeSavedPassword(
+        this.activePassword.item.entry.index);
     this.fire('iron-announce', {text: this.$.undoLabel.textContent});
     this.$.undoToast.show();
     /** @type {CrActionMenuElement} */ (this.$.menu).close();
@@ -448,8 +461,7 @@ Polymer({
     var target = /** @type {!HTMLElement} */ (event.detail.target);
 
     this.activePassword =
-        /** @type {!chrome.passwordsPrivate.PasswordUiEntry} */ (
-            event.detail.item);
+        /** @type {!PasswordListItemElement} */ (event.detail.listItem);
     menu.showAt(target);
     this.activeDialogAnchor_ = target;
   },
@@ -510,8 +522,8 @@ Polymer({
    */
   showPassword_: function(event) {
     this.passwordManager_.getPlaintextPassword(
-        /** @type {!number} */ (event.detail.item.index), item => {
-          event.detail.password = item.plaintextPassword;
+        /** @type {!number} */ (event.detail.item.entry.index), item => {
+          event.detail.set('item.password', item.plaintextPassword);
         });
   },
 

chrome/browser/resources/settings/passwords_and_forms_page/show_password_behavior.js

@@ -13,19 +13,9 @@ var ShowPasswordBehavior = {
   properties: {
     /**
      * The password that is being displayed.
-     * @type {!chrome.passwordsPrivate.PasswordUiEntry}
+     * @type {!ShowPasswordBehavior.UiEntryWithPassword}
      */
     item: Object,
-
-    /**
-     * Holds the plaintext password when requested.
-     * Initializing it to the empty string is necessary to indicate that the
-     * password hasn't been fetched yet.
-     */
-    password: {
-      type: String,
-      value: '',
-    },
   },
 
   /**
@@ -34,7 +24,8 @@ var ShowPasswordBehavior = {
    * @private
    */
   getPasswordInputType_: function() {
-    return this.password || this.item.federationText ? 'text' : 'password';
+    return this.item.password || this.item.entry.federationText ? 'text' :
+                                                                  'password';
   },
 
   /**
@@ -54,7 +45,7 @@ var ShowPasswordBehavior = {
    * @private
    */
   getIconClass_: function() {
-    return this.password ? 'icon-visibility-off' : 'icon-visibility';
+    return this.item.password ? 'icon-visibility-off' : 'icon-visibility';
   },
 
   /**
@@ -66,9 +57,8 @@ var ShowPasswordBehavior = {
   getPassword_: function() {
     if (!this.item)
       return '';
-
-    return this.item.federationText || this.password ||
-        ' '.repeat(this.item.numCharactersInPassword);
+    return this.item.entry.federationText || this.item.password ||
+        ' '.repeat(this.item.entry.numCharactersInPassword);
   },
 
   /**
@@ -77,9 +67,16 @@ var ShowPasswordBehavior = {
    * @private
    */
   onShowPasswordButtonTap_: function() {
-    if (this.password)
-      this.password = '';
+    if (this.item.password)
+      this.set('item.password', '');
     else
       this.fire('show-password', this);  // Request the password.
   },
 };
+
+/** @typedef {{
+ *    entry: !chrome.passwordsPrivate.PasswordUiEntry,
+ *    password: string
+ * }}
+ */
+ShowPasswordBehavior.UiEntryWithPassword;

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

@@ -191,7 +191,10 @@ TEST_F('PasswordsAndFormsBrowserTest', 'uiTests', function() {
         passwordManager.lastCallback.addSavedPasswordListChangedListener(list);
         Polymer.dom.flush();
 
-        assertEquals(list, element.$$('#passwordSection').savedPasswords);
+        assertDeepEquals(
+            list,
+            element.$$('#passwordSection')
+                .savedPasswords.map(entry => entry.entry));
 
         // The callback is coming from the manager, so the element shouldn't
         // have additional calls to the manager after the base expectations.

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

@@ -144,7 +144,7 @@ TEST_F('SettingsPasswordSectionBrowserTest', 'uiTests', function() {
    */
   function createPasswordListItem(passwordItem) {
     var passwordListItem = document.createElement('password-list-item');
-    passwordListItem.item = passwordItem;
+    passwordListItem.item = {entry: passwordItem, password: ''};
     document.body.appendChild(passwordListItem);
     Polymer.dom.flush();
     return passwordListItem;
@@ -158,7 +158,7 @@ TEST_F('SettingsPasswordSectionBrowserTest', 'uiTests', function() {
    */
   function createPasswordDialog(passwordItem) {
     var passwordDialog = document.createElement('password-edit-dialog');
-    passwordDialog.item = passwordItem;
+    passwordDialog.item = {entry: passwordItem, password: ''};
     document.body.appendChild(passwordDialog);
     Polymer.dom.flush();
     return passwordDialog;
@@ -250,7 +250,9 @@ TEST_F('SettingsPasswordSectionBrowserTest', 'uiTests', function() {
 
       // Assert that the data is passed into the iron list. If this fails,
       // then other expectations will also fail.
-      assertEquals(passwordList, passwordsSection.$.passwordList.items);
+      assertDeepEquals(
+          passwordList,
+          passwordsSection.$.passwordList.items.map(entry => entry.entry));
 
       validatePasswordList(passwordsSection.$.passwordList, passwordList);
 
@@ -272,10 +274,12 @@ TEST_F('SettingsPasswordSectionBrowserTest', 'uiTests', function() {
       validatePasswordList(passwordsSection.$.passwordList, passwordList);
       // Simulate 'longwebsite.com' being removed from the list.
       passwordsSection.splice('savedPasswords', 1, 1);
+      passwordList.splice(1, 1);
       flushPasswordSection(passwordsSection);
 
-      assertFalse(listContainsUrl(passwordsSection.savedPasswords,
-                                       'longwebsite.com'));
+      assertFalse(listContainsUrl(
+          passwordsSection.savedPasswords.map(entry => entry.entry),
+          'longwebsite.com'));
       assertFalse(listContainsUrl(passwordList, 'longwebsite.com'));
 
       validatePasswordList(passwordsSection.$.passwordList, passwordList);
@@ -495,7 +499,7 @@ TEST_F('SettingsPasswordSectionBrowserTest', 'uiTests', function() {
 
       assertFalse(passwordDialog.$.showPasswordButton.hidden);
 
-      passwordDialog.password = PASSWORD;
+      passwordDialog.set('item.password', PASSWORD);
       Polymer.dom.flush();
 
       assertEquals(PASSWORD,
@@ -513,7 +517,7 @@ TEST_F('SettingsPasswordSectionBrowserTest', 'uiTests', function() {
       // Hidden passwords should be disabled.
       assertTrue(passwordListItem.$$('#password').disabled);
 
-      passwordListItem.password = PASSWORD;
+      passwordListItem.set('item.password', PASSWORD);
       Polymer.dom.flush();
 
       assertEquals(PASSWORD, passwordListItem.$$('#password').value);
@@ -536,9 +540,10 @@ TEST_F('SettingsPasswordSectionBrowserTest', 'uiTests', function() {
         var actualItem = event.detail.item;
         assertEquals(
             expectedItem.loginPair.urls.origin,
-            actualItem.loginPair.urls.origin);
+            actualItem.entry.loginPair.urls.origin);
         assertEquals(
-            expectedItem.loginPair.username, actualItem.loginPair.username);
+            expectedItem.loginPair.username,
+            actualItem.entry.loginPair.username);
         done();
       });
 
@@ -553,9 +558,10 @@ TEST_F('SettingsPasswordSectionBrowserTest', 'uiTests', function() {
         var actualItem = event.detail.item;
         assertEquals(
             expectedItem.loginPair.urls.origin,
-            actualItem.loginPair.urls.origin);
+            actualItem.entry.loginPair.urls.origin);
         assertEquals(
-            expectedItem.loginPair.username, actualItem.loginPair.username);
+            expectedItem.loginPair.username,
+            actualItem.entry.loginPair.username);
         done();
       });
 

third_party/closure_compiler/externs/passwords_private.js

@@ -114,11 +114,13 @@ chrome.passwordsPrivate.getSavedPasswordList = function(callback) {};
 chrome.passwordsPrivate.getPasswordExceptionList = function(callback) {};
 
 /**
+ * Triggers the Password Manager password import functionality.
  * @see https://developer.chrome.com/extensions/passwordsPrivate#method-importPasswords
  */
 chrome.passwordsPrivate.importPasswords = function() {};
 
 /**
+ * Triggers the Password Manager password export functionality.
  * @see https://developer.chrome.com/extensions/passwordsPrivate#method-exportPasswords
  */
 chrome.passwordsPrivate.exportPasswords = function() {};