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

Fix Crash when tapping on Notification action button in Android 12 #1548

Merged
merged 2 commits into from
Apr 5, 2022

Conversation

vincent-paing
Copy link
Contributor

@vincent-paing vincent-paing commented Mar 15, 2022

Description

One Line Summary

Fixes a crash in Android 12 when interacting with action button due to notification drawer behavior changes

Details

Motivation

This PR addresses the issue #1543 and fixes the crash.

Scope

This will impact the functionality of dismiss on action press. By default, it will remove this close drawer feature completely since it's no longer supported starting from Android 12+. However, if the consumer of the lib uses overrideLibrary flag to change the lib's target sdk, the behavior will still works up to Android 11. But on Android 12, it will does a silent no-op and log the error.

Testing

Unit testing

Not sure if I need to update unit test for this, let me know if it need changes.

Affected code checklist

  • Notifications
    • Display
    • Open
    • Push Processing
    • Confirm Deliveries
  • Outcomes
  • Sessions
  • In-App Messaging
  • REST API requests
  • Public API changes

This change is Reviewable

Copy link
Member

@jkasten2 jkasten2 left a comment

Choose a reason for hiding this comment

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

Thanks for the PR I left some comments in-line on the code. Also could you include the Android versions you have tested on? Does the Notification Shade still close when clicking on an action button?

@vincent-paing
Copy link
Contributor Author

vincent-paing commented Mar 16, 2022

@jkasten2 I didn't particularly tested with the sdk but I made a sample project to replicate the behavior. It will close the notification shade on Android 11 and lower. But it won't on Android 12.

Copy link
Member

@jkasten2 jkasten2 left a comment

Choose a reason for hiding this comment

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

@vincent-paing Thanks for the fix ups, sorry for the delay. This looks good to me to fix the crash.

Since you noted the behavior on Android 12 is that the shade does not close when tapping on a notification button this still needs to be addressed, however I consider this a much lower priority than crashing and therefore can be followed up in another PR.

@nan-li or @Jeasmine could you take a quick review as well and merge in if you think it looks good too.

@nan-li
Copy link
Contributor

nan-li commented Apr 3, 2022

Hi @vincent-paing,
Thank you for submitting this PR!

It will close the notification shade on Android 11 and lower. But it won't on Android 12.

I did some very basic testing with just this SDK's example app, and for the flows I tried, tapping on an action button does close the notification shade for Android 12 as well. It also appeared that handleDismissFromActionButtonPress didn't seem needed either. I may have something different from your setup, or even misunderstanding the problem.

Can you share anything about your sample project or what you tried, that had the behavior of not closing the notification shade on Android 12?

Here's a screencast of an Android 12 emulator with targetSdkVersion 31 set in the app.

action.button.short.clip.sdk.31.api.31.mov

@nan-li nan-li self-requested a review April 4, 2022 02:53
@vincent-paing
Copy link
Contributor Author

@nan-li Are you starting an activity on pressing action? In that case, it will close the shade and launch that activity.

@vincent-paing
Copy link
Contributor Author

vincent-paing commented Apr 4, 2022

@nan-li I made a demo repo where I reproduce the dismiss behavior on pressing an action (https://github.com/vincent-paing/NotificationDemo).

I believe in your code, pressing an action sends intent to NotificationOpenedReceiver, which in turns called NotificationOpenedProcessor.processFromContext(this, intent), which is responsible for executing handleDismissFromActionButtonPress. I basically copied the code from processFromContext to replicate in my repo.

Here's a video recording (left is Android 11, right is Android 12), you can observe

  • It dismiss the notification shade on Android 11
  • It DOES NOT dismiss the notification shade on Android 12
  • It dismiss on Android 12 only if there's no more notification in the panel. This is because of NotificationManagerCompat.cancel function. The OS closes the notification shade if you cancel your notification and it's the only one in the shade.
one_signal_dismiss_demo.mov

@jkasten2 As far as my research goes, there's no way to close the system dialog starting from Android 12 unless you run an accessibility service. I don't think it's possible to address a workaround for the OS changes

@jkasten2
Copy link
Member

jkasten2 commented Apr 5, 2022

@nan-li @vincent-paing Thanks for testing!

Accessibility Service Work around

@jkasten2 As far as my research goes, there's no way to close the system dialog starting from Android 12 unless you run an accessibility service. I don't think it's possible to address a workaround for the OS changes

I agree, we don't wan to use it as it isn't what it is for. Google's docs state the following:

Accessibility services should only be used to assist users with disabilities in using Android devices and apps.

Details on auto close

This automatic close of the notification shade noted here is interesting, however it states "...showing a window that is on top the notification shade" in Google's docs.

  • Your app targets Android 11 or lower and is showing a window that is on top of the notification drawer.

Note: If your app targets Android 12, you don't need to use ACTION_CLOSE_SYSTEM_DIALOGS in this situation. That's because, if your app calls startActivity() while a window is on top of the notification drawer, the system closes the notification drawer automatically.

However in your testing @nan-li it seems even though the the startActivity() isn't showing on top of the notification shade it still seems to close the shade.

@vincent-paing
Copy link
Contributor Author

If it's helpful, In my test if you replace PendingIntent with the one for Activity instead of BroadcastReciever. It always close the shade regardless of whether the app is active or not.

@jkasten2
Copy link
Member

jkasten2 commented Apr 5, 2022

@vincent-paing Thanks for confirming!
@nan-li This looks the behavior we want, so I think this PR does cover the crash and the consistently issue to me. Approve the PR if you think this covers it as well.

@jkasten2
Copy link
Member

jkasten2 commented Apr 5, 2022

If it's helpful, In my test if you replace PendingIntent with the one for Activity instead of BroadcastReciever. It always close the shade regardless of whether the app is active or not.

Ah, this was probably the whole reason for PR #18 added ACTION_CLOSE_SYSTEM_DIALOGS in the first place! The SDK at the time did indeed use BroadcastReciever and was switched to an Activity in the 4.0.0 release. This means we most likely don't need ACTION_CLOSE_SYSTEM_DIALOGS for any version of Android. Since it will take extra testing to confirm this on other Android versions I am still ok with merging this PR in to address the crash.

Copy link
Contributor

@nan-li nan-li left a comment

Choose a reason for hiding this comment

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

I agree with jkasten2 and appreciate the thoroughness @vincent-paing!

I am merging this PR.

@nan-li nan-li merged commit b937465 into OneSignal:main Apr 5, 2022
nan-li added a commit that referenced this pull request Apr 8, 2022
@nan-li nan-li mentioned this pull request Apr 8, 2022
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.

3 participants