Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[PM-14435] Accessibility enabled settings changes to address older and custom Android phone versions #4756

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

aj-rosado
Copy link
Contributor

@aj-rosado aj-rosado commented Feb 20, 2025

🎟️ Tracking

https://bitwarden.atlassian.net/browse/PM-14435

📔 Objective

Testing on a Xiaomi phone with Android 10 showed some discrepancies to android 15 and emulator versions.

The AccessibilityManager.isEnabled was returning true if any Accessibility app was turned on.

Reading from Settings.Secure.ENABLED_ACCESSIBILITY_SERVICES and checking is this service exists there addresses this behaviour.

AccessibilityStateChangeListener also only was called when any of the accessibility apps was turned on or all turned off

Updating the mutableIsAccessibilityEnabledStateFlow when AccessibilityService's onServiceConnected and onUnbind keeps the value correctly updated.

In some scenarios, shortcuts get inconsistent and stopped updating correctly the accessibility switch, MAUI app showed a similar behaviour although it always had the correct value when entering the view.
Checking the Settings.Secure.ENABLED_ACCESSIBILITY_SERVICES when onResume method is called to ensure it has the most recent setting is always displayed

⏰ Reminders before review

  • Contributor guidelines followed
  • All formatters and local linters executed and passed
  • Written new unit and / or integration tests where applicable
  • Used internationalization (i18n) for all UI strings
  • CI builds passed
  • Communicated to DevOps any deployment requirements
  • Updated any necessary documentation or informed the documentation team

🦮 Reviewer guidelines

  • 👍 (:+1:) or similar for great changes
  • 📝 (:memo:) or ℹ️ (:information_source:) for notes or general info
  • ❓ (:question:) for questions
  • 🤔 (:thinking:) or 💭 (:thought_balloon:) for more open inquiry that's not quite a confirmed
    issue and could potentially benefit from discussion
  • 🎨 (:art:) for suggestions / improvements
  • ❌ (:x:) or ⚠️ (:warning:) for more significant problems or concerns needing attention
  • 🌱 (:seedling:) or ♻️ (:recycle:) for future improvements or indications of technical debt
  • ⛏ (:pick:) for minor or nitpick changes

…nabledState

Checking the Settings.Secure.ENABLED_ACCESSIBILITY_SERVICES when navigating to Autofill settings screen.
Copy link
Contributor

Logo
Checkmarx One – Scan Summary & Detailsaa8ca1ce-1988-4fcd-a8fe-99ab473713c2

Great job, no security vulnerabilities found in this Pull Request

Copy link
Contributor

Warning

@aj-rosado Uploading code coverage report failed. Please check the "Upload to codecov.io" step of Process Test Reports job for more details.

LivecycleEventEffect { _, event ->
when (event) {
Lifecycle.Event.ON_RESUME -> {
viewModel.trySendAction(AutoFillAction.LifecycleResume)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should not be bound to a specific screen, can we listen to foreground events in a manager, we already do similar things like this in the AutofillActivityManagerImpl.

Copy link
Contributor Author

@aj-rosado aj-rosado Feb 21, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We are updating onForeground, although if an user uses a shortcut to update and doesn't leave the app, in some scenarios we might not have the most recent setting.

also only this screen listens to the stateflow, so I guess updating on this screen is ok

)

init {
accessibilityManager.addAccessibilityStateChangeListener(
Copy link
Collaborator

@david-livefront david-livefront Feb 20, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we just use this callback and instead of using the passed in boolean, use context.isAccessibilityServiceEnabled.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On contrary to emulator and Android 15, on a xiaomi with android 10 that I've used to test this, this is only triggered when any accessibility is turned on or all turned off.

If there is other accessibility app active (like accessibility menu) and the user activates bitwarden, this listener won't be called.

OnServiceConnected and OnUnbind properly detects AccessibilityService being detected on that device

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmmm, are the OnServiceConnected and OnUnbind callbacks enough by themselves or do we still need the onResumed listener?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it should, but for some reason, they stop listening after some time. When this happens, the accessibility shortcuts also start having an odd behaviour, they always show a dialog turning on the accessibility, never turning accessibility off (usually it works fine turning on and off). Restarting the application doesn't solve this, only reinstalling.

I'm experiencing a similar behaviour (always turning on) on the old MAUI app so I don't believe this is a regression, but MAUI app was showing the correct state when entering the view and the onResume call is to address that

Copy link
Contributor

Warning

@aj-rosado Uploading code coverage report failed. Please check the "Upload to codecov.io" step of Process Test Reports job for more details.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants