Skip to content

Commit b36a0ac

Browse files
sganovsganov
committed
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
1 parent 91ec0b7 commit b36a0ac

File tree

6 files changed

+135
-33
lines changed

6 files changed

+135
-33
lines changed

‎core/java/android/view/View.java‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3788,6 +3788,11 @@ void clearFocusForRemoval() {
37883788

37893789
onFocusChanged(false, 0, null);
37903790
refreshDrawableState();
3791+
3792+
// The view cleared focus and invoked the callbacks, so now is the
3793+
// time to give focus to the the first focusable from the top to
3794+
// ensure that the gain focus is announced after clear focus.
3795+
getRootView().requestFocus(FOCUS_FORWARD);
37913796
}
37923797
}
37933798

‎core/java/android/view/ViewGroup.java‎

Lines changed: 11 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -675,11 +675,14 @@ public void clearChildFocus(View child) {
675675
*/
676676
@Override
677677
public void clearFocus() {
678-
super.clearFocus();
679-
680-
// clear any child focus if it exists
681-
if (mFocused != null) {
678+
if (DBG) {
679+
System.out.println(this + " clearFocus()");
680+
}
681+
if (mFocused == null) {
682+
super.clearFocus();
683+
} else {
682684
mFocused.clearFocus();
685+
mFocused = null;
683686
}
684687
}
685688

@@ -691,12 +694,12 @@ void unFocus() {
691694
if (DBG) {
692695
System.out.println(this + " unFocus()");
693696
}
694-
695-
super.unFocus();
696-
if (mFocused != null) {
697+
if (mFocused == null) {
698+
super.unFocus();
699+
} else {
697700
mFocused.unFocus();
701+
mFocused = null;
698702
}
699-
mFocused = null;
700703
}
701704

702705
/**

‎core/java/android/view/ViewRootImpl.java‎

Lines changed: 19 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -174,6 +174,7 @@ public final class ViewRootImpl extends Handler implements ViewParent,
174174
View mView;
175175
View mFocusedView;
176176
View mRealFocusedView; // this is not set to null in touch mode
177+
View mOldFocusedView;
177178
int mViewVisibility;
178179
boolean mAppVisible = true;
179180
int mOrigWindowType = -1;
@@ -2272,32 +2273,33 @@ boolean scrollToRectOrFocus(Rect rectangle, boolean immediate) {
22722273

22732274
public void requestChildFocus(View child, View focused) {
22742275
checkThread();
2275-
if (mFocusedView != focused) {
2276-
mAttachInfo.mTreeObserver.dispatchOnGlobalFocusChange(mFocusedView, focused);
2277-
scheduleTraversals();
2276+
2277+
if (DEBUG_INPUT_RESIZE) {
2278+
Log.v(TAG, "Request child focus: focus now " + focused);
22782279
}
2280+
2281+
mAttachInfo.mTreeObserver.dispatchOnGlobalFocusChange(mOldFocusedView, focused);
2282+
scheduleTraversals();
2283+
22792284
mFocusedView = mRealFocusedView = focused;
2280-
if (DEBUG_INPUT_RESIZE) Log.v(TAG, "Request child focus: focus now "
2281-
+ mFocusedView);
22822285
}
22832286

22842287
public void clearChildFocus(View child) {
22852288
checkThread();
22862289

2287-
View oldFocus = mFocusedView;
2290+
if (DEBUG_INPUT_RESIZE) {
2291+
Log.v(TAG, "Clearing child focus");
2292+
}
2293+
2294+
mOldFocusedView = mFocusedView;
22882295

2289-
if (DEBUG_INPUT_RESIZE) Log.v(TAG, "Clearing child focus");
2290-
mFocusedView = mRealFocusedView = null;
2291-
if (mView != null && !mView.hasFocus()) {
2292-
// If a view gets the focus, the listener will be invoked from requestChildFocus()
2293-
if (!mView.requestFocus(View.FOCUS_FORWARD)) {
2294-
mAttachInfo.mTreeObserver.dispatchOnGlobalFocusChange(oldFocus, null);
2295-
}
2296-
} else if (oldFocus != null) {
2297-
mAttachInfo.mTreeObserver.dispatchOnGlobalFocusChange(oldFocus, null);
2296+
// Invoke the listener only if there is no view to take focus
2297+
if (focusSearch(null, View.FOCUS_FORWARD) == null) {
2298+
mAttachInfo.mTreeObserver.dispatchOnGlobalFocusChange(mOldFocusedView, null);
22982299
}
2299-
}
23002300

2301+
mFocusedView = mRealFocusedView = null;
2302+
}
23012303

23022304
public void focusableViewAvailable(View v) {
23032305
checkThread();
@@ -2770,6 +2772,7 @@ private boolean enterTouchMode() {
27702772
mView.unFocus();
27712773
mAttachInfo.mTreeObserver.dispatchOnGlobalFocusChange(focused, null);
27722774
mFocusedView = null;
2775+
mOldFocusedView = null;
27732776
return true;
27742777
}
27752778
}

‎core/tests/coretests/Android.mk‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -22,7 +22,7 @@ LOCAL_SRC_FILES := \
2222
$(call all-java-files-under, EnabledTestApp/src)
2323

2424
LOCAL_DX_FLAGS := --core-library
25-
LOCAL_STATIC_JAVA_LIBRARIES := core-tests android-common frameworks-core-util-lib mockwebserver guava
25+
LOCAL_STATIC_JAVA_LIBRARIES := core-tests android-common frameworks-core-util-lib mockwebserver guava littlemock
2626
LOCAL_JAVA_LIBRARIES := android.test.runner
2727
LOCAL_PACKAGE_NAME := FrameworksCoreTests
2828

‎core/tests/coretests/src/android/widget/focus/RequestFocus.java‎

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -21,9 +21,7 @@
2121
import android.app.Activity;
2222
import android.os.Bundle;
2323
import android.os.Handler;
24-
import android.widget.LinearLayout;
2524
import android.widget.Button;
26-
import android.view.View;
2725

2826
/**
2927
* Exercises cases where elements of the UI are requestFocus()ed.

‎core/tests/coretests/src/android/widget/focus/RequestFocusTest.java‎

Lines changed: 99 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -16,21 +16,28 @@
1616

1717
package android.widget.focus;
1818

19-
import android.widget.focus.RequestFocus;
20-
import com.android.frameworks.coretests.R;
19+
import static com.google.testing.littlemock.LittleMock.inOrder;
20+
import static com.google.testing.littlemock.LittleMock.mock;
2121

2222
import android.os.Handler;
23-
import android.test.ActivityInstrumentationTestCase;
23+
import android.test.ActivityInstrumentationTestCase2;
24+
import android.test.UiThreadTest;
2425
import android.test.suitebuilder.annotation.LargeTest;
2526
import android.test.suitebuilder.annotation.MediumTest;
26-
import android.widget.Button;
2727
import android.util.AndroidRuntimeException;
28+
import android.view.View;
29+
import android.view.View.OnFocusChangeListener;
30+
import android.view.ViewTreeObserver.OnGlobalFocusChangeListener;
31+
import android.widget.Button;
32+
33+
import com.android.frameworks.coretests.R;
34+
import com.google.testing.littlemock.LittleMock.InOrder;
2835

2936
/**
3037
* {@link RequestFocusTest} is set up to exercise cases where the views that
3138
* have focus become invisible or GONE.
3239
*/
33-
public class RequestFocusTest extends ActivityInstrumentationTestCase<RequestFocus> {
40+
public class RequestFocusTest extends ActivityInstrumentationTestCase2<RequestFocus> {
3441

3542
private Button mTopLeftButton;
3643
private Button mBottomLeftButton;
@@ -39,7 +46,7 @@ public class RequestFocusTest extends ActivityInstrumentationTestCase<RequestFoc
3946
private Handler mHandler;
4047

4148
public RequestFocusTest() {
42-
super("com.android.frameworks.coretests", RequestFocus.class);
49+
super(RequestFocus.class);
4350
}
4451

4552
@Override
@@ -94,4 +101,90 @@ public void testWrongThreadRequestFocusFails() throws Exception {
94101
e.getClass().getName());
95102
}
96103
}
104+
105+
/**
106+
* This tests checks the case in which the first focusable View clears focus.
107+
* In such a case the framework tries to give the focus to another View starting
108+
* from the top. Hence, the framework will try to give focus to the view that
109+
* wants to clear its focus.
110+
*
111+
* @throws Exception If an error occurs.
112+
*/
113+
@UiThreadTest
114+
public void testOnFocusChangeCallbackOrderWhenClearingFocusOfFirstFocusable()
115+
throws Exception {
116+
// Get the first focusable.
117+
Button clearingFocusButton = mTopLeftButton;
118+
Button gainingFocusButton = mTopLeftButton;
119+
120+
// Make sure that the clearing focus View is the first focusable.
121+
View focusCandidate = clearingFocusButton.getRootView().getParent().focusSearch(null,
122+
View.FOCUS_FORWARD);
123+
assertSame("The clearing focus button is the first focusable.",
124+
clearingFocusButton, focusCandidate);
125+
assertSame("The gaining focus button is the first focusable.",
126+
gainingFocusButton, focusCandidate);
127+
128+
// Focus the clearing focus button.
129+
clearingFocusButton.requestFocus();
130+
assertTrue(clearingFocusButton.hasFocus());
131+
132+
// Register the invocation order checker.
133+
CombinedListeners mock = mock(CombinedListeners.class);
134+
clearingFocusButton.setOnFocusChangeListener(mock);
135+
gainingFocusButton.setOnFocusChangeListener(mock);
136+
clearingFocusButton.getViewTreeObserver().addOnGlobalFocusChangeListener(mock);
137+
138+
// Try to clear focus.
139+
clearingFocusButton.clearFocus();
140+
141+
// Check that no callback was invoked since focus did not move.
142+
InOrder inOrder = inOrder(mock);
143+
inOrder.verify(mock).onFocusChange(clearingFocusButton, false);
144+
inOrder.verify(mock).onGlobalFocusChanged(clearingFocusButton, gainingFocusButton);
145+
inOrder.verify(mock).onFocusChange(gainingFocusButton, true);
146+
}
147+
148+
public interface CombinedListeners extends OnFocusChangeListener, OnGlobalFocusChangeListener {}
149+
150+
/**
151+
* This tests check whether the on focus change callbacks are invoked in
152+
* the proper order when a View loses focus and the framework gives it to
153+
* the fist focusable one.
154+
*
155+
* @throws Exception
156+
*/
157+
@UiThreadTest
158+
public void testOnFocusChangeCallbackOrderWhenClearingFocusOfNotFirstFocusable()
159+
throws Exception {
160+
Button clearingFocusButton = mTopRightButton;
161+
Button gainingFocusButton = mTopLeftButton;
162+
163+
// Make sure that the clearing focus View is not the first focusable.
164+
View focusCandidate = clearingFocusButton.getRootView().getParent().focusSearch(null,
165+
View.FOCUS_FORWARD);
166+
assertNotSame("The clearing focus button is not the first focusable.",
167+
clearingFocusButton, focusCandidate);
168+
assertSame("The gaining focus button is the first focusable.",
169+
gainingFocusButton, focusCandidate);
170+
171+
// Focus the clearing focus button.
172+
clearingFocusButton.requestFocus();
173+
assertTrue(clearingFocusButton.hasFocus());
174+
175+
// Register the invocation order checker.
176+
CombinedListeners mock = mock(CombinedListeners.class);
177+
clearingFocusButton.setOnFocusChangeListener(mock);
178+
gainingFocusButton.setOnFocusChangeListener(mock);
179+
clearingFocusButton.getViewTreeObserver().addOnGlobalFocusChangeListener(mock);
180+
181+
// Try to clear focus.
182+
clearingFocusButton.clearFocus();
183+
184+
// Check that no callback was invoked since focus did not move.
185+
InOrder inOrder = inOrder(mock);
186+
inOrder.verify(mock).onFocusChange(clearingFocusButton, false);
187+
inOrder.verify(mock).onGlobalFocusChanged(clearingFocusButton, gainingFocusButton);
188+
inOrder.verify(mock).onFocusChange(gainingFocusButton, true);
189+
}
97190
}

0 commit comments

Comments
 (0)