1288972 - The media control is hidden when user set the security to "Hide sensitive notification content"

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect, P2)

Description

[William Hsu [:whsu]]

Attachment: MEDIA_CONTROL_IS_HIDDEN_0725.png

@ Environment: 
  Device: Nexus 5 (Android 5.1.1);
  Build: Nightly 50.0a1 (2016-07-19);

@ Steps to reproduce:
  1. Set Sleep=15 seconds on Android Settings app
  2. "Enable pattern lock on Android Settings app (Security->Screen lock->Pattern)
  3. Set notification = "Hide sensitive notification content"
  4. Open Firefox, and then play a video on youtube
  5. Press power button to enter sleep mode
  6. Press power button to wake up device
  7. Use media control to play or pause video

@ Expected result:
  User can use media control to play or pause video

@ Actual result:
  The media control is hidden.

@ Note:
  1. Attach the screenshot (MEDIA_CONTROL_IS_HIDDEN_0725.png)
  2. Please confirm if the test result matches up to UX expectation.

Comment 1

[Rachelle Yang [:ryang][[e-mail]]]

Dear Sebastian, 

Could you please kindly help us confirm if this is the expected UX design as described in the description above  ? 

Thank you very much!!

Comment 2

[Sebastian Kaspari (:sebastian; :pocmo)]

We should be able to control this by setting the visibility:
https://developer.android.com/reference/android/support/v4/app/NotificationCompat.Builder.html#setVisibility(int)

I'm surprised that we hide content because I thought VISIBILITY_PUBLIC would be the default. Anyways, I guess it makes the most sense to use PUBLIC for most tabs, but maybe PRIVATE/SECRET for private tabs (to avoid "leaking" anything from private tabs).

Comment 3

[Rachelle Yang [:ryang][[e-mail]]]

Dear Sebastian, 

thank you very much for your response and useful information.
Since we plan testing remote media control related feature and scenario based on this meta Bug 1268368.
Do you mind providing us UX design guidance on Fennec media control for us in advance ? 

Then we could study the correctness of its behavior before we reach out to keep bothering you...
How do you think ? Thank you very much again !

Comment 4

[Sebastian Kaspari (:sebastian; :pocmo)]

I don't think there are any UX documents on the media controls (at least the notification) but I'll flag antlam; He has been our browser UX guy and might have something.

This scenario here wasn't considered and I think we should just follow the concept of private/normal tabs: Normal = Just show everything. Private = Do not expose it outside of the app. Is there anything in particular that you want to know?

Comment 5

[Rachelle Yang [:ryang][[e-mail]]]

Thank you, Sebastian.I didn't look for anything in particular on UX design of Fennec media control.
Just wanna get an overview of its UX design if any;
But dont worry, it's fine. Thank you very much again !!

Comment 6

[Alastor Wu [:alwu]]

Attachment: Bug 1288972 - set the visibility of the notification depend on tab's private mode.

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

Comment 8

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8776436 [details]
Bug 1288972 - set the visibility of the notification depend on tab's private mode.

https://reviewboard.mozilla.org/r/68200/#review65312

::: mobile/android/base/java/org/mozilla/gecko/media/MediaControlService.java:281
(Diff revision 1)
>  
>          final Notification.MediaStyle style = new Notification.MediaStyle();
>          style.setShowActionsInCompactView(0);
>  
>          final boolean isMediaPlaying = action.equals(ACTION_PAUSE);
> +        final int visibility = mTabReference.get().isPrivate() ?

You should use the tab object passed in as a parameter to updateNotification().

Calling get() on a weak reference can return null. We already did this check earlier and only call this method with a non null tab object.

Comment 9

[Alastor Wu [:alwu]]

Comment on attachment 8776436 [details]
Bug 1288972 - set the visibility of the notification depend on tab's private mode.

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

Comment 10

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8776436 [details]
Bug 1288972 - set the visibility of the notification depend on tab's private mode.

https://reviewboard.mozilla.org/r/68200/#review65332

Comment 11

[Alastor Wu [:alwu]]

It seems that the orange fails doesn't be related with this patch.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=1a3dd7ff10a7

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/1afdac38f42a
set the visibility of the notification depend on tab's private mode. r=sebastian

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/1afdac38f42a

Comment 14

[Sorina Florean [:sflorean]]

Tested using:
Device: LG G4 (Android 5.1);
Build: Nightly 51.0a1 (2016-08-05);
User can use media control to play or pause video, following the steps from comment 0.

Comment 15

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

(In reply to Sebastian Kaspari (:sebastian) from comment #4)
> I don't think there are any UX documents on the media controls (at least the
> notification) but I'll flag antlam; He has been our browser UX guy and might
> have something.
> 
> This scenario here wasn't considered and I think we should just follow the
> concept of private/normal tabs: Normal = Just show everything. Private = Do
> not expose it outside of the app. Is there anything in particular that you
> want to know?

What was the final decision here?

Comment 16

[Alastor Wu [:alwu]]

Just follow what Sebastian said in comment4.
If the tab is private, the information of the control interface would be hided when user decide to hide sensitive notification content. (like the attachment in comment0)

Comment 17

[Sorina Florean [:sflorean]]

Attachment: Screenshot_20160901-165713.png

Verified as fixed in build 51.0a1 (2016-08-31);
Device: Nexus 5 (Android 6.0.1).