Fixing a memory leak in ViewRootImpl and a focus change callback issue.

1. ViewRootImpl was keeping reference to the old focused view so it can
   call back the global on focus change listener when another view gets
   focus. The stashed reference, however was not cleared which caused a
   memory leak if the last focused view was removed from the view tree.
   In general keeping additional state for the last focus in ViewRootImpl
   is not a good idea since this add complexity due to additional book
   keeping work that is required. The view tree already keeps track of
   where the focus is and it should be the only place that holds this
   data. Since focus does not change that frequently it is fine to look
   up the focus since this operation is O(m) where m is the depth of the
   view tree. This change removes the duplicate book keeping from
   ViewRootImpl and the focus is looked up as needed.

2. ViewRootImpl was calling the global focus change callbacks when focus
   is gained, lost, or transferred to another view. This was done in
   *ChildFocus methods. In the case of a child losing focus, i.e. in
   clearChildFocus, there was a check whether focus searh yields a view
   to take focus and if so it did not call back the global focus listener
   assuming the the found view will take focus (the view tree gives focus
   to the first focusable when a view looses focus). This is not a correct
   assumption since some views override methods called as a result of
   View.requestFocus that determine what the next focused view should
   be. For example, HorizontalScrollView overrides onRequestFocusInDescendants
   and changes the direction of the search. In fact focus search does not
   take into accound ViewGroup descendant focusability. Hence, the view found
   by calling the focus search from the root is not necessarily the one
   that will get focus after calling requestFocus. Actually, it is
   possible that the focus search will find a view but no view will
   take focus. Now the View class is responsible for calling the
   global focus listeners which avoids the above problem. Also this
   saves book keeping in ViewRootImpl.

bug:7962363

Change-Id: Ic95a18b364e997021f3f6bb46943559aac07d95a

diff --git a/core/java/android/view/View.java b/core/java/android/view/View.java
index 97287c3..a4e4f37 100644 (file)
--- a/core/java/android/view/View.java
+++ b/core/java/android/view/View.java
@@ -4376,10 +4376,16 @@ public class View implements Drawable.Callback, KeyEvent.Callback,
         if ((mPrivateFlags & PFLAG_FOCUSED) == 0) {
             mPrivateFlags |= PFLAG_FOCUSED;
 
+            View oldFocus = (mAttachInfo != null) ? getRootView().findFocus() : null;
+
             if (mParent != null) {
                 mParent.requestChildFocus(this, this);
             }
 
+            if (mAttachInfo != null) {
+                mAttachInfo.mTreeObserver.dispatchOnGlobalFocusChange(oldFocus, this);
+            }
+
             onFocusChanged(true, direction, previouslyFocusedRect);
             refreshDrawableState();
 
@@ -4486,7 +4492,9 @@ public class View implements Drawable.Callback, KeyEvent.Callback,
 
             refreshDrawableState();
 
-            ensureInputFocusOnFirstFocusable();
+            if (!rootViewRequestFocus()) {
+                notifyGlobalFocusCleared(this);
+            }
 
             if (AccessibilityManager.getInstance(mContext).isEnabled()) {
                 notifyAccessibilityStateChanged();
@@ -4494,11 +4502,18 @@ public class View implements Drawable.Callback, KeyEvent.Callback,
         }
     }
 
-    void ensureInputFocusOnFirstFocusable() {
+    void notifyGlobalFocusCleared(View oldFocus) {
+        if (oldFocus != null && mAttachInfo != null) {
+            mAttachInfo.mTreeObserver.dispatchOnGlobalFocusChange(oldFocus, null);
+        }
+    }
+
+    boolean rootViewRequestFocus() {
         View root = getRootView();
         if (root != null) {
-            root.requestFocus();
+            return root.requestFocus();
         }
+        return false;
     }
 
     /**

diff --git a/core/java/android/view/ViewGroup.java b/core/java/android/view/ViewGroup.java
index c274f27..a4898fc 100644 (file)
--- a/core/java/android/view/ViewGroup.java
+++ b/core/java/android/view/ViewGroup.java
@@ -3697,7 +3697,9 @@ public abstract class ViewGroup extends View implements ViewParent, ViewManager
             clearChildFocus = true;
         }
 
-        view.clearAccessibilityFocus();
+        if (view.isAccessibilityFocused()) {
+            view.clearAccessibilityFocus();
+        }
 
         cancelTouchTarget(view);
         cancelHoverTarget(view);
@@ -3719,11 +3721,9 @@ public abstract class ViewGroup extends View implements ViewParent, ViewManager
 
         if (clearChildFocus) {
             clearChildFocus(view);
-            ensureInputFocusOnFirstFocusable();
-        }
-
-        if (view.isAccessibilityFocused()) {
-            view.clearAccessibilityFocus();
+            if (!rootViewRequestFocus()) {
+                notifyGlobalFocusCleared(this);
+            }
         }
 
         onViewRemoved(view);
@@ -3765,7 +3765,7 @@ public abstract class ViewGroup extends View implements ViewParent, ViewManager
     private void removeViewsInternal(int start, int count) {
         final View focused = mFocused;
         final boolean detach = mAttachInfo != null;
-        View clearChildFocus = null;
+        boolean clearChildFocus = false;
 
         final View[] children = mChildren;
         final int end = start + count;
@@ -3779,10 +3779,12 @@ public abstract class ViewGroup extends View implements ViewParent, ViewManager
 
             if (view == focused) {
                 view.unFocus();
-                clearChildFocus = view;
+                clearChildFocus = true;
             }
 
-            view.clearAccessibilityFocus();
+            if (view.isAccessibilityFocused()) {
+                view.clearAccessibilityFocus();
+            }
 
             cancelTouchTarget(view);
             cancelHoverTarget(view);
@@ -3805,9 +3807,11 @@ public abstract class ViewGroup extends View implements ViewParent, ViewManager
 
         removeFromArray(start, count);
 
-        if (clearChildFocus != null) {
-            clearChildFocus(clearChildFocus);
-            ensureInputFocusOnFirstFocusable();
+        if (clearChildFocus) {
+            clearChildFocus(focused);
+            if (!rootViewRequestFocus()) {
+                notifyGlobalFocusCleared(focused);
+            }
         }
     }
 
@@ -3849,7 +3853,7 @@ public abstract class ViewGroup extends View implements ViewParent, ViewManager
 
         final View focused = mFocused;
         final boolean detach = mAttachInfo != null;
-        View clearChildFocus = null;
+        boolean clearChildFocus = false;
 
         needGlobalAttributesUpdate(false);
 
@@ -3862,10 +3866,12 @@ public abstract class ViewGroup extends View implements ViewParent, ViewManager
 
             if (view == focused) {
                 view.unFocus();
-                clearChildFocus = view;
+                clearChildFocus = true;
             }
 
-            view.clearAccessibilityFocus();
+            if (view.isAccessibilityFocused()) {
+                view.clearAccessibilityFocus();
+            }
 
             cancelTouchTarget(view);
             cancelHoverTarget(view);
@@ -3887,9 +3893,11 @@ public abstract class ViewGroup extends View implements ViewParent, ViewManager
             children[i] = null;
         }
 
-        if (clearChildFocus != null) {
-            clearChildFocus(clearChildFocus);
-            ensureInputFocusOnFirstFocusable();
+        if (clearChildFocus) {
+            clearChildFocus(focused);
+            if (!rootViewRequestFocus()) {
+                notifyGlobalFocusCleared(focused);
+            }
         }
     }
 

diff --git a/core/java/android/view/ViewRootImpl.java b/core/java/android/view/ViewRootImpl.java
index f9f955e..6a96893 100644 (file)
--- a/core/java/android/view/ViewRootImpl.java
+++ b/core/java/android/view/ViewRootImpl.java
@@ -169,9 +169,6 @@ public final class ViewRootImpl implements ViewParent,
     int mSeq;
 
     View mView;
-    View mFocusedView;
-    View mRealFocusedView;  // this is not set to null in touch mode
-    View mOldFocusedView;
 
     View mAccessibilityFocusedHost;
     AccessibilityNodeInfo mAccessibilityFocusedVirtualView;
@@ -269,7 +266,7 @@ public final class ViewRootImpl implements ViewParent,
 
     boolean mScrollMayChange;
     int mSoftInputMode;
-    View mLastScrolledFocus;
+    WeakReference<View> mLastScrolledFocus;
     int mScrollY;
     int mCurScrollY;
     Scroller mScroller;
@@ -1526,7 +1523,9 @@ public final class ViewRootImpl implements ViewParent,
                 } else if (!mSurface.isValid()) {
                     // If the surface has been removed, then reset the scroll
                     // positions.
-                    mLastScrolledFocus = null;
+                    if (mLastScrolledFocus != null) {
+                        mLastScrolledFocus.clear();
+                    }
                     mScrollY = mCurScrollY = 0;
                     if (mScroller != null) {
                         mScroller.abortAnimation();
@@ -1802,13 +1801,11 @@ public final class ViewRootImpl implements ViewParent,
             if (mView != null) {
                 if (!mView.hasFocus()) {
                     mView.requestFocus(View.FOCUS_FORWARD);
-                    mFocusedView = mRealFocusedView = mView.findFocus();
                     if (DEBUG_INPUT_RESIZE) Log.v(TAG, "First: requested focused view="
-                            + mFocusedView);
+                            + mView.findFocus());
                 } else {
-                    mRealFocusedView = mView.findFocus();
                     if (DEBUG_INPUT_RESIZE) Log.v(TAG, "First: existing focused view="
-                            + mRealFocusedView);
+                            + mView.findFocus());
                 }
             }
             if ((relayoutResult & WindowManagerGlobal.RELAYOUT_RES_ANIMATING) != 0) {
@@ -2514,17 +2511,12 @@ public final class ViewRootImpl implements ViewParent,
             // requestChildRectangleOnScreen() call (in which case 'rectangle'
             // is non-null and we just want to scroll to whatever that
             // rectangle is).
-            View focus = mRealFocusedView;
-
-            // When in touch mode, focus points to the previously focused view,
-            // which may have been removed from the view hierarchy. The following
-            // line checks whether the view is still in our hierarchy.
-            if (focus == null || focus.mAttachInfo != mAttachInfo) {
-                mRealFocusedView = null;
+            View focus = mView.findFocus();
+            if (focus == null) {
                 return false;
             }
-
-            if (focus != mLastScrolledFocus) {
+            View lastScrolledFocus = (mLastScrolledFocus != null) ? mLastScrolledFocus.get() : null;
+            if (lastScrolledFocus != null && focus != lastScrolledFocus) {
                 // If the focus has changed, then ignore any requests to scroll
                 // to a rectangle; first we want to make sure the entire focus
                 // view is visible.
@@ -2533,8 +2525,7 @@ public final class ViewRootImpl implements ViewParent,
             if (DEBUG_INPUT_RESIZE) Log.v(TAG, "Eval scroll: focus=" + focus
                     + " rectangle=" + rectangle + " ci=" + ci
                     + " vi=" + vi);
-            if (focus == mLastScrolledFocus && !mScrollMayChange
-                    && rectangle == null) {
+            if (focus == lastScrolledFocus && !mScrollMayChange && rectangle == null) {
                 // Optimization: if the focus hasn't changed since last
                 // time, and no layout has happened, then just leave things
                 // as they are.
@@ -2544,7 +2535,7 @@ public final class ViewRootImpl implements ViewParent,
                 // We need to determine if the currently focused view is
                 // within the visible part of the window and, if not, apply
                 // a pan so it can be seen.
-                mLastScrolledFocus = focus;
+                mLastScrolledFocus = new WeakReference<View>(focus);
                 mScrollMayChange = false;
                 if (DEBUG_INPUT_RESIZE) Log.v(TAG, "Need to scroll?");
                 // Try to find the rectangle from the focus view.
@@ -2671,33 +2662,19 @@ public final class ViewRootImpl implements ViewParent,
     }
 
     public void requestChildFocus(View child, View focused) {
-        checkThread();
-
         if (DEBUG_INPUT_RESIZE) {
             Log.v(TAG, "Request child focus: focus now " + focused);
         }
-
-        mAttachInfo.mTreeObserver.dispatchOnGlobalFocusChange(mOldFocusedView, focused);
+        checkThread();
         scheduleTraversals();
-
-        mFocusedView = mRealFocusedView = focused;
     }
 
     public void clearChildFocus(View child) {
-        checkThread();
-
         if (DEBUG_INPUT_RESIZE) {
             Log.v(TAG, "Clearing child focus");
         }
-
-        mOldFocusedView = mFocusedView;
-
-        // 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;
+        checkThread();
+        scheduleTraversals();
     }
 
     @Override
@@ -2714,14 +2691,13 @@ public final class ViewRootImpl implements ViewParent,
                 // the one case where will transfer focus away from the current one
                 // is if the current view is a view group that prefers to give focus
                 // to its children first AND the view is a descendant of it.
-                mFocusedView = mView.findFocus();
-                boolean descendantsHaveDibsOnFocus =
-                        (mFocusedView instanceof ViewGroup) &&
-                            (((ViewGroup) mFocusedView).getDescendantFocusability() ==
-                                    ViewGroup.FOCUS_AFTER_DESCENDANTS);
-                if (descendantsHaveDibsOnFocus && isViewDescendantOf(v, mFocusedView)) {
-                    // If a view gets the focus, the listener will be invoked from requestChildFocus()
-                    v.requestFocus();
+                View focused = mView.findFocus();
+                if (focused instanceof ViewGroup) {
+                    ViewGroup group = (ViewGroup) focused;
+                    if (group.getDescendantFocusability() == ViewGroup.FOCUS_AFTER_DESCENDANTS
+                            && isViewDescendantOf(v, focused)) {
+                        v.requestFocus();
+                    }
                 }
             }
         }
@@ -3199,7 +3175,6 @@ public final class ViewRootImpl implements ViewParent,
                 // set yet.
                 final View focused = mView.findFocus();
                 if (focused != null && !focused.isFocusableInTouchMode()) {
-
                     final ViewGroup ancestorToTakeFocus =
                             findAncestorToTakeFocusInTouchMode(focused);
                     if (ancestorToTakeFocus != null) {
@@ -3208,10 +3183,7 @@ public final class ViewRootImpl implements ViewParent,
                         return ancestorToTakeFocus.requestFocus();
                     } else {
                         // nothing appropriate to have focus in touch mode, clear it out
-                        mView.unFocus();
-                        mAttachInfo.mTreeObserver.dispatchOnGlobalFocusChange(focused, null);
-                        mFocusedView = null;
-                        mOldFocusedView = null;
+                        focused.unFocus();
                         return true;
                     }
                 }
@@ -3246,12 +3218,11 @@ public final class ViewRootImpl implements ViewParent,
     private boolean leaveTouchMode() {
         if (mView != null) {
             if (mView.hasFocus()) {
-                // i learned the hard way to not trust mFocusedView :)
-                mFocusedView = mView.findFocus();
-                if (!(mFocusedView instanceof ViewGroup)) {
+                View focusedView = mView.findFocus();
+                if (!(focusedView instanceof ViewGroup)) {
                     // some view has focus, let it keep it
                     return false;
-                } else if (((ViewGroup)mFocusedView).getDescendantFocusability() !=
+                } else if (((ViewGroup) focusedView).getDescendantFocusability() !=
                         ViewGroup.FOCUS_AFTER_DESCENDANTS) {
                     // some view group has focus, and doesn't prefer its children
                     // over itself for focus, so let them keep it.