Closed Bug 975469 Opened 6 years ago Closed 6 years ago

Sync promo banner does not hide on Sync sign-in

Categories

(Firefox for Android :: General, defect)

30 Branch
ARM
Android
defect
Not set

Tracking

()

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

People

(Reporter: aaronmt, Assigned: Margaret)

References

Details

Attachments

(1 file)

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
Assignee: nobody → margaret.leibovic
Blocks: 974496
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.
Flags: needinfo?(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
Flags: needinfo?(ibarlow)
Missed this one during triage, tracking 29 to keep track of this, although I'm not sure if it's a ship blocker.
tracking-fennec: --- → 29+
Status: NEW → ASSIGNED
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.
Attachment #8386976 - Flags: review?(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.
Attachment #8386976 - Flags: review?(bnicholson) → review+
(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 :)
https://hg.mozilla.org/mozilla-central/rev/b4fd23f69cef
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 30
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
Attachment #8386976 - Flags: approval-mozilla-aurora?
Attachment #8386976 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
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
Duplicate of this bug: 982179
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.
Status: RESOLVED → VERIFIED
(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.
You need to log in before you can comment on or make changes to this bug.