1477700 - Android media notification appears swiped down after video starts

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Levente Sacal]

Device(s):
 -  Samsung S8+ (Android 8.0.0);
 -  Google Pixel XL (Android P);

Build(s):
 - Nightly 63.0a1 (2018-07-22);


Steps to reproduce:
 1. Start any video.
 2. Wait for the video to start


Expected result:
- Android media notification only appears when swiping down the status bar

Actual result:
- Android media notification appears swiped down after video starts 

Notes:
- Android media notification appears when pausing/resuming the video

https://youtu.be/-20l0sG96ZE

Comment 1

[Jan Henning [:JanH]]

Same basic issue as bug 1476966 - because of bug 1450447 everything is using only one notification channel which has been defined as high priority.

Comment 2

[Andrei Lazar]

Attachment: Bug 1477700 Android media notification appears swiped down after video starts

Created a dedicated channel for media notifications.

Review commit: https://reviewboard.mozilla.org/r/259396/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/259396/

Comment 3

[Jan Henning [:JanH]]

Comment on attachment 8994854 [details]
Bug 1477700 Android media notification appears swiped down after video starts

https://reviewboard.mozilla.org/r/259396/#review266412

::: mobile/android/base/java/org/mozilla/gecko/GeckoApplication.java:453
(Diff revision 1)
>              notificationManager.createNotificationChannel(defaultNotificationChannel);
>          }
>      }
>  
> +    @TargetApi(26)
> +    private void createMediaNotificationChannel() {

The amount of Notificaion channels will only go up, so maybe it might make sense to move the whole infrastructure for that into a separate (static?) helper class?

::: mobile/android/base/java/org/mozilla/gecko/GeckoApplication.java:455
(Diff revision 1)
>      }
>  
> +    @TargetApi(26)
> +    private void createMediaNotificationChannel() {
> +        final String DEFAULT_CHANNEL = AppConstants.MOZ_APP_DISPLAYNAME + getString(R.string.notification_channel_media);
> +        final String DEFAULT_NAME = AppConstants.MOZ_APP_DISPLAYNAME + getString(R.string.notification_channel_media);

I don't have a phone running Oreo, but the UI for managing notification channels [looks something like this](https://developer.android.com/training/notify-user/channels), right?

In that context, including the app name within the user visible channel name doesn't seem necessary.

::: mobile/android/base/locales/en-US/android_strings.dtd:896
(Diff revision 1)
>  <!ENTITY pip_play_button_title "Play">
>  <!ENTITY pip_play_button_description "Resume playing">
>  <!ENTITY pip_pause_button_title "Pause">
>  <!ENTITY pip_pause_button_description "Pause playing">
> +
> +<!ENTITY notification_channel_media " media channel">

"Media channel" is too technical, within the context of the settings menu (see above) users won't care that those things are called "channels".

Just use something descriptive for the kind of notification it encompasses, like maybe "Media playback"? If in doubt, ask someone from UX.

Comment 4

[Jan Henning [:JanH]]

Comment on attachment 8994854 [details]
Bug 1477700 Android media notification appears swiped down after video starts

https://reviewboard.mozilla.org/r/259396/#review266486

::: mobile/android/base/java/org/mozilla/gecko/GeckoApplication.java:456
(Diff revision 1)
>  
> +    @TargetApi(26)
> +    private void createMediaNotificationChannel() {
> +        final String DEFAULT_CHANNEL = AppConstants.MOZ_APP_DISPLAYNAME + getString(R.string.notification_channel_media);
> +        final String DEFAULT_NAME = AppConstants.MOZ_APP_DISPLAYNAME + getString(R.string.notification_channel_media);
> +        final int DEFAULT_IMPORTANCE = NotificationManager.IMPORTANCE_DEFAULT;

Also those are just local variables, so shouldn't they rather be camel-cased?

Comment 5

[Andrei Lazar]

Comment on attachment 8994854 [details]
Bug 1477700 Android media notification appears swiped down after video starts

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/259396/diff/1-2/

Comment 6

[Jan Henning [:JanH]]

Comment on attachment 8994854 [details]
Bug 1477700 Android media notification appears swiped down after video starts

https://reviewboard.mozilla.org/r/259396/#review268812

Okay with the string change.

::: mobile/android/base/locales/en-US/android_strings.dtd:900
(Diff revision 2)
>  <!ENTITY pip_pause_button_description "Pause playing">
>  
>  <!-- Notification channels names -->
>  <!ENTITY default_notification_channel "&brandShortName;">
>  <!ENTITY mls_notification_channel "&vendorShortName; Location Service">
> +<!ENTITY media_notification_channel "Media notifications">

I think I'd still prefer "Media playback" here - an App Notifications preference screen where half of our notification categories end in "... notifications" doesn't seem that elegant, either.

Comment 7

[Andrei Lazar]

Comment on attachment 8994854 [details]
Bug 1477700 Android media notification appears swiped down after video starts

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/259396/diff/2-3/

Comment 8

[Mozilla Autoland]

We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s 4a4ff33406ab98124b758d9560c20dfa1b74cfbb -d 925d2fd06a24: rebasing 477531:4a4ff33406ab "Bug 1477700 Android media notification appears swiped down after video starts r=JanH" (tip)
merging mobile/android/base/java/org/mozilla/gecko/notifications/NotificationHelper.java
merging mobile/android/base/locales/en-US/android_strings.dtd
merging mobile/android/base/strings.xml.in
warning: conflicts while merging mobile/android/base/java/org/mozilla/gecko/notifications/NotificationHelper.java! (edit, then use 'hg resolve --mark')
warning: conflicts while merging mobile/android/base/locales/en-US/android_strings.dtd! (edit, then use 'hg resolve --mark')
warning: conflicts while merging mobile/android/base/strings.xml.in! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)

Comment 9

[Cristian Brindusan [:cbrindusan]]

I couldn't land your patch. Andrei, please set the issues opened by the reviewer as fixed by commit so review board allows to land them. Thank you.

Comment 10

[Andrei Lazar]

Comment on attachment 8994854 [details]
Bug 1477700 Android media notification appears swiped down after video starts

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/259396/diff/3-4/

Comment 11

[Andrei Lazar]

Comment on attachment 8994854 [details]
Bug 1477700 Android media notification appears swiped down after video starts

https://reviewboard.mozilla.org/r/259396/#review269436

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/33d4952d34f9
Android media notification appears swiped down after video starts r=JanH

Comment 13

[Arthur Iakab [arthur_iakab]]

https://hg.mozilla.org/mozilla-central/rev/33d4952d34f9

Comment 14

[Oana Horvath [:ohorvath]]

Verified as fixed on Nightly 63.
Google Pixel (Android P Beta)
Samsung Galaxy S8 (Android 8.0)
Nokia 6 (Android 7.1.1)
Huawei P9 Lite (Android 6.0)