Closed Bug 984873 Opened 6 years ago Closed 6 years ago

After tapping a home banner, delay showing another one

Categories

(Firefox for Android :: General, defect)

30 Branch
x86
macOS
defect
Not set

Tracking

()

VERIFIED FIXED
Firefox 31
Tracking Status
firefox29 --- verified
firefox30 --- verified
firefox31 --- verified
fennec 29+ ---

People

(Reporter: ibarlow, Assigned: Margaret)

References

Details

Attachments

(1 file, 1 obsolete file)

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.
Blocks: home-banner
tracking-fennec: --- → ?
Assignee: nobody → 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?
Flags: needinfo?(ibarlow)
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 :(
Attachment #8393038 - Flags: review?(bnicholson)
(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... 

:(
Flags: needinfo?(ibarlow)
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.
Attachment #8393038 - Flags: review?(bnicholson) → feedback+
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.
Attachment #8393038 - Attachment is obsolete: true
Attachment #8393165 - Flags: review?(bnicholson)
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 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.
Attachment #8393165 - Flags: review?(bnicholson) → review+
https://hg.mozilla.org/mozilla-central/rev/387c93121a6a
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 31
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
Attachment #8393165 - Flags: approval-mozilla-beta?
Attachment #8393165 - Flags: approval-mozilla-aurora?
Attachment #8393165 - Flags: approval-mozilla-beta?
Attachment #8393165 - Flags: approval-mozilla-beta+
Attachment #8393165 - Flags: approval-mozilla-aurora?
Attachment #8393165 - Flags: approval-mozilla-aurora+
tracking-fennec: ? → 29+
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).
You need to log in before you can comment on or make changes to this bug.