Incorrect behavior of View clear focus.

The framework tries to have a focused view all the time. For
that purpose when a view's focus is cleared the focus is given
to the first focusable found from the top. The implementation
of this behavior was causing the following issues:

1. If the fist focusable View tries to clear its focus it
   was getting focus but the onFocusChange callbacks were not
   properly invoked. Specifically, the onFocusChange for
   gaining focus was called first and then the same
   callback for clearing focus. Note that the callback
   for clearing focus is called when the View is already
   focused. Also note that at the end the View did not
   clear its focus, hence no focus change callbacks
   should be invoked.

2. If not the first focusable View tries to clear focus,
   the focus is given to another one but the callback
   for getting focus was called before the one for clearing,
   so client code may be mislead that there is more than
   one focused view at a time.

3. (Nit) The implementaion of clearFocus and unFocus in ViewGroup
   was calling the super implementaion when there is a
   focused child. Since there could be only one focused View,
   having a focused child means that the group is not focused
   and the call to the super implementation is not needed.

4. Added unit tests that verify the correct behavior, i.e.
   the focus of the first focused view cannot be cleared
   which means that no focus change callbacks are invoked.
   The callbacks should be called in expected order.
   Now the view focus clear precedes the view focus gain
   callback. However, in between is invoked the global
   focus change callback with the correct values. We may
   want to call that one after the View callbacks. If
   needed we can revisit this.

Change-Id: Iee80baf5c75c82d3cda09679e4949483cad475f1

5 files changed, 207 insertions, 33 deletions

diff --git a/core/java/android/view/View.java b/core/java/android/view/View.java
index 8cac57d..501c6dc 100644
--- a/core/java/android/view/View.java
+++ b/core/java/android/view/View.java
@@ -3770,6 +3770,14 @@ public class View implements Drawable.Callback, Drawable.Callback2, KeyEvent.Cal
         }
 
         if ((mPrivateFlags & FOCUSED) != 0) {
+            // If this is the first focusable do not clear focus since the we
+            // try to give it focus every time a view clears its focus. Hence,
+            // the view that would gain focus already has it.
+            View firstFocusable = getFirstFocusable();
+            if (firstFocusable == this) {
+                return;
+            }
+
             mPrivateFlags &= ~FOCUSED;
 
             if (mParent != null) {
@@ -3778,9 +3786,24 @@ public class View implements Drawable.Callback, Drawable.Callback2, KeyEvent.Cal
 
             onFocusChanged(false, 0, null);
             refreshDrawableState();
+
+            // The view cleared focus and invoked the callbacks, so  now is the
+            // time to give focus to the the first focusable to ensure that the
+            // gain focus is announced after clear focus.
+            if (firstFocusable != null) {
+                firstFocusable.requestFocus(FOCUS_FORWARD);
+            }
         }
     }
 
+    private View getFirstFocusable() {
+        ViewRootImpl viewRoot = getViewRootImpl();
+        if (viewRoot != null) {
+            return viewRoot.focusSearch(null, FOCUS_FORWARD);
+        }
+        return null;
+    }
+
     /**
      * Called to clear the focus of a view that is about to be removed.
      * Doesn't call clearChildFocus, which prevents this view from taking
diff --git a/core/java/android/view/ViewGroup.java b/core/java/android/view/ViewGroup.java
index 5c63366..5ac4b36 100644
--- a/core/java/android/view/ViewGroup.java
+++ b/core/java/android/view/ViewGroup.java
@@ -675,11 +675,14 @@ public abstract class ViewGroup extends View implements ViewParent, ViewManager
      */
     @Override
     public void clearFocus() {
-        super.clearFocus();
-
-        // clear any child focus if it exists
-        if (mFocused != null) {
+        if (DBG) {
+            System.out.println(this + " clearFocus()");
+        }
+        if (mFocused == null) {
+            super.clearFocus();
+        } else {
             mFocused.clearFocus();
+            mFocused = null;
         }
     }
 
@@ -691,12 +694,12 @@ public abstract class ViewGroup extends View implements ViewParent, ViewManager
         if (DBG) {
             System.out.println(this + " unFocus()");
         }
-
-        super.unFocus();
-        if (mFocused != null) {
+        if (mFocused == null) {
+            super.unFocus();
+        } else {
             mFocused.unFocus();
+            mFocused = null;
         }
-        mFocused = null;
     }
 
     /**
diff --git a/core/java/android/view/ViewRootImpl.java b/core/java/android/view/ViewRootImpl.java
index 1a4bdf4..2ef843b 100644
--- a/core/java/android/view/ViewRootImpl.java
+++ b/core/java/android/view/ViewRootImpl.java
@@ -168,6 +168,7 @@ public final class ViewRootImpl extends Handler implements ViewParent,
     View mView;
     View mFocusedView;
     View mRealFocusedView;  // this is not set to null in touch mode
+    View mOldFocusedView;
     int mViewVisibility;
     boolean mAppVisible = true;
     int mOrigWindowType = -1;
@@ -2226,32 +2227,33 @@ public final class ViewRootImpl extends Handler implements ViewParent,
 
     public void requestChildFocus(View child, View focused) {
         checkThread();
-        if (mFocusedView != focused) {
-            mAttachInfo.mTreeObserver.dispatchOnGlobalFocusChange(mFocusedView, focused);
-            scheduleTraversals();
+
+        if (DEBUG_INPUT_RESIZE) {
+            Log.v(TAG, "Request child focus: focus now " + focused);
         }
+
+        mAttachInfo.mTreeObserver.dispatchOnGlobalFocusChange(mOldFocusedView, focused);
+        scheduleTraversals();
+
         mFocusedView = mRealFocusedView = focused;
-        if (DEBUG_INPUT_RESIZE) Log.v(TAG, "Request child focus: focus now "
-                + mFocusedView);
     }
 
     public void clearChildFocus(View child) {
         checkThread();
 
-        View oldFocus = mFocusedView;
+        if (DEBUG_INPUT_RESIZE) {
+            Log.v(TAG, "Clearing child focus");
+        }
+
+        mOldFocusedView = mFocusedView;
 
-        if (DEBUG_INPUT_RESIZE) Log.v(TAG, "Clearing child focus");
-        mFocusedView = mRealFocusedView = null;
-        if (mView != null && !mView.hasFocus()) {
-            // If a view gets the focus, the listener will be invoked from requestChildFocus()
-            if (!mView.requestFocus(View.FOCUS_FORWARD)) {
-                mAttachInfo.mTreeObserver.dispatchOnGlobalFocusChange(oldFocus, null);
-            }
-        } else if (oldFocus != null) {
-            mAttachInfo.mTreeObserver.dispatchOnGlobalFocusChange(oldFocus, null);
+        // Invoke the listener only if there is no view to take focus
+        if (focusSearch(null, View.FOCUS_FORWARD) == null) {
+            mAttachInfo.mTreeObserver.dispatchOnGlobalFocusChange(mOldFocusedView, null);
         }
-    }
 
+        mFocusedView = mRealFocusedView = null;
+    }
 
     public void focusableViewAvailable(View v) {
         checkThread();
@@ -2724,6 +2726,7 @@ public final class ViewRootImpl extends Handler implements ViewParent,
                         mView.unFocus();
                         mAttachInfo.mTreeObserver.dispatchOnGlobalFocusChange(focused, null);
                         mFocusedView = null;
+                        mOldFocusedView = null;
                         return true;
                     }
                 }
diff --git a/core/tests/coretests/src/android/widget/focus/RequestFocus.java b/core/tests/coretests/src/android/widget/focus/RequestFocus.java
index af9ee17..21d762a 100644
--- a/core/tests/coretests/src/android/widget/focus/RequestFocus.java
+++ b/core/tests/coretests/src/android/widget/focus/RequestFocus.java
@@ -21,9 +21,7 @@ import com.android.frameworks.coretests.R;
 import android.app.Activity;
 import android.os.Bundle;
 import android.os.Handler;
-import android.widget.LinearLayout;
 import android.widget.Button;
-import android.view.View;
 
 /**
  * Exercises cases where elements of the UI are requestFocus()ed.
diff --git a/core/tests/coretests/src/android/widget/focus/RequestFocusTest.java b/core/tests/coretests/src/android/widget/focus/RequestFocusTest.java
index a78b0c9..baf587e 100644
--- a/core/tests/coretests/src/android/widget/focus/RequestFocusTest.java
+++ b/core/tests/coretests/src/android/widget/focus/RequestFocusTest.java
@@ -16,21 +16,27 @@
 
 package android.widget.focus;
 
-import android.widget.focus.RequestFocus;
-import com.android.frameworks.coretests.R;
-
 import android.os.Handler;
-import android.test.ActivityInstrumentationTestCase;
+import android.test.ActivityInstrumentationTestCase2;
+import android.test.UiThreadTest;
 import android.test.suitebuilder.annotation.LargeTest;
 import android.test.suitebuilder.annotation.MediumTest;
-import android.widget.Button;
 import android.util.AndroidRuntimeException;
+import android.view.View;
+import android.view.View.OnFocusChangeListener;
+import android.view.ViewTreeObserver.OnGlobalFocusChangeListener;
+import android.widget.Button;
+
+import com.android.frameworks.coretests.R;
+
+import java.util.ArrayList;
+import java.util.List;
 
 /**
  * {@link RequestFocusTest} is set up to exercise cases where the views that
  * have focus become invisible or GONE.
  */
-public class RequestFocusTest extends ActivityInstrumentationTestCase<RequestFocus> {
+public class RequestFocusTest extends ActivityInstrumentationTestCase2<RequestFocus> {
 
     private Button mTopLeftButton;
     private Button mBottomLeftButton;
@@ -39,7 +45,7 @@ public class RequestFocusTest extends ActivityInstrumentationTestCase<RequestFoc
     private Handler mHandler;
 
     public RequestFocusTest() {
-        super("com.android.frameworks.coretests", RequestFocus.class);
+        super(RequestFocus.class);
     }
 
     @Override
@@ -94,4 +100,145 @@ public class RequestFocusTest extends ActivityInstrumentationTestCase<RequestFoc
                          e.getClass().getName());
         }
     }
+
+    /**
+     * This tests checks the case in which the first focusable View clears focus.
+     * In such a case the framework tries to give the focus to another View starting
+     * from the top. Hence, the framework will try to give focus to the view that
+     * wants to clear its focus. From a client perspective, the view does not loose
+     * focus after the call, therefore no callback for focus change should be invoked.
+     *
+     * @throws Exception If an error occurs.
+     */
+    @UiThreadTest
+    public void testOnFocusChangeNotCalledIfFocusDoesNotMove() throws Exception {
+        // Get the first focusable.
+        Button button = mTopLeftButton;
+
+        // Make sure that the button is the first focusable and focus it.
+        button.getRootView().requestFocus(View.FOCUS_DOWN);
+        assertTrue(button.hasFocus());
+
+        // Attach on focus change listener that should not be called.
+        button.setOnFocusChangeListener(new OnFocusChangeListener() {
+            @Override
+            public void onFocusChange(View v, boolean hasFocus) {
+                throw new IllegalStateException("Unexpeced call to"
+                        + "OnFocusChangeListener#onFocusChange");
+            }
+        });
+
+        // Attach on global focus change listener that should not be called.
+        button.getViewTreeObserver().addOnGlobalFocusChangeListener(
+                new OnGlobalFocusChangeListener() {
+            @Override
+            public void onGlobalFocusChanged(View oldFocus, View newFocus) {
+                throw new IllegalStateException("Unexpeced call to"
+                        + "OnFocusChangeListener#onFocusChange");
+            }
+        });
+
+        // Try to clear focus.
+        button.clearFocus();
+    }
+
+    /**
+     * This tests check whether the on focus change callbacks are invoked in
+     * the proper order when a View loses focus and the framework gives it to
+     * the fist focusable one.
+     *
+     * @throws Exception
+     */
+    @UiThreadTest
+    public void testOnFocusChangeCallbackOrder() throws Exception {
+        // Get the first focusable.
+        Button clearingFocusButton = mTopRightButton;
+        Button gainingFocusButton = mTopLeftButton;
+
+        // Make sure that the clearing focus is not the first focusable.
+        View focusCandidate = clearingFocusButton.getRootView().getParent().focusSearch(null,
+                View.FOCUS_FORWARD);
+        assertNotSame("The clearing focus button is not the first focusable.",
+                clearingFocusButton, focusCandidate);
+        assertSame("The gaining focus button is the first focusable.",
+                gainingFocusButton, focusCandidate);
+
+        // Focus the clearing focus button.
+        clearingFocusButton.requestFocus();
+        assertTrue(clearingFocusButton.hasFocus());
+
+        // Register the invocation order checker.
+        CallbackOrderChecker checker = new CallbackOrderChecker(clearingFocusButton,
+                gainingFocusButton);
+        clearingFocusButton.setOnFocusChangeListener(checker);
+        gainingFocusButton.setOnFocusChangeListener(checker);
+        clearingFocusButton.getViewTreeObserver().addOnGlobalFocusChangeListener(checker);
+
+        // Try to clear focus.
+        clearingFocusButton.clearFocus();
+
+        // Check that no callback was invoked since focus did not move.
+        checker.verify();
+    }
+
+    /**
+     * This class check whether the focus change callback are invoked in order.
+     */
+    private class CallbackOrderChecker implements OnFocusChangeListener,
+            OnGlobalFocusChangeListener {
+
+        private class CallbackInvocation {
+            final String mMethodName;
+            final Object[] mArguments;
+
+            CallbackInvocation(String methodName, Object[] arguments) {
+                mMethodName = methodName;
+                mArguments = arguments;
+            }
+        }
+
+        private final View mClearingFocusView;
+        private final View mGainingFocusView;
+
+        private final List<CallbackInvocation> mInvocations = new ArrayList<CallbackInvocation>();
+
+        public CallbackOrderChecker(View clearingFocusView, View gainingFocusView) {
+            mClearingFocusView = clearingFocusView;
+            mGainingFocusView = gainingFocusView;
+        }
+
+        @Override
+        public void onFocusChange(View view, boolean hasFocus) {
+            CallbackInvocation invocation = new CallbackInvocation(
+                    "OnFocusChangeListener#onFocusChange", new Object[] {view, hasFocus});
+            mInvocations.add(invocation);
+        }
+
+        @Override
+        public void onGlobalFocusChanged(View oldFocus, View newFocus) {
+            CallbackInvocation invocation = new CallbackInvocation(
+                    "OnFocusChangeListener#onFocusChange", new Object[] {oldFocus, newFocus});
+            mInvocations.add(invocation);
+        }
+
+        public void verify() {
+            assertSame("All focus change callback should be invoked.", 3, mInvocations.size());
+            assertInvioked("Callback for View clearing focus explected.", 0,
+                    "OnFocusChangeListener#onFocusChange",
+                    new Object[] {mClearingFocusView, false});
+            assertInvioked("Callback for View global focus change explected.", 1,
+                    "OnFocusChangeListener#onFocusChange", new Object[] {mClearingFocusView,
+                    mGainingFocusView});
+            assertInvioked("Callback for View gaining focus explected.", 2,
+                    "OnFocusChangeListener#onFocusChange", new Object[] {mGainingFocusView, true});
+        }
+
+        private void assertInvioked(String message, int order, String methodName,
+                Object[] arguments) {
+            CallbackInvocation invocation = mInvocations.get(order);
+            assertEquals(message, methodName, invocation.mMethodName);
+            assertEquals(message, arguments[0], invocation.mArguments[0]);
+            assertEquals(message, arguments[1], invocation.mArguments[1]);
+        }
+    }
 }