984873 - After tapping a home banner, delay showing another one

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Ian Barlow (:ibarlow)]

In bug 961523, we decided that after a user taps "x" on a home banner, we should delay showing another one for a period of time. 

In practice, we have found that we would *also* like this behaviour to occur when a user taps the banner to open its content. 

So in other words, doing anything with the banner (other than ignoring it) should hide it for a period of time.

Comment 1

[:Margaret Leibovic]

To clarify the behavior here, do we only want to delay showing a new message if the user has clicked on the banner? So if the user didn't click on the banner, we would still want to rotate in a new message the next time they visited about:home?

Comment 2

[:Margaret Leibovic]

Attachment: Delay updating the banner after the user has clicked on it

This is my quick attempt at a solution for this bug. I decided to use a timestamp to keep track of when the user last clicked the banner, and make sure that we don't update it again for some interval after that.

If ibarlow agrees with my last comment, this patch will do the right thing and rotate messages if the user hasn't clicked the banner (or if enough time has passed and we start showing the banner again).

I'm sad that this banner is so complicated :(

Comment 3

[Ian Barlow (:ibarlow)]

(In reply to :Margaret Leibovic from comment #1)
> To clarify the behavior here, do we only want to delay showing a new message
> if the user has clicked on the banner? So if the user didn't click on the
> banner, we would still want to rotate in a new message the next time they
> visited about:home?

Yeah

> I'm sad that this banner is so complicated :(

Yeah... 

:(

Comment 4

[Brian Nicholson (:bnicholson)]

Comment on attachment 8393038 [details] [diff] [review]
Delay updating the banner after the user has clicked on it

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

It's inconsistent that clicking the X closes the banner for the "session", whereas tapping the banner closes it for an arbitrary amount of time. Shouldn't we use the same close behavior for both? It sounds like that's what's expected from comment 0, so I'm not sure we need the timer at all.

::: mobile/android/base/home/HomeBanner.java
@@ +37,5 @@
>                          implements GeckoEventListener {
>      private static final String LOGTAG = "GeckoHomeBanner";
>  
> +    // How often to update the banner with a new message after the user clicked it (1 hour).
> +    private static final long UPDATE_INTERVAL_MILLIS = 3600000;

Nit: I usually like to factor these numbers into units to make them easier to verify (e.g., 60 * 60 * 1000), but your call.

@@ +160,5 @@
> +     */
> +    private void hideOnClick() {
> +        HomeBanner.this.setVisibility(View.GONE);
> +        HomeBanner.this.setEnabled(false);
> +        lastClickedMillis = System.currentTimeMillis();

It's usually preferable to use System.uptimeMillis() when possible since the "wall" clock can be changed by the user, but uptime cannot.

Comment 5

[:Margaret Leibovic]

Attachment: Hide banner for the rest of the session after the user has clicked on it

I like your consistency argument, let's just do what we're currently doing for the close button (not showing it again for the rest of the session).

We should file a follow-up bug to refine this behavior in the future, but this seems like a low-risk approach that we can uplift.

Comment 6

[:Margaret Leibovic]

We could also back out the Snippets.js change that landed in bug 975469, since removing the sync banner from the rotation when it's clicked won't matter anymore.

Comment 7

[Brian Nicholson (:bnicholson)]

Comment on attachment 8393165 [details] [diff] [review]
Hide banner for the rest of the session after the user has clicked on it

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

Nice, I like how simple this is.

Comment 8

[:Margaret Leibovic]

https://hg.mozilla.org/integration/fx-team/rev/387c93121a6a

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/387c93121a6a

Comment 10

[:Margaret Leibovic]

Comment on attachment 8393165 [details] [diff] [review]
Hide banner for the rest of the session after the user has clicked on it

[Approval Request Comment]
Bug caused by (feature/regressing bug #): enabling snippets
User impact if declined: snippet messages will appear frequently, potentially annoying users
Testing completed (on m-c, etc.): landed on m-c today
Risk to taking this patch (and alternatives if risky): low-risk, removes banner after user clicks on it
String or IDL/UUID changes made by this patch: none

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/3c6774e9040e
https://hg.mozilla.org/releases/mozilla-beta/rev/75cf0bcefc1b

Comment 12

[Flaviu Cos]

Verified as fixed in builds:
- 29 beta 2; 
- 30.0a2 (2014-03-25);
- 31.0a1 (2014-03-25);
Device: Google Nexus 10 (Android 4.4.2).