897711 - [guest] Provide an indicator for guest mode in the UI

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Wesley Johnston (:wesj)]

Right now its a bit confusing for users to know whether they're in guest mode or not, which can lead to you not knowing what's happened to your data. It will probably get more confusing if we enforce always restarting in guest mode.I wonder if we can do something simple like a built in persona to help them tell the difference?

Comment 1

[Lucas Rocha (:lucasr)]

Something similar to private browsing maybe (special colour on toolbar)?

Comment 2

[Ian Barlow (:ibarlow)]

One approach might be to 'hack' our pageaction icon area and place an icon there when you enter a Guest Browsing session. http://cl.ly/image/41030a2v0C2k

Comment 3

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to Ian Barlow (:ibarlow) from comment #2)
> One approach might be to 'hack' our pageaction icon area and place an icon
> there when you enter a Guest Browsing session.
> http://cl.ly/image/41030a2v0C2k

Part of me wants to like this idea. The other part says we are using the *page* actions for an *application* indicator.

Comment 4

[Ian Barlow (:ibarlow)]

Assigning to Anthony for some visual design goodness. :)

Comment 5

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

Focused on the motif of a "guest badge" for this idea. What do you guys think of translucent browser chrome to indicate this "Guest mode" browsing? For me, it really drives home the idea of being "temporary" and I came up with a quick mock up to illustrate my point. 

Obviously the transparency will only become evident upon scrolling back up (since the chrome hides as soon as you scroll down). As an answer to this, I've made the colors by default to be a softer variant of the default chrome colors. This is just enough to show that it's different, without feeling completely foreign. 

http://cl.ly/image/3Y212B1E442w

Comment 6

[Aaron Train [:aaronmt]]

I think it looks like a bug to be honest. I don't think it's context is clear.

Comment 7

[Mark Finkle (:mfinkle) (use needinfo?)]

If I might offer a different approach. We could use an indicator in the system notification try. Kinda like the one we use for an active WebRTC session, this would be a small "guest browsing" image that is persistent while Firefox is in guest mode.

Comment 8

[Ian Barlow (:ibarlow)]

(In reply to Mark Finkle (:mfinkle) from comment #7)
> If I might offer a different approach. We could use an indicator in the
> system notification try. Kinda like the one we use for an active WebRTC
> session, this would be a small "guest browsing" image that is persistent
> while Firefox is in guest mode.

That could help, yeah. I also think the colour shift of the title bar between normal and guest browsing should be a little stronger. The way this feature works is that the browser restarts itself between sessions, so Anthony I worry that the current colour in your current design may be a little too close to the normal browser bar and users might not pick up on the difference.

As an aside, I actually really like the transparent toolbar idea. So much in fact that I *don't* want to rush to use it here, but think through whether it's a thing we would want as part of a potential kit-kat mini refresh, since transparency is becoming more common there. Something to think about some more.

Comment 9

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

(In reply to Mark Finkle (:mfinkle) from comment #7)
> If I might offer a different approach. We could use an indicator in the
> system notification try. Kinda like the one we use for an active WebRTC
> session, this would be a small "guest browsing" image that is persistent
> while Firefox is in guest mode.

Great idea mfinkle! Android uses the bar like that for notifications and I think it makes sense for Guest Mode browsing to do the same.

(In reply to Ian Barlow (:ibarlow) from comment #8)
> That could help, yeah. I also think the colour shift of the title bar
> between normal and guest browsing should be a little stronger. The way this
> feature works is that the browser restarts itself between sessions, so
> Anthony I worry that the current colour in your current design may be a
> little too close to the normal browser bar and users might not pick up on
> the difference.
> 
> As an aside, I actually really like the transparent toolbar idea. So much in
> fact that I *don't* want to rush to use it here, but think through whether
> it's a thing we would want as part of a potential kit-kat mini refresh,
> since transparency is becoming more common there. Something to think about
> some more.

Fair points all around. I've had concerns with that being "obvious enough" or not as well. I think the change might be an obvious enough factor for the user. If not, I think the icon that Mark suggested would drive it home as it would be ever present along the top. In any case, I made some revisions as per the feedback. Maybe the "quick win" here would be to just do the icon in the notification bar?

http://cl.ly/image/2V3L0G1D171l

Comment 10

[Ian Barlow (:ibarlow)]

The notification bar icon definitely helps. We would need some kind of text to go with it, like

--------------------------------------
Firefox
Guest browsing is on
--------------------------------------

or something. 

As for the new title bar colour, this is nice too, though still fairly subtle. Would it be possible, Mark or Lucas, to get a test build with the notification bar stuff in place, as well as this new title bar colour?

Comment 11

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to Ian Barlow (:ibarlow) from comment #8)

> That could help, yeah. I also think the colour shift of the title bar
> between normal and guest browsing should be a little stronger. The way this
> feature works is that the browser restarts itself between sessions, so
> Anthony I worry that the current colour in your current design may be a
> little too close to the normal browser bar and users might not pick up on
> the difference.

For browser restarts, I think we should display a simple button toast at startup, letting the user know they are Guest Browsing, and a button to exit. I favor this approach because it's explicit. I don't think the color change is strong enough.

Comment 12

[Ian Barlow (:ibarlow)]

It might be time to pick this up again. Telemetry is showing us that for every group of users who enter Guest Browsing, a much smaller number of people are shown to exit. Which implies that they don't realize they are still stuck in this in-between state.

One idea that just came up in IRC was to use the android system notifications to provide a more obvious indicator that Firefox is in a different state. So when a user enters Guest Browsing, they would see a persistent notification with a Firefox icon that might read something like 

----------------------------------------------
[title] Guest Browsing is currently enabled
[text] Tap to exit
----------------------------------------------

This notification would persist until either 
a. they tap this to restart Firefox in normal mode, or 
b. they use the menu > exit Guest Browsing path to do the same thing

Comment 13

[Wesley Johnston (:wesj)]

Attachment: Patch 1/3 - Make NotificationHelper work in Java.

I started to do this in JS today, and realized there could be a wait between startup and the JS code having a chance to run. A cleaner and better way would probably be to do this in Java. NotificationHelper isn't really ready to be used from Java, so this gets it there.

We could just use NotificationCompat directly here, but that meant injecting (more) listeners for intents in BrowserApp. I'd like to find better ways to do that, so here I'm trying.

This first patch just abstracts NotificationHelper to generate an Info object instead, with all of the info about what we'll show in a Notification. This includes the new handler "keys" that I created in bug 1004495, so that we can register handlers to be notified when a notification they care about is clicked.

Future me wants to make the info serializable so we can store it with the notification's pending intents.

Comment 14

[Wesley Johnston (:wesj)]

Attachment: Patch 2/3 - Register handlers in Java

This patch lets you register a "handler" in java that gets notified when a notification intent comes back that has the right handler "key", just like in JS. There's also a jsHandler that forwards all of these on to Gecko so that they can be handled there.

Comment 15

[Wesley Johnston (:wesj)]

Attachment: Patch 3/3 - Show notification in guest mode

And this shows a notification in guest mode. I'd hoped to do that all in GuestMode.java. i.e. when GuestMode is enabled, we show a notification! Unfortunately, we never really "enable" GuestMode. We just get a profile. Sometimes its the guest one. Sometimes not. So for now BrowserApp just shows this.

Comment 16

[Wesley Johnston (:wesj)]

Attachment: Patch 4/3 - JS Notifications

Here's the JS Notifications patch. Saved for history to enjoy.

Comment 17

[Brian Nicholson (:bnicholson)]

Comment on attachment 8456555 [details] [diff] [review]
Patch 1/3 - Make NotificationHelper work in Java.

Review of attachment 8456555 [details] [diff] [review]:
-----------------------------------------------------------------

I think this looks good, but just want some clarification of your comment before r+. At first glance, it seems like we should be able to drop the Info class and just use NotificationCompat.Builder directly like you said, so I'd like to understand why that won't work well.

(In reply to Wesley Johnston (:wesj) from comment #13)
> We could just use NotificationCompat directly here, but that meant injecting
> (more) listeners for intents in BrowserApp.

Can you elaborate? Just trying to figure out how using a NotificationCompat.Builder would affect our listener pattern.

::: mobile/android/base/NotificationHelper.java
@@ +267,5 @@
> +                info = new Info(message.getString(TITLE_ATTR),
> +                                message.getString(TEXT_ATTR),
> +                                message.getString(ID_ATTR));
> +            } catch (JSONException ex) {
> +                Log.i(LOGTAG, "Error parsing", ex);

Nit: Log.e (here and others below)

Comment 18

[Brian Nicholson (:bnicholson)]

Comment on attachment 8456557 [details] [diff] [review]
Patch 2/3 - Register handlers in Java

Review of attachment 8456557 [details] [diff] [review]:
-----------------------------------------------------------------

::: mobile/android/base/NotificationHelper.java
@@ +29,3 @@
>  
>  public final class NotificationHelper implements GeckoEventListener {
> +    public interface Handler {

Handler is kind of a confusing name since there's already an android.os.Handler, which we use frequently. Maybe change this to IntentHandler, or even NotificationIntentHandler?

@@ +187,5 @@
> +                    final String actionName = data.getQueryParameter(ACTION_ID_ATTR);
> +                    args.put(ACTION_ID_ATTR, actionName);
> +                }
> +
> +                Log.i(LOGTAG, "Send " + args.toString());

Should probably remove this before landing (or at least change to Log.d).

@@ +194,5 @@
> +                Log.e(LOGTAG, "Error building JSON notification arguments.", e);
> +            }
> +
> +            // If the notification was clicked, we are closing it. This must be executed after
> +            // sending the eventst to js side because when the notification is canceled no event can be

s/eventst/event/

@@ +196,5 @@
> +
> +            // If the notification was clicked, we are closing it. This must be executed after
> +            // sending the eventst to js side because when the notification is canceled no event can be
> +            // handled.
> +            if (CLICK_EVENT.equals(notificationType) && !i.getBooleanExtra(ONGOING_ATTR, false)) {

Could we move this block to the end of handleNotificationIntent instead, since it's separate from the JS handling?

Comment 19

[Brian Nicholson (:bnicholson)]

Comment on attachment 8456560 [details] [diff] [review]
Patch 3/3 - Show notification in guest mode

Review of attachment 8456560 [details] [diff] [review]:
-----------------------------------------------------------------

::: mobile/android/base/BrowserApp.java
@@ +620,5 @@
>              }
>          }
> +
> +        if (GuestMode.enabled(this)) {
> +            GuestMode.getInstance(this).showNotification(this);

Can we just init GuestMode once with an application context somewhere, or does it need an activity context? It would be nice to avoid going crazy with passing "this" around everywhere. (Remember that the application context doesn't change, so there's no potential for leaking if that's what you're worried about).

::: mobile/android/base/GuestMode.java
@@ +177,5 @@
>              restart(context, !enable);
>          }
>      }
>  
> +    public static GuestMode getInstance(Context context) {

This separation of static methods with the singleton GuestMode is a bit weird. Instead of putting the new getInstance/constructor on GuestMode, could you create a separate nested class for the notification handling? That way you could do something like this:

GuestMode.getNotificationInstance(this).show(info);

Or, maybe even better, show() could lazily init the notification instance on its own (since show() is the only method that needs the instance anyway). So perhaps something like this:

GuestMode.showNotification(this, info);

which would allow actual instance logic to remain internal.

Comment 20

[Brian Nicholson (:bnicholson)]

Comment on attachment 8456555 [details] [diff] [review]
Patch 1/3 - Make NotificationHelper work in Java.

Switching to f+ for now until comment 17 is answered.

Comment 21

[Wesley Johnston (:wesj)]

(In reply to Brian Nicholson (:bnicholson) from comment #17)
> Can you elaborate? Just trying to figure out how using a
> NotificationCompat.Builder would affect our listener pattern.

I mostly hate that we can't pass a listener along with the builder to be called later. We have to register an intent in the manifest, grab it in BrowserApp, and then forward it on to the real handler which feels clunky, but makes sense given the lifecycles of these things. I'm going to try and do this with the Compat.Builder.

Comment 22

[Gervase Markham [:gerv]]

(In reply to Ian Barlow (:ibarlow) from comment #12)
> Telemetry is showing us that for
> every group of users who enter Guest Browsing, a much smaller number of
> people are shown to exit. Which implies that they don't realize they are
> still stuck in this in-between state.

Well, I seem to have got my Aurora stuck in a permanent guest mode (filed bug 1061263), with no option to exit. Perhaps that bug is common, and this is what you are seeing?

Gerv

Comment 23

[Wesley Johnston (:wesj)]

Attachment: Patch

Doing this the non-dumb way. i.e. using NotificationCompat directly.

Comment 24

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

I see work is picking up again on this bug. Mark, is the old icon still good for now? (for the notification bar). It seems a bit dated and I've forgotten where we got that from now..

Comment 25

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to Anthony Lam (:antlam) from comment #24)
> I see work is picking up again on this bug. Mark, is the old icon still good
> for now? (for the notification bar). It seems a bit dated and I've forgotten
> where we got that from now..

I have no idea what the icon looks like :)

Comment 26

[Wesley Johnston (:wesj)]

I reused the one we show in menus (made white for v11+ Android). Ironically we don't actually use the menu one on Android 11+.

Comment 27

[Wesley Johnston (:wesj)]

Attachment: Notification bar screenshot

Couple screenshots for you.

Comment 28

[Wesley Johnston (:wesj)]

Attachment: Notification screen

Comment 29

[Brian Nicholson (:bnicholson)]

Comment on attachment 8483070 [details] [diff] [review]
Patch

Review of attachment 8483070 [details] [diff] [review]:
-----------------------------------------------------------------

r+ with some questions.

::: mobile/android/base/BrowserApp.java
@@ +2791,5 @@
>  
>          return super.onOptionsItemSelected(item);
>      }
>  
> +    public void showGuestModeDialog(final GuestModeDialog type) {

Why not just keep this private? See comment below.

@@ +2902,5 @@
>  
>          // Only solicit feedback when the app has been launched from the icon shortcut.
>          if (!Intent.ACTION_MAIN.equals(action)) {
>              return;
> +        } else if (GuestSession.NOTIFICATION_INTENT.equals(action)) {

Nit: else if -> if

::: mobile/android/base/GeckoProfile.java
@@ +335,5 @@
>          try {
>              // If this dir doesn't exist getDir will create it for us
>              final File lockFile = new File(getDir(), LOCK_FILE_NAME);
>              final boolean result = lockFile.createNewFile();
> +            if (lockFile.exists()) {

Are you changing this because the lockFile might already exist? If so, it seems like we should also update the return value so these stay in sync.

::: mobile/android/base/GuestSession.java
@@ +64,5 @@
>  
>          return profile.locked();
>      }
> +
> +    private static PendingIntent getNotificationIntent(Context context) {

I would prefer having an init() method that gets called once in GeckoApplication, where GuestSession then holds a reference to context.getApplicationContext(). The application context never changes, so there's no harm in holding it, and it saves us from having to pass around the context everywhere else.

I think I've mentioned this before though, so if you really like the pass-heavy (sorry, NFL started yesterday) strategy better, I won't block you on it.

@@ +94,5 @@
> +        }
> +    }
> +
> +    public static void handleIntent(BrowserApp context, Intent intent) {
> +        context.showGuestModeDialog(BrowserApp.GuestModeDialog.LEAVING);

Nits: context -> browser, remove intent arg

But I'd remove this method entirely. BrowserApp is the only client of this method, and showGuestModeDialog is defined in BrowserApp, so BrowserApp could just call this directly (allowing you to keep showGuestModeDialog private). This method just takes the BrowserApp that's passed to it and plays ping-pong with the showGuestModeDialog call, so I don't see much value unless this is part of a larger refactor.

::: mobile/android/base/prompts/Prompt.java
@@ +165,5 @@
>              throws IllegalStateException {
> +        final LayerView view = GeckoAppShell.getLayerView();
> +        if (view != null) {
> +            view.abortPanning();
> +        }

What's going on here? Is this related, or just an extra fix?

::: mobile/android/base/resources/values/ids.xml
@@ +4,5 @@
> +   - file, You can obtain one at http://mozilla.org/MPL/2.0/. -->
> +
> +<resources>
> +
> +    <item type="id" name="guestNotification"/>

R.id is traditionally used for views/layouts, so I wouldn't put this here. One approach we use elsewhere is generating the ID from the LOGTAG string (http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/fxa/sync/FxAccountSyncAdapter.java#69). Could we use that instead?

Comment 30

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

(In reply to Wesley Johnston (:wesj) from comment #27)
> Created attachment 8485039 [details]
> Notification bar screenshot
> 
> Couple screenshots for you.

Looking good.

Comment 31

[Wesley Johnston (:wesj)]

(In reply to Brian Nicholson (:bnicholson) from comment #29)
> Are you changing this because the lockFile might already exist? If so, it
> seems like we should also update the return value so these stay in sync.
Yes. If it exists, result = false, even though the profile IS locked. This fixes that. I updated the return value to not care about what createNewFile does at all (I don't think many callers pay attention to that return anyway).

> I think I've mentioned this before though, so if you really like the
> pass-heavy (sorry, NFL started yesterday) strategy better, I won't block you
> on it.

I just find holding references to contexts causes problems so I avoid it. Right now GuestSession is just static utility methods. I don't mind leaving it that way...

> But I'd remove this method entirely. BrowserApp is the only client of this
> method, and showGuestModeDialog is defined in BrowserApp, so BrowserApp
> could just call this directly (allowing you to keep showGuestModeDialog
> private). This method just takes the BrowserApp that's passed to it and
> plays ping-pong with the showGuestModeDialog call, so I don't see much value
> unless this is part of a larger refactor.

Just trying to encapsulate things. Because its the entry point for everything, BrowserApp/GeckoApp's Intent handling code is large. Trying to keep from adding (much) more if I can. I can pull it out if you want (and we can move it back in later someday).

> ::: mobile/android/base/prompts/Prompt.java
> @@ +165,5 @@
> >              throws IllegalStateException {
> > +        final LayerView view = GeckoAppShell.getLayerView();
> > +        if (view != null) {
> > +            view.abortPanning();
> > +        }
> 
> What's going on here? Is this related, or just an extra fix?

The view can be null if a dialog is created early in startup. Don't want to crash.

> R.id is traditionally used for views/layouts, so I wouldn't put this here.
> One approach we use elsewhere is generating the ID from the LOGTAG string
> (http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/fxa/sync/
> FxAccountSyncAdapter.java#69). Could we use that instead?

The default hashCode() implementations in Java aren't great at avoiding collisions (but they're apparently ok), so I've been trying to avoid them. We can if you want. I thought/still think this was/is more elegant (i.e. R.id. is just meant to generate unique ids. That its used frequently to generate ids for views doesn't really matter to me).

Looking through the Android source, they do... lots of different things, ranging from runtime generation:
http://androidxref.com/4.2.2_r1/xref/packages/apps/Contacts/src/com/android/contacts/vcard/NotificationImportExportListener.java#89
http://androidxref.com/4.2.2_r1/xref/packages/apps/Calendar/src/com/android/calendar/alerts/AlertService.java#258

to reusing a layout or string id:
http://androidxref.com/4.2.2_r1/xref/development/samples/ApiDemos/src/com/example/android/apis/app/StatusBarNotifications.java#MOOD_NOTIFICATIONS
http://androidxref.com/4.2.2_r1/xref/development/samples/Alarm/src/com/example/android/newalarm/AlarmService.java#212

to a hardcoded list:
http://androidxref.com/4.2.2_r1/xref/packages/apps/Phone/src/com/android/phone/NotificationMgr.java#85

Comment 32

[Wesley Johnston (:wesj)]

Try seemed happy. Landed:
https://hg.mozilla.org/integration/fx-team/rev/14f00ee7be7d

Comment 33

[Ryan VanderMeulen [:RyanVM]]

Backed out for Android S4 failures.
https://hg.mozilla.org/integration/fx-team/rev/3b61d93b670d

https://tbpl.mozilla.org/php/getParsedLog.php?id=47835049&tree=Fx-Team

Comment 34

[Wesley Johnston (:wesj)]

https://hg.mozilla.org/integration/fx-team/rev/947183ddd247

Comment 35

[Wesley Johnston (:wesj)]

Grr. I had to add GuestSession.java in this patch because the patch including it is held up. Forgot to hg add it when done. Pushed a follow up:

https://hg.mozilla.org/integration/fx-team/rev/53a74c045378

Comment 36

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/947183ddd247
https://hg.mozilla.org/mozilla-central/rev/53a74c045378