From a8416b073eb17790c070cc52684c478535bccc25 Mon Sep 17 00:00:00 2001 From: David Perez Date: Mon, 11 Nov 2024 15:08:05 -0600 Subject: [PATCH] Improve accessibility autofill performance (#4276) --- .../BitwardenAccessibilityService.kt | 3 +- .../BitwardenAccessibilityProcessor.kt | 10 +- .../BitwardenAccessibilityProcessorImpl.kt | 23 ++- .../BitwardenAccessibilityProcessorTest.kt | 175 ++++++++++++------ 4 files changed, 143 insertions(+), 68 deletions(-) diff --git a/app/src/main/java/com/x8bit/bitwarden/data/autofill/accessibility/BitwardenAccessibilityService.kt b/app/src/main/java/com/x8bit/bitwarden/data/autofill/accessibility/BitwardenAccessibilityService.kt index 5c4106866..1f0911d57 100644 --- a/app/src/main/java/com/x8bit/bitwarden/data/autofill/accessibility/BitwardenAccessibilityService.kt +++ b/app/src/main/java/com/x8bit/bitwarden/data/autofill/accessibility/BitwardenAccessibilityService.kt @@ -22,8 +22,7 @@ class BitwardenAccessibilityService : AccessibilityService() { lateinit var processor: BitwardenAccessibilityProcessor override fun onAccessibilityEvent(event: AccessibilityEvent) { - if (rootInActiveWindow?.packageName != event.packageName) return - processor.processAccessibilityEvent(rootAccessibilityNodeInfo = event.source) + processor.processAccessibilityEvent(event = event) { rootInActiveWindow } } override fun onInterrupt() = Unit diff --git a/app/src/main/java/com/x8bit/bitwarden/data/autofill/accessibility/processor/BitwardenAccessibilityProcessor.kt b/app/src/main/java/com/x8bit/bitwarden/data/autofill/accessibility/processor/BitwardenAccessibilityProcessor.kt index a75227804..a08e868a3 100644 --- a/app/src/main/java/com/x8bit/bitwarden/data/autofill/accessibility/processor/BitwardenAccessibilityProcessor.kt +++ b/app/src/main/java/com/x8bit/bitwarden/data/autofill/accessibility/processor/BitwardenAccessibilityProcessor.kt @@ -1,5 +1,6 @@ package com.x8bit.bitwarden.data.autofill.accessibility.processor +import android.view.accessibility.AccessibilityEvent import android.view.accessibility.AccessibilityNodeInfo /** @@ -7,7 +8,12 @@ import android.view.accessibility.AccessibilityNodeInfo */ interface BitwardenAccessibilityProcessor { /** - * Processes the [AccessibilityNodeInfo] for autofill options. + * Processes the [AccessibilityEvent] for autofill options and grant access to the current + * [AccessibilityNodeInfo] via the [rootAccessibilityNodeInfoProvider] (note that calling the + * `rootAccessibilityNodeInfoProvider` is expensive). */ - fun processAccessibilityEvent(rootAccessibilityNodeInfo: AccessibilityNodeInfo?) + fun processAccessibilityEvent( + event: AccessibilityEvent, + rootAccessibilityNodeInfoProvider: () -> AccessibilityNodeInfo?, + ) } diff --git a/app/src/main/java/com/x8bit/bitwarden/data/autofill/accessibility/processor/BitwardenAccessibilityProcessorImpl.kt b/app/src/main/java/com/x8bit/bitwarden/data/autofill/accessibility/processor/BitwardenAccessibilityProcessorImpl.kt index f80675c65..40cb33825 100644 --- a/app/src/main/java/com/x8bit/bitwarden/data/autofill/accessibility/processor/BitwardenAccessibilityProcessorImpl.kt +++ b/app/src/main/java/com/x8bit/bitwarden/data/autofill/accessibility/processor/BitwardenAccessibilityProcessorImpl.kt @@ -2,6 +2,7 @@ package com.x8bit.bitwarden.data.autofill.accessibility.processor import android.content.Context import android.os.PowerManager +import android.view.accessibility.AccessibilityEvent import android.view.accessibility.AccessibilityNodeInfo import android.widget.Toast import com.x8bit.bitwarden.R @@ -25,15 +26,18 @@ class BitwardenAccessibilityProcessorImpl( private val launcherPackageNameManager: LauncherPackageNameManager, private val powerManager: PowerManager, ) : BitwardenAccessibilityProcessor { - override fun processAccessibilityEvent(rootAccessibilityNodeInfo: AccessibilityNodeInfo?) { - val rootNode = rootAccessibilityNodeInfo ?: return + override fun processAccessibilityEvent( + event: AccessibilityEvent, + rootAccessibilityNodeInfoProvider: () -> AccessibilityNodeInfo?, + ) { + val eventNode = event.source ?: return // Ignore the event when the phone is inactive if (!powerManager.isInteractive) return // We skip if the system package - if (rootNode.isSystemPackage) return + if (eventNode.isSystemPackage) return // We skip any package that is a launcher or unsupported - if (rootNode.shouldSkipPackage || - launcherPackageNameManager.launcherPackages.any { it == rootNode.packageName } + if (eventNode.shouldSkipPackage || + launcherPackageNameManager.launcherPackages.any { it == eventNode.packageName } ) { // Clear the action since this event needs to be ignored completely accessibilityAutofillManager.accessibilityAction = null @@ -42,14 +46,19 @@ class BitwardenAccessibilityProcessorImpl( // Only process the event if the tile was clicked val accessibilityAction = accessibilityAutofillManager.accessibilityAction ?: return + // We only call for the root node once after all other checks + // have passed because it is significant performance hit + if (rootAccessibilityNodeInfoProvider()?.packageName != event.packageName) return + + // Clear the action since we are now acting on it accessibilityAutofillManager.accessibilityAction = null when (accessibilityAction) { is AccessibilityAction.AttemptFill -> { - handleAttemptFill(rootNode = rootNode, attemptFill = accessibilityAction) + handleAttemptFill(rootNode = eventNode, attemptFill = accessibilityAction) } - AccessibilityAction.AttemptParseUri -> handleAttemptParseUri(rootNode = rootNode) + AccessibilityAction.AttemptParseUri -> handleAttemptParseUri(rootNode = eventNode) } } diff --git a/app/src/test/java/com/x8bit/bitwarden/data/autofill/accessibility/processor/BitwardenAccessibilityProcessorTest.kt b/app/src/test/java/com/x8bit/bitwarden/data/autofill/accessibility/processor/BitwardenAccessibilityProcessorTest.kt index 19c682ecc..40217beb8 100644 --- a/app/src/test/java/com/x8bit/bitwarden/data/autofill/accessibility/processor/BitwardenAccessibilityProcessorTest.kt +++ b/app/src/test/java/com/x8bit/bitwarden/data/autofill/accessibility/processor/BitwardenAccessibilityProcessorTest.kt @@ -3,6 +3,7 @@ package com.x8bit.bitwarden.data.autofill.accessibility.processor import android.content.Context import android.net.Uri import android.os.PowerManager +import android.view.accessibility.AccessibilityEvent import android.view.accessibility.AccessibilityNodeInfo import android.widget.Toast import com.bitwarden.vault.CipherView @@ -74,10 +75,12 @@ class BitwardenAccessibilityProcessorTest { } @Test - fun `processAccessibilityEvent with null rootNode should return`() { - bitwardenAccessibilityProcessor.processAccessibilityEvent( - rootAccessibilityNodeInfo = null, - ) + fun `processAccessibilityEvent with null event source should return`() { + val event = mockk { + every { source } returns null + } + + bitwardenAccessibilityProcessor.processAccessibilityEvent(event = event) { null } verify(exactly = 0) { powerManager.isInteractive @@ -86,12 +89,12 @@ class BitwardenAccessibilityProcessorTest { @Test fun `processAccessibilityEvent with powerManager not interactive should return`() { - val rootNode = mockk() + val event = mockk { + every { source } returns mockk() + } every { powerManager.isInteractive } returns false - bitwardenAccessibilityProcessor.processAccessibilityEvent( - rootAccessibilityNodeInfo = rootNode, - ) + bitwardenAccessibilityProcessor.processAccessibilityEvent(event = event) { null } verify(exactly = 1) { powerManager.isInteractive @@ -100,38 +103,40 @@ class BitwardenAccessibilityProcessorTest { @Test fun `processAccessibilityEvent with system package should return`() { - val rootNode = mockk { + val node = mockk { every { isSystemPackage } returns true } + val event = mockk { + every { source } returns node + } every { powerManager.isInteractive } returns true - bitwardenAccessibilityProcessor.processAccessibilityEvent( - rootAccessibilityNodeInfo = rootNode, - ) + bitwardenAccessibilityProcessor.processAccessibilityEvent(event = event) { null } verify(exactly = 1) { powerManager.isInteractive - rootNode.isSystemPackage + node.isSystemPackage } } @Test fun `processAccessibilityEvent with skippable package should return`() { - val rootNode = mockk { + val node = mockk { every { isSystemPackage } returns false every { shouldSkipPackage } returns true } + val event = mockk { + every { source } returns node + } every { powerManager.isInteractive } returns true every { accessibilityAutofillManager.accessibilityAction = null } just runs - bitwardenAccessibilityProcessor.processAccessibilityEvent( - rootAccessibilityNodeInfo = rootNode, - ) + bitwardenAccessibilityProcessor.processAccessibilityEvent(event = event) { null } verify(exactly = 1) { powerManager.isInteractive - rootNode.isSystemPackage - rootNode.shouldSkipPackage + node.isSystemPackage + node.shouldSkipPackage accessibilityAutofillManager.accessibilityAction = null } } @@ -139,23 +144,24 @@ class BitwardenAccessibilityProcessorTest { @Test fun `processAccessibilityEvent with launcher package should return`() { val testPackageName = "com.google.android.launcher" - val rootNode = mockk { + val node = mockk { every { isSystemPackage } returns false every { shouldSkipPackage } returns false every { packageName } returns testPackageName } + val event = mockk { + every { source } returns node + } every { launcherPackageNameManager.launcherPackages } returns listOf(testPackageName) every { powerManager.isInteractive } returns true every { accessibilityAutofillManager.accessibilityAction = null } just runs - bitwardenAccessibilityProcessor.processAccessibilityEvent( - rootAccessibilityNodeInfo = rootNode, - ) + bitwardenAccessibilityProcessor.processAccessibilityEvent(event = event) { null } verify(exactly = 1) { powerManager.isInteractive - rootNode.isSystemPackage - rootNode.shouldSkipPackage + node.isSystemPackage + node.shouldSkipPackage launcherPackageNameManager.launcherPackages accessibilityAutofillManager.accessibilityAction = null } @@ -164,23 +170,58 @@ class BitwardenAccessibilityProcessorTest { @Test fun `processAccessibilityEvent without accessibility action should return`() { val testPackageName = "com.android.chrome" - val rootNode = mockk { + val node = mockk { every { isSystemPackage } returns false every { shouldSkipPackage } returns false every { packageName } returns testPackageName } + val event = mockk { + every { source } returns node + } every { launcherPackageNameManager.launcherPackages } returns emptyList() every { accessibilityAutofillManager.accessibilityAction } returns null every { powerManager.isInteractive } returns true - bitwardenAccessibilityProcessor.processAccessibilityEvent( - rootAccessibilityNodeInfo = rootNode, - ) + bitwardenAccessibilityProcessor.processAccessibilityEvent(event = event) { null } verify(exactly = 1) { powerManager.isInteractive - rootNode.shouldSkipPackage - rootNode.isSystemPackage + node.shouldSkipPackage + node.isSystemPackage + launcherPackageNameManager.launcherPackages + accessibilityAutofillManager.accessibilityAction + } + } + + @Test + fun `processAccessibilityEvent with mismatched package name should return`() { + val testPackageName = "com.android.chrome" + val rootNode = mockk { + every { packageName } returns "other.package.name" + } + val node = mockk { + every { isSystemPackage } returns false + every { shouldSkipPackage } returns false + every { packageName } returns testPackageName + } + val event = mockk { + every { source } returns node + every { packageName } returns testPackageName + } + every { powerManager.isInteractive } returns true + every { launcherPackageNameManager.launcherPackages } returns emptyList() + every { + accessibilityAutofillManager.accessibilityAction + } returns AccessibilityAction.AttemptParseUri + every { accessibilityAutofillManager.accessibilityAction = null } just runs + every { accessibilityParser.parseForUriOrPackageName(rootNode = node) } returns null + + bitwardenAccessibilityProcessor.processAccessibilityEvent(event = event) { rootNode } + + verify(exactly = 1) { + powerManager.isInteractive + node.isSystemPackage + node.shouldSkipPackage launcherPackageNameManager.launcherPackages accessibilityAutofillManager.accessibilityAction } @@ -190,30 +231,35 @@ class BitwardenAccessibilityProcessorTest { fun `processAccessibilityEvent with AttemptParseUri and a invalid uri should show a toast`() { val testPackageName = "com.android.chrome" val rootNode = mockk { + every { packageName } returns testPackageName + } + val node = mockk { every { isSystemPackage } returns false every { shouldSkipPackage } returns false every { packageName } returns testPackageName } + val event = mockk { + every { source } returns node + every { packageName } returns testPackageName + } every { powerManager.isInteractive } returns true every { launcherPackageNameManager.launcherPackages } returns emptyList() every { accessibilityAutofillManager.accessibilityAction } returns AccessibilityAction.AttemptParseUri every { accessibilityAutofillManager.accessibilityAction = null } just runs - every { accessibilityParser.parseForUriOrPackageName(rootNode = rootNode) } returns null + every { accessibilityParser.parseForUriOrPackageName(rootNode = node) } returns null - bitwardenAccessibilityProcessor.processAccessibilityEvent( - rootAccessibilityNodeInfo = rootNode, - ) + bitwardenAccessibilityProcessor.processAccessibilityEvent(event = event) { rootNode } verify(exactly = 1) { powerManager.isInteractive - rootNode.isSystemPackage - rootNode.shouldSkipPackage + node.isSystemPackage + node.shouldSkipPackage launcherPackageNameManager.launcherPackages accessibilityAutofillManager.accessibilityAction accessibilityAutofillManager.accessibilityAction = null - accessibilityParser.parseForUriOrPackageName(rootNode = rootNode) + accessibilityParser.parseForUriOrPackageName(rootNode = node) Toast .makeText(context, R.string.autofill_tile_uri_not_found, Toast.LENGTH_LONG) .show() @@ -225,10 +271,17 @@ class BitwardenAccessibilityProcessorTest { fun `processAccessibilityEvent with AttemptParseUri and a valid uri should start the main activity`() { val testPackageName = "com.android.chrome" val rootNode = mockk { + every { packageName } returns testPackageName + } + val node = mockk { every { isSystemPackage } returns false every { shouldSkipPackage } returns false every { packageName } returns testPackageName } + val event = mockk { + every { source } returns node + every { packageName } returns testPackageName + } every { powerManager.isInteractive } returns true every { launcherPackageNameManager.launcherPackages } returns emptyList() every { @@ -243,20 +296,18 @@ class BitwardenAccessibilityProcessorTest { uri = any(), ) } returns mockk() - every { accessibilityParser.parseForUriOrPackageName(rootNode = rootNode) } returns mockk() + every { accessibilityParser.parseForUriOrPackageName(rootNode = node) } returns mockk() - bitwardenAccessibilityProcessor.processAccessibilityEvent( - rootAccessibilityNodeInfo = rootNode, - ) + bitwardenAccessibilityProcessor.processAccessibilityEvent(event = event) { rootNode } verify(exactly = 1) { powerManager.isInteractive - rootNode.isSystemPackage - rootNode.shouldSkipPackage + node.isSystemPackage + node.shouldSkipPackage launcherPackageNameManager.launcherPackages accessibilityAutofillManager.accessibilityAction accessibilityAutofillManager.accessibilityAction = null - accessibilityParser.parseForUriOrPackageName(rootNode = rootNode) + accessibilityParser.parseForUriOrPackageName(rootNode = node) createAutofillSelectionIntent( context = context, framework = AutofillSelectionData.Framework.ACCESSIBILITY, @@ -276,23 +327,28 @@ class BitwardenAccessibilityProcessorTest { val uri = mockk() val attemptFill = AccessibilityAction.AttemptFill(cipherView = cipherView, uri = uri) val rootNode = mockk { + every { packageName } returns testPackageName + } + val node = mockk { every { isSystemPackage } returns false every { shouldSkipPackage } returns false every { packageName } returns testPackageName } + val event = mockk { + every { source } returns node + every { packageName } returns testPackageName + } every { powerManager.isInteractive } returns true every { launcherPackageNameManager.launcherPackages } returns emptyList() every { accessibilityAutofillManager.accessibilityAction } returns attemptFill every { accessibilityAutofillManager.accessibilityAction = null } just runs - bitwardenAccessibilityProcessor.processAccessibilityEvent( - rootAccessibilityNodeInfo = rootNode, - ) + bitwardenAccessibilityProcessor.processAccessibilityEvent(event = event) { rootNode } verify(exactly = 1) { powerManager.isInteractive - rootNode.isSystemPackage - rootNode.shouldSkipPackage + node.isSystemPackage + node.shouldSkipPackage launcherPackageNameManager.launcherPackages accessibilityAutofillManager.accessibilityAction accessibilityAutofillManager.accessibilityAction = null @@ -325,31 +381,36 @@ class BitwardenAccessibilityProcessorTest { val uri = mockk() val attemptFill = AccessibilityAction.AttemptFill(cipherView = cipherView, uri = uri) val rootNode = mockk { + every { packageName } returns testPackageName + } + val node = mockk { every { isSystemPackage } returns false every { shouldSkipPackage } returns false every { packageName } returns testPackageName } + val event = mockk { + every { source } returns node + every { packageName } returns testPackageName + } every { powerManager.isInteractive } returns true every { launcherPackageNameManager.launcherPackages } returns emptyList() every { accessibilityAutofillManager.accessibilityAction } returns attemptFill every { accessibilityAutofillManager.accessibilityAction = null } just runs every { - accessibilityParser.parseForFillableFields(rootNode = rootNode, uri = uri) + accessibilityParser.parseForFillableFields(rootNode = node, uri = uri) } returns fillableFields - bitwardenAccessibilityProcessor.processAccessibilityEvent( - rootAccessibilityNodeInfo = rootNode, - ) + bitwardenAccessibilityProcessor.processAccessibilityEvent(event = event) { rootNode } verify(exactly = 1) { powerManager.isInteractive - rootNode.isSystemPackage - rootNode.shouldSkipPackage + node.isSystemPackage + node.shouldSkipPackage launcherPackageNameManager.launcherPackages accessibilityAutofillManager.accessibilityAction accessibilityAutofillManager.accessibilityAction = null cipherView.login - accessibilityParser.parseForFillableFields(rootNode = rootNode, uri = uri) + accessibilityParser.parseForFillableFields(rootNode = node, uri = uri) mockUsernameField.fillTextField(testUsername) mockPasswordField.fillTextField(testPassword) }