961523 - Refine home banner close button behavior for snippets

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Aaron Train [:aaronmt]]

In bug 958891 we added (behind a preference) the ability to see an advertisement banner on about:home. I tapped the 'x' close button in hopes to never see it again. The next-time I visited about:home or re-opened the browser it re-appeared.

The code below doesn't seem to have the logic

http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/home/HomeBanner.java#52

This will frustrate people.

Comment 1

[:Margaret Leibovic]

What actually is the spec for the close button? That code comment says "never show again in this session". That seems reasonable to me (and reasonable to implement with a boolean in memory), but I want to double check with UX.

Comment 2

[Ian Barlow (:ibarlow)]

That was the original discussion yeah. Aaron does have a point though. Maybe 'close' should trigger a longer delay for that specific message. A few days? A week?

Comment 3

[:Margaret Leibovic]

(In reply to Ian Barlow (:ibarlow) from comment #2)
> That was the original discussion yeah. Aaron does have a point though. Maybe
> 'close' should trigger a longer delay for that specific message. A few days?
> A week?

Theoretically, it would be nice if we could just never show the user *that* message again, but we don't currently have logic in place to do that, and that seems complicated. Also, given our current dynamic way of adding home banner messages, I don't think we'd be able to persist this between sessions either.

Comment 4

[Aaron Train [:aaronmt]]

Akin to how product announcements can be configured for status, I would imagine having the banner advertisement should be configurable too. I can imagine it being one of the first things users would want to disable (and be vocal about).

Comment 5

[:Margaret Leibovic]

Attachment: Make close button dismiss banner for rest of session

Given the way the current code is structured, it looks like it was written with incorrect assumptions about the lifecycle of HomeBanner. Currently, HomeBanner is tied to TopSitesPanel's lifecycle, which means it's getting recreated all the time. Because of this, we can't use HomeBanner's visibility to keep track of whether or not it was dismissed (or use a flag in HomeBanner for that matter).

I decided that for right now, the simplest fix is to just let JS control whether or not the banner was dismissed. And I know it's kinda scope creep, but I took this opportunity to also let Home.banner consumers register an "ondismiss" listener on messages they add, since it might be useful to know that a user hit the "X" button when they were looking at your message (could also be useful for telemetry on things like the sync promo banner).

Comment 6

[Brian Nicholson (:bnicholson)]

Comment on attachment 8364752 [details] [diff] [review]
Make close button dismiss banner for rest of session

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

Going through Gecko to save a UI state like this feels kind of sketchy. TopSitesPanel is a Fragment, and Fragment has state saving built-in by default (one of their main advantages), so I'd recommend using it if at all possible.

Here's how I would do this:
* Change HomeBanner to be a ViewStub inside of TopSitesPanel (http://developer.android.com/reference/android/view/ViewStub.html)
* In HomeBanner, create an interface call OnBannerClosed, called inside of the close button's OnClickListener.
* Have TopSitesPanel implement this callback. In the callback's implementation, have it set a class-level boolean mBannerHidden to true.
* Override TopSitePanel's onSaveInstanceState and do outState.putBoolean("bannerHidden", mBannerHidden);
* Finally, TopSitesPanel#onViewCreated can now look like this:

if (!savedInstanceState.getBoolean("bannerHidden")) {
    HomeBanner banner = ((ViewStub) view.findViewById(R.id.home_banner)).inflate();
    banner.setOnBannerClosed(...); // callback to set mBannerHidden to true
}

One reason to utilize Fragment's state saving like this is that it maintains your session as part of the Android lifecycle. Android Activities are killed/resumed on a regular basis (meaning Gecko is killed and restarted with it), so the _dismissed value that you're holding in memory gets lost. For example, if you were to close the banner, receive a phone call, and go back to Fennec after ending your phone call, it's possible that the banner would show up again (since Fennec may have been restarted in the process). Bundles and onSaveInstanceState were designed for this exact reason -- so that you can restore the UI to where you left off.

Comment 7

[:Margaret Leibovic]

Thanks for the feedback. That sounds like a much better idea, I was just lazy :/

jdover is actually working in bug 960359 to make HomeBanner part of the HomePager instead of TopSitesPanel, since a user can disable the "Top Sites Panel", but we still want to be able to show them the home banner.

With that in mind, I'm going to hold off on working on a fragment style fix here, and perhaps jdover can just incorporate this into whatever he ends up doing.

Comment 8

[:Margaret Leibovic]

Comment on attachment 8364752 [details] [diff] [review]
Make close button dismiss banner for rest of session

It could be cool to land a patch like this one day for the "ondismiss" handler bit, but that's out of scope for this bug, so I filed bug 963561.

Comment 9

[:Margaret Leibovic]

This should be fixed by bug 960359. However, we can keep this open for now and verify when that patch lands.

Comment 10

[:Margaret Leibovic]

Based on some discussions with ibarlow, as a first step, we want the close button to make the banner go away for some set period of time. That's what we can implement here for 29.

In the future, we want to make this close button more intelligent, and use it to never show specific messages again. But we should do this in a follow-up bug.

ibarlow, is this summary correct?

Comment 11

[Ian Barlow (:ibarlow)]

I seem to recall our "first step" plan being that pressing close hides the banner for a period of time, *and* marks that particular message as read. But if we need to do the "mark as read" part in a follow-up that's fine.

Comment 12

[:Margaret Leibovic]

With my changes in bug 963561, we'll be able to let the snippets message know that the user hit the "x". However, we would need to introduce some other component of snippets storage to decide never to show that particular snippet again, so that would be best done in a separate bug (although that may be an upliftable change as well).

Comment 13

[:Margaret Leibovic]

Attachment: WIP - Never show a snippet again after a user dismisses it

Rather than making banner go away for some period of time, I decided to try just removing a specific snippet after the user hits the close button.

This patch works (I left in my testing prefs in case anyone wants to try it), but I'm concerned about having yet another file read in here. Given that the server is dumb and doesn't know anything about a user dismissing a message, we need to somehow keep track of these dismissed messages locally, so I wasn't able to come up with a better solution.

Another issue is that this snippets-removed.json file will just grow forever, so we'll want to add some code to clean that up. I was thinking that maybe we could just wipe it once a month when we update the geo code.

What do you guys think of all this?

Comment 14

[Brian Nicholson (:bnicholson)]

Comment on attachment 8378647 [details] [diff] [review]
WIP - Never show a snippet again after a user dismisses it

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

Since this just keeps track of a simple string that gets appended to over time, I wonder if a simple pref might be more appropriate than a full-on JSON file?

::: mobile/android/components/Snippets.js
@@ +171,5 @@
> +
> +  // Filter out any snippets that should be removed.
> +  let promise = OS.File.read(gSnippetsRemovedPath);
> +  promise.then(array => {
> +    let removedSnippetIds = gDecoder.decode(array).split(",");

If you are going to store this as a JSON file, it would make more sense to store these entries as a JSON array and getting them back via JSON.parse() instead of storing/splitting an ad hoc comma-delimited string.

Comment 15

[Brian Nicholson (:bnicholson)]

(In reply to Brian Nicholson (:bnicholson) from comment #14)
> If you are going to store this as a JSON file, it would make more sense to
> store these entries as a JSON array and getting them back via JSON.parse()
> instead of storing/splitting an ad hoc comma-delimited string.

Actually you aren't using JSON at all here; it's just a basic comma-delimited string like I said before, so 'snippets-removed.json' is a misnomer. I would use a different file extension.

Comment 16

[:Margaret Leibovic]

(In reply to Brian Nicholson (:bnicholson) from comment #14)
> Comment on attachment 8378647 [details] [diff] [review]
> WIP - Never show a snippet again after a user dismisses it
> 
> Review of attachment 8378647 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Since this just keeps track of a simple string that gets appended to over
> time, I wonder if a simple pref might be more appropriate than a full-on
> JSON file?

Yeah, that sounds like it could be simpler, although we'd always need to read the pref and modify it when we want to add a new id to it. Also, I wonder what kinds of limits there are on the length of a char pref... probably pretty long, but I can look into that.

> ::: mobile/android/components/Snippets.js
> @@ +171,5 @@
> > +
> > +  // Filter out any snippets that should be removed.
> > +  let promise = OS.File.read(gSnippetsRemovedPath);
> > +  promise.then(array => {
> > +    let removedSnippetIds = gDecoder.decode(array).split(",");
> 
> If you are going to store this as a JSON file, it would make more sense to
> store these entries as a JSON array and getting them back via JSON.parse()
> instead of storing/splitting an ad hoc comma-delimited string.

You're right, this just shouldn't be a JSON file. I wanted to do the ad hoc string so that I could just append to the end of the file without worrying about reading a data structure, modifying it, then re-saving it.

Comment 17

[:Margaret Leibovic]

Attachment: Never show a snippet again after a user dismisses it

I decided to rename the file to a .txt file, since that's what it is. I like the approach of using a file, since we can just append to it, instead of needing to read it first.

I can file a follow-up bug to just periodically wipe this file. However, doing that runs the risk of potentially showing a user a snippet they've recently dismissed, so we may want to consider other options.

Comment 18

[:Margaret Leibovic]

Attachment: Never show a snippet again after a user dismisses it (prefs style)

Following conversation on IRC, we decided to go with a prefs-based approach.

I actually like that this is a bit easier to read.

Comment 19

[Brian Nicholson (:bnicholson)]

Comment on attachment 8379408 [details] [diff] [review]
Never show a snippet again after a user dismisses it (prefs style)

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

Nice, looks good to me.

Comment 20

[:Margaret Leibovic]

https://hg.mozilla.org/integration/fx-team/rev/14d9c2321e0e

We can let this shake out on Nightly a bit then uplift it, although there isn't much to test unless we push out another test snippet.

Comment 21

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/14d9c2321e0e

Comment 22

[:Margaret Leibovic]

This doesn't actually affect the sync banner, filed bug 975423 for that.

Comment 23

[:Margaret Leibovic]

Comment on attachment 8379408 [details] [diff] [review]
Never show a snippet again after a user dismisses it (prefs style)

[Approval Request Comment]
Bug caused by (feature/regressing bug #): dynamic snippets (bug 962349)
User impact if declined: snippets will be shown to the user again after they've dismissed them
Testing completed (on m-c, etc.): tested locally and landed on m-c 2/21, but we won't get real Nightly test coverage until we make another test snippet
Risk to taking this patch (and alternatives if risky): only affects snippets
String or IDL/UUID changes made by this patch: none

Comment 24

[:Margaret Leibovic]

Comment on attachment 8379408 [details] [diff] [review]
Never show a snippet again after a user dismisses it (prefs style)

Oops, didn't mean to request beta approval.

Comment 25

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/351b38e8c46f