1357029 - Should add the Add-on tour in the onBoarding overlay

Status: VERIFIED FIXED

(Firefox :: General, enhancement, P1)

Description

[Fischer [:Fischer]]

Should add the Add-on tour in the onBoarding overlay

Comment 2

[[e-mail]]

I'd like to help land this

Comment 3

[KM Lee [:rexboy] (inactivate after Sep. 22)]

Attachment: WIP for remaining tours

By discussion the working item changed yesterday, so I'll hang out a few remaining working tours for Fred. This is a WIP patch for 6 tours: sync, overlay, one-off search, add-on, private browsing, and set default browser.

It's an earlier version, we still need to change it to met l10n and the button listeners based on the architecture landed in m-c.

Comment 4

[[e-mail]]

bug 1354707 comment 14 provide the new strings

[2: Add-ons]
Navigation link: Add-ons
Headline: Add more functionality.
Body: Add-ons expand Firefox’s built-in features, so Firefox works the way you do. Compare prices, check the weather or express your personality with a custom theme.
Button: Show Add-ons in Menu

Comment 5

[[e-mail]]

Attachment: Polyfill - Add private browsing and search tour to onboarding overlay.

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

Comment 6

[[e-mail]]

Attachment: Bug 1357029 - Should add the Add-on tour in the onBoarding overlay;

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

Comment 7

[[e-mail]]

Please ignore `Polyfill - Add private browsing and search tour to onboarding overlay.`, which will be replaced after `Bug 1357046 - Should add the Private Browsing tour in the onBoarding overlay` is landed.

Comment 8

[[e-mail]]

Comment on attachment 8876555 [details]
Bug 1357029 - Should add the Add-on tour in the onBoarding overlay;

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

Comment 9

[[e-mail]]

Attachment: Bug 1357029 - Should add the Customize Firefox tour in the onBoarding overlay;

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

Comment 10

[[e-mail]]

Attachment: Bug 1357029 - Should add the Default Browser tour in the onBoarding overlay;

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

Comment 11

[[e-mail]]

Comment on attachment 8876562 [details]
Bug 1357029 - Should add the Customize Firefox tour in the onBoarding overlay;

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

Comment 12

[[e-mail]]

Comment on attachment 8876564 [details]
Bug 1357029 - Should add the Default Browser tour in the onBoarding overlay;

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

Comment 13

[Fischer [:Fischer]]

Comment on attachment 8876555 [details]
Bug 1357029 - Should add the Add-on tour in the onBoarding overlay;

https://reviewboard.mozilla.org/r/147894/#review152262

::: browser/extensions/onboarding/locales/en-US/onboarding.properties:17
(Diff revision 2)
>  onboarding.tour-private-browsing.title=Browse by yourself.
>  onboarding.tour-private-browsing.description=There’s no reason to share your online life with trackers every time you browse. Want to keep something to yourself? Use Private Browsing with Tracking Protection.
>  onboarding.tour-private-browsing.button=Show Private Browsing in Menu
> +onboarding.tour-addons=Add-ons
> +onboarding.tour-addons.title=Add more functionality.
> +onboarding.tour-addons.description=Add-ons expand %S’s built-in features, so %S works the way you do. Compare prices, check the weather or express your personality with a custom theme.

Nit: Please add one comment like what was done on  "onboarding.tour-title" saying %S is brandShortName. This would help doing tranlation, thanks.

Comment 14

[Fischer [:Fischer]]

Comment on attachment 8876562 [details]
Bug 1357029 - Should add the Customize Firefox tour in the onBoarding overlay;

https://reviewboard.mozilla.org/r/147904/#review152266

::: browser/extensions/onboarding/locales/en-US/onboarding.properties:21
(Diff revision 2)
>  onboarding.tour-addons.title=Add more functionality.
>  onboarding.tour-addons.description=Add-ons expand %S’s built-in features, so %S works the way you do. Compare prices, check the weather or express your personality with a custom theme.
>  onboarding.tour-addons.button=Show Add-ons in Menu
> +onboarding.tour-customize=Customize
> +onboarding.tour-customize.title=Do things your way.
> +onboarding.tour-customize.description=Drag, drop, and reorder %S’s toolbar and menu to fit your needs. You can even select a compact theme to give websites more room.

Nit: Please add one comment like what was done on  "onboarding.tour-title" saying %S is brandShortName. This would help doing tranlation, thanks.

Comment 15

[Fischer [:Fischer]]

Comment on attachment 8876564 [details]
Bug 1357029 - Should add the Default Browser tour in the onBoarding overlay;

https://reviewboard.mozilla.org/r/147906/#review152268

Thanks for adding these 3 tours.

::: browser/extensions/onboarding/locales/en-US/onboarding.properties:25
(Diff revision 2)
>  onboarding.tour-customize.title=Do things your way.
>  onboarding.tour-customize.description=Drag, drop, and reorder %S’s toolbar and menu to fit your needs. You can even select a compact theme to give websites more room.
>  onboarding.tour-customize.button=Show Customize in Menu
> +onboarding.tour-default-browser=Default Browser
> +onboarding.tour-default-browser.title=We’re there for you.
> +onboarding.tour-default-browser.description=Love %S? Set it as your default browser. Then when you open a link from another application, %S has you covered.

Nit: Please add one comment like what was done on  "onboarding.tour-title" saying %S is brandShortName. This would help doing tranlation, thanks.

Comment 16

[Francesco Lodolo [:flod]]

Comment on attachment 8876564 [details]
Bug 1357029 - Should add the Default Browser tour in the onBoarding overlay;

https://reviewboard.mozilla.org/r/147906/#review152310

::: browser/extensions/onboarding/locales/en-US/onboarding.properties:27
(Diff revision 2)
>  onboarding.tour-customize.button=Show Customize in Menu
> +onboarding.tour-default-browser=Default Browser
> +onboarding.tour-default-browser.title=We’re there for you.
> +onboarding.tour-default-browser.description=Love %S? Set it as your default browser. Then when you open a link from another application, %S has you covered.
> +onboarding.tour-default-browser.button=Open Default Browser Settings
> +onboarding.tour-default-browser.win7.button=Make Firefox Your Default Browser

Please add comment explaining why we have two strings for the button label

Comment 17

[[e-mail]]

Comment on attachment 8876554 [details]
Polyfill - Add private browsing and search tour to onboarding overlay.

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

Comment 18

[[e-mail]]

Comment on attachment 8876555 [details]
Bug 1357029 - Should add the Add-on tour in the onBoarding overlay;

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

Comment 19

[[e-mail]]

Comment on attachment 8876562 [details]
Bug 1357029 - Should add the Customize Firefox tour in the onBoarding overlay;

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

Comment 20

[[e-mail]]

Comment on attachment 8876564 [details]
Bug 1357029 - Should add the Default Browser tour in the onBoarding overlay;

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

Comment 21

[[e-mail]]

> > +onboarding.tour-default-browser.button=Open Default Browser Settings
> > +onboarding.tour-default-browser.win7.button=Make Firefox Your Default Browser
> 
> Please add comment explaining why we have two strings for the button label


Hi Michael, do you have some comment suggestion to explain why we need win7 specific button string?

Comment 22

[Verdi [:verdi] Best to slack me]

(In reply to Fred Lin [:gasolin] from comment #21)
> > > +onboarding.tour-default-browser.button=Open Default Browser Settings
> > > +onboarding.tour-default-browser.win7.button=Make Firefox Your Default Browser
> > 
> > Please add comment explaining why we have two strings for the button label
> 
> 
> Hi Michael, do you have some comment suggestion to explain why we need win7
> specific button string?

Because in Windows 7 we can directly set Firefox as the default browser. We can't do that in Windows 8, 10 or on Mac.

Comment 23

[Dave Townsend [:mossop]]

Comment on attachment 8876564 [details]
Bug 1357029 - Should add the Default Browser tour in the onBoarding overlay;

https://reviewboard.mozilla.org/r/147906/#review152540

::: browser/extensions/onboarding/locales/en-US/onboarding.properties:28
(Diff revision 3)
> +onboarding.tour-default-browser.button=Open Default Browser Settings
> +onboarding.tour-default-browser.win7.button=Make Firefox Your Default Browser

Please add a localization note describing in which cases these buttons are used.

Comment 24

[Dave Townsend [:mossop]]

Comment on attachment 8876555 [details]
Bug 1357029 - Should add the Add-on tour in the onBoarding overlay;

https://reviewboard.mozilla.org/r/147894/#review152536

::: browser/extensions/onboarding/content/onboarding.js:232
(Diff revision 3)
> +          div.querySelector("#onboarding-tour-addons-description").textContent =
> +            this._bundle.formatStringFromName("onboarding.tour-addons.description",
> +              [BRAND_SHORT_NAME, BRAND_SHORT_NAME], 2);

Rather than passing BRAND_SHORT_NAME twice you can use %1$S in the string to use the first argument twice.

Common style is to align the arguments to the start of the arguments in the line above.

This line is a little long, break it up with an intermediate variable.

These comments apply to all three patches.

Comment 25

[Dave Townsend [:mossop]]

Comment on attachment 8876562 [details]
Bug 1357029 - Should add the Customize Firefox tour in the onBoarding overlay;

https://reviewboard.mozilla.org/r/147904/#review152542

See the note from the earlier patch

Comment 26

[[e-mail]]

Comment on attachment 8876555 [details]
Bug 1357029 - Should add the Add-on tour in the onBoarding overlay;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/147894/diff/3-4/

Comment 27

[[e-mail]]

Comment on attachment 8876562 [details]
Bug 1357029 - Should add the Customize Firefox tour in the onBoarding overlay;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/147904/diff/3-4/

Comment 28

[[e-mail]]

Comment on attachment 8876564 [details]
Bug 1357029 - Should add the Default Browser tour in the onBoarding overlay;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/147906/diff/3-4/

Comment 29

[Francesco Lodolo [:flod]]

Comment on attachment 8876555 [details]
Bug 1357029 - Should add the Add-on tour in the onBoarding overlay;

https://reviewboard.mozilla.org/r/147894/#review152728

::: browser/extensions/onboarding/locales/en-US/onboarding.properties:16
(Diff revision 4)
>  onboarding.tour-private-browsing.title=Browse by yourself.
>  onboarding.tour-private-browsing.description=There’s no reason to share your online life with trackers every time you browse. Want to keep something to yourself? Use Private Browsing with Tracking Protection.
>  onboarding.tour-private-browsing.button=Show Private Browsing in Menu
> +onboarding.tour-addons=Add-ons
> +onboarding.tour-addons.title=Add more functionality.
> +# LOCALIZATION NOTE(onboarding.tour-addons.description): This string will be used in the addon tour description. %1$S is brandShortName

nit: addon -> add-on

Comment 30

[Francesco Lodolo [:flod]]

Comment on attachment 8876564 [details]
Bug 1357029 - Should add the Default Browser tour in the onBoarding overlay;

https://reviewboard.mozilla.org/r/147906/#review152732

::: browser/extensions/onboarding/locales/en-US/onboarding.properties:28
(Diff revision 4)
>  onboarding.tour-customize.button=Show Customize in Menu
> +onboarding.tour-default-browser=Default Browser
> +onboarding.tour-default-browser.title=We’re there for you.
> +# LOCALIZATION NOTE(onboarding.tour-default-browser.description): This string will be used in the default browser tour description. %1$S is brandShortName
> +onboarding.tour-default-browser.description=Love %1$S? Set it as your default browser. Then when you open a link from another application, %1$S has you covered.
> +# LOCALIZATION NOTE(onboarding.tour-default-browser.win7.button): This string will be used in the default browser tour button.

Maybe better:

Label for a button to open the default browser settings in operative systems, like macOS or Win10, where it's not possible to set the default browser directly.

::: browser/extensions/onboarding/locales/en-US/onboarding.properties:30
(Diff revision 4)
> +onboarding.tour-default-browser.title=We’re there for you.
> +# LOCALIZATION NOTE(onboarding.tour-default-browser.description): This string will be used in the default browser tour description. %1$S is brandShortName
> +onboarding.tour-default-browser.description=Love %1$S? Set it as your default browser. Then when you open a link from another application, %1$S has you covered.
> +# LOCALIZATION NOTE(onboarding.tour-default-browser.win7.button): This string will be used in the default browser tour button.
> +onboarding.tour-default-browser.button=Open Default Browser Settings
> +# LOCALIZATION NOTE(onboarding.tour-default-browser.win7.button): This string will be used in the default browser tour button. Windows 7 allow directly set Firefox as the default browser.

typo: allow -> allows

Label for a button to set directly Firefox as default browser, only used on Windows 7.

Comment 31

[KM Lee [:rexboy] (inactivate after Sep. 22)]

Comment on attachment 8876555 [details]
Bug 1357029 - Should add the Add-on tour in the onBoarding overlay;

https://reviewboard.mozilla.org/r/147894/#review152730

::: browser/extensions/onboarding/content/onboarding.js:231
(Diff revision 4)
>        itemsFrag.appendChild(li);
>        // Dynamically create tour pages
>        let div = tour.getPage(this._window);
>  
> +      switch (tour.id) {
> +        case "onboarding-tour-addons":

Note that the latest patch is going to use formatStringFromName for every description so the logic here will no longer required. Please remember to rebase before land.

Comment 32

[KM Lee [:rexboy] (inactivate after Sep. 22)]

Comment on attachment 8876562 [details]
Bug 1357029 - Should add the Customize Firefox tour in the onBoarding overlay;

https://reviewboard.mozilla.org/r/147904/#review152754

::: browser/extensions/onboarding/content/onboarding.js:263
(Diff revision 4)
>              this._bundle.formatStringFromName("onboarding.tour-addons.description", [BRAND_SHORT_NAME], 1);
>            break;
> +        case "onboarding-tour-customize":
> +          div.querySelector("#onboarding-tour-customize-description").textContent =
> +            this._bundle.formatStringFromName("onboarding.tour-customize.description", [BRAND_SHORT_NAME], 1);
> +          break;

Same as the addons case. Please remember to rebase before land.

Comment 33

[KM Lee [:rexboy] (inactivate after Sep. 22)]

Comment on attachment 8876564 [details]
Bug 1357029 - Should add the Default Browser tour in the onBoarding overlay;

https://reviewboard.mozilla.org/r/147906/#review152762

::: browser/extensions/onboarding/content/onboarding.js:288
(Diff revision 4)
>              this._bundle.formatStringFromName("onboarding.tour-addons.description", [BRAND_SHORT_NAME], 1);
>            break;
> +        case "onboarding-tour-default-browser":
> +          div.querySelector("#onboarding-tour-default-browser-description").textContent =
> +            this._bundle.formatStringFromName("onboarding.tour-default-browser.description", [BRAND_SHORT_NAME], 1);
> +          break;

Same as the addons case. Please remember to rebase before land.

Comment 34

[[e-mail]]

Comment on attachment 8876554 [details]
Polyfill - Add private browsing and search tour to onboarding overlay.

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

Comment 35

[[e-mail]]

Comment on attachment 8876555 [details]
Bug 1357029 - Should add the Add-on tour in the onBoarding overlay;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/147894/diff/4-5/

Comment 36

[[e-mail]]

Comment on attachment 8876562 [details]
Bug 1357029 - Should add the Customize Firefox tour in the onBoarding overlay;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/147904/diff/4-5/

Comment 37

[[e-mail]]

Comment on attachment 8876564 [details]
Bug 1357029 - Should add the Default Browser tour in the onBoarding overlay;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/147906/diff/4-5/

Comment 38

[Dave Townsend [:mossop]]

Comment on attachment 8876555 [details]
Bug 1357029 - Should add the Add-on tour in the onBoarding overlay;

https://reviewboard.mozilla.org/r/147894/#review153128

Comment 39

[Dave Townsend [:mossop]]

Comment on attachment 8876562 [details]
Bug 1357029 - Should add the Customize Firefox tour in the onBoarding overlay;

https://reviewboard.mozilla.org/r/147904/#review153132

Comment 40

[Dave Townsend [:mossop]]

Comment on attachment 8876564 [details]
Bug 1357029 - Should add the Default Browser tour in the onBoarding overlay;

https://reviewboard.mozilla.org/r/147906/#review153140

::: browser/extensions/onboarding/locales/en-US/onboarding.properties:28
(Diff revision 5)
> +# LOCALIZATION NOTE(onboarding.tour-default-browser.button): Label for a button to open the default browser settings in OS, like macOS or Win10, where it's not possible to set the default browser directly.
> +onboarding.tour-default-browser.button=Open Default Browser Settings
> +# LOCALIZATION NOTE(onboarding.tour-default-browser.win7.button): Label for a button to directly set Firefox as the default browser (only Windows 7 allows that).
> +onboarding.tour-default-browser.win7.button=Make Firefox Your Default Browser

Phrase them like this:

Label for a button to open the OS default browser settings where it's not possible to set the default browser directly (OSX, Windows 8 and higher).

Label for a button to directly set the default browser (Windows 7).

Add Linux to whichever of those is appropriate.

Comment 41

[[e-mail]]

Comment on attachment 8876554 [details]
Polyfill - Add private browsing and search tour to onboarding overlay.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/147892/diff/3-4/

Comment 42

[[e-mail]]

Comment on attachment 8876555 [details]
Bug 1357029 - Should add the Add-on tour in the onBoarding overlay;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/147894/diff/5-6/

Comment 43

[[e-mail]]

Comment on attachment 8876562 [details]
Bug 1357029 - Should add the Customize Firefox tour in the onBoarding overlay;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/147904/diff/5-6/

Comment 44

[[e-mail]]

Comment on attachment 8876564 [details]
Bug 1357029 - Should add the Default Browser tour in the onBoarding overlay;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/147906/diff/5-6/

Comment 45

[[e-mail]]

updated based on bug 1357046, 1357049, will remove the polyfill once bug 1357046, 1357049 in central.
The update PR also add button actions for Customize and add-on tour. 

The button text and action of default browser tour are a bit different, so I'll set review in a separate patch.

Comment 49

[[e-mail]]

push by rex's mozreview hint, thanks

Comment 50

[Mozilla Autoland]

We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s 5c0dd24ceebc -d c8497caddebb: rebasing 401976:5c0dd24ceebc "Bug 1357029 - Should add the Add-on tour in the onBoarding overlay;r=Fischer,mossop,rexboy"
merging browser/extensions/onboarding/content/onboarding.css
merging browser/extensions/onboarding/content/onboarding.js
merging browser/extensions/onboarding/locales/en-US/onboarding.properties
warning: conflicts while merging browser/extensions/onboarding/locales/en-US/onboarding.properties! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)

Comment 51

[Dave Townsend [:mossop]]

Comment on attachment 8876564 [details]
Bug 1357029 - Should add the Default Browser tour in the onBoarding overlay;

https://reviewboard.mozilla.org/r/147906/#review154128

::: browser/extensions/onboarding/locales/en-US/onboarding.properties:43
(Diff revision 6)
> +onboarding.tour-default-browser.description=Love %1$S? Set it as your default browser. Then when you open a link from another application, %1$S has you covered.
> +
> +# LOCALIZATION NOTE(onboarding.tour-default-browser.button): Label for a button to open the OS default browser settings where it's not possible to set the default browser directly. (OSX, Linux, Windows 8 and higher)
> +onboarding.tour-default-browser.button=Open Default Browser Settings
> +
> +# LOCALIZATION NOTE(onboarding.tour-default-browser.win7.button): Label for a button to directly set  the default browser (Windows 7). %S is brandShortName

Nit: double-space here.

Comment 52

[[e-mail]]

Comment on attachment 8876555 [details]
Bug 1357029 - Should add the Add-on tour in the onBoarding overlay;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/147894/diff/6-7/

Comment 53

[[e-mail]]

Comment on attachment 8876562 [details]
Bug 1357029 - Should add the Customize Firefox tour in the onBoarding overlay;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/147904/diff/6-7/

Comment 54

[[e-mail]]

Comment on attachment 8876564 [details]
Bug 1357029 - Should add the Default Browser tour in the onBoarding overlay;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/147906/diff/6-7/

Comment 55

[[e-mail]]

patch updated, the conflict is caused because extra string is added for hide overlay checkbox

Comment 56

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/1bd20be7a42c
Should add the Add-on tour in the onBoarding overlay;r=Fischer,mossop,rexboy
https://hg.mozilla.org/integration/autoland/rev/288234ae0dfa
Should add the Customize Firefox tour in the onBoarding overlay;r=Fischer,mossop,rexboy
https://hg.mozilla.org/integration/autoland/rev/78f9b7e7a20e
Should add the Default Browser tour in the onBoarding overlay;r=Fischer,mossop,rexboy

Comment 57

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/1bd20be7a42c
https://hg.mozilla.org/mozilla-central/rev/288234ae0dfa
https://hg.mozilla.org/mozilla-central/rev/78f9b7e7a20e

Comment 58

[Md. Majedul islam]

This feature "add the Add-on tour in the onBoarding overlay" has been implemented with Latest Nightly 56.0a1 on Ubuntu 16.04 (64 bit).

Build ID :   20170620100236
User Agent : Mozilla/5.0 (X11; Linux x86_64; rv:56.0) Gecko/20100101 Firefox/56.0

Comment 59

[Saddam Hossain]

This feature "add the Add-on tour in the onBoarding overlay" has been implemented with Latest Nightly 56.0a1  on Windows 10, 64 Bit!

Build ID :   20170620030208
User Agent : Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:56.0) Gecko/20100101 Firefox/56.0

[bugday-20170621]

Comment 60

[Md. Majedul islam]

As per Comment 58 and Comment 59, I am marking this bug as verified fixed.