Closed Bug 1268647 Opened 7 years ago Closed 7 years ago

Implement new set of feature-focused firstrun panels

Categories

(Firefox for Android Graveyard :: General, defect)

ARM
Android
defect
Not set
normal

Tracking

(firefox49 verified, relnote-firefox 49+)

VERIFIED FIXED
Firefox 49
Tracking Status
firefox49 --- verified
relnote-firefox --- 49+

People

(Reporter: liuche, Assigned: liuche)

References

()

Details

Attachments

(12 files)

157.96 KB, application/zip
Details
58 bytes, text/x-review-board-request
sebastian
: review+
Details
58 bytes, text/x-review-board-request
sebastian
: review+
Details
58 bytes, text/x-review-board-request
sebastian
: review+
Details
58 bytes, text/x-review-board-request
sebastian
: review+
Details
58 bytes, text/x-review-board-request
sebastian
: review+
Details
58 bytes, text/x-review-board-request
sebastian
: review+
Details
58 bytes, text/x-review-board-request
sebastian
: review+
Details
58 bytes, text/x-review-board-request
sebastian
: review+
Details
58 bytes, text/x-review-board-request
sebastian
: review+
Details
2.46 MB, video/mp4
Details
100.10 KB, image/png
Details
This includes updating the images and text of first run slides to showcase Tab Queue, Saving, Reader View, and update the sync resources.
Let's get some of these assets for the new slides out!
Flags: needinfo?(alam)
Attached file v3slides_XXHDPI.zip
Flags: needinfo?(alam) → needinfo?(liuche)
Flags: needinfo?(liuche)
This is running on a Nexus 7 (pre-Marshmallow, so there isn't a prompt requesting the drawOverScreen permission).

Some specific feedback questions for you, Barbara and Anthony:

I added an auto-advance for 1.2 seconds after tab queue is enabled - this needs a tiny tweak to cancel the auto-advance if the user then toggles this off, but let me know what you think - should we add it in?

I have telemetry for turning tab queues on and off. However, I did not add telemetry for the toggle if the permission prompt for drawOverScreen in Marshmallow is shown - do we want telemetry on showing that prompt? And when the user turns on the prompt (or rejects the permission) do we want a separate telemetry probe for that specific case, or is the basic on/off probe enough?

Note: For uplifting to Aurora, I'll also have to make switchboard changes to "document" this being en_US, and add code to do this only on en_US.
Assignee: nobody → liuche
Status: NEW → ASSIGNED
Attachment #8751103 - Flags: feedback?(bbermes)
Attachment #8751103 - Flags: feedback?(alam)
Also, Barbara, since this is nearly done, let me know about bug 1270641 so that this bug can get uplifted right after we pick a winner through 47.
(In reply to Chenxia Liu [:liuche] from comment #14)
> Created attachment 8751103 [details]
> Screencast: Feature first run
> 
> This is running on a Nexus 7 (pre-Marshmallow, so there isn't a prompt
> requesting the drawOverScreen permission).

Awesome!

> Some specific feedback questions for you, Barbara and Anthony:
> 
> I added an auto-advance for 1.2 seconds after tab queue is enabled - this
> needs a tiny tweak to cancel the auto-advance if the user then toggles this
> off, but let me know what you think - should we add it in?

I kinda like this. Especially with the animation, it's obvious that the user can "swipe back". But, how did we arrive at 1.2 seconds?  I think 1 second is fine :) 

That said, are we still allowing the user to press the images to flip the switch? 

> I have telemetry for turning tab queues on and off. However, I did not add
> telemetry for the toggle if the permission prompt for drawOverScreen in
> Marshmallow is shown - do we want telemetry on showing that prompt? And when
> the user turns on the prompt (or rejects the permission) do we want a
> separate telemetry probe for that specific case, or is the basic on/off
> probe enough?

I'll leave this one for Barbara, but my thoughts are  we want to measure the cases where a permission prompt basically creates an obstacle so big for our users that they do not want to enable this feature.
Flags: needinfo?(liuche)
Flags: needinfo?(bbermes)
Comment on attachment 8751088 [details]
MozReview Request: Bug 1268647 - Remove obsolete firstrun. r=sebastian

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/51241/diff/1-2/
Attachment #8751088 - Attachment description: MozReview Request: Bug 1268647 - Remove obsolete firstrun. → MozReview Request: Bug 1268647 - Remove obsolete firstrun. r=sebastian
Attachment #8751089 - Attachment description: MozReview Request: Bug 1268647 - Add Switch. → MozReview Request: Bug 1268647 - Add Switch to layout. r=sebastian
Attachment #8751090 - Attachment description: MozReview Request: Bug 1268647 - Add localized strings. → MozReview Request: Bug 1268647 - Add localized strings. r=sebastian
Attachment #8751091 - Attachment description: MozReview Request: Bug 1268647 - Add to strings.xml.in. → MozReview Request: Bug 1268647 - Add to strings.xml.in. r=sebastian
Attachment #8751092 - Attachment description: MozReview Request: Bug 1268647 - Add crushed images. → MozReview Request: Bug 1268647 - Add crushed images. r=sebastian
Attachment #8751093 - Attachment description: MozReview Request: Bug 1268647 - Add new basic slides. → MozReview Request: Bug 1268647 - Add new basic slides. r=sebastian
Attachment #8751094 - Attachment description: MozReview Request: Bug 1268647 - Add new sync slide. → MozReview Request: Bug 1268647 - Add new sync slide. r=sebastian
Attachment #8751095 - Attachment description: MozReview Request: Bug 1268647 - Add Tab queue panel. → MozReview Request: Bug 1268647 - Add Tab queue panel. r=sebastian
Attachment #8751096 - Attachment description: MozReview Request: Bug 1268647 - Add auto-advance. → MozReview Request: Bug 1268647 - Add control. r=sebastian
Attachment #8751088 - Flags: review?(s.kaspari)
Attachment #8751089 - Flags: review?(s.kaspari)
Attachment #8751090 - Flags: review?(s.kaspari)
Attachment #8751091 - Flags: review?(s.kaspari)
Attachment #8751092 - Flags: review?(s.kaspari)
Attachment #8751093 - Flags: review?(s.kaspari)
Attachment #8751094 - Flags: review?(s.kaspari)
Attachment #8751095 - Flags: review?(s.kaspari)
Attachment #8751096 - Flags: review?(s.kaspari)
Comment on attachment 8751089 [details]
MozReview Request: Bug 1268647 - Add Switch to layout. r=sebastian

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/51811/diff/1-2/
Comment on attachment 8751090 [details]
MozReview Request: Bug 1268647 - Add localized strings. r=sebastian

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/51813/diff/1-2/
Comment on attachment 8751091 [details]
MozReview Request: Bug 1268647 - Add to strings.xml.in. r=sebastian

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/51815/diff/1-2/
Comment on attachment 8751092 [details]
MozReview Request: Bug 1268647 - Add crushed images. r=sebastian

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/51817/diff/1-2/
Comment on attachment 8751093 [details]
MozReview Request: Bug 1268647 - Add new basic slides. r=sebastian

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/51819/diff/1-2/
Comment on attachment 8751094 [details]
MozReview Request: Bug 1268647 - Add new sync slide. r=sebastian

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/51821/diff/1-2/
Comment on attachment 8751095 [details]
MozReview Request: Bug 1268647 - Add Tab queue panel. r=sebastian

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/51823/diff/1-2/
Comment on attachment 8751096 [details]
MozReview Request: Bug 1268647 - Add control. r=sebastian

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/51825/diff/1-2/
I added a probe for when the prompt is shown, and also a probe for when what the user response is to the prompt (accept or reject the permission). I also removed the auto-advance because there were some edge cases that I'd rather not deal with if we want to uplift this to aurora.
Flags: needinfo?(liuche)
Comment on attachment 8751088 [details]
MozReview Request: Bug 1268647 - Remove obsolete firstrun. r=sebastian

https://reviewboard.mozilla.org/r/51241/#review49413

::: mobile/android/base/java/org/mozilla/gecko/util/Experiments.java:40
(Diff revision 2)
> -    public static final String ONBOARDING2_A = "onboarding2-a"; // Control: Single (blue) welcome screen
> -    public static final String ONBOARDING2_B = "onboarding2-b"; // 4 static Feature slides
> -    public static final String ONBOARDING2_C = "onboarding2-c"; // 4 static + 1 clickable (Data saving) Feature slides
> +    public static final String ONBOARDING3_A = "onboarding3-a"; // Control: No first run
> +    public static final String ONBOARDING3_B = "onboarding3-b"; // 4 static Feature + 1 dynamic slides
> +    public static final String ONBOARDING3_C = "onboarding3-c"; // Differentiating features slides

Oh, in the "remove" commit is a hidden new configuration of experiments.. :)

::: mobile/android/base/java/org/mozilla/gecko/util/Experiments.java:92
(Diff revision 2)
> -        if (SwitchBoard.isInBucket(context, 0, 33)) {
> -            return Experiments.ONBOARDING2_A.equals(experiment);
> -        } else if (SwitchBoard.isInBucket(context, 33, 66)) {
> -            return Experiments.ONBOARDING2_B.equals(experiment);
> -        } else if (SwitchBoard.isInBucket(context, 66, 100)) {
> -            return Experiments.ONBOARDING2_C.equals(experiment);
> +        if (SwitchBoard.isInBucket(context, 0, 20)) {
> +            return Experiments.ONBOARDING3_A.equals(experiment);
> +        } else if (SwitchBoard.isInBucket(context, 20, 60)) {
> +            return Experiments.ONBOARDING3_B.equals(experiment);
> +        } else if (SwitchBoard.isInBucket(context, 60, 100)) {
> +            return Experiments.ONBOARDING3_C.equals(experiment);

If A is no first run and A/B are different first runs. What is bucket 20-40?
Attachment #8751088 - Flags: review?(s.kaspari) → review+
Attachment #8751089 - Flags: review?(s.kaspari) → review+
Comment on attachment 8751089 [details]
MozReview Request: Bug 1268647 - Add Switch to layout. r=sebastian

https://reviewboard.mozilla.org/r/51811/#review49415

::: mobile/android/base/resources/layout/firstrun_basepanel_checkable_fragment.xml:43
(Diff revision 2)
>                    android:paddingTop="20dp"
>                    android:paddingBottom="30dp"
>                    android:gravity="center"
>                    android:textAppearance="@style/TextAppearance.FirstrunRegular.Body"/>
>  
> -        <View android:layout_weight="1"
> +        <android.support.v7.widget.SwitchCompat

With minSdkVersion 15 we should be able to use 'Switch' from the system framework.
Comment on attachment 8751090 [details]
MozReview Request: Bug 1268647 - Add localized strings. r=sebastian

https://reviewboard.mozilla.org/r/51813/#review49417
Attachment #8751090 - Flags: review?(s.kaspari) → review+
Attachment #8751091 - Flags: review?(s.kaspari) → review+
Comment on attachment 8751091 [details]
MozReview Request: Bug 1268647 - Add to strings.xml.in. r=sebastian

https://reviewboard.mozilla.org/r/51815/#review49419

.. those are small commits .. :)
Comment on attachment 8751092 [details]
MozReview Request: Bug 1268647 - Add crushed images. r=sebastian

https://reviewboard.mozilla.org/r/51817/#review49421

(Mozreview really should display images; just found bug 1239879)
Attachment #8751092 - Flags: review?(s.kaspari) → review+
Attachment #8751093 - Flags: review?(s.kaspari) → review+
Comment on attachment 8751093 [details]
MozReview Request: Bug 1268647 - Add new basic slides. r=sebastian

https://reviewboard.mozilla.org/r/51819/#review49423

::: mobile/android/base/java/org/mozilla/gecko/firstrun/FirstrunPagerConfig.java:103
(Diff revision 2)
>          public static final FirstrunPanelConfig urlbarPanelConfig = new FirstrunPanelConfig(FirstrunPanel.class.getName(), R.string.firstrun_panel_title_welcome, R.drawable.firstrun_urlbar, R.string.firstrun_urlbar_message, R.string.firstrun_urlbar_subtext);
>          public static final FirstrunPanelConfig bookmarksPanelConfig = new FirstrunPanelConfig(FirstrunPanel.class.getName(), R.string.firstrun_bookmarks_title, R.drawable.firstrun_bookmarks, R.string.firstrun_bookmarks_message, R.string.firstrun_bookmarks_subtext);
>          public static final FirstrunPanelConfig syncPanelConfig = new FirstrunPanelConfig(FirstrunPanel.class.getName(), R.string.firstrun_sync_title, R.drawable.firstrun_sync, R.string.firstrun_sync_message, R.string.firstrun_sync_subtext);
>          public static final FirstrunPanelConfig dataPanelConfig = new FirstrunPanelConfig(DataPanel.class.getName(), R.string.firstrun_data_title, R.drawable.firstrun_data_off, R.string.firstrun_data_message, R.string.firstrun_data_subtext);
> +
> +        public static final FirstrunPanelConfig notificationsPanelConfig = new FirstrunPanelConfig(FirstrunPanel.class.getName(), R.string.firstrun_notifications_title, R.drawable.firstrun_notifications, R.string.firstrun_notifications_message, R.string.firstrun_notifications_subtext);

nit: I wonder why those and SimplePanelConfigs are public/protected. Couldn't this all be private? Is the SimplePanelConfigs class even needed?
Comment on attachment 8751094 [details]
MozReview Request: Bug 1268647 - Add new sync slide. r=sebastian

https://reviewboard.mozilla.org/r/51821/#review49425
Attachment #8751094 - Flags: review?(s.kaspari) → review+
Comment on attachment 8751095 [details]
MozReview Request: Bug 1268647 - Add Tab queue panel. r=sebastian

https://reviewboard.mozilla.org/r/51823/#review49427

This diff was a bit difficult to read because of the move DataPanel -> TabQueuePanel. (Maybe it's more of a rm/add than a mv in this case).
Attachment #8751095 - Flags: review?(s.kaspari) → review+
Attachment #8751096 - Flags: review?(s.kaspari) → review+
FYI: Back when implementing runtime permissions I filed bug 1240709 to improve the prompt for asking for the "draw over other apps" permission. Right now the prompt is just a hacked version of the prompt we show after opening X external links. This works but might not be the best way to get the message across.
See Also: → 1240709
https://reviewboard.mozilla.org/r/51811/#review49473

::: mobile/android/base/resources/layout/firstrun_basepanel_checkable_fragment.xml:43
(Diff revision 2)
>                    android:paddingTop="20dp"
>                    android:paddingBottom="30dp"
>                    android:gravity="center"
>                    android:textAppearance="@style/TextAppearance.FirstrunRegular.Body"/>
>  
> -        <View android:layout_weight="1"
> +        <android.support.v7.widget.SwitchCompat

I originally used Switch, but for some reason, it didn't use the AppCompat styling for Switch for pre-5.0 but then did use AppCompat with 5.0+, so it was inconsistent and didn't show up (and it also wasn't possible to provide track: and thumb: for pre-5.0 and not for 5.0+). Using SwitchCompat fixed these discrepancies between versions though.
https://reviewboard.mozilla.org/r/51819/#review49491

::: mobile/android/base/java/org/mozilla/gecko/firstrun/FirstrunPagerConfig.java:103
(Diff revision 2)
>          public static final FirstrunPanelConfig urlbarPanelConfig = new FirstrunPanelConfig(FirstrunPanel.class.getName(), R.string.firstrun_panel_title_welcome, R.drawable.firstrun_urlbar, R.string.firstrun_urlbar_message, R.string.firstrun_urlbar_subtext);
>          public static final FirstrunPanelConfig bookmarksPanelConfig = new FirstrunPanelConfig(FirstrunPanel.class.getName(), R.string.firstrun_bookmarks_title, R.drawable.firstrun_bookmarks, R.string.firstrun_bookmarks_message, R.string.firstrun_bookmarks_subtext);
>          public static final FirstrunPanelConfig syncPanelConfig = new FirstrunPanelConfig(FirstrunPanel.class.getName(), R.string.firstrun_sync_title, R.drawable.firstrun_sync, R.string.firstrun_sync_message, R.string.firstrun_sync_subtext);
>          public static final FirstrunPanelConfig dataPanelConfig = new FirstrunPanelConfig(DataPanel.class.getName(), R.string.firstrun_data_title, R.drawable.firstrun_data_off, R.string.firstrun_data_message, R.string.firstrun_data_subtext);
> +
> +        public static final FirstrunPanelConfig notificationsPanelConfig = new FirstrunPanelConfig(FirstrunPanel.class.getName(), R.string.firstrun_notifications_title, R.drawable.firstrun_notifications, R.string.firstrun_notifications_message, R.string.firstrun_notifications_subtext);

That's true, I will make them private. The SimplePanelConfigs class isn't technically needed, but I was using the class to abstract that piece of the code - and it felt messy to have a ton of PanelConfigs floating around, and it was too verbose to instantiate them in the actual code.
Landing this with Version C, because there isn't a clear winner. If we decide to go with version B, it'll be easier to remove the dynamic panel (that will turn C into B).
Flags: needinfo?(bbermes)
Fixed bustage, forgot to add file to moz.build.
Verified as fixed using:
Device: Moto X (Android 4.4)
Build: Firefox for Android 49.0a1 (2016-05-18)
 
> > I have telemetry for turning tab queues on and off. However, I did not add
> > telemetry for the toggle if the permission prompt for drawOverScreen in
> > Marshmallow is shown - do we want telemetry on showing that prompt? And when
> > the user turns on the prompt (or rejects the permission) do we want a
> > separate telemetry probe for that specific case, or is the basic on/off
> > probe enough?
> 
> I'll leave this one for Barbara, but my thoughts are  we want to measure the
> cases where a permission prompt basically creates an obstacle so big for our
> users that they do not want to enable this feature.

Sorry for the late delay, if possible, we should add telemetry for the prompt as well, and have a simple accepted/not accepted. sounds good?
Flags: needinfo?(liuche)
Flags: needinfo?(liuche)
Attachment #8751103 - Flags: feedback?(bbermes)
Release Note Request (optional, but appreciated)
[Why is this notable]:
[Suggested wording]: Updated First Run with fresh and exciting slides highlighting features
[Links (documentation, blog post, etc)]:
relnote-firefox: --- → ?
Relnote 49+ added "Updated First Run to showcase several exciting features"
Verified as fixed in build 49.0a2 (2016-06-15);
Device: 
- Nexus 5 (Android 6.0.1);
- Asus ZenPad 8 (Android 5.0.2);
- Motorola Razr (Android 4.4.4).
And based on comment 44 I will mark this as verified.
Status: RESOLVED → VERIFIED
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.