[CrOS Settings] Regression: Update lock screen type radio button logic.

When the user switches the radio button from "PIN" to "PASSWORD" back
to "PIN" quickly, the "PASSWORD" button is re-selected on it's own.
In the brief moment that that "PIN" is selected, the button on the
RHS, which should be labeled as "Set up Pin" is "Change Pin".

In order to prevent the radio button from toggling from "PIN" back
to "Password" on its own (without user action), the selectedUnlockType
is checked again after active lockscreen preferences are changed
asynchronously.

Replacing "hidden" with a dom-if template shortens the delay of
"Set up PIN" and "Change PIN" in a noticeable way, but it doesn't
resolve the fact that "incorrect strings may be shown for a brief
period, and updating them causes UI flicker" [1]. This still happens
from time to time, and is not consistent.

[1] https://tinyurl.com/vdxo262

In order to reduce UI flicker, which is unavoidable in instances of
failure (fail to change from PIN to password), the "Set up PIN" and
"Change PIN" button is modified to change immediately on user action
to the intended value: if the user changes from password to PIN,
"Set up PIN" becomes the default immediately (as it should).

If the lock type fails to change from PIN to password, then UI
flicker occurs in that "Set up PIN" changes back to
"Change PIN" (as opposed to seeing the "Change PIN" change to
"Set up PIN" with higher frequency).  There is not a good way around
this, the alternative being that a pinwheel of some sort should be
visible as async tasks are being performed, and additional UI to
show that CrOS was unable to change from PIN to password and vice
versa.

Bug: 1054327
Change-Id: Iade95bb3c3a44c90a69b069faa160f17b093107b
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2079834
Commit-Queue: Regan Hsu <hsuregan@chromium.org>
Reviewed-by: Kyle Horimoto <khorimoto@chromium.org>
Cr-Commit-Position: refs/heads/master@{#746676}

chrome/browser/resources/settings/chromeos/os_people_page/lock_screen.html

@@ -99,20 +99,22 @@
               <cr-radio-button name="pin+password" class="list-item-start"
                   label=$i18n{lockScreenPinOrPassword}>
               </cr-radio-button>
-              <div class="list-item-end"
-                  hidden="[[!showConfigurePinButton_(selectedUnlockType)]]">
-                <div class="separator"></div>
-                <div id="pinPasswordSecondaryActionDiv"
-                    class="secondary-action">
-                  <!-- Use stop-keyboard-event-propagation to prevent
-                       triggering this when focused after closing the
-                       dialog. -->
-                  <cr-button id="setupPinButton" on-click="onConfigurePin_"
-                      stop-keyboard-event-propagation>
-                    [[getSetupPinText_(hasPin)]]
-                  </cr-button>
+              <template is="dom-if"
+                  if="[[showConfigurePinButton_(selectedUnlockType)]]">
+                <div class="list-item-end">
+                  <div class="separator"></div>
+                  <div id="pinPasswordSecondaryActionDiv"
+                      class="secondary-action">
+                    <!-- Use stop-keyboard-event-propagation to prevent
+                         triggering this when focused after closing the
+                         dialog. -->
+                    <cr-button id="setupPinButton" on-click="onConfigurePin_"
+                        stop-keyboard-event-propagation>
+                      [[getSetupPinText_(hasPin)]]
+                    </cr-button>
+                  </div>
                 </div>
-              </div>
+              </template>
             </cr-radio-group>
           </div>
         </div>

chrome/browser/resources/settings/chromeos/os_people_page/lock_screen.js

@@ -167,7 +167,7 @@ Polymer({
    */
   currentRouteChanged(newRoute, oldRoute) {
     if (newRoute == settings.routes.LOCK_SCREEN) {
-      this.updateUnlockType();
+      this.updateUnlockType(/*activeModesChanged=*/ false);
       this.updateNumFingerprints_();
     }
 
@@ -201,11 +201,19 @@ Polymer({
     }
 
     if (selected != LockScreenUnlockType.PIN_PASSWORD && this.setModes_) {
+      // If the user selects PASSWORD only (which sends an asynchronous
+      // setModes_.call() to clear the quick unlock capability), indicate to the
+      // user immediately that the quick unlock capability is cleared by setting
+      // |hasPin| to false. If there is an error clearing quick unlock, revert
+      // |hasPin| to true. This prevents setupPinButton UI delays, except in the
+      // small chance that CrOS fails to remove the quick unlock capability. See
+      // https://crbug.com/1054327 for details.
+      this.hasPin = false;
       this.setModes_.call(null, [], [], function(result) {
         assert(result, 'Failed to clear quick unlock modes');
-        if (!result) {
-          console.error('Failed to clear quick unlock modes');
-        }
+        // Revert |hasPin| to true in the event setModes fails to set lock state
+        // to PASSWORD only.
+        this.hasPin = true;
       });
     }
   },

chrome/browser/resources/settings/chromeos/os_people_page/lock_state_behavior.js

@@ -63,7 +63,8 @@ const LockStateBehaviorImpl = {
 
   /** @override */
   attached() {
-    this.boundOnActiveModesChanged_ = this.updateUnlockType.bind(this);
+    this.boundOnActiveModesChanged_ =
+        this.updateUnlockType.bind(this, /*activeModesChanged=*/ true);
     this.quickUnlockPrivate.onActiveModesChanged.addListener(
         this.boundOnActiveModesChanged_);
 
@@ -77,7 +78,7 @@ const LockStateBehaviorImpl = {
       this.hasPinLogin = cachedHasPinLogin;
     }
 
-    this.updateUnlockType();
+    this.updateUnlockType(/*activeModesChanged=*/ false);
   },
 
   /** @override */
@@ -90,13 +91,32 @@ const LockStateBehaviorImpl = {
    * Updates the selected unlock type radio group. This function will get called
    * after preferences are initialized, after the quick unlock mode has been
    * changed, and after the lockscreen preference has changed.
+   *
+   * @param {boolean} activeModesChanged If the function is called because
+   *     active modes have changed.
    */
-  updateUnlockType() {
+  updateUnlockType(activeModesChanged) {
     this.quickUnlockPrivate.getActiveModes(modes => {
       if (modes.includes(chrome.quickUnlockPrivate.QuickUnlockMode.PIN)) {
         this.hasPin = true;
         this.selectedUnlockType = LockScreenUnlockType.PIN_PASSWORD;
       } else {
+        // A race condition can occur:
+        // (1) User selects PIN_PASSSWORD, and successfully sets a pin, adding
+        //     QuickUnlockMode.PIN to active modes.
+        // (2) User selects PASSWORD, QuickUnlockMode.PIN capability is cleared
+        //     from the active modes, notifying LockStateBehavior to call
+        //     updateUnlockType to fetch the active modes asynchronously.
+        // (3) User selects PIN_PASSWORD, but the process from step 2 has
+        //     not yet completed.
+        // In this case, do not forcibly select the PASSWORD radio button even
+        // though the unlock type is still PASSWORD (|hasPin| is false). If the
+        // user wishes to set a pin, they will have to click the set pin button.
+        // See https://crbug.com/1054327 for details.
+        if (activeModesChanged && !this.hasPin &&
+            this.selectedUnlockType == LockScreenUnlockType.PIN_PASSWORD) {
+          return;
+        }
         this.hasPin = false;
         this.selectedUnlockType = LockScreenUnlockType.PASSWORD;
       }

chrome/test/data/webui/settings/chromeos/quick_unlock_authenticate_browsertest_chromeos.js

@@ -389,6 +389,39 @@ cr.define('settings_people_page_quick_unlock', function() {
         assertDeepEquals([], quickUnlockPrivateApi.activeModes);
       });
 
+      // Tests correct UI conflict resolution in the event of a race condition
+      // that may occur when:
+      // (1) User selects PIN_PASSSWORD, and successfully sets a pin, adding
+      //     QuickUnlockMode.PIN to active modes.
+      // (2) User selects PASSWORD, QuickUnlockMode.PIN capability is cleared
+      //     from the active modes, notifying LockStateBehavior to call
+      //     updateUnlockType to fetch the active modes asynchronously.
+      // (3) User selects PIN_PASSWORD, but the process from step 2 has
+      //     not yet completed.
+      // See https://crbug.com/1054327 for details.
+      test('UserSelectsPinBeforePasswordOnlyStateSet', function() {
+        setActiveModes([QuickUnlockMode.PIN]);
+        assertRadioButtonChecked(pinPasswordRadioButton);
+        assertTrue(isSetupPinButtonVisible());
+        Polymer.dom.flush();
+        assertEquals(testElement.$$('#setupPinButton').innerText, 'Change PIN');
+
+        // Clicking will trigger an async call which setActiveModes([]) fakes.
+        passwordRadioButton.click();
+        assertFalse(isSetupPinButtonVisible());
+
+        pinPasswordRadioButton.click();
+        assertTrue(isSetupPinButtonVisible());
+
+        // Simulate the state change to PASSWORD after selecting PIN radio.
+        setActiveModes([]);
+
+        Polymer.dom.flush();
+        assertRadioButtonChecked(pinPasswordRadioButton);
+        assertTrue(isSetupPinButtonVisible());
+        assertEquals(testElement.$$('#setupPinButton').innerText, 'Set up PIN');
+      });
+
       // Tapping the PIN configure button opens up the setup PIN dialog, and
       // records a chose pin or password uma.
       test('TappingConfigureOpensSetupPin', function() {