From 22e0d48bbe15293da067eefe3a73ef59fa66b062 Mon Sep 17 00:00:00 2001 From: Phil Weaver Date: Thu, 4 May 2017 15:15:29 -0700 Subject: [PATCH] Match attributes to actions for pip a11y When we strip and replace actions for UIs inside a picture- in-picture window, make sure attributes match. For example, if an node was clickable and exposes ACTION_CLICK, when removing actions we must clear both the click action and the clickable attribute. Bug: 37923645 Test: Updating unit tests Change-Id: I84de5cae136bccb20b2234aef59bf5e0a7c15949 --- .../ActionReplacingCallback.java | 25 ++++- .../ActionReplacingCallbackTest.java | 94 +++++++++++++------ 2 files changed, 85 insertions(+), 34 deletions(-) diff --git a/services/accessibility/java/com/android/server/accessibility/ActionReplacingCallback.java b/services/accessibility/java/com/android/server/accessibility/ActionReplacingCallback.java index 0e30fb28f3d74..de8a5181b2cff 100644 --- a/services/accessibility/java/com/android/server/accessibility/ActionReplacingCallback.java +++ b/services/accessibility/java/com/android/server/accessibility/ActionReplacingCallback.java @@ -26,6 +26,7 @@ import android.view.accessibility.IAccessibilityInteractionConnection; import android.view.accessibility.IAccessibilityInteractionConnectionCallback; import com.android.internal.annotations.GuardedBy; +import java.util.ArrayList; import java.util.List; import java.util.concurrent.atomic.AtomicInteger; @@ -96,7 +97,7 @@ public class ActionReplacingCallback extends IAccessibilityInteractionConnection mNodeFromOriginalWindow = info; } else { Slog.e(LOG_TAG, "Callback with unexpected interactionId"); - throw new RuntimeException("Callback with unexpected interactionId"); // Remove + return; } mSingleNodeCallbackHappened = true; @@ -119,7 +120,7 @@ public class ActionReplacingCallback extends IAccessibilityInteractionConnection mNodesWithReplacementActions = infos; } else { Slog.e(LOG_TAG, "Callback with unexpected interactionId"); - throw new RuntimeException("Callback with unexpected interactionId"); // Remove + return; } callbackForSingleNode = mSingleNodeCallbackHappened; callbackForMultipleNodes = mMultiNodeCallbackHappened; @@ -147,7 +148,7 @@ public class ActionReplacingCallback extends IAccessibilityInteractionConnection if (DEBUG) { Slog.e(LOG_TAG, "Extra callback"); } - throw new RuntimeException("Extra callback"); // Replace with return before submit + return; } if (mNodeFromOriginalWindow != null) { replaceActionsOnInfoLocked(mNodeFromOriginalWindow); @@ -172,7 +173,7 @@ public class ActionReplacingCallback extends IAccessibilityInteractionConnection if (DEBUG) { Slog.e(LOG_TAG, "Extra callback"); } - throw new RuntimeException("Extra callback"); // Replace with return before submit + return; } if (mNodesFromOriginalWindow != null) { for (int i = 0; i < mNodesFromOriginalWindow.size(); i++) { @@ -180,7 +181,8 @@ public class ActionReplacingCallback extends IAccessibilityInteractionConnection } } recycleReplaceActionNodesLocked(); - nodesToReturn = mNodesFromOriginalWindow; + nodesToReturn = (mNodesFromOriginalWindow == null) + ? null : new ArrayList<>(mNodesFromOriginalWindow); mDone = true; } try { @@ -195,6 +197,12 @@ public class ActionReplacingCallback extends IAccessibilityInteractionConnection @GuardedBy("mLock") private void replaceActionsOnInfoLocked(AccessibilityNodeInfo info) { info.removeAllActions(); + info.setClickable(false); + info.setFocusable(false); + info.setContextClickable(false); + info.setScrollable(false); + info.setLongClickable(false); + info.setDismissable(false); // We currently only replace actions for the root node if ((info.getSourceNodeId() == AccessibilityNodeInfo.ROOT_NODE_ID) && mNodesWithReplacementActions != null) { @@ -213,6 +221,12 @@ public class ActionReplacingCallback extends IAccessibilityInteractionConnection info.addAction(AccessibilityAction.ACTION_ACCESSIBILITY_FOCUS); info.addAction(AccessibilityAction.ACTION_CLEAR_ACCESSIBILITY_FOCUS); } + info.setClickable(nodeWithReplacementActions.isClickable()); + info.setFocusable(nodeWithReplacementActions.isFocusable()); + info.setContextClickable(nodeWithReplacementActions.isContextClickable()); + info.setScrollable(nodeWithReplacementActions.isScrollable()); + info.setLongClickable(nodeWithReplacementActions.isLongClickable()); + info.setDismissable(nodeWithReplacementActions.isDismissable()); } } } @@ -220,6 +234,7 @@ public class ActionReplacingCallback extends IAccessibilityInteractionConnection @GuardedBy("mLock") private void recycleReplaceActionNodesLocked() { + if (mNodesWithReplacementActions == null) return; for (int i = mNodesWithReplacementActions.size() - 1; i >= 0; i--) { AccessibilityNodeInfo nodeWithReplacementAction = mNodesWithReplacementActions.get(i); nodeWithReplacementAction.recycle(); diff --git a/services/tests/servicestests/src/com/android/server/accessibility/ActionReplacingCallbackTest.java b/services/tests/servicestests/src/com/android/server/accessibility/ActionReplacingCallbackTest.java index 8afe853895482..72820f15315ef 100644 --- a/services/tests/servicestests/src/com/android/server/accessibility/ActionReplacingCallbackTest.java +++ b/services/tests/servicestests/src/com/android/server/accessibility/ActionReplacingCallbackTest.java @@ -25,6 +25,9 @@ import android.view.accessibility.AccessibilityWindowInfo; import android.view.accessibility.IAccessibilityInteractionConnection; import android.view.accessibility.IAccessibilityInteractionConnectionCallback; +import org.hamcrest.BaseMatcher; +import org.hamcrest.Description; +import org.hamcrest.Matcher; import org.junit.Before; import org.junit.Test; import org.mockito.ArgumentCaptor; @@ -45,12 +48,13 @@ import static junit.framework.TestCase.assertTrue; import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertFalse; import static org.junit.Assert.assertNotEquals; +import static org.junit.Assert.assertThat; +import static org.mockito.Matchers.anyInt; import static org.mockito.Matchers.anyObject; import static org.mockito.Matchers.eq; import static org.mockito.Mockito.doThrow; import static org.mockito.Mockito.verify; import static org.mockito.Mockito.verifyNoMoreInteractions; -import static org.mockito.Mockito.when; import static org.mockito.MockitoAnnotations.initMocks; /** @@ -63,15 +67,50 @@ public class ActionReplacingCallbackTest { private static final int NON_ROOT_NODE_ID = 0xAAAA5555; private static final long INTERROGATING_TID = 0x1234FACE; - private static final AccessibilityAction[] ACTIONS_FROM_REPLACER = - {ACTION_CLICK, ACTION_EXPAND}; - private static final AccessibilityAction[] A11Y_FOCUS_ACTIONS = - {ACTION_ACCESSIBILITY_FOCUS, ACTION_CLEAR_ACCESSIBILITY_FOCUS}; // We expect both the replacer actions and a11y focus actions to appear private static final AccessibilityAction[] REQUIRED_ACTIONS_ON_ROOT_TO_SERVICE = {ACTION_CLICK, ACTION_EXPAND, ACTION_ACCESSIBILITY_FOCUS, ACTION_CLEAR_ACCESSIBILITY_FOCUS}; + private static final Matcher HAS_NO_ACTIONS = + new BaseMatcher() { + @Override + public boolean matches(Object o) { + AccessibilityNodeInfo node = (AccessibilityNodeInfo) o; + if (!node.getActionList().isEmpty()) return false; + return (!node.isScrollable() && !node.isLongClickable() && !node.isClickable() + && !node.isContextClickable() && !node.isDismissable() && !node.isFocusable()); + } + + @Override + public void describeTo(Description description) { + description.appendText("Has no actions"); + } + }; + + private static final Matcher HAS_EXPECTED_ACTIONS_ON_ROOT = + new BaseMatcher() { + @Override + public boolean matches(Object o) { + AccessibilityNodeInfo node = (AccessibilityNodeInfo) o; + List actions = node.getActionList(); + if ((actions.size() != 4) || !actions.contains(ACTION_CLICK) + || !actions.contains(ACTION_EXPAND) + || !actions.contains(ACTION_ACCESSIBILITY_FOCUS)) { + return false; + } + return (!node.isScrollable() && !node.isLongClickable() + && !node.isLongClickable() && node.isClickable() + && !node.isContextClickable() && !node.isDismissable() + && !node.isFocusable()); + } + + @Override + public void describeTo(Description description) { + description.appendText("Has only 4 actions expected on root"); + } + }; + @Mock IAccessibilityInteractionConnectionCallback mMockServiceCallback; @Mock IAccessibilityInteractionConnection mMockReplacerConnection; @@ -118,9 +157,10 @@ public class ActionReplacingCallbackTest { eq(INTERACTION_ID)); AccessibilityNodeInfo infoSentToService = mInfoCaptor.getValue(); assertEquals(AccessibilityNodeInfo.ROOT_NODE_ID, infoSentToService.getSourceNodeId()); - assertInfoHasExactlyTheseActions(infoSentToService, REQUIRED_ACTIONS_ON_ROOT_TO_SERVICE); + assertThat(infoSentToService, HAS_EXPECTED_ACTIONS_ON_ROOT); } + @Test public void testCallbacks_singleNonrootNodeThenReplacer_returnsNodeWithNoActions() throws RemoteException { AccessibilityNodeInfo infoFromApp = AccessibilityNodeInfo.obtain(); @@ -136,9 +176,10 @@ public class ActionReplacingCallbackTest { eq(INTERACTION_ID)); AccessibilityNodeInfo infoSentToService = mInfoCaptor.getValue(); assertEquals(NON_ROOT_NODE_ID, infoSentToService.getSourceNodeId()); - assertTrue(infoSentToService.getActionList().isEmpty()); + assertThat(infoSentToService, HAS_NO_ACTIONS); } + @Test public void testCallbacks_replacerThenSingleRootNode_returnsNodeWithReplacedActions() throws RemoteException { mActionReplacingCallback.setFindAccessibilityNodeInfosResult(getReplacerNodes(), @@ -154,9 +195,10 @@ public class ActionReplacingCallbackTest { eq(INTERACTION_ID)); AccessibilityNodeInfo infoSentToService = mInfoCaptor.getValue(); assertEquals(AccessibilityNodeInfo.ROOT_NODE_ID, infoSentToService.getSourceNodeId()); - assertInfoHasExactlyTheseActions(infoSentToService, REQUIRED_ACTIONS_ON_ROOT_TO_SERVICE); + assertThat(infoSentToService, HAS_EXPECTED_ACTIONS_ON_ROOT); } + @Test public void testCallbacks_multipleNodesThenReplacer_clearsActionsAndAddsSomeToRoot() throws RemoteException { mActionReplacingCallback @@ -173,11 +215,11 @@ public class ActionReplacingCallbackTest { mInfoListCaptor.getValue(), AccessibilityNodeInfo.ROOT_NODE_ID); AccessibilityNodeInfo otherInfoSentToService = getNodeWithIdFromList( mInfoListCaptor.getValue(), NON_ROOT_NODE_ID); - assertInfoHasExactlyTheseActions( - rootInfoSentToService, REQUIRED_ACTIONS_ON_ROOT_TO_SERVICE); - assertTrue(otherInfoSentToService.getActionList().isEmpty()); + assertThat(rootInfoSentToService, HAS_EXPECTED_ACTIONS_ON_ROOT); + assertThat(otherInfoSentToService, HAS_NO_ACTIONS); } + @Test public void testCallbacks_replacerThenMultipleNodes_clearsActionsAndAddsSomeToRoot() throws RemoteException { mActionReplacingCallback.setFindAccessibilityNodeInfosResult(getReplacerNodes(), @@ -194,18 +236,18 @@ public class ActionReplacingCallbackTest { mInfoListCaptor.getValue(), AccessibilityNodeInfo.ROOT_NODE_ID); AccessibilityNodeInfo otherInfoSentToService = getNodeWithIdFromList( mInfoListCaptor.getValue(), NON_ROOT_NODE_ID); - assertInfoHasExactlyTheseActions( - rootInfoSentToService, REQUIRED_ACTIONS_ON_ROOT_TO_SERVICE); - assertTrue(otherInfoSentToService.getActionList().isEmpty()); + assertThat(rootInfoSentToService, HAS_EXPECTED_ACTIONS_ON_ROOT); + assertThat(otherInfoSentToService, HAS_NO_ACTIONS); } + @Test public void testConstructor_actionReplacerThrowsException_passesDataToService() throws RemoteException { doThrow(RemoteException.class).when(mMockReplacerConnection) .findAccessibilityNodeInfoByAccessibilityId(eq(AccessibilityNodeInfo.ROOT_NODE_ID), - (Region) anyObject(), mInteractionIdCaptor.capture(), - eq(mActionReplacingCallback), eq(0), eq(INTERROGATING_PID), - eq(INTERROGATING_TID), (MagnificationSpec) anyObject(), eq(null)); + (Region) anyObject(), anyInt(), (ActionReplacingCallback) anyObject(), + eq(0), eq(INTERROGATING_PID), eq(INTERROGATING_TID), + (MagnificationSpec) anyObject(), eq(null)); ActionReplacingCallback actionReplacingCallback = new ActionReplacingCallback( mMockServiceCallback, mMockReplacerConnection, INTERACTION_ID, INTERROGATING_PID, INTERROGATING_TID); @@ -214,16 +256,17 @@ public class ActionReplacingCallbackTest { AccessibilityNodeInfo infoFromApp = AccessibilityNodeInfo.obtain(); infoFromApp.setSourceNodeId(AccessibilityNodeInfo.ROOT_NODE_ID, APP_WINDOW_ID); infoFromApp.addAction(ACTION_CONTEXT_CLICK); + infoFromApp.setContextClickable(true); actionReplacingCallback.setFindAccessibilityNodeInfoResult(infoFromApp, INTERACTION_ID); verify(mMockServiceCallback).setFindAccessibilityNodeInfoResult(mInfoCaptor.capture(), eq(INTERACTION_ID)); AccessibilityNodeInfo infoSentToService = mInfoCaptor.getValue(); assertEquals(AccessibilityNodeInfo.ROOT_NODE_ID, infoSentToService.getSourceNodeId()); - assertEquals(1, infoSentToService.getActionList().size()); - assertEquals(ACTION_CONTEXT_CLICK, infoSentToService.getActionList().get(0)); + assertThat(infoSentToService, HAS_NO_ACTIONS); } + @Test public void testSetPerformAccessibilityActionResult_actsAsPassThrough() throws RemoteException { mActionReplacingCallback.setPerformAccessibilityActionResult(true, INTERACTION_ID); verify(mMockServiceCallback).setPerformAccessibilityActionResult(true, INTERACTION_ID); @@ -236,9 +279,9 @@ public class ActionReplacingCallbackTest { AccessibilityNodeInfo root = AccessibilityNodeInfo.obtain(); root.setSourceNodeId(AccessibilityNodeInfo.ROOT_NODE_ID, AccessibilityWindowInfo.PICTURE_IN_PICTURE_ACTION_REPLACER_WINDOW_ID); - for (AccessibilityAction action : ACTIONS_FROM_REPLACER) { - root.addAction(action); - } + root.addAction(ACTION_CLICK); + root.addAction(ACTION_EXPAND); + root.setClickable(true); // Second node should have no effect AccessibilityNodeInfo other = AccessibilityNodeInfo.obtain(); @@ -249,13 +292,6 @@ public class ActionReplacingCallbackTest { return Arrays.asList(root, other); } - private void assertInfoHasExactlyTheseActions( - AccessibilityNodeInfo info, AccessibilityAction[] actions) { - List nodeActions = info.getActionList(); - assertEquals(new HashSet(nodeActions), - new HashSet(Arrays.asList(actions))); - } - private AccessibilityNodeInfo getNodeWithIdFromList( List infos, long id) { for (AccessibilityNodeInfo info : infos) {