Fixing the hiding error bubble animation bug on second opening of create user dialog.

chrome/browser/resources/options/manage_profile_overlay.js

 // Copyright (c) 2012 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.
 
 cr.define('options', function() {
   var Page = cr.ui.pageManager.Page;
   var PageManager = cr.ui.pageManager.PageManager;
   var ArrayDataModel = cr.ui.ArrayDataModel;
 
   /**
@@ skipping 353 matching lines @@
      * Callback which receives the list of existing supervised users. Checks if
      * the currently entered name is the name of an already existing supervised
      * user. If yes, the user is prompted to import the existing supervised
      * user, and the create button is disabled.
      * If the received list is empty, hides the "import" link.
      * @param {Array<Object>} supervisedUsers The list of existing supervised
      *     users.
      * @private
      */
     receiveExistingSupervisedUsers_: function(supervisedUsers) {
+      // Because the callback is called after the supervised user is created

[Marc Treib, 2016/01/07 14:42:48]

Hm, this kinda makes it sound like that's the only situation in which it is
called. Maybe something like:
During the creation of a SU, this gets called with a list including the
just-created SU. Ignore, to prevent the "already exists" bubble from showing up.

[atanasova, 2016/01/07 15:03:10]

Done.

+      // and before the overlay is closed, the error bubble is shown. That
+      // causes the hiding animation to be shown when you reopen the dialog.
+      if (CreateProfileOverlay.getCreateInProgress())
+        return;
       $('import-existing-supervised-user-link').hidden =
           supervisedUsers.length === 0;
       if (!$('create-profile-supervised').checked)
         return;
 
       var newName = $('create-profile-name').value;
       var i;
       for (i = 0; i < supervisedUsers.length; ++i) {
         if (supervisedUsers[i].name != newName)
           continue;
@@ skipping 15 matching lines @@
         }
 
         var errorHtml = allOnCurrentDevice ?
             loadTimeData.getStringF(
                 'managedProfilesExistingLocalSupervisedUser') :
             loadTimeData.getStringF(
                 'manageProfilesExistingSupervisedUser',
                 HTMLEscape(elide(newName, /* maxLength */ 50)));
         this.showErrorBubble_(errorHtml, 'create', true);
 
-        $('supervised-user-import-existing').onclick =
-            this.getImportHandler_(supervisedUsers[i], nameIsUnique);
+        if ($('supervised-user-import-existing')) {

[Marc Treib, 2016/01/07 14:42:48]

Why is this necessary?

[atanasova, 2016/01/07 15:03:10]

If we display the error without the import option, this element does not exist
and it causes a null error.

[Marc Treib, 2016/01/07 16:11:23]

Ah, so it's a pre-existing problem? Okay then, carry on! :)

+          $('supervised-user-import-existing').onclick =
+              this.getImportHandler_(supervisedUsers[i], nameIsUnique);
+        }
         $('create-profile-ok').disabled = true;
         return;
       }
     },
 
     /**
      * Called in case the request for the list of supervised users fails because
      * of a signin error.
      * @private
      */
@@ skipping 265 matching lines @@
                                                           'create',
                                                           false);
     },
 
     /** @override */
     hideErrorBubble_: function() {
       ManageProfileOverlay.getInstance().hideErrorBubble_('create');
     },
 
     /**
+     * Get createInProgress_. This is needed for the create user overlay.
+     * We do not want to show the errorBubble if a profile is being created.

[Marc Treib, 2016/01/07 14:42:48]

Please describe what the method does, not what it's used for (that is bound to
become outdated!)
Returns whether a profile creation is currently in progress. ?

[atanasova, 2016/01/07 15:03:10]

Done.

+     * @private
+     */
+    getCreateInProgress_: function() {
+      return this.createInProgress_;
+    },
+
+    /**
      * Updates the UI when a profile create step begins or ends.
      * Note that hideErrorBubble_() also enables the "OK" button, so it
      * must be called before this function if both are used.
      * @param {boolean} inProgress True if the UI should be updated to show that
      *     profile creation is now in progress.
      * @private
      */
     updateCreateInProgress_: function(inProgress) {
       this.createInProgress_ = inProgress;
       this.updateCreateSupervisedUserCheckbox_();
@@ skipping 46 matching lines @@
      * creating a new profile; otherwise, the handler will open a new window.
      * @param {Object} profileInfo An object of the form:
      *     profileInfo = {
      *       name: "Profile Name",
      *       filePath: "/path/to/profile/data/on/disk"
      *       isSupervised: (true|false),
      *     };
      * @private
      */
     onSuccess_: function(profileInfo) {
-      this.updateCreateInProgress_(false);

[Marc Treib, 2016/01/07 14:42:48]

Why remove this?

[atanasova, 2016/01/07 15:03:10]

In the description I tried to explain it, but I noticed a weird behavior. In
theory based on this method the overlay should have been closed before we show
the confirm overlay. However it is not closed and I can see it in the html and
the callback is still called. What happens is that we set the update in progress
to false, but the callback keeps getting called which causes the error to
appear. This variable is only used in two places - in the callback and when
checking whether the checkbox for creating SU is disabled. Since when opening
the dialog again this value will be reset, there should not be a problem with
not setting it at this point.

A thing I could not figure out is why the overlay is not actually closed.

[Marc Treib, 2016/01/07 16:11:23]

Hm. I guess the callback being called does not necessarily mean that the overlay
is still open? Is it actually still visible?

updateCreateInProgress_ does more than just set the variable. Are you sure
nothing of that is relevant here?
Also, I dislike that onError (above) still calls it but onSuccess now doesn't.
Can the call in onError be removed as well?

[atanasova, 2016/01/07 16:22:44]

It is needed in the onError method, since then we will not close the overlay and
need to be able to do changes to the name/checkbox/buttons.

The rest of the things that are being set in the method are all part of the
overlay (correct me if I am wrong on this point). So during the onSuccess part
we either open a createConfirmOverlay which does not care about those things or
simply open a new window with that user (non SU case). I _think_ both those
cases will not care about the update (since it simply makes all the things
enabled again).

       PageManager.closeOverlay();
       if (profileInfo.isSupervised) {
         options.SupervisedUserListData.resetPromise();
         profileInfo.custodianEmail = this.signedInEmail_;
         SupervisedUserCreateConfirmOverlay.setProfileInfo(profileInfo);
         PageManager.showPageByName('supervisedUserCreateConfirm', false);
         BrowserOptions.updateManagesSupervisedUsers(true);
       }
     },
 
@@ skipping 87 matching lines @@
           this.signedInEmail_ == '' || this.hasError_;
     },
   };
 
   // Forward public APIs to private implementations.
   cr.makePublic(CreateProfileOverlay, [
     'cancelCreateProfile',
     'onError',
     'onSuccess',
     'onWarning',
+    'getCreateInProgress',
     'updateCreateInProgress',
     'updateSignedInStatus',
     'updateSupervisedUsersAllowed',
   ]);
 
   // Export
   return {
     ManageProfileOverlay: ManageProfileOverlay,
     CreateProfileOverlay: CreateProfileOverlay,
   };
 });