ionic-team / capacitor

Build cross-platform Native Progressive Web Apps for iOS, Android, and the Web ⚡️
https://capacitorjs.com
MIT License
11.47k stars 977 forks source link

[Android] tapping push notification does nothing when app not running or in background #1475

Closed CFT-Chris closed 5 years ago

CFT-Chris commented 5 years ago

Description of the problem:

Affected platform

OS of the development machine

Other information: This is the same as issue #1192, which was closed under the impression it was fixed with the latest beta (22). It still reproduces for me, but I have investigated it and am putting in a PR very soon.

Capacitor version: beta.22

node version: 10.13

npm version: 6.4

CocoaPods version: n/a

Steps to reproduce:

Send Firebase (FCM) message to Capacitor app not running in foreground. Tap on notification in Android's notification panel.

Either nothing happens (app is not started or background app not brought to foreground), or the app is started up without sending the notification data to any of its listeners pushNotificationReceived or pushNotificationActionPerformed.

Link to sample project: n/a

jcesarmobile commented 5 years ago

Can you show your notification payload? It works fine for me without all the changes in your PR. Edit: as you are using firebase console, It works for me even when sending from there. What’s your android version? Is it a new app or an update?

CFT-Chris commented 5 years ago

Thanks for the quick reply.

I am testing on Android 5.0 (minimum required by Capacitor).

I tried all payload combinations. Without "notification", then without "data", with both "notification" and "data", with "click_event" and without, etc.

Just an example of what I am sending (via Node.js backend POST request to Firebase cloud) which works with the changes I proposed in the PR:

request({
  url: 'https://fcm.googleapis.com/fcm/send',
  method: 'POST',
  headers: {
    'Content-Type' :' application/json',
    'Authorization': 'key=' + /* FCM key */
  },
  body: JSON.stringify({
    to: / *** PushReg Id ***/,
    data: { tm: Date.now() },
    notification: {
      sound: 'default',
      click_action: "CAPACITOR_FIREBASE_MESSAGING_ACTIVITY"
    }
  }
})

Without "notification" the notification doesn't even arrive at all if the app is not running (background or killed).

Just looking at the code itself, I can't figure out how Android ends up calling handleOnNewIntent in PushNotifications.java as there is nothing in the AndroidManifests, apart from CapacitorFirebaseMessagingService, that might deal with incoming Firebase messages when the app is not running in the foreground.

In my case, when the Capacitor app is in the background or not running, the Firebase message results in a notification in Android's notification panel, and tapping that notification does not make CapacitorFirebaseMessagingService's onMessageReceived handle it.

Could be old Android behaviour that doesn't repro in later OS versions, but regardless, how would the code inside handleOnNewIntent (pushnotifications.java) get called, and how does any listener to pushNotificationActionPerformed get anything? Do later versions of Android somehow know to hook into the PushNotifications plugin and send an Intent there?

Sorry if I overlooked something obvious.

CFT-Chris commented 5 years ago

I guess I should also elaborate that I have just read #1349 and noticed that @jurepurgar mentioned that for Android, his fix is partial and provides a step for the changes I'm proposing.

Inadvertently, I think I may have come up with the other half of the fix (setting MainActivity to singleTask, sending the data if the app is closed once a listener is added, etc.), and the way I get it to call his onNewIntent code for Android is to use the "click_action", which is a method employed by the Cordova FCM plugin for Ionic Native.

And one more thing, my payload example above omitted "title" and "body", but they are there. I mistakenly edited them out when copying from my code and pasting to here.

jcesarmobile commented 5 years ago

Supposedly, if there is no click action it should open the BridgeActivity, and BridgeActivity have the code to handle the intent, it pass the data to the Bridge and the bridge send it to the plugins. But will double check tomorrow

CFT-Chris commented 5 years ago

Ok, that info is very helpful. I didn't realize that MainActivity eventually had its Intents routed to handleOnNewIntent in the PushNotifications plugin. I think I can accurately narrow down the cases that fail for me.

tl;dr: The notification in the payload must be present, but click_action must not be in the notification!

Sample FCM payloads: Bad: { data: { title: 'foo', body: 'bar' } } Bad: { data: { title: 'foo', body: 'bar' }, notification: { click_action: 'FCM_PLUGIN_ACTIVITY' } } Good: { data: { title: 'foo', body: 'bar' }, notification: {} } Good: { data: { title: 'foo', body: 'bar' }, notification: { sound: true } }

I still think android:launchMode="singleTop" (anything but standard) is necessary for MainActivity.

Here were my test cases and the results (Android 5.0):

I'm glad that's all cleared up now! Still would like to push a PR though to change the launchMode to singleTop to prevent the Ionic app restarting as a result of a new activity being created every time a push notification is tapped. As a result of recreating MainActivity we get the problems of re-init'ing/re-loading BridgeActivity leading to the bad behaviour described above.

solojuve1897 commented 5 years ago

Does this mean the solution is using singleTop and that other options like singleTask or singleInstance doesn't work fully? Because I'm pretty sure I have some of the issues you are referring to even though I'm using singleTask. Need to test this more today. Will write a comment about my results.

solojuve1897 commented 5 years ago

My bad. Works fine and I'm using singleTask. Would be good to know though which one is best and why: singleTop, singleInstance or singleTask.

CFT-Chris commented 5 years ago

@solojuve1897 It is described in the Android docs albeit in language that isn't exactly clear and devoid of useful examples. I'll be the first to admit that I may have interpreted the whole launchMode thing wrong.

We can use "singleTop" here because to my understanding, after @jcesarmobile pointed to me that all the plugins get their Intents via Bridge (which is what MainActivity extends through BridgeActivity). In that case, there are no other activities than MainActivity that should live on the stack.

But in my opinion, because of that paradigm, there is no reason why we couldn't have it be "singleTask" or "singleInstance", because it will always just be that singular MainActivity and nothing else. I mentioned that I believe any mode but "standard" will do.

But now that you've got me thinking more about it, I peeked into the AndroidManifest.xml of capacitor-cordova-android-plugins and it is still possible for other activities to exist having "standard" launchMode, in which case "singleTop" may not be desirable anymore for MainActivity. If other activities are on the stack, the next push notification that gets tapped will cause MainActivity to be recreated, leading to aforementioned bad behaviour. "SingleTask" thus might be more appropriate in my PR.

On the other hand, what sort of activities (from Cordova plugins) have a long enough lifecycle that they are not destroyed by the time MainActivity receives another Intent to give to Capacitor plugins? There possibly are apps out there that do want the behaviour of "singleTop" over "singleTask" because they have activities in their plugins with long lifecycles and do want MainActivity to recreate itself. I can't think of any such plugins and scenarios at the moment, but I bet they exist.

All I believe is that the "standard" launchMode for MainActivity has to be the least desirable when we are considering how the Capacitor PushNotifications and LocalNotifications plugins are installed by default and they get their Intents via MainActivity. They need to avoid the problems of a new instance popping up, or the app appearing to restart every time with the possibility of data loss, every time they get an Intent.

@jcesarmobile can enlighten us!

jcesarmobile commented 5 years ago

I've been giving some thought and testing and I think it should be singleTask instead of singleTop.

The problem with singleTop is, if you background the app while using some plugin that presents and activity (camera, browser, etc), and a new intent is launched, as the Capacitor activity is not on the top, then a new one is created, resulting in a duplicated activity.

By using singleTask, the activities that are over the Capacitor activity will be destroyed and current Capacitor activity will be used, so it won't be duplicated ever. The only problem with this is the plugin activities being destroyed, but I don't think its a big problem as if the user left the app with a plugin activity open was probably by mistake and I think it's better to have the app starting from the Capacitor activity than having multiple Capacitor activities.

@CFT-Chris if yo agree, can you update your PR to use singleTask instead?

CFT-Chris commented 5 years ago

Sounds good to me. I couldn't think of common scenarios where singleTop would be preferred, but I think any apps that desire such behavior are free to change the launchMode in AndroidManifest.xml manually.

PR is updated.

jcesarmobile commented 5 years ago

Fixed by https://github.com/ionic-team/capacitor/pull/1477

vadimrostok commented 3 years ago

We are moving to Capacitor and needed to support old and new systems, so notifications contained click_action: ST_ACTIVATE field. The solution was to add following intent-filter to main activity in AndroidManifest.xml:

<intent-filter>
  <action android:name="ST_ACTIVATE" />
  <category android:name="android.intent.category.DEFAULT" />
</intent-filter>
ionitron-bot[bot] commented 1 year ago

Thanks for the issue! This issue is being locked to prevent comments that are not relevant to the original issue. If this is still an issue with the latest version of Capacitor, please create a new issue and ensure the template is fully filled out.