From b36a0ac9709e9e1c7098559c0435cfbdc09e6c46 Mon Sep 17 00:00:00 2001 From: Svetoslav Ganov Date: Tue, 14 Feb 2012 17:46:47 -0800 Subject: Incorrect behavior of View clear focus v2.0. 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. 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: I8cfb141c948141703093cf6fa2037be60861cee0 --- core/java/android/view/View.java | 5 + core/java/android/view/ViewGroup.java | 19 ++-- core/java/android/view/ViewRootImpl.java | 35 +++---- core/tests/coretests/Android.mk | 2 +- .../src/android/widget/focus/RequestFocus.java | 2 - .../src/android/widget/focus/RequestFocusTest.java | 105 +++++++++++++++++++-- 6 files changed, 135 insertions(+), 33 deletions(-) diff --git a/core/java/android/view/View.java b/core/java/android/view/View.java index 87104f4..38f055f 100644 --- a/core/java/android/view/View.java +++ b/core/java/android/view/View.java @@ -3788,6 +3788,11 @@ 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 from the top to + // ensure that the gain focus is announced after clear focus. + getRootView().requestFocus(FOCUS_FORWARD); } } diff --git a/core/java/android/view/ViewGroup.java b/core/java/android/view/ViewGroup.java index bc147ac..a2e85a3 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 1c3bbfa..1930a5e 100644 --- a/core/java/android/view/ViewRootImpl.java +++ b/core/java/android/view/ViewRootImpl.java @@ -174,6 +174,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; @@ -2272,32 +2273,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(); @@ -2770,6 +2772,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/Android.mk b/core/tests/coretests/Android.mk index b81f774..88f3f34 100644 --- a/core/tests/coretests/Android.mk +++ b/core/tests/coretests/Android.mk @@ -22,7 +22,7 @@ LOCAL_SRC_FILES := \ $(call all-java-files-under, EnabledTestApp/src) LOCAL_DX_FLAGS := --core-library -LOCAL_STATIC_JAVA_LIBRARIES := core-tests android-common frameworks-core-util-lib mockwebserver guava +LOCAL_STATIC_JAVA_LIBRARIES := core-tests android-common frameworks-core-util-lib mockwebserver guava littlemock LOCAL_JAVA_LIBRARIES := android.test.runner LOCAL_PACKAGE_NAME := FrameworksCoreTests 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..f2eba23 100644 --- a/core/tests/coretests/src/android/widget/focus/RequestFocusTest.java +++ b/core/tests/coretests/src/android/widget/focus/RequestFocusTest.java @@ -16,21 +16,28 @@ package android.widget.focus; -import android.widget.focus.RequestFocus; -import com.android.frameworks.coretests.R; +import static com.google.testing.littlemock.LittleMock.inOrder; +import static com.google.testing.littlemock.LittleMock.mock; 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 com.google.testing.littlemock.LittleMock.InOrder; /** * {@link RequestFocusTest} is set up to exercise cases where the views that * have focus become invisible or GONE. */ -public class RequestFocusTest extends ActivityInstrumentationTestCase { +public class RequestFocusTest extends ActivityInstrumentationTestCase2 { private Button mTopLeftButton; private Button mBottomLeftButton; @@ -39,7 +46,7 @@ public class RequestFocusTest extends ActivityInstrumentationTestCase