From cb03d794224ceb8ea3521cfa123961c6a5e7cd61 Mon Sep 17 00:00:00 2001
From: Kevin Chyn <kchyn@google.com>
Date: Fri, 13 Nov 2020 11:45:29 -0800
Subject: [PATCH] Un-overload Biometric UI negative button

Use different buttons for different actions. Makes code easier to
read, and also easier to test.

Bug: 171357779
Test: Demo app
Test: atest com.android.systemui.biometrics

Change-Id: I77ed66b3e387cc8e1b0c13d908425bff5b85bec4
---
 .../res/layout/auth_biometric_contents.xml    | 25 ++++-
 .../biometrics/AuthBiometricFaceView.java     |  2 +-
 .../biometrics/AuthBiometricView.java         | 99 +++++++++++++------
 .../biometrics/AuthBiometricFaceViewTest.java | 13 ++-
 .../biometrics/AuthBiometricViewTest.java     | 69 ++++++++++---
 5 files changed, 160 insertions(+), 48 deletions(-)

diff --git a/packages/SystemUI/res/layout/auth_biometric_contents.xml b/packages/SystemUI/res/layout/auth_biometric_contents.xml
index 2439fedd05182..aed067c302536 100644
--- a/packages/SystemUI/res/layout/auth_biometric_contents.xml
+++ b/packages/SystemUI/res/layout/auth_biometric_contents.xml
@@ -88,7 +88,8 @@
             android:layout_width="8dp"
             android:layout_height="match_parent"
             android:visibility="visible" />
-        <!-- Negative Button -->
+
+        <!-- Negative Button, reserved for app -->
         <Button android:id="@+id/button_negative"
             android:layout_width="wrap_content"
             android:layout_height="wrap_content"
@@ -97,13 +98,32 @@
             android:ellipsize="end"
             android:maxLines="2"
             android:maxWidth="@dimen/biometric_dialog_button_negative_max_width"/>
+        <!-- Cancel Button, replaces negative button when biometric is accepted -->
+        <Button android:id="@+id/button_cancel"
+            android:layout_width="wrap_content"
+            android:layout_height="wrap_content"
+            style="@*android:style/Widget.DeviceDefault.Button.Borderless.Colored"
+            android:layout_gravity="center_vertical"
+            android:maxWidth="@dimen/biometric_dialog_button_negative_max_width"
+            android:text="@string/cancel"
+            android:visibility="gone"/>
+        <!-- "Use Credential" Button, replaces if device credential is allowed -->
+        <Button android:id="@+id/button_use_credential"
+            android:layout_width="wrap_content"
+            android:layout_height="wrap_content"
+            style="@*android:style/Widget.DeviceDefault.Button.Borderless.Colored"
+            android:layout_gravity="center_vertical"
+            android:maxWidth="@dimen/biometric_dialog_button_negative_max_width"
+            android:visibility="gone"/>
+
         <Space android:id="@+id/middleSpacer"
             android:layout_width="0dp"
             android:layout_height="match_parent"
             android:layout_weight="1"
             android:visibility="visible"/>
+
         <!-- Positive Button -->
-        <Button android:id="@+id/button_positive"
+        <Button android:id="@+id/button_confirm"
             android:layout_width="wrap_content"
             android:layout_height="wrap_content"
             style="@*android:style/Widget.DeviceDefault.Button.Colored"
@@ -124,6 +144,7 @@
             android:maxWidth="@dimen/biometric_dialog_button_positive_max_width"
             android:text="@string/biometric_dialog_try_again"
             android:visibility="gone"/>
+
         <Space android:id="@+id/rightSpacer"
             android:layout_width="8dp"
             android:layout_height="match_parent"
diff --git a/packages/SystemUI/src/com/android/systemui/biometrics/AuthBiometricFaceView.java b/packages/SystemUI/src/com/android/systemui/biometrics/AuthBiometricFaceView.java
index 9a40541ac754e..e4f6d6cc68874 100644
--- a/packages/SystemUI/src/com/android/systemui/biometrics/AuthBiometricFaceView.java
+++ b/packages/SystemUI/src/com/android/systemui/biometrics/AuthBiometricFaceView.java
@@ -196,7 +196,7 @@ public class AuthBiometricFaceView extends AuthBiometricView {
     public void onAuthenticationFailed(String failureReason) {
         if (getSize() == AuthDialog.SIZE_MEDIUM) {
             mTryAgainButton.setVisibility(View.VISIBLE);
-            mPositiveButton.setVisibility(View.GONE);
+            mConfirmButton.setVisibility(View.GONE);
         }
 
         // Do this last since wa want to know if the button is being animated (in the case of
diff --git a/packages/SystemUI/src/com/android/systemui/biometrics/AuthBiometricView.java b/packages/SystemUI/src/com/android/systemui/biometrics/AuthBiometricView.java
index 0608ca236f86d..c748ab21b8223 100644
--- a/packages/SystemUI/src/com/android/systemui/biometrics/AuthBiometricView.java
+++ b/packages/SystemUI/src/com/android/systemui/biometrics/AuthBiometricView.java
@@ -116,8 +116,16 @@ public abstract class AuthBiometricView extends LinearLayout {
             return mBiometricView.findViewById(R.id.button_negative);
         }
 
-        public Button getPositiveButton() {
-            return mBiometricView.findViewById(R.id.button_positive);
+        public Button getCancelButton() {
+            return mBiometricView.findViewById(R.id.button_cancel);
+        }
+
+        public Button getUseCredentialButton() {
+            return mBiometricView.findViewById(R.id.button_use_credential);
+        }
+
+        public Button getConfirmButton() {
+            return mBiometricView.findViewById(R.id.button_confirm);
         }
 
         public Button getTryAgainButton() {
@@ -176,8 +184,16 @@ public abstract class AuthBiometricView extends LinearLayout {
     private View mIconHolderView;
     protected ImageView mIconView;
     @VisibleForTesting protected TextView mIndicatorView;
+
+    // Negative button position, exclusively for the app-specified behavior
     @VisibleForTesting Button mNegativeButton;
-    @VisibleForTesting Button mPositiveButton;
+    // Negative button position, exclusively for cancelling auth after passive auth success
+    @VisibleForTesting Button mCancelButton;
+    // Negative button position, shown if device credentials are allowed
+    @VisibleForTesting Button mUseCredentialButton;
+
+    // Positive button position,
+    @VisibleForTesting Button mConfirmButton;
     @VisibleForTesting Button mTryAgainButton;
 
     // Measurements when biometric view is showing text, buttons, etc.
@@ -303,6 +319,7 @@ public abstract class AuthBiometricView extends LinearLayout {
             mDescriptionView.setVisibility(View.GONE);
             mIndicatorView.setVisibility(View.GONE);
             mNegativeButton.setVisibility(View.GONE);
+            mUseCredentialButton.setVisibility(View.GONE);
 
             final float iconPadding = getResources()
                     .getDimension(R.dimen.biometric_dialog_icon_padding);
@@ -336,6 +353,7 @@ public abstract class AuthBiometricView extends LinearLayout {
                 mTitleView.setAlpha(opacity);
                 mIndicatorView.setAlpha(opacity);
                 mNegativeButton.setAlpha(opacity);
+                mCancelButton.setAlpha(opacity);
                 mTryAgainButton.setAlpha(opacity);
 
                 if (!TextUtils.isEmpty(mSubtitleView.getText())) {
@@ -355,7 +373,12 @@ public abstract class AuthBiometricView extends LinearLayout {
                     super.onAnimationStart(animation);
                     mTitleView.setVisibility(View.VISIBLE);
                     mIndicatorView.setVisibility(View.VISIBLE);
-                    mNegativeButton.setVisibility(View.VISIBLE);
+
+                    if (isDeviceCredentialAllowed()) {
+                        mUseCredentialButton.setVisibility(View.VISIBLE);
+                    } else {
+                        mNegativeButton.setVisibility(View.VISIBLE);
+                    }
                     mTryAgainButton.setVisibility(View.VISIBLE);
 
                     if (!TextUtils.isEmpty(mSubtitleView.getText())) {
@@ -447,15 +470,17 @@ public abstract class AuthBiometricView extends LinearLayout {
             case STATE_AUTHENTICATING:
                 removePendingAnimations();
                 if (mRequireConfirmation) {
-                    mPositiveButton.setEnabled(false);
-                    mPositiveButton.setVisibility(View.VISIBLE);
+                    mConfirmButton.setEnabled(false);
+                    mConfirmButton.setVisibility(View.VISIBLE);
                 }
                 break;
 
             case STATE_AUTHENTICATED:
                 if (mSize != AuthDialog.SIZE_SMALL) {
-                    mPositiveButton.setVisibility(View.GONE);
+                    mConfirmButton.setVisibility(View.GONE);
                     mNegativeButton.setVisibility(View.GONE);
+                    mUseCredentialButton.setVisibility(View.GONE);
+                    mCancelButton.setVisibility(View.GONE);
                     mIndicatorView.setVisibility(View.INVISIBLE);
                 }
                 announceForAccessibility(getResources()
@@ -466,10 +491,11 @@ public abstract class AuthBiometricView extends LinearLayout {
 
             case STATE_PENDING_CONFIRMATION:
                 removePendingAnimations();
-                mNegativeButton.setText(R.string.cancel);
-                mNegativeButton.setContentDescription(getResources().getString(R.string.cancel));
-                mPositiveButton.setEnabled(true);
-                mPositiveButton.setVisibility(View.VISIBLE);
+                mNegativeButton.setVisibility(View.GONE);
+                mCancelButton.setVisibility(View.VISIBLE);
+                mUseCredentialButton.setVisibility(View.GONE);
+                mConfirmButton.setEnabled(true);
+                mConfirmButton.setVisibility(View.VISIBLE);
                 mIndicatorView.setTextColor(mTextColorHint);
                 mIndicatorView.setText(R.string.biometric_dialog_tap_confirm);
                 mIndicatorView.setVisibility(View.VISIBLE);
@@ -595,23 +621,29 @@ public abstract class AuthBiometricView extends LinearLayout {
         mIconView = mInjector.getIconView();
         mIconHolderView = mInjector.getIconHolderView();
         mIndicatorView = mInjector.getIndicatorView();
+
+        // Negative-side (left) buttons
         mNegativeButton = mInjector.getNegativeButton();
-        mPositiveButton = mInjector.getPositiveButton();
+        mCancelButton = mInjector.getCancelButton();
+        mUseCredentialButton = mInjector.getUseCredentialButton();
+
+        // Positive-side (right) buttons
+        mConfirmButton = mInjector.getConfirmButton();
         mTryAgainButton = mInjector.getTryAgainButton();
 
         mNegativeButton.setOnClickListener((view) -> {
-            if (mState == STATE_PENDING_CONFIRMATION) {
-                mCallback.onAction(Callback.ACTION_USER_CANCELED);
-            } else {
-                if (isDeviceCredentialAllowed()) {
-                    startTransitionToCredentialUI();
-                } else {
-                    mCallback.onAction(Callback.ACTION_BUTTON_NEGATIVE);
-                }
-            }
+            mCallback.onAction(Callback.ACTION_BUTTON_NEGATIVE);
         });
 
-        mPositiveButton.setOnClickListener((view) -> {
+        mCancelButton.setOnClickListener((view) -> {
+            mCallback.onAction(Callback.ACTION_USER_CANCELED);
+        });
+
+        mUseCredentialButton.setOnClickListener((view) -> {
+            startTransitionToCredentialUI();
+        });
+
+        mConfirmButton.setOnClickListener((view) -> {
             updateState(STATE_AUTHENTICATED);
         });
 
@@ -645,31 +677,36 @@ public abstract class AuthBiometricView extends LinearLayout {
     void onAttachedToWindowInternal() {
         setText(mTitleView, mPromptInfo.getTitle());
 
-        final CharSequence negativeText;
         if (isDeviceCredentialAllowed()) {
-
+            final CharSequence credentialButtonText;
             final @Utils.CredentialType int credentialType =
                     Utils.getCredentialType(mContext, mEffectiveUserId);
-
             switch (credentialType) {
                 case Utils.CREDENTIAL_PIN:
-                    negativeText = getResources().getString(R.string.biometric_dialog_use_pin);
+                    credentialButtonText =
+                            getResources().getString(R.string.biometric_dialog_use_pin);
                     break;
                 case Utils.CREDENTIAL_PATTERN:
-                    negativeText = getResources().getString(R.string.biometric_dialog_use_pattern);
+                    credentialButtonText =
+                            getResources().getString(R.string.biometric_dialog_use_pattern);
                     break;
                 case Utils.CREDENTIAL_PASSWORD:
-                    negativeText = getResources().getString(R.string.biometric_dialog_use_password);
+                    credentialButtonText =
+                            getResources().getString(R.string.biometric_dialog_use_password);
                     break;
                 default:
-                    negativeText = getResources().getString(R.string.biometric_dialog_use_password);
+                    credentialButtonText =
+                            getResources().getString(R.string.biometric_dialog_use_password);
                     break;
             }
 
+            mNegativeButton.setVisibility(View.GONE);
+
+            mUseCredentialButton.setText(credentialButtonText);
+            mUseCredentialButton.setVisibility(View.VISIBLE);
         } else {
-            negativeText = mPromptInfo.getNegativeButtonText();
+            setText(mNegativeButton, mPromptInfo.getNegativeButtonText());
         }
-        setText(mNegativeButton, negativeText);
 
         setTextOrHide(mSubtitleView, mPromptInfo.getSubtitle());
 
diff --git a/packages/SystemUI/tests/src/com/android/systemui/biometrics/AuthBiometricFaceViewTest.java b/packages/SystemUI/tests/src/com/android/systemui/biometrics/AuthBiometricFaceViewTest.java
index b907cdb54bbff..043bd5cd6ba5f 100644
--- a/packages/SystemUI/tests/src/com/android/systemui/biometrics/AuthBiometricFaceViewTest.java
+++ b/packages/SystemUI/tests/src/com/android/systemui/biometrics/AuthBiometricFaceViewTest.java
@@ -50,8 +50,12 @@ public class AuthBiometricFaceViewTest extends SysuiTestCase {
     private TestableFaceView mFaceView;
 
     @Mock private Button mNegativeButton;
-    @Mock private Button mPositiveButton;
+    @Mock private Button mCancelButton;
+    @Mock private Button mUseCredentialButton;
+
+    @Mock private Button mConfirmButton;
     @Mock private Button mTryAgainButton;
+
     @Mock private TextView mErrorView;
 
     @Before
@@ -60,9 +64,14 @@ public class AuthBiometricFaceViewTest extends SysuiTestCase {
         mFaceView = new TestableFaceView(mContext);
         mFaceView.mIconController = mock(TestableFaceView.TestableIconController.class);
         mFaceView.setCallback(mCallback);
+
         mFaceView.mNegativeButton = mNegativeButton;
-        mFaceView.mPositiveButton = mPositiveButton;
+        mFaceView.mCancelButton = mCancelButton;
+        mFaceView.mUseCredentialButton = mUseCredentialButton;
+
+        mFaceView.mConfirmButton = mConfirmButton;
         mFaceView.mTryAgainButton = mTryAgainButton;
+
         mFaceView.mIndicatorView = mErrorView;
     }
 
diff --git a/packages/SystemUI/tests/src/com/android/systemui/biometrics/AuthBiometricViewTest.java b/packages/SystemUI/tests/src/com/android/systemui/biometrics/AuthBiometricViewTest.java
index e2517f27c0ea0..49282ee360e25 100644
--- a/packages/SystemUI/tests/src/com/android/systemui/biometrics/AuthBiometricViewTest.java
+++ b/packages/SystemUI/tests/src/com/android/systemui/biometrics/AuthBiometricViewTest.java
@@ -60,8 +60,12 @@ public class AuthBiometricViewTest extends SysuiTestCase {
     @Mock private AuthPanelController mPanelController;
 
     @Mock private Button mNegativeButton;
+    @Mock private Button mCancelButton;
+    @Mock private Button mUseCredentialButton;
+
     @Mock private Button mPositiveButton;
     @Mock private Button mTryAgainButton;
+
     @Mock private TextView mTitleView;
     @Mock private TextView mSubtitleView;
     @Mock private TextView mDescriptionView;
@@ -89,15 +93,31 @@ public class AuthBiometricViewTest extends SysuiTestCase {
 
     @Test
     public void testOnAuthenticationSucceeded_confirmationRequired_updatesDialogContents() {
-        initDialog(mContext, false /* allowDeviceCredential */, mCallback, new MockInjector());
+        final Button negativeButton = new Button(mContext);
+        final Button cancelButton = new Button(mContext);
+        initDialog(mContext, false /* allowDeviceCredential */, mCallback, new MockInjector() {
+            @Override
+            public Button getNegativeButton() {
+                return negativeButton;
+            }
+
+            @Override
+            public Button getCancelButton() {
+                return cancelButton;
+            }
+        });
 
         mBiometricView.setRequireConfirmation(true);
         mBiometricView.onAuthenticationSucceeded();
         waitForIdleSync();
         assertEquals(AuthBiometricView.STATE_PENDING_CONFIRMATION, mBiometricView.mState);
         verify(mCallback, never()).onAction(anyInt());
-        verify(mBiometricView.mNegativeButton).setText(eq(R.string.cancel));
-        verify(mBiometricView.mPositiveButton).setEnabled(eq(true));
+
+        assertEquals(View.GONE, negativeButton.getVisibility());
+        assertEquals(View.VISIBLE, cancelButton.getVisibility());
+        assertTrue(cancelButton.isEnabled());
+
+        verify(mBiometricView.mConfirmButton).setEnabled(eq(true));
         verify(mIndicatorView).setText(eq(R.string.biometric_dialog_tap_confirm));
         verify(mIndicatorView).setVisibility(eq(View.VISIBLE));
     }
@@ -107,7 +127,7 @@ public class AuthBiometricViewTest extends SysuiTestCase {
         Button button = new Button(mContext);
         initDialog(mContext, false /* allowDeviceCredential */, mCallback, new MockInjector() {
            @Override
-            public Button getPositiveButton() {
+            public Button getConfirmButton() {
                return button;
            }
         });
@@ -137,18 +157,26 @@ public class AuthBiometricViewTest extends SysuiTestCase {
     }
 
     @Test
-    public void testNegativeButton_whenPendingConfirmation_sendsActionUserCanceled() {
-        Button button = new Button(mContext);
+    public void testCancelButton_whenPendingConfirmation_sendsActionUserCanceled() {
+        Button cancelButton = new Button(mContext);
+        Button negativeButton = new Button(mContext);
         initDialog(mContext, false /* allowDeviceCredential */, mCallback, new MockInjector() {
             @Override
             public Button getNegativeButton() {
-                return button;
+                return negativeButton;
+            }
+            @Override
+            public Button getCancelButton() {
+                return cancelButton;
             }
         });
 
         mBiometricView.setRequireConfirmation(true);
         mBiometricView.onAuthenticationSucceeded();
-        button.performClick();
+
+        assertEquals(View.GONE, negativeButton.getVisibility());
+
+        cancelButton.performClick();
         waitForIdleSync();
 
         verify(mCallback).onAction(AuthBiometricView.Callback.ACTION_USER_CANCELED);
@@ -282,16 +310,23 @@ public class AuthBiometricViewTest extends SysuiTestCase {
     }
 
     @Test
-    public void testNegativeButton_whenDeviceCredentialAllowed() {
-        Button negativeButton = new Button(mContext);
+    public void testCredentialButton_whenDeviceCredentialAllowed() {
+        final Button negativeButton = new Button(mContext);
+        final Button useCredentialButton = new Button(mContext);
         initDialog(mContext, true /* allowDeviceCredential */, mCallback, new MockInjector() {
             @Override
             public Button getNegativeButton() {
                 return negativeButton;
             }
+
+            @Override
+            public Button getUseCredentialButton() {
+                return useCredentialButton;
+            }
         });
 
-        negativeButton.performClick();
+        assertEquals(View.GONE, negativeButton.getVisibility());
+        useCredentialButton.performClick();
         waitForIdleSync();
 
         verify(mCallback).onAction(AuthBiometricView.Callback.ACTION_USE_DEVICE_CREDENTIAL);
@@ -361,7 +396,17 @@ public class AuthBiometricViewTest extends SysuiTestCase {
         }
 
         @Override
-        public Button getPositiveButton() {
+        public Button getCancelButton() {
+            return mCancelButton;
+        }
+
+        @Override
+        public Button getUseCredentialButton() {
+            return mUseCredentialButton;
+        }
+
+        @Override
+        public Button getConfirmButton() {
             return mPositiveButton;
         }