975469 - Sync promo banner does not hide on Sync sign-in

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Aaron Train [:aaronmt]]

Toggle browser.snippets.syncPromo.enabled; true

Tap the link the banner

Sign-in to Sync

Tap 'Back to Browsing'

Promotion banner still shown

This banner should not be shown after a user successfully adds an account

Comment 1

[:Margaret Leibovic]

Fixing this will actually require a change to HomeBanner, either adding a new API for consumers to use to dismiss the banner, or dismissing it automatically after it is clicked...

The main question we need to answer is that after the user clicks on one message, should we show other messages when they re-visit about:home? Or should the banner just be gone for the rest of that app run?

It would be easy to just hide the banner for the rest of the app lifetime, but if the app is around for a long time, this would prevent us from showing other messages. 
This problem raises similar questions to those in bug 975239. If we implement the polling solution that's suggested in that bug, that would probably take care of showing a new message.

Comment 2

[Ian Barlow (:ibarlow)]

Yeah, the main part here to me is as Aaron mentioned, that the 'set up sync' banner should not be shown after a user successfully adds an account.

If we are unable to tell if they were successful in adding account after they tapped the banner, though, we should probably still mark the message as read and not show it again. 

To Margaret's question, I think it's ok to cycle a different message into the home banner, a while after a user clicks this one. See my comment here https://bugzilla.mozilla.org/show_bug.cgi?id=975239#c5

Comment 3

[:Margaret Leibovic]

Missed this one during triage, tracking 29 to keep track of this, although I'm not sure if it's a ship blocker.

Comment 4

[:Margaret Leibovic]

Attachment: Remove sync promo banner from rotation after it has been tapped

With this patch, we always hide the banner after it has been clicked (whether it is a snippet or sync promo message or whatever), and we remove the sync promo message from the rotation, so it won't show again for the rest of the app lifetime.

I decided to make this change to the generic banner behavior because I didn't want to have to do something special for just the sync banner. 

This isn't a perfect solution, so if the user fails to sign in, the banner will still be gone, but it will show again the next time they launch the app, so I think that's okay. Like usual, I would rather us err on the side of the banner being gone than showing when it shouldn't.

Comment 5

[Brian Nicholson (:bnicholson)]

Comment on attachment 8386976 [details] [diff] [review]
Remove sync promo banner from rotation after it has been tapped

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

::: mobile/android/base/home/HomeBanner.java
@@ +97,5 @@
>              @Override
>              public void onClick(View v) {
> +                // Hide the banner. This does not remove the message from the rotation, so it may appear
> +                // again if the JS onclick handler doesn't choose to remove it.
> +                HomeBanner.this.setVisibility(View.GONE);

I wonder if update() to instantly show the next snippet in rotation would be better than hiding it here.

Comment 6

[:Margaret Leibovic]

(In reply to Brian Nicholson (:bnicholson) from comment #5)
> Comment on attachment 8386976 [details] [diff] [review]
> Remove sync promo banner from rotation after it has been tapped
> 
> Review of attachment 8386976 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: mobile/android/base/home/HomeBanner.java
> @@ +97,5 @@
> >              @Override
> >              public void onClick(View v) {
> > +                // Hide the banner. This does not remove the message from the rotation, so it may appear
> > +                // again if the JS onclick handler doesn't choose to remove it.
> > +                HomeBanner.this.setVisibility(View.GONE);
> 
> I wonder if update() to instantly show the next snippet in rotation would be
> better than hiding it here.

If there are no messages in the rotation, we don't actually send a "HomeBanner:Data" response to Java, so we would need to do something to make the banner go away in that case.

I feel like I like this solution because once a user clicks on one banner message and goes back to about:home, I think it's nice to give them a breather from showing them another message, at least until they open about:home again :)

Comment 7

[:Margaret Leibovic]

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

Comment 8

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/b4fd23f69cef

Comment 9

[:Margaret Leibovic]

Comment on attachment 8386976 [details] [diff] [review]
Remove sync promo banner from rotation after it has been tapped

[Approval Request Comment]
Bug caused by (feature/regressing bug #): sync promo banner
User impact if declined: the banner will not hide after the user clicks it
Testing completed (on m-c, etc.): landed on m-c 3/7, tested locally
Risk to taking this patch (and alternatives if risky): low-risk, just removes banner more agressively
String or IDL/UUID changes made by this patch: none

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/ec783a38f231

Comment 11

[Teodora Vermesan (:TeoVermesan)]

The banner will hide after the user clicks it, so:
Verified fixed on:
Builds: Firefox for Android 30.0a1 (2014-03-11)
Device: LG Nexus 4
OS: Android 4.4

Comment 13

[Cristina Madaras, QA [:CristinaM]]

This is not reproducible on latest Nightly (2014-06-19) and latest Aurora (2014-06-19). 
Considering that the Firefox 30 was released I will change the flag for FF29 to wontfix and based on Teodora's comment 11 I will mark the bug as verified fixed.

Comment 14

[Cristina Madaras, QA [:CristinaM]]

(In reply to cristina.madaras from comment #13)
> This is not reproducible on latest Nightly (2014-06-19) and latest Aurora
> (2014-06-19). 
> Considering that the Firefox 30 was released I will change the flag for FF29
> to wontfix and based on Teodora's comment 11 I will mark the bug as verified
> fixed.

I mean, I will change the flag for FF29 to verified.