Redesign sign in and sign in confirmation screens for Narnia 2.0

chrome/android/java/src/org/chromium/chrome/browser/signin/AccountSigninView.java

Index: chrome/android/java/src/org/chromium/chrome/browser/signin/AccountSigninView.java
diff --git a/chrome/android/java/src/org/chromium/chrome/browser/signin/AccountSigninView.java b/chrome/android/java/src/org/chromium/chrome/browser/signin/AccountSigninView.java
index 4cd73c362e7118fb3cd3e6084e4fe2c4cbc85628..9af8eba6efc03946e52380241b6c3a43fd29ef3e 100644
--- a/chrome/android/java/src/org/chromium/chrome/browser/signin/AccountSigninView.java
+++ b/chrome/android/java/src/org/chromium/chrome/browser/signin/AccountSigninView.java
@@ -7,26 +7,22 @@ package org.chromium.chrome.browser.signin;
 import android.app.FragmentManager;
 import android.content.Context;
 import android.graphics.Bitmap;
-import android.graphics.drawable.Drawable;
-import android.os.Bundle;
 import android.text.TextUtils;
 import android.text.method.LinkMovementMethod;
 import android.util.AttributeSet;
+import android.view.LayoutInflater;
+import android.view.MotionEvent;
 import android.view.View;
 import android.widget.AdapterView;
-import android.widget.ArrayAdapter;
 import android.widget.Button;
+import android.widget.FrameLayout;
 import android.widget.ImageView;
-import android.widget.LinearLayout;
-import android.widget.Spinner;
+import android.widget.ListView;
 import android.widget.TextView;
 
 import org.chromium.base.ApiCompatibilityUtils;
 import org.chromium.base.metrics.RecordUserAction;
 import org.chromium.chrome.R;
-import org.chromium.chrome.browser.firstrun.FirstRunView;
-import org.chromium.chrome.browser.firstrun.ImageCarousel;
-import org.chromium.chrome.browser.firstrun.ImageCarousel.ImageCarouselPositionChangeListener;
 import org.chromium.chrome.browser.firstrun.ProfileDataCache;
 import org.chromium.chrome.browser.preferences.PrefServiceBridge;
 import org.chromium.chrome.browser.profiles.ProfileDownloader;
@@ -38,9 +34,13 @@ import org.chromium.sync.signin.AccountManagerHelper;
 import org.chromium.ui.text.NoUnderlineClickableSpan;
 import org.chromium.ui.text.SpanApplier;
 import org.chromium.ui.text.SpanApplier.SpanInfo;
+import org.chromium.ui.widget.ButtonCompat;
 
 import java.util.List;
 
+// TODO(gogerald): add lanscape mode (http://crbug.com/599139).
+// TODO(gogerald): refactor common part into one place after redesgin all sign in screens.
+
 /**
  * This view allows the user to select an account to log in to, add an account,
  * cancel account selection, etc. Users of this class should
@@ -48,9 +48,8 @@ import java.util.List;
  * {@link AccountSigninView#setDelegate(Delegate)} after the view has been inflated.
  */
 
-public class AccountSigninView extends FirstRunView
-        implements ImageCarouselPositionChangeListener, ProfileDownloader.Observer {
-
+public class AccountSigninView
+        extends FrameLayout implements AdapterView.OnItemClickListener, ProfileDownloader.Observer {
     /**
      * Callbacks for various account selection events.
      */
@@ -92,66 +91,36 @@ public class AccountSigninView extends FirstRunView
         public FragmentManager getFragmentManager();
     }
 
-    private class SpinnerOnItemSelectedListener implements AdapterView.OnItemSelectedListener {
-        @Override
-        public void onItemSelected(AdapterView<?> parent, View view, int pos, long id) {
-            String accountName = parent.getItemAtPosition(pos).toString();
-            if (accountName.equals(mAddAnotherAccount)) {
-                // Don't allow "add account" to remain selected. http://crbug.com/421052
-                int oldPosition = mArrayAdapter.getPosition(mAccountName);
-                if (oldPosition == -1) oldPosition = 0;
-                mSpinner.setSelection(oldPosition, false);
-
-                mListener.onNewAccount();
-                RecordUserAction.record("Signin_AddAccountToDevice");
-            } else {
-                mAccountName = accountName;
-                if (!mPositionSetProgrammatically) mImageCarousel.scrollTo(pos, false, false);
-                mPositionSetProgrammatically = false;
-            }
-        }
-        @Override
-        public void onNothingSelected(AdapterView<?> parent) {
-            mAccountName = parent.getItemAtPosition(0).toString();
-        }
-    }
-
-    private static final String TAG = "AccountFirstRunView";
-
-    private static final int EXPERIMENT_TITLE_VARIANT_MASK = 1;
-    private static final int EXPERIMENT_SUMMARY_VARIANT_MASK = 2;
-    private static final int EXPERIMENT_LAYOUT_VARIANT_MASK = 4;
-    private static final int EXPERIMENT_MAX_VALUE = 7;
+    private static final String TAG = "AccountSigninView";
 
     private static final String SETTINGS_LINK_OPEN = "<LINK1>";
     private static final String SETTINGS_LINK_CLOSE = "</LINK1>";
 
     private AccountManagerHelper mAccountManagerHelper;
     private List<String> mAccountNames;
-    private ArrayAdapter<CharSequence> mArrayAdapter;
-    private ImageCarousel mImageCarousel;
+    private View mSigninView;
+    private AccountListAdapter mAccountListAdapter;
+    private ListView mAccountListView;
     private Button mPositiveButton;
     private Button mNegativeButton;
     private TextView mTitle;
     private TextView mDescriptionText;
     private Listener mListener;
     private Delegate mDelegate;
-    private Spinner mSpinner;
-    private TextView mConfirmAccountEmail;
-    private Drawable mSpinnerBackground;
     private String mForcedAccountName;
-    private String mAccountName;
     private String mAddAnotherAccount;
     private ProfileDataCache mProfileData;
     private boolean mSignedIn;
-    private boolean mPositionSetProgrammatically;
-    private int mDescriptionTextId;
     private int mCancelButtonTextId;
     private boolean mIsChildAccount;
-    private boolean mHorizontalModeEnabled = true;
-    private boolean mDynamicPaddingEnabled = true;
     private boolean mShowSettingsSpan = true;
 
+    private AccountSigninConfirmationView mSigninConfirmationView;
+    private ImageView mSigninAccountImage;
+    private TextView mSigninAccountName;
+    private TextView mSigninAccountEmail;
+    private TextView mSigninChromeSyncDescription;
+
     public AccountSigninView(Context context, AttributeSet attrs) {
         super(context, attrs);
         mAccountManagerHelper = AccountManagerHelper.get(getContext().getApplicationContext());
@@ -162,16 +131,9 @@ public class AccountSigninView extends FirstRunView
      * @param profileData ProfileDataCache that will be used to call to retrieve user account info.
      */
     public void init(ProfileDataCache profileData) {
-        setProfileDataCache(profileData);
-    }
-
-    /**
-     * Sets the profile data cache.
-     * @param profileData ProfileDataCache that will be used to call to retrieve user account info.
-     */
-    public void setProfileDataCache(ProfileDataCache profileData) {
         mProfileData = profileData;
         mProfileData.setObserver(this);
+        mAccountListAdapter.setProfileDataCache(mProfileData);
         updateProfileImages();
     }
 
@@ -179,10 +141,48 @@ public class AccountSigninView extends FirstRunView
     protected void onFinishInflate() {
         super.onFinishInflate();
 
-        mImageCarousel = (ImageCarousel) findViewById(R.id.image);
-        mImageCarousel.setListener(this);
+        mSigninView = (View) findViewById(R.id.signin);

[newt (away), 2016/04/01 23:43:11]

unnecessary cast

[gogerald1, 2016/04/12 01:21:13]

Done.

+
+        mTitle = (TextView) findViewById(R.id.signin_title);
+
+        mAccountListView = (ListView) findViewById(R.id.signin_account_list);
+        mAccountListView.setOnItemClickListener(this);
+        mAccountListAdapter = new AccountListAdapter(getContext().getApplicationContext());

[newt (away), 2016/04/01 23:43:12]

Remove "getApplicationContext()". There's no need for it and using the activity
context is generally preferable since the activity has a theme applied to it and
the application context doesn't (so styles will be broken).

[gogerald1, 2016/04/12 01:21:13]

Done.

+        mAccountListView.setAdapter(mAccountListAdapter);
+        mDescriptionText =

[newt (away), 2016/04/01 23:43:11]

This variable name is vague. What is it? Can you describe it better, or at least
add a comment above the variable's declaration?

[gogerald1, 2016/04/12 01:21:13]

Done.

+                (TextView) LayoutInflater.from(getContext().getApplicationContext())
+                        .inflate(R.layout.account_signin_choice_description_view, null, false);
+        mDescriptionText.setOnClickListener(null);
+        mAccountListView.addHeaderView(mDescriptionText);
+
+        // Once the user has touched this ListView, prevent the parent view from handling touch
+        // events. This allows the ListView to behave reasonably when nested inside a ListView.

[newt (away), 2016/04/01 23:43:11]

?? Why is a ListView nested inside a ListView? What is this referring to? This
doesn't seem needed, and if it is, that's a red flag.

[gogerald1, 2016/04/12 01:21:13]

Currently, it has been used in recent tabs view which is a expandable list view,
but we have plan to change it, so add a TODO here.

[newt (away), 2016/04/13 18:13:30]

Thanks for explaining. And yes, please remove this soon :)

+        mAccountListView.setOnTouchListener(new ListView.OnTouchListener() {
+            @Override
+            public boolean onTouch(View view, MotionEvent event) {
+                int action = event.getAction();
+                switch (action) {
+                    case MotionEvent.ACTION_DOWN:
+                        view.getParent().requestDisallowInterceptTouchEvent(true);
+                        break;
+
+                    case MotionEvent.ACTION_UP:
+                        view.performClick();
+                        view.getParent().requestDisallowInterceptTouchEvent(false);
+                        break;
+                    default:
+                        // Ignore.
+                        break;
+                }
+
+                view.onTouchEvent(event);
+                return true;
+            }
+        });
 
         mPositiveButton = (Button) findViewById(R.id.positive_button);

[newt (away), 2016/04/01 23:43:12]

just cast to ButtonCompat here and change mPositiveButton's type to
ButtonCompat.

[gogerald1, 2016/04/12 01:21:13]

Done.

+        // Remove drop shadow effect.
+        ((ButtonCompat) mPositiveButton).setRaised(false);
         mNegativeButton = (Button) findViewById(R.id.negative_button);
 
         // A workaround for Android support library ignoring padding set in XML. b/20307607
@@ -190,48 +190,51 @@ public class AccountSigninView extends FirstRunView
         ApiCompatibilityUtils.setPaddingRelative(mPositiveButton, padding, 0, padding, 0);
         ApiCompatibilityUtils.setPaddingRelative(mNegativeButton, padding, 0, padding, 0);
 
-        mTitle = (TextView) findViewById(R.id.title);
-        mDescriptionText = (TextView) findViewById(R.id.description);
-        // For the spans to be clickable.
-        mDescriptionText.setMovementMethod(LinkMovementMethod.getInstance());
-        mDescriptionTextId = R.string.signin_account_choice_description;
-
         // TODO(peconn): Ensure this is changed to R.string.cancel when used in Settings > Sign In.
         mCancelButtonTextId = R.string.no_thanks;
 
         mAddAnotherAccount = getResources().getString(R.string.signin_add_account);
 
-        mConfirmAccountEmail = (TextView) findViewById(R.id.confirm_account_email);
-        mSpinner = (Spinner) findViewById(R.id.google_accounts_spinner);
-        mSpinnerBackground = mSpinner.getBackground();
-        mArrayAdapter = new ArrayAdapter<CharSequence>(getContext().getApplicationContext(),
-                R.layout.fre_spinner_text);
-
-        mArrayAdapter.setDropDownViewResource(R.layout.fre_spinner_dropdown);
-        mSpinner.setAdapter(mArrayAdapter);
-        mSpinner.setOnItemSelectedListener(new SpinnerOnItemSelectedListener());
-
-        // Only set the spinner's content description right before the accessibility action is going
-        // to be performed. Otherwise, the the content description is read when the
-        // AccountFirstRunView is created because setting the spinner's adapter causes a
-        // TYPE_VIEW_SELECTED event. ViewPager loads the next and previous pages according to
-        // it's off-screen page limit, which is one by default, so without this the content
-        // description ends up being read when the card before this one shown.
-        mSpinner.setAccessibilityDelegate(new AccessibilityDelegate() {
+        mSigninConfirmationView =
+                (AccountSigninConfirmationView) findViewById(R.id.signin_confirmation_layout);
+        mSigninConfirmationView.setObserver(new AccountSigninConfirmationView.Observer() {
             @Override
-            public boolean performAccessibilityAction(View host, int action, Bundle args) {
-                if (mSpinner.getContentDescription() == null) {
-                    mSpinner.setContentDescription(getResources().getString(
-                            R.string.accessibility_signin_account_spinner));
+            public void onPersonalizeServiceCheckBoxVisibilityChanged(boolean isVisible) {
+                if (isVisible) {
+                    setPositiveButtonActive();
+                } else {
+                    setPositiveButtonInActive();
                 }
-                return super.performAccessibilityAction(host, action, args);
             }
         });
 
+        mSigninAccountImage = (ImageView) findViewById(R.id.signin_account_image);
+        mSigninAccountName = (TextView) findViewById(R.id.signin_account_name);
+        mSigninAccountEmail = (TextView) findViewById(R.id.signin_account_email);
+        mSigninChromeSyncDescription = (TextView) findViewById(R.id.signin_chrome_sync_description);
+        // For the spans to be clickable.
+        mSigninChromeSyncDescription.setMovementMethod(LinkMovementMethod.getInstance());
+
         showSigninPage();
     }
 
     @Override
+    protected void onMeasure(int widthMeasureSpec, int heightMeasureSpec) {
+        // This assumes that view's layout_width is set to match_parent.
+        assert MeasureSpec.getMode(widthMeasureSpec) == MeasureSpec.EXACTLY;

[newt (away), 2016/04/01 23:43:11]

add a similar assert for the heigh

[gogerald1, 2016/04/12 01:21:13]

Done.

+
+        int width = MeasureSpec.getSize(widthMeasureSpec);
+        int height = MeasureSpec.getSize(heightMeasureSpec);
+
+        // Sets title aspect ratio to be 16:9.
+        if (height > width) {
+            mTitle.setHeight(width * 9 / 16);
+        }
+
+        super.onMeasure(widthMeasureSpec, heightMeasureSpec);
+    }
+
+    @Override
     protected void onAttachedToWindow() {
         super.onAttachedToWindow();
         updateAccounts();
@@ -250,70 +253,17 @@ public class AccountSigninView extends FirstRunView
         }
     }
 
-    @Override
-    protected boolean isHorizontalModeEnabled() {
-        return mHorizontalModeEnabled;
-    }
-
-    @Override
-    protected boolean isDynamicPaddingEnabled() {
-        return mDynamicPaddingEnabled;
-    }
-
     /**
      * Changes the visuals slightly for when this view appears in the recent tabs page instead of
-     * in first run. For example, the title text is changed as well as the button style.
+     * in first run.
      * This is currently used in the Recent Tabs Promo and the bookmarks page.
      */
     public void configureForRecentTabsOrBookmarksPage() {
-        mHorizontalModeEnabled = false;
-        mDynamicPaddingEnabled = false;
         mShowSettingsSpan = false;
 
         setBackgroundResource(R.color.ntp_bg);
-        mTitle.setText(R.string.sign_in_to_chrome);
-
         mCancelButtonTextId = R.string.cancel;
         setUpCancelButton();
-
-        setPadding(0, 0, 0,
-                getResources().getDimensionPixelOffset(R.dimen.sign_in_promo_padding_bottom));
-    }
-
-    /**
-     * Changes the visuals slightly for when this view appears in the settings menu.
-     */
-    public void configureForSettingsPage() {
-        mHorizontalModeEnabled = false;
-    }
-
-    /**
-     * Changes the visuals slightly for when this view is shown in a subsequent run after user adds
-     * a Google account to the device.
-     */
-    public void configureForAddAccountPromo() {
-        int experimentGroup = SigninManager.getAndroidSigninPromoExperimentGroup();
-        assert experimentGroup >= 0 && experimentGroup <= EXPERIMENT_MAX_VALUE;
-
-        if ((experimentGroup & EXPERIMENT_TITLE_VARIANT_MASK) != 0) {
-            mTitle.setText(R.string.make_chrome_yours);
-        }
-
-        mDescriptionTextId = (experimentGroup & EXPERIMENT_SUMMARY_VARIANT_MASK) != 0
-                ? R.string.signin_sign_in_to_chrome_summary_variant
-                : R.string.sign_in_to_chrome_summary;
-
-        if ((experimentGroup & EXPERIMENT_LAYOUT_VARIANT_MASK) != 0) {
-            mImageCarousel.setVisibility(GONE);
-
-            ImageView illustrationView = new ImageView(getContext());
-            illustrationView.setImageResource(R.drawable.signin_promo_illustration);
-            illustrationView.setBackgroundColor(ApiCompatibilityUtils.getColor(getResources(),
-                    R.color.illustration_background_color));
-
-            LinearLayout linearLayout = (LinearLayout) findViewById(R.id.fre_main_layout);
-            linearLayout.addView(illustrationView, 0);
-        }
     }
 
     /**
@@ -361,32 +311,25 @@ public class AccountSigninView extends FirstRunView
                 return false;
             }
         } else {
-            accountToSelect = getIndexOfNewElement(
-                    oldAccountNames, mAccountNames, mSpinner.getSelectedItemPosition());
+            accountToSelect = getIndexOfNewElement(oldAccountNames, mAccountNames,
+                    mAccountListAdapter.getSelectedAccountPosition());
         }
 
-        mArrayAdapter.clear();
+        mAccountListAdapter.clear();
+        mAccountListView.setVisibility(View.VISIBLE);

[newt (away), 2016/04/01 23:43:12]

not needed; the list is already/always visible, right?

[gogerald1, 2016/04/12 01:21:13]

Done.

         if (!mAccountNames.isEmpty()) {
-            mSpinner.setVisibility(View.VISIBLE);
-            mArrayAdapter.addAll(mAccountNames);
-            mArrayAdapter.add(mAddAnotherAccount);
+            mAccountListAdapter.addAll(mAccountNames);
+            mAccountListAdapter.add(mAddAnotherAccount);
 
             setUpSigninButton(true);
-            mDescriptionText.setText(mDescriptionTextId);
-
         } else {
-            mSpinner.setVisibility(View.GONE);
-            mArrayAdapter.add(mAddAnotherAccount);
             setUpSigninButton(false);
-            mDescriptionText.setText(R.string.signin_no_account_choice_description);
         }
 
         if (mProfileData != null) mProfileData.update();
         updateProfileImages();
 
-        mSpinner.setSelection(accountToSelect);
-        mAccountName = mArrayAdapter.getItem(accountToSelect).toString();
-        mImageCarousel.scrollTo(accountToSelect, false, false);
+        selectAccount(accountToSelect);
 
         return oldAccountNames != null
                 && !(oldAccountNames.size() == mAccountNames.size()
@@ -424,34 +367,33 @@ public class AccountSigninView extends FirstRunView
     private void updateProfileImages() {
         if (mProfileData == null) return;
 
-        int count = mAccountNames.size();
-
-        Bitmap[] images;
-        if (count == 0) {
-            images = new Bitmap[1];
-            images[0] = mProfileData.getImage(null);
-        } else {
-            images = new Bitmap[count];
-            for (int i = 0; i < count; ++i) {
-                images[i] = mProfileData.getImage(mAccountNames.get(i));
-            }
+        int totalChildViews = mAccountListView.getCount();
+        int totalHeaderViews = mAccountListView.getHeaderViewsCount();
+        // Do not update header views and the last (mAddAnotherAccount) view.
+        for (int i = totalHeaderViews; i < totalChildViews - 1; i++) {
+            View accountView = mAccountListView.getChildAt(i);
+            if (accountView == null) continue;
+            ImageView accountImage = (ImageView) accountView.findViewById(R.id.account_image);
+            accountImage.setImageBitmap(
+                    mProfileData.getImage(mAccountNames.get(i - totalHeaderViews)));
         }
 
-        mImageCarousel.setImages(images);
-        updateProfileName();
+        if (mSignedIn) updateSignedInAccountInfo();
     }
 
-    private void updateProfileName() {
-        if (!mSignedIn) return;
+    private void updateSignedInAccountInfo() {
+        if (mProfileData == null) return;

[newt (away), 2016/04/01 23:43:12]

Can mProfileData be null here? If not, make this an assert. If so, we should
still show what little information we can (e.g. the user's email address).

[gogerald1, 2016/04/12 01:21:13]

Done.

 
+        String selectedAccountEmail =
+                mAccountNames.get(mAccountListAdapter.getSelectedAccountPosition());
+        mSigninAccountImage.setImageBitmap(mProfileData.getImage(selectedAccountEmail));
         String name = null;
-        if (mProfileData != null) {
-            if (mIsChildAccount) name = mProfileData.getGivenName(mAccountName);
-            if (name == null) name = mProfileData.getFullName(mAccountName);
-        }
-        if (name == null) name = mAccountName;
+        if (mIsChildAccount) name = mProfileData.getGivenName(selectedAccountEmail);
+        if (name == null) name = mProfileData.getFullName(selectedAccountEmail);
+        if (name == null) name = selectedAccountEmail;
         String text = String.format(getResources().getString(R.string.signin_hi_name), name);
-        mTitle.setText(text);
+        mSigninAccountName.setText(text);
+        mSigninAccountEmail.setText(selectedAccountEmail);
     }
 
     /**
@@ -464,31 +406,24 @@ public class AccountSigninView extends FirstRunView
         showConfirmSigninPage();
     }
 
-    private void configureSpinner(boolean signinPage) {
-        mSpinner.setEnabled(signinPage);
-        mSpinner.setVisibility(signinPage ? View.VISIBLE : View.GONE);
-        mConfirmAccountEmail.setVisibility(signinPage ? View.GONE : View.VISIBLE);
-        mConfirmAccountEmail.setText(mAccountName);
-    }
-
     private void showSigninPage() {
         mSignedIn = false;
-        mTitle.setText(R.string.sign_in_to_chrome);
 
-        configureSpinner(true);
-        mImageCarousel.setVisibility(VISIBLE);
+        mSigninConfirmationView.setVisibility(View.GONE);
+        mSigninView.setVisibility(View.VISIBLE);
 
         setUpCancelButton();
         updateAccounts();
-
-        mImageCarousel.setSignedInMode(false);
     }
 
     private void showConfirmSigninPage() {
         mSignedIn = true;
-        updateProfileName();
 
-        configureSpinner(false);
+        updateSignedInAccountInfo();
+
+        mSigninView.setVisibility(View.GONE);
+        mSigninConfirmationView.setVisibility(View.VISIBLE);
+
         setUpConfirmButton();
         setUpUndoButton();
 
@@ -496,20 +431,20 @@ public class AccountSigninView extends FirstRunView
             NoUnderlineClickableSpan settingsSpan = new NoUnderlineClickableSpan() {
                 @Override
                 public void onClick(View widget) {
-                    mListener.onAccountSelected(mAccountName, true);
+                    mListener.onAccountSelected(
+                            mAccountNames.get(mAccountListAdapter.getSelectedAccountPosition()),
+                            true);
                 }
             };
-            mDescriptionText.setText(SpanApplier.applySpans(getSignedInDescription(mIsChildAccount),
-                    new SpanInfo(SETTINGS_LINK_OPEN, SETTINGS_LINK_CLOSE, settingsSpan)));
+            mSigninChromeSyncDescription.setText(
+                    SpanApplier.applySpans(getSignedInDescription(mIsChildAccount),
+                            new SpanInfo(SETTINGS_LINK_OPEN, SETTINGS_LINK_CLOSE, settingsSpan)));
         } else {
             // If we aren't showing the span, get rid of the LINK1 annotations.
-            mDescriptionText.setText(getSignedInDescription(mIsChildAccount)
-                                             .replace(SETTINGS_LINK_OPEN, "")
-                                             .replace(SETTINGS_LINK_CLOSE, ""));
+            mSigninChromeSyncDescription.setText(getSignedInDescription(mIsChildAccount)
+                                                         .replace(SETTINGS_LINK_OPEN, "")
+                                                         .replace(SETTINGS_LINK_CLOSE, ""));
         }
-
-        mImageCarousel.setVisibility(VISIBLE);
-        mImageCarousel.setSignedInMode(true);
     }
 
     private void showConfirmSigninPageAccountTrackerServiceCheck() {
@@ -534,14 +469,11 @@ public class AccountSigninView extends FirstRunView
     }
 
     private void showConfirmSigninPagePreviousAccountCheck() {
-        if (SigninInvestigator.investigate(mAccountName)
-                == InvestigatedScenario.DIFFERENT_ACCOUNT) {
-
+        String accountName = mAccountNames.get(mAccountListAdapter.getSelectedAccountPosition());

[newt (away), 2016/04/01 23:43:12]

I think you should make a small method "getSelectedAccountName()" that returns
"mAccountNames.get(mAccountListAdapter.getSelectedAccountPosition())", since you
keep repeating the latter code.

[gogerald1, 2016/04/12 01:21:13]

Done.

+        if (SigninInvestigator.investigate(accountName) == InvestigatedScenario.DIFFERENT_ACCOUNT) {
             ConfirmImportSyncDataDialog.showNewInstance(
-                    PrefServiceBridge.getInstance().getSyncLastAccountName(),
-                    mAccountName,
-                    ImportSyncType.PREVIOUS_DATA_FOUND,
-                    mDelegate.getFragmentManager(),
+                    PrefServiceBridge.getInstance().getSyncLastAccountName(), accountName,
+                    ImportSyncType.PREVIOUS_DATA_FOUND, mDelegate.getFragmentManager(),
                     new ConfirmImportSyncDataDialog.Listener() {
                         @Override
                         public void onConfirm() {
@@ -584,6 +516,7 @@ public class AccountSigninView extends FirstRunView
                 }
             });
         }
+        setPositiveButtonActive();
     }
 
     private void setUpUndoButton() {
@@ -605,7 +538,8 @@ public class AccountSigninView extends FirstRunView
         mPositiveButton.setOnClickListener(new OnClickListener() {
             @Override
             public void onClick(View v) {
-                mListener.onAccountSelected(mAccountName, false);
+                mListener.onAccountSelected(
+                        mAccountNames.get(mAccountListAdapter.getSelectedAccountPosition()), false);
             }
         });
     }
@@ -643,9 +577,11 @@ public class AccountSigninView extends FirstRunView
     public void switchToForcedAccountMode(String forcedAccountName) {
         mForcedAccountName = forcedAccountName;
         updateAccounts();
-        assert TextUtils.equals(mAccountName, mForcedAccountName);
+        assert TextUtils.equals(mAccountNames.get(mAccountListAdapter.getSelectedAccountPosition()),
+                mForcedAccountName);
         switchToSignedMode();
-        assert TextUtils.equals(mAccountName, mForcedAccountName);
+        assert TextUtils.equals(mAccountNames.get(mAccountListAdapter.getSelectedAccountPosition()),
+                mForcedAccountName);
     }
 
     /**
@@ -662,9 +598,60 @@ public class AccountSigninView extends FirstRunView
         return mForcedAccountName != null;
     }
 
+    /**
+    * Overrides AdapterView.OnItemClickListener.

[newt (away), 2016/04/01 23:43:12]

fix indentation, or just use "// Overrides AdapterView.OnItemClickListener"

[gogerald1, 2016/04/12 01:21:13]

Done.

+    */
     @Override
-    public void onPositionChanged(int i) {
-        mPositionSetProgrammatically = true;
-        mSpinner.setSelection(i);
+    public void onItemClick(AdapterView<?> adapter, View view, int position, long id) {
+        TextView accountNameView = (TextView) view.findViewById(R.id.account_name);
+        if (mAddAnotherAccount.equalsIgnoreCase(accountNameView.getText().toString())) {

[newt (away), 2016/04/01 23:43:12]

use the position or id, not the text, to determine if they clicked "add account"

[gogerald1, 2016/04/12 01:21:13]

Done.

+            mListener.onNewAccount();
+            RecordUserAction.record("Signin_AddAccountToDevice");
+        } else {
+            selectAccount(position - mAccountListView.getHeaderViewsCount());
+        }
+    }
+
+    private void selectAccount(int position) {
+        int oldPosition = mAccountListAdapter.getSelectedAccountPosition();
+        if (oldPosition == position) return;
+
+        int oldPositionInViews = oldPosition + mAccountListView.getHeaderViewsCount();
+        View preSelectedAccount = mAccountListView.getChildAt(
+                oldPositionInViews - mAccountListView.getFirstVisiblePosition());
+        // If the previously selected account view is not visible (preSelectedAccount == null), let
+        // it be updated by AccountListAdapter when necessary.
+        if (preSelectedAccount != null) {
+            preSelectedAccount.findViewById(R.id.account_selection_mark).setVisibility(View.GONE);

[newt (away), 2016/04/01 23:43:12]

No no, this is the wrong way to update items in a list view. Instead, you should
call notifyDataSetChanged on the adapter and the views in the list will be
regenerated.

[gogerald1, 2016/04/12 01:21:13]

Done, the reason to update accounts in this way is for efficiency, but
considering number of accounts is small, let's keep it simple.

[newt (away), 2016/04/13 18:13:30]

You know what they say about premature optimization... :)

In this case, I suspect, the majority of time is dominated by the extra,
unavoidable measure/layout/draw pass (which recurses over the entire visible
view hierarchy). Asking the adapter to update itself is probably only a tiny
fraction of that time.

+        }
+
+        mAccountListAdapter.setSelectedAccountPosition(position);
+        int newPositionInViews = position + mAccountListView.getHeaderViewsCount();
+        if (mAccountListView.getLastVisiblePosition() < newPositionInViews) {
+            mAccountListView.smoothScrollToPosition(newPositionInViews);
+        }
+        mAccountListView.getChildAt(newPositionInViews - mAccountListView.getFirstVisiblePosition())
+                .findViewById(R.id.account_selection_mark)
+                .setVisibility(View.VISIBLE);
+    }
+
+    private void setPositiveButtonActive() {

[newt (away), 2016/04/01 23:43:12]

I'd call this "setPositiveButtonEnabled()", to reuse the standard terminology.

[gogerald1, 2016/04/12 01:21:13]

Done.

+        ((ButtonCompat) mPositiveButton)

[newt (away), 2016/04/01 23:43:11]

change the type of mPositiveButton to ButtonCompat so you can avoid these casts.

[gogerald1, 2016/04/12 01:21:13]

Done.

+                .setButtonColor(
+                        ApiCompatibilityUtils.getColor(getResources(), R.color.light_active_color));
+        mPositiveButton.setTextColor(
+                ApiCompatibilityUtils.getColor(getResources(), R.color.signin_head_background));
+        mPositiveButton.setAlpha(1f);
+        mPositiveButton.setEnabled(true);
+    }
+
+    private void setPositiveButtonInActive() {

[newt (away), 2016/04/01 23:43:11]

likewise, setPositiveButtonDisabled()

[gogerald1, 2016/04/12 01:21:13]

Done.

+        ((ButtonCompat) mPositiveButton)
+                .setButtonColor(ApiCompatibilityUtils.getColor(
+                        getResources(), R.color.signin_button_disabled_color));
+        mPositiveButton.setTextColor(
+                ApiCompatibilityUtils.getColor(getResources(), R.color.fre_text_color));
+        mPositiveButton.setAlpha(0.26f);
+        mPositiveButton.setEnabled(false);
     }
 }