Closed Bug 1268647 Opened 5 years ago Closed 4 years ago

Implement new set of feature-focused firstrun panels

Categories

(Firefox for Android :: General, defect)

ARM
Android
defect
Not set
normal

Tracking

()

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

People

(Reporter: liuche, Assigned: liuche)

References

(Blocks 1 open bug, )

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.
Duplicate of this bug: 1259833
Duplicate of this bug: 1259835
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
You need to log in before you can comment on or make changes to this bug.