From 2ae1bf568b68ea18431ab10aceeff3ec62aedb7b Mon Sep 17 00:00:00 2001 From: Evan Rosky Date: Thu, 11 May 2017 11:18:45 -0700 Subject: [PATCH] Improve consistency in focusAfterDescendants behavior - focusAfterDescendant ViewGroups that were also clusters would continue to be remembered for restoreFocusedInCluster after gaining focusable children. This caused undesired cluster-focus loops - focusableViewAvailable wasn't being called in all cases (specifically when views were added). This meant focusAfterDescendant views wouldn't transfer focus to children in some cases. - 0-area views which became focusable weren't reporting focusableViewAvailable. Since views gaining area doesn't report focusableViewAvailable, this was another case of state change not being accounted for. Also, this restriction doesn't exist for gaining focus so these views are actually focusable despite having 0 area. - focusableViewAvailable was reporting focusable views even when ancestors (and therefore the new focusables) were not visible. - restoreFocusNotInCluster tried to focus invisible views - restoreFocusNotInCluster was sending focus to focusAfterDescendant viewgroups which had focusable children IN a cluster. Bug: 38010274 Bug: 38009598 Bug: 38352147 Test: cycling works in situations reported in bugs. Added CTS tests around focusableViewAvailable and FOCUS_AFTER_DESCENDANTS Change-Id: I77f214f5384dcf9092324e08fc1daa3f1155bbad --- core/java/android/view/View.java | 16 ++++++++--- core/java/android/view/ViewGroup.java | 41 ++++++++++++++++++--------- 2 files changed, 39 insertions(+), 18 deletions(-) diff --git a/core/java/android/view/View.java b/core/java/android/view/View.java index 07258fc307048..3f3a4949e9544 100644 --- a/core/java/android/view/View.java +++ b/core/java/android/view/View.java @@ -9957,12 +9957,20 @@ public class View implements Drawable.Callback, KeyEvent.Callback, // Going from one cluster to another, so save last-focused. // This covers cluster jumps because they are always FOCUS_DOWN oldFocus.setFocusedInCluster(oldCluster); + if (!(oldFocus.mParent instanceof ViewGroup)) { + return; + } if (direction == FOCUS_FORWARD || direction == FOCUS_BACKWARD) { // This is a result of ordered navigation so consider navigation through // the previous cluster "complete" and clear its last-focused memory. - if (oldFocus.mParent instanceof ViewGroup) { - ((ViewGroup) oldFocus.mParent).clearFocusedInCluster(oldFocus); - } + ((ViewGroup) oldFocus.mParent).clearFocusedInCluster(oldFocus); + } else if (oldFocus instanceof ViewGroup + && ((ViewGroup) oldFocus).getDescendantFocusability() + == ViewGroup.FOCUS_AFTER_DESCENDANTS + && ViewRootImpl.isViewDescendantOf(this, oldFocus)) { + // This means oldFocus is not focusable since it obviously has a focusable + // child (this). Don't restore focus to it in the future. + ((ViewGroup) oldFocus.mParent).clearFocusedInCluster(oldFocus); } } } @@ -13009,7 +13017,7 @@ public class View implements Drawable.Callback, KeyEvent.Callback, // about in case nothing has focus. even if this specific view // isn't focusable, it may contain something that is, so let // the root view try to give this focus if nothing else does. - if ((mParent != null) && (mBottom > mTop) && (mRight > mLeft)) { + if ((mParent != null)) { mParent.focusableViewAvailable(this); } } diff --git a/core/java/android/view/ViewGroup.java b/core/java/android/view/ViewGroup.java index 50593f28d9e39..b9c611fdf6d97 100644 --- a/core/java/android/view/ViewGroup.java +++ b/core/java/android/view/ViewGroup.java @@ -57,6 +57,7 @@ import android.view.animation.Animation; import android.view.animation.AnimationUtils; import android.view.animation.LayoutAnimationController; import android.view.animation.Transformation; + import com.android.internal.R; import java.util.ArrayList; @@ -832,8 +833,9 @@ public abstract class ViewGroup extends View implements ViewParent, ViewManager public void focusableViewAvailable(View v) { if (mParent != null // shortcut: don't report a new focusable view if we block our descendants from - // getting focus + // getting focus or if we're not visible && (getDescendantFocusability() != FOCUS_BLOCK_DESCENDANTS) + && ((mViewFlags & VISIBILITY_MASK) == VISIBLE) && (isFocusableInTouchMode() || !shouldBlockFocusForTouchscreen()) // shortcut: don't report a new focusable view if we already are focused // (and we don't prefer our descendants) @@ -1158,18 +1160,25 @@ public abstract class ViewGroup extends View implements ViewParent, ViewManager // Determine whether we have a focused descendant. final int descendantFocusability = getDescendantFocusability(); if (descendantFocusability != FOCUS_BLOCK_DESCENDANTS) { - final int count = mChildrenCount; - final View[] children = mChildren; + return hasFocusableChild(dispatchExplicit); + } - for (int i = 0; i < count; i++) { - final View child = children[i]; + return false; + } - // In case the subclass has overridden has[Explicit]Focusable, dispatch - // to the expected one for each child even though we share logic here. - if ((dispatchExplicit && child.hasExplicitFocusable()) - || (!dispatchExplicit && child.hasFocusable())) { - return true; - } + boolean hasFocusableChild(boolean dispatchExplicit) { + // Determine whether we have a focusable descendant. + final int count = mChildrenCount; + final View[] children = mChildren; + + for (int i = 0; i < count; i++) { + final View child = children[i]; + + // In case the subclass has overridden has[Explicit]Focusable, dispatch + // to the expected one for each child even though we share logic here. + if ((dispatchExplicit && child.hasExplicitFocusable()) + || (!dispatchExplicit && child.hasFocusable())) { + return true; } } @@ -3229,7 +3238,7 @@ public abstract class ViewGroup extends View implements ViewParent, ViewManager // will refer to a view not-in a cluster. return restoreFocusInCluster(View.FOCUS_DOWN); } - if (isKeyboardNavigationCluster()) { + if (isKeyboardNavigationCluster() || (mViewFlags & VISIBILITY_MASK) != VISIBLE) { return false; } int descendentFocusability = getDescendantFocusability(); @@ -3247,7 +3256,7 @@ public abstract class ViewGroup extends View implements ViewParent, ViewManager return true; } } - if (descendentFocusability == FOCUS_AFTER_DESCENDANTS) { + if (descendentFocusability == FOCUS_AFTER_DESCENDANTS && !hasFocusableChild(false)) { return super.requestFocus(FOCUS_DOWN, null); } return false; @@ -4913,7 +4922,8 @@ public abstract class ViewGroup extends View implements ViewParent, ViewManager child.mParent = this; } - if (child.hasFocus()) { + final boolean childHasFocus = child.hasFocus(); + if (childHasFocus) { requestChildFocus(child, child.findFocus()); } @@ -4926,6 +4936,9 @@ public abstract class ViewGroup extends View implements ViewParent, ViewManager needGlobalAttributesUpdate(true); } ai.mKeepScreenOn = lastKeepOn; + if (!childHasFocus && child.hasFocusable()) { + focusableViewAvailable(child); + } } if (child.isLayoutDirectionInherited()) {