1247689 - Third time an item is RV'd, use Helper UI to tell users they can bookmark it

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

Just like bug 1246238, we want to teach the user that they can now bookmark RV items rather than the old "Add to RL" interaction.

IF the user never bookmarks an item after going into RV 3 separate times, we should also show a helper UI. Hopefully, this will help tell users they can Bookmark these things. It will also help us see how many times our users use the RV button comparatively. 

The big orange button should trigger the menu AND actually bookmark the item as well. Thereby displaying a filled star in the menu and adding the item to Bookmarks.

Note: we should probably only show either this, or the one in bug 1246238 and not both...

Comment 1

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

Attachment: prev_firsttime_feedback2.png

Comment 2

[Mark Finkle (:mfinkle) (use needinfo?)]

Make this an A/B experiment. The success outcome is increased # of bookmarked pages in RV.

Comment 3

[Andrzej Hunt :ahunt]

Depends on Bug 1246238 since we'll be reusing the SimpleHelperUI from there.

Comment 4

[Andrzej Hunt :ahunt]

Attachment: MozReview Request: Bug 1247689 - Pre: Move bookmark snackbars / helper UIs into BookmarkStateChangeDelegate r?sebastian

Review commit: https://reviewboard.mozilla.org/r/48015/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/48015/

Comment 5

[:Margaret Leibovic]

Are we concerned that the visuals for this bug are the same as for bug 1246238? Quickly looking at the mock-ups, I thought they were the same thing, so I wonder if users are going to think they've already seen this and quickly dismiss it.

I agree with mfinkle that this would make a good A/B test.

Given the timeframe, I think this is something that should wait until 49. Or it could be a good en-US-only experiment on 48.

Comment 6

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

(In reply to :Margaret Leibovic from comment #5)
> Are we concerned that the visuals for this bug are the same as for bug
> 1246238? Quickly looking at the mock-ups, I thought they were the same
> thing, so I wonder if users are going to think they've already seen this and
> quickly dismiss it.

The idea here is to show either or. Not both. Otherwise our messaging/voice gets really repetitive.

(In reply to Anthony Lam (:antlam) from comment #0)
> Note: we should probably only show either this, or the one in bug 1246238
> and not both...

Comment 7

[Andrzej Hunt :ahunt]

(In reply to :Margaret Leibovic from comment #5)
> Are we concerned that the visuals for this bug are the same as for bug
> 1246238? Quickly looking at the mock-ups, I thought they were the same
> thing, so I wonder if users are going to think they've already seen this and
> quickly dismiss it.

The way this is implemented should result in us only ever showing one of these panels. I'm reusing the same preference for both of them, so what happens is one of the two possibilities

A) User bookmarks an RV item first AND has never seen either prompt -> prompt shown, pref set, neither prompt ever shown again.
B) User RVs a page 3 times AND is in the experiment AND has never seen either prompt -> prompt shown, pref set, neither prompt ever shown again

Since it's an experiment, I believe B is entirely disabled until we land the switchboard config.

Comment 8

[:Margaret Leibovic]

Excellent, thank you for easing my fears!

Do you have a document outlining this experiment? I'd like to know what the different groups are, and what the analysis will look like.

Comment 9

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8743757 [details]
MozReview Request: Bug 1247689 - Pre: Move bookmark snackbars / helper UIs into BookmarkStateChangeDelegate r?sebastian

https://reviewboard.mozilla.org/r/48015/#review44889

In general I think this is good. However I would like to see whether we can centralize the implementation for this in one spot instead of spreading it over the code base.

::: mobile/android/base/java/org/mozilla/gecko/BrowserApp.java:354
(Diff revision 1)
> +                if (tab.getTimesEnteredReaderMode() == 3) {
> +                    // Loading the preferences here leads to us having extra nested if statements,
> +                    // however this avoids us from unnecessarily loading preferences every single
> +                    // time we have a LOCATION_CHANGE.
> +                    final SharedPreferences prefs = GeckoSharedPrefs.forProfile(this);
> +
> +                    // We reuse the same preference as for the first offline reader view bookmark
> +                    // as we only want to show one of the two UIs (they both explain the same
> +                    // functionality).
> +                    if (!prefs.getBoolean(SimpleHelperUI.PREF_FIRST_RVBP_SHOWN, false)) {
> +                        // Similarly avoid loading SwitchBoard until as late as possible, since we're
> +                        // rarely likely to need this value.
> +                        boolean experimentEnabled = SwitchBoard.isInExperiment(this, Experiments.TRIPLE_READERVIEW_BOOKMARK_PROMPT);
> +
> +                        if (experimentEnabled) {
> +                            SimpleHelperUI.show(this,
> +                                    SimpleHelperUI.TRIPLE_READERVIEW_OPEN_TELEMETRYEXTRA,
> +                                    ACTIVITY_REQUEST_TRIPLE_READERVIEW,
> +                                    R.string.helper_triple_readerview_open_title,
> +                                    R.string.helper_triple_readerview_open_message,
> +                                    R.drawable.helper_first_readerview_bookmark, // We share the icon with the usual helper UI
> +                                    R.string.helper_triple_readerview_open_button,
> +                                    ACTIVITY_RESULT_TRIPLE_READERVIEW_ADD_BOOKMARK,
> +                                    ACTIVITY_RESULT_TRIPLE_READERVIEW_IGNORE);
> +
> +                            GeckoSharedPrefs.forProfile(this)
> +                                    .edit()
> +                                    .putBoolean(SimpleHelperUI.PREF_FIRST_RVBP_SHOWN, true)
> +                                    .apply();
> +                        }
> +                    }
> +                }

Is it possible to move this code into its own class (like AddHomeScreenPromotion)?

Actually this reminds me of this "delegate" idea that I already wanted to explore for the home screen experiment. I just filed bug 1266477 and pushed some code. This doesn't need to wait for it but maybe we can already move it to a different class.

::: mobile/android/base/java/org/mozilla/gecko/Tab.java:634
(Diff revision 1)
> +        if (mEnteringReaderMode) {
> +            mTimesEnteredReaderMode++;
> +        } else if (exitingReaderMode) {
> +            // Do nothing: we keep the existing count
> +        } else {
> +            mTimesEnteredReaderMode = 0;
> +        }
> +

Is this really something we want to count inside a Tab? Shouldn't this prompt be independently from whether I use reader mode in the same tab or in three different tabs?

Can this be moved to a place (maybe the same class I propose above) that listens to tab load events and counts reader mode loads?

If the tab is really needed this other class could map soft references or tab ids to counts etc.

Comment 10

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

image assets updated in bug 1246238

Comment 11

[Andrzej Hunt :ahunt]

Just to confirm that I'm doing the right thing, do we want to show this helper:

- If a user has used reader-view three times total, regardless of what page they're on?
OR
- If a user has used reader-view three times on one page, without navigating to any other pages in that tab?

I'm blocking this on BrowserAppDelegate (Bug 1266477) for now, since that will allow us to move most of the decision code into a separate class. This will involve adding onActivityResult to BrowserAppDelegate to handle any activities that we might spawn. We will also want to move the code that was added in BrowserApp in Bug 1246238 to this new class (I'm landing Bug 1246238 now so that it makes it into 48, whereas we could make this bug an english-only experiment if necessary).

Comment 12

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

(In reply to Andrzej Hunt :ahunt from comment #11)
> Just to confirm that I'm doing the right thing, do we want to show this
> helper:
> 
> - If a user has used reader-view three times total, regardless of what page
> they're on?
> OR
> - If a user has used reader-view three times on one page, without navigating
> to any other pages in that tab?

I think it should be just after the RV button is pressed 3 times. Regardless of what web page they're on.

Comment 13

[Andrzej Hunt :ahunt]

Attachment: available_offline_promotion.png

(In reply to Anthony Lam (:antlam) from comment #10)
> image assets updated in bug 1246238

Here's a screenshot of the current appearance!

Comment 14

[Andrzej Hunt :ahunt]

Attachment: MozReview Request: Bug 1247689 - Experiment: show prompt on third opening of reader view r?sebastian

Review commit: https://reviewboard.mozilla.org/r/49675/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/49675/

Comment 15

[Andrzej Hunt :ahunt]

Comment on attachment 8743757 [details]
MozReview Request: Bug 1247689 - Pre: Move bookmark snackbars / helper UIs into BookmarkStateChangeDelegate r?sebastian

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/48015/diff/1-2/

Comment 16

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8743757 [details]
MozReview Request: Bug 1247689 - Pre: Move bookmark snackbars / helper UIs into BookmarkStateChangeDelegate r?sebastian

https://reviewboard.mozilla.org/r/48015/#review46461

Awesome! :)

::: mobile/android/base/java/org/mozilla/gecko/promotion/BookmarkStateChangeDelegate.java:42
(Diff revision 2)
> +import org.mozilla.gecko.util.DrawableUtil;
> +import org.mozilla.gecko.util.ThreadUtils;
> +
> +import java.lang.ref.WeakReference;
> +
> +public class BookmarkStateChangeDelegate extends BrowserAppDelegate implements Tabs.OnTabsChangedListener {

Maybe add a short javadoc explaining what this delegate does?

::: mobile/android/base/java/org/mozilla/gecko/promotion/BookmarkStateChangeDelegate.java:49
(Diff revision 2)
> +
> +    private WeakReference<BrowserApp> mBrowserApp;
> +
> +    @Override
> +    public void onCreate(BrowserApp browserApp, Bundle savedInstanceState) {
> +        mBrowserApp =  new WeakReference<>(browserApp);

NIT: Two spaces: "=  "

::: mobile/android/base/java/org/mozilla/gecko/promotion/BookmarkStateChangeDelegate.java:87
(Diff revision 2)
> +        switch (requestCode) {
> +            case BrowserApp.ACTIVITY_REQUEST_FIRST_READERVIEW_BOOKMARK:
> +                if (resultCode == BrowserApp.ACTIVITY_RESULT_FIRST_READERVIEW_BOOKMARKS_GOTO_BOOKMARKS) {
> +                    browserApp.openUrlAndStopEditing("about:home?panel=" + HomeConfig.getIdForBuiltinPanelType(HomeConfig.PanelType.BOOKMARKS));
> +                } else if (resultCode == BrowserApp.ACTIVITY_RESULT_FIRST_READERVIEW_BOOKMARKS_IGNORE) {
> +                    showReaderModeBookmarkAddedSnackbar();
> +                }
> +                break;
> +        }

nit: A switch statement seems to be overkill here, where we do not expect to add more cases for now? :)

::: mobile/android/base/java/org/mozilla/gecko/promotion/BookmarkStateChangeDelegate.java:104
(Diff revision 2)
> +        final BrowserApp browserApp = mBrowserApp.get();
> +        if (browserApp == null) {
> +            return false;
> +        }
> +
> +        final SharedPreferences prefs = GeckoSharedPrefs.forProfile(browserApp.getContext());

nit: BrowserApp is already a context (No need to call getContext())

Comment 17

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8747012 [details]
MozReview Request: Bug 1247689 - Experiment: show prompt on third opening of reader view r?sebastian

https://reviewboard.mozilla.org/r/49675/#review46463

Nice! So far the delegates seem to be useful. :)

::: mobile/android/base/java/org/mozilla/gecko/BrowserApp.java:360
(Diff revision 1)
>                  if (Tabs.getInstance().isSelectedTab(tab)) {
>                      updateHomePagerForTab(tab);
>                  }
>  
>                  mDynamicToolbar.persistTemporaryVisibility();
> +

nit: Not related new line?

::: mobile/android/base/java/org/mozilla/gecko/promotion/ReaderViewBookmarkPromotion.java:26
(Diff revision 1)
> +import org.mozilla.gecko.util.Experiments;
> +
> +import java.lang.ref.WeakReference;
> +
> +public class ReaderViewBookmarkPromotion extends BrowserAppDelegate implements Tabs.OnTabsChangedListener {
> +    private WeakReference<BrowserApp> mBrowserApp;

We are using this pattern quite often. By now it might make sense to create a new abstract class (based on BrowserAppDelegate) that does just the reference keeping (and adds a handy getter).

As more patterns emerge we can create more helpful delegate base classes.

::: mobile/android/base/java/org/mozilla/gecko/promotion/ReaderViewBookmarkPromotion.java:28
(Diff revision 1)
> +import java.lang.ref.WeakReference;
> +
> +public class ReaderViewBookmarkPromotion extends BrowserAppDelegate implements Tabs.OnTabsChangedListener {
> +    private WeakReference<BrowserApp> mBrowserApp;
> +
> +    private int mTimesEnteredReaderMode = 0;

nit: The int will be 0 by default (No need to initialize it here)

::: mobile/android/base/java/org/mozilla/gecko/promotion/ReaderViewBookmarkPromotion.java:32
(Diff revision 1)
> +
> +    private int mTimesEnteredReaderMode = 0;
> +
> +    @Override
> +    public void onCreate(BrowserApp browserApp, Bundle savedInstanceState) {
> +        mBrowserApp =  new WeakReference<>(browserApp);

nit: two spaces

::: mobile/android/base/java/org/mozilla/gecko/promotion/ReaderViewBookmarkPromotion.java:66
(Diff revision 1)
> +    public void onActivityResult(BrowserApp browserApp, int requestCode, int resultCode,
> +                                 Intent data) {

nit: This can be one line (There are much longer lines in this file)

::: mobile/android/base/java/org/mozilla/gecko/promotion/ReaderViewBookmarkPromotion.java:97
(Diff revision 1)
> +        boolean experimentEnabled = SwitchBoard.isInExperiment(browserApp, Experiments.TRIPLE_READERVIEW_BOOKMARK_PROMPT);
> +
> +        if (!experimentEnabled) {
> +            return;
> +        }

Why not just:
> if (SwitchBoard.isInExperiment()) {
> ..
> }

However I'm wondering if we shouldn't just run this in onCreate() and avoid running all this other code all the time if the experiment is not enabled anyways. In the worst case we might not show the promotion for the first run with the experiment enabled, but this shouldn't be a big problem?

::: mobile/android/base/java/org/mozilla/gecko/promotion/ReaderViewBookmarkPromotion.java:108
(Diff revision 1)
> +        SimpleHelperUI.show(browserApp,
> +                SimpleHelperUI.TRIPLE_READERVIEW_OPEN_TELEMETRYEXTRA,
> +                BrowserApp.ACTIVITY_REQUEST_TRIPLE_READERVIEW,
> +                R.string.helper_triple_readerview_open_title,
> +                R.string.helper_triple_readerview_open_message,
> +                R.drawable.helper_first_readerview_bookmark, // We share the icon with the usual helper UI

nit: If we share the icon, should we use a more generic name for it?

Comment 18

[Andrzej Hunt :ahunt]

https://reviewboard.mozilla.org/r/48015/#review46461

> nit: A switch statement seems to be overkill here, where we do not expect to add more cases for now? :)

I'm adding one more case in the followup commit, but alone this would be overkill. (I'm not convinced we shouldn't have an if/else if though, but I find it a bit easier to read a switch even for just 2 cases.)

Comment 19

[Andrzej Hunt :ahunt]

https://reviewboard.mozilla.org/r/49675/#review46463

> We are using this pattern quite often. By now it might make sense to create a new abstract class (based on BrowserAppDelegate) that does just the reference keeping (and adds a handy getter).
> 
> As more patterns emerge we can create more helpful delegate base classes.

Filed as Bug 1269406 since I'm not too confident with the naming there yet.

Comment 20

[Andrzej Hunt :ahunt]

https://reviewboard.mozilla.org/r/48015/#review46461

> I'm adding one more case in the followup commit, but alone this would be overkill. (I'm not convinced we shouldn't have an if/else if though, but I find it a bit easier to read a switch even for just 2 cases.)

Ooops, I was looking at the wrong switch: yes, this should probably be an if statement.

Comment 21

[Andrzej Hunt :ahunt]

https://hg.mozilla.org/integration/fx-team/rev/8c4fe4b4fa80121419b45c156f8e4ad64e2c721a
Bug 1247689 - Pre: Move bookmark snackbars / helper UIs into BookmarkStateChangeDelegate r=sebastian

https://hg.mozilla.org/integration/fx-team/rev/33dd69f591fc472c6f2635327c1d431dba3ad1ae
Bug 1247689 - Experiment: show prompt on third opening of reader view r=sebastian

Comment 22

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/8c4fe4b4fa80
https://hg.mozilla.org/mozilla-central/rev/33dd69f591fc

Comment 23

[Teodora Vermesan (:TeoVermesan)]

Tested using:
Device: Nexus 6 (Android 6.0)
Build: Firefox for Android (2015-06-08)

Went to news.google.com and opened 3 articles in new tabs. Pressed the reader icon from URL Bar to enter RV, but there is no Helper UI to tell that RV items can be bookmarked to read them offline.

Comment 24

[Andrzej Hunt :ahunt]

(In reply to Teodora Vermesan (:TeoVermesan) from comment #23)
> Tested using:
> Device: Nexus 6 (Android 6.0)
> Build: Firefox for Android (2015-06-08)
> 
> Went to news.google.com and opened 3 articles in new tabs. Pressed the
> reader icon from URL Bar to enter RV, but there is no Helper UI to tell that
> RV items can be bookmarked to read them offline.

Unfortunately this landed in a broken state (after review I moved the switchboard check into onCreate, which doesn't work because switchboard doesn't have data yet, so we won't show the prompt ever).

I'll be fixing this in Bug 1270880!

Comment 25

[Andrzej Hunt :ahunt]

(In reply to Andrzej Hunt :ahunt from comment #24)
> 
> I'll be fixing this in Bug 1270880!

Note, that bug is primarily about showing the prompt the first time you open RV, but as a side effect this will also make the prompt show (whereas nightly won't ever show the prompt).