Since we're not going to display paid sponsorship for Fx 40, we're going to simplify the onboarding message with the bare necessity. Attached is the mock up. Uplift to beta 40 needed.

There's currently 2 onboarding flows. Is this replacing both?

which other flow are you thinking?

There's the new user onboarding and existing user onboarding.

Is this bug replacing bug 1176985? Or is this a temporary solution?

(In reply to Marina Samuel [:emtwo] from comment #4) > Is this bug replacing bug 1176985? Or is this a temporary solution? This is replacing bug 1176985. Moving forward we'll use this as the onboarding message for both existing and new users. For 40, the notice is for En-US users only. For 41, it should be localized for all (global) users.

Update from legal regarding links: Mozilla Privacy Notice should link to https://www.mozilla.org/en-US/privacy/ More about New Tab should link to https://www.mozilla.org/en-US/firefox/tiles/ Delete the "More about your privacy" at the bottom.

Talked to Doug, glad to hear that this is out of l10n scope for 40. Technical question, how is this dialog triggered? Are we going to show that as part of software update, or would we only show that once we actually have suggested tiles delivered to the user? Flod also mentioned that the https://www.mozilla.org/firefox/tiles/ page in its current form isn't localized yet, and back in bug 1124724 comment 5, it says that we shouldn't start that yet. Been a while, what's the status there? For 41, we're already string-frozen for Firefox, and mid-cycle. Quite a few locales think they're done, so we should be mindful on whether suggested tiles are actually coming up in localized builds in the 41 timeframe.

I also just found out bug 1150180 about further updates to the same page.

> Technical question, how is this dialog triggered? Are we going to show that > as part of software update, or would we only show that once we actually have > suggested tiles delivered to the user? The current implementation triggers the dialog on update (or first install). > For 41, we're already string-frozen for Firefox, and mid-cycle. Quite a few > locales think they're done, so we should be mindful on whether suggested > tiles are actually coming up in localized builds in the 41 timeframe. I just wanted to note that it *might* be possible to avoid breaking string freeze in 41 if we re-use existing strings. We have strings already in 41 with the message we want to portray here but they are a little more verbose.

Combining sentences into a paragraph could be tricky/dangerous for l10n. And the strings don't match exactly with the new copy: 57 newtab.intro.paragraph9=Now when you open New Tab, you'll also see sites we think might be interesting to you. 52 newtab.intro.paragraph7=Some of the sites you will see may be suggested by Mozilla and may be sponsored by a Mozilla partner. We'll always indicate which sites are sponsored. 39 newtab.intro.paragraph2=In order to provide this service, Mozilla collects and uses certain analytics information relating to your use of the tiles in accordance with our %1$S. 44 newtab.intro.paragraph4.2=You can turn off this service by clicking the gear (%1$S) button and unchecking "Include suggested sites" in the %2$S menu.

The current onboarding has two titles depending on the type of onboarding: 67 newtab.intro.header.welcome=Welcome to New Tab on %1$S! 68 newtab.intro.header.update=New Tab got an update! Are we always showing the latter for update?

This is how current intro looks like in the fix. Note that it does not have "X" image in the upper-right corner. If image is to be added and/or look and feel is not proper, please provide UI spec and the image attached. I have no way to figure from the screen shot what css parameters should be and which image button to use.

(In reply to maxim zhilyaev from comment #12) > Note that it does not have "X" image in the upper-right corner. My recommendation is not to add "X" at all - it does not add any useful functionality to intro page

Max, I will provide further guidance with a light Style Guide later tomorrow. You're very, very close! Thank you :)

Max, per the "X"... In principal, I agree with you. The "X" is a duplication of the giant blue button. In addition, clicking anywhere outside the window should close it and restore the user's default New Tab view. However... In many tests we've done that include a layover window, we find that user's specially ask for the "X" button. It's weird, but they literally do not read a friggin' thing. So I've been including the "X" as a gesture of friendship to those who are literal. If they keep asking us for it, should we not consider it? What are your thoughts?

Please get copy review, I found grammar bugs, but I also think there might be wordings that are repetitive without being helpful that way. Matej Novak is a good candidate. Also, the localization note did make me look into the code, and I'm not sure I found what I expected. In other news, it'd be good if we could run our tests independent of en-US, no idea to which extent that's an existing problem in this part of the code.

What's the onboarding experience for 40 non en-US users? We still need to show something because there's sponsored directory tiles.

Comment on attachment 8634968 [details] [diff] [review] v1. aurora patch - first cut Review of attachment 8634968 [details] [diff] [review]: ----------------------------------------------------------------- I'm still really confused by this bug. Is this a patch only for fx41, or will it land on mozilla-central as well? Do we actually need to remove the other strings? Are they dropped for good? ::: browser/locales/en-US/chrome/browser/newTab.properties @@ +33,5 @@ > # LOCALIZATION NOTE(newtab.enhanced.explain): %1$S will be replaced inline by > # the gear icon used to customize the new tab window. %2$S will be replaced by > # an active link using string newtab.learn.link as text. > newtab.enhanced.explain=A Mozilla partner has visually enhanced this tile, replacing the screenshot. You can turn off enhanced tiles by clicking the %1$S button for your preferences. %2$S > +newtab.intro.paragraph1=Now when you open New Tab, you'll also see sites we think might be interesting to you. Some sites may be suggested by Mozilla and may be sponsored by a Mozilla partner. Don't use "newtab.intro.paragraph1" as string ID. We used to have it (it's currently in the release channel), which means we might end up showing obsolete translations. «When you open a new tab, you’ll see tiles from the sites you frequently visit, along with tiles that we think might be of interest to you. Some of these tiles may be sponsored by Mozilla partners. We’ll always indicate to you which tiles are sponsored. %1$S» @@ +37,5 @@ > +newtab.intro.paragraph1=Now when you open New Tab, you'll also see sites we think might be interesting to you. Some sites may be suggested by Mozilla and may be sponsored by a Mozilla partner. > +# LOCALIZATION NOTE(newtab.intro.paragraph8): %1$S will be replaced inline by > +# active link using string newtab.privacy.link as text. %2$S will be replaced > +# inline by the gear icon used to customize the new tab window. > +newtab.intro.paragraph2=In order to provide the service, some data is automatically sent to Mozilla in accordance with our %1$S. You can turn off thos service by unchking the option under gear icon (%2$S). Besides the amount of typos in the second sentence, you need a new ID if you change an existing string. Also, need to match the ID in the comment with the actual ID. https://developer.mozilla.org/en-US/docs/Mozilla/Localization/Localization_content_best_practices#Changing_existing_strings

(In reply to Axel Hecht [:Pike] from comment #17) > Please get copy review, I found grammar bugs, but I also think there might > be wordings that are repetitive without being helpful that way. Matej Novak > is a good candidate. I assume we already had copy approval for the attachment by Kevin. Is that correct, Kevin? Max just needs to make sure he copied the text over correctly.

We had legal copy review done. Let's have Matej review marketing copy.

(In reply to Aaron from comment #16) > In addition, clicking anywhere outside the window should close it > and restore the user's default New Tab view. This is not how current intro works in release. The use has to click the intro button to get out of it. So, I would like to avoid a change in behavior if possible. > > So I've been including the "X" as a gesture of friendship to those who are > literal. If they keep asking us for it, should we not consider it? Generally, I am fixing this in a hurry to have it ready for uplift. In this situation, the less we do the better - if we can push "X" into next release - great, otherwise, attach an image to the bug and i'll try to glue it in

Matej, I stumbled over the sequence of "may"s in the first paragraph, wondering what happened to those tiles that are neither. I felt somewhat uninformed at that point.

Max, don't worry about the "X" or clicking outside window to close for this round. We'll address those in the next version.

(In reply to Ed Lee :Mardak from comment #18) > What's the onboarding experience for 40 non en-US users? We still need to > show something because there's sponsored directory tiles. I think our options are either to use the existing localized strings to provide them a message or perhaps we do need to get stuff localized in beta :( I'd suggest falling back to the "What is this page?" text like we did before, but I believe we removed that. Kevin?

(In reply to Ed Lee :Mardak from comment #11) > The current onboarding has two titles depending on the type of onboarding: > > 67 newtab.intro.header.welcome=Welcome to New Tab on %1$S! > 68 newtab.intro.header.update=New Tab got an update! > > Are we always showing the latter for update? I think it would make more sense to still have separate titles. Aaron, you might already have an answer to this in your style guide.. but what do you think?

see comment 26

(In reply to Marina Samuel [:emtwo] from comment #25) > I think our options are either to use the existing localized strings to > provide them a message or perhaps we do need to get stuff localized in beta As in comment 10, we could potentially reuse the existing translated strings with potential l10n composition issues. Here's the proposed text from attachment 8633829 [details]: Now when you open New Tab, you'll also see sites we think might be interesting to you. Some sites may be suggested by Mozilla and may be sponsored by a Mozilla partner. In order to provide this service, some data is automatically sent to Mozilla in accordance with our Privacy Notice. You can turn off this service by unchecking the option under the gear icon (⚙). If we reused existing strings: Now when you open New Tab, you'll also see sites we think might be interesting to you. Some of the sites you will see may be suggested by Mozilla and may be sponsored by a Mozilla partner. We'll always indicate which sites are sponsored. In order to provide this service, Mozilla collects and uses certain analytics information relating to your use of the tiles in accordance with our Privacy Notice. You can turn off this service by clicking the gear (⚙) button and unchecking "Include suggested sites" in the New Tab Controls menu.

Max, here's a Style Guide for the Privacy Notice

I would have preferred not to have the last sentence in the first paragraph because it doesn't serve a purpose but I'm good with eds the proposal and the re-used strings.

this patch contains modifications to newTab.properties files and is suitable for nightly

Comment on attachment 8635489 [details] [diff] [review] v2. re-using current property strings and fixing look and feel Review of attachment 8635489 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/base/content/newtab/intro.js @@ +40,4 @@ > // Set the paragraphs > let paragraphNodes = this._nodes.text.getElementsByTagName("p"); > + paragraphNodes[0].innerHTML = this._paragraphs[0]; > + paragraphNodes[1].innerHTML = this._paragraphs[1]; These would be better in a loop so we can accommodate a variable number of paragraphs and avoid indexing paragraphNodes without knowing its length @@ +66,5 @@ > let customizeIcon = '<input type="button" class="newtab-control newtab-customize"/>'; > + this._paragraphs.push(newTabString("intro.paragraph9") + " " + newTabString("intro.paragraph7")); > + this._paragraphs.push( > + newTabString("intro.paragraph2", [this._link(TILES_PRIVACY_LINK, newTabString("privacy.link"))]) + " " + > + newTabString("intro.paragraph4.2", [customizeIcon,this._bold(newTabString("intro.controls"))])); It may be nicer to user template strings for the paragraphs: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/template_strings We're using them in the _bold() and _link() functions @@ +99,3 @@ > ].forEach((arg, index) => { > footerLinkNodes[index].innerHTML = arg; > }); footerLinkNodes[0].innerHTML = this._link(TILES_INTRO_LINK, newTabString("learn.link2")) is simpler ::: browser/base/content/newtab/newTab.xul @@ +65,5 @@ > <p/><p/> > </div> > </div> > <div id="newtab-intro-buttons"> > + <input type="button" default="ture" onclick="gIntro._exitIntro()"/> ture -> true ::: browser/base/content/test/newtab/browser_newtab_intro.js @@ +17,5 @@ > Services.prefs.setBoolPref(PRELOAD_PREF, false); > > let intro; > let brand = Services.strings.createBundle("chrome://branding/locale/brand.properties"); > let brandShortName = brand.GetStringFromName("brandShortName"); You can probably remove these two lines above. brandShortName is no longer used @@ +44,2 @@ > > // Test clicking the 'next' and 'back' buttons. Please update this comment to reflect the changes

> @@ +99,3 @@ > > ].forEach((arg, index) => { > > footerLinkNodes[index].innerHTML = arg; > > }); > > footerLinkNodes[0].innerHTML = this._link(TILES_INTRO_LINK, > newTabString("learn.link2")) is simpler On second thought, can we give the <li> in the footer an ID and use getElementByID() to reference it here instead? I think it's better than using a direct index on footerLinkNodes. We should probably do the same for the button too since we only have 1 now as well.

Comment on attachment 8635489 [details] [diff] [review] v2. re-using current property strings and fixing look and feel >+++ b/browser/base/content/newtab/intro.js > // The maximum paragraph ID listed for 'newtab.intro.paragraph' > // strings in newTab.properties > const MAX_PARAGRAPH_ID = 9; > const NUM_INTRO_PAGES = 3; These are unused. > init: function() { > let brand = Services.strings.createBundle("chrome://branding/locale/brand.properties"); > this._brandShortName = brand.GetStringFromName("brandShortName"); emtwo mentioned the unused brand for the test. These should be unused now, so remove from intro.js. > _generateParagraphs: function() { >+ this._paragraphs.push(newTabString("intro.paragraph9") + " " + newTabString("intro.paragraph7")); >+ this._paragraphs.push( >+ newTabString("intro.paragraph2", [this._link(TILES_PRIVACY_LINK, newTabString("privacy.link"))]) + " " + >+ newTabString("intro.paragraph4.2", [customizeIcon,this._bold(newTabString("intro.controls"))])); nit: space after "customizeIcon," if that remains after emtwo's suggestion for template strings. f?flod for anything we should watch out for concatenating two sentences with a space.

Comment on attachment 8635489 [details] [diff] [review] v2. re-using current property strings and fixing look and feel >+++ b/browser/locales/en-US/chrome/browser/newTab.properties > newtab.intro.paragraph5=New Tab will show the sites you visit most frequently, along with sites we think might be of interest to you. To get started, you'll see several sites from Mozilla. This paragraph is unused

Finalized nightly patch: intro.js concatenates existing strings from newTab.properties to avoid strings modifications. Ready to land upon positive feedback from flod

Are we only reusing strings for aurora and beta -> new strings on nightly? Or reused strings everywhere?

(In reply to Ed Lee :Mardak from comment #38) > Are we only reusing strings for aurora and beta -> new strings on nightly? > Or reused strings everywhere? My understanding was that we reuse strings on nightly as well. So, the nightly patch only differs from aurora patch by not having changes to newTab.properties

Aurora patch - identical to nightly patch, but does not have changes to newTab.properties

You'll want to remove the update intro pref from various places: http://mxr.mozilla.org/mozilla-central/source/browser/app/profile/firefox.js#1683 http://mxr.mozilla.org/mozilla-central/source/testing/profiles/prefs_general.js#197

Comment on attachment 8635665 [details] [diff] [review] v2. aurora patch - keeps newTab.properties unchanged Review of attachment 8635665 [details] [diff] [review]: ----------------------------------------------------------------- I think this approach should be safe for Aurora. Most important, it lets us move on without breaking string freeze.

Comment on attachment 8635664 [details] [diff] [review] v6. remove unsued pref Review of attachment 8635664 [details] [diff] [review]: ----------------------------------------------------------------- While this approach is more than fine for Aurora, I think we need to make things clearer in mozilla-central since we don't have the problem with string freeze: * Avoid unnecessary concatenations. * Fix the strings so that IDs make sense, and put them in the right order in the file (right now the order is 9->7->2->4.2). Eventually update the text. One thing that I'm a bit concerned about is the window size (for both branches). Can someone give it a try with longer strings? Italian or German are usually good candidates http://hg.mozilla.org/releases/l10n/mozilla-aurora/it/file/default/browser/chrome/browser/newTab.properties http://hg.mozilla.org/releases/l10n/mozilla-aurora/de/file/default/browser/chrome/browser/newTab.properties

> One thing that I'm a bit concerned about is the window size (for both > branches). Can someone give it a try with longer strings? Italian or German > are usually good candidates > http://hg.mozilla.org/releases/l10n/mozilla-aurora/it/file/default/browser/ > chrome/browser/newTab.properties > http://hg.mozilla.org/releases/l10n/mozilla-aurora/de/file/default/browser/ > chrome/browser/newTab.properties attached Italian and German screenshots...they look good to me. flod?

Thanks for checking, both look great.

Touching strings first time, hence these naive questions: - removing catanation means changing strings. Do i need to assign different IDs for new strings, or reusing old ones are fine? I am not clear what a property with same ID but different string may break. - Is it save to remove last sentence of first para that Jishnu disliked: "We'll always indicate which sites are sponsored."? What could go wrong of anything?

(In reply to maxim zhilyaev from comment #51) > - removing catanation means changing strings. Do i need to assign different > IDs for new strings, or reusing old ones are fine? I am not clear what a > property with same ID but different string may break. Yes, you need to use brand new IDs. That would also fix the confusing order. More information here https://developer.mozilla.org/en-US/docs/Mozilla/Localization/Localization_content_best_practices#Changing_existing_strings Strings will be considered as brand new. If you reuse string IDs, nobody will notice that the strings have changed, and we'll keep shipping the old translations. > - Is it save to remove last sentence of first para that Jishnu disliked: > "We'll always indicate which sites are sponsored."? What could go wrong of > anything? See above. Strings will be translated again, so no risk unless they decide to copy and paste the old translation (unlikely, impossible if they use tools with translation memory).

Comment on attachment 8635665 [details] [diff] [review] v2. aurora patch - keeps newTab.properties unchanged Approval Request Comment [Feature/regressing bug #]: 1183932 [User impact if declined]: High Per dougt request, a confusing 3 page on-boarding needs to be replaced with a single page intro message [Describe test coverage new/current, TreeHerder]: mochitest - browser/base/content/test/newtab/browser_newtab_intro.js TreeHerder - https://treeherder.mozilla.org/#/jobs?repo=try&revision=cfe942eb3bb9 [Risks and why]: Minimal, changes were tested on multile locales. [String/UUID change made/needed]: NO

Comment on attachment 8635665 [details] [diff] [review] v2. aurora patch - keeps newTab.properties unchanged Approval Request Comment [Feature/regressing bug #]: 1183932 [User impact if declined]: High Per dougt request, a confusing 3 page on-boarding needs to be replaced with a single page intro message [Describe test coverage new/current, TreeHerder]: mochitest - browser/base/content/test/newtab/browser_newtab_intro.js TreeHerder - https://treeherder.mozilla.org/#/jobs?repo=try&revision=cfe942eb3bb9 [Risks and why]: Minimal, changes were tested on multile locales. [String/UUID change made/needed]: NO

Comment on attachment 8635832 [details] [diff] [review] v7. fixed newTab.properties issues and removed concatanation in intro.js Review of attachment 8635832 [details] [diff] [review]: ----------------------------------------------------------------- I would personally go with newtab.intro1.paragraph1 and newtab.intro1.paragraph2, but that's nitpicking and you should free to ignore my suggestion. String-wise this looks good. ::: browser/locales/en-US/chrome/browser/newTab.properties @@ +35,5 @@ > # an active link using string newtab.learn.link as text. > newtab.enhanced.explain=A Mozilla partner has visually enhanced this tile, replacing the screenshot. You can turn off enhanced tiles by clicking the %1$S button for your preferences. %2$S > +newtab.intro.paragraph10=Now when you open New Tab, you'll also see sites we think might be interesting to you. Some of the sites you will see may be suggested by Mozilla and may be sponsored by a Mozilla partner. > +# LOCALIZATION NOTE(newtab.intro.paragraph11): %1$S will be replaced inline by > +# active link using string newtab.privacy.link as text. %2$S will be replaced "an" active link

I got a little lost in the different versions here. Can someone clarify which one I should be looking at? Thanks.

(In reply to Francesco Lodolo [:flod] from comment #56) > Comment on attachment 8635832 [details] [diff] [review] > v7. fixed newTab.properties issues and removed concatanation in intro.js > > Review of attachment 8635832 [details] [diff] [review]: > ----------------------------------------------------------------- > > I would personally go with newtab.intro1.paragraph1 and > newtab.intro1.paragraph2, but that's nitpicking and you should free to > ignore my suggestion. Excellent suggestion - I will do precisely that. thanks for pointing out.

(In reply to maxim zhilyaev from comment #58) > Excellent suggestion - I will do precisely that. thanks for pointing out. I take it this means that we need a new patch before uplifting. Before uplifting, is there a reason that this fix shouldn't land on m-c? Have you verified that fix in tree? Tracking 40 as we need a change in this version.

(In reply to Lawrence Mandel [:lmandel] (use needinfo) from comment #59) > (In reply to maxim zhilyaev from comment #58) > > Excellent suggestion - I will do precisely that. thanks for pointing out. > > I take it this means that we need a new patch before uplifting. Lawrence, this comment was specific to nightly patch - which I will be changing. However, the aurora patch https://bugzilla.mozilla.org/attachment.cgi?id=8635665&action=edit Has to be uplifted to aurora and beta as is: the patch is made to avoid changes to strings > Before uplifting, is there a reason that this fix shouldn't land on m-c? > Have you verified that fix in tree? The nightly path will go few strings modification and will land on m-c hopefully today provided flod's feedback+. The patch was verified on treeherder: https://treeherder.mozilla.org/#/jobs?repo=try&revision=cfe942eb3bb9 Do you recommend any extra verifications? > Tracking 40 as we need a change in this version.

Comment on attachment 8635665 [details] [diff] [review] v2. aurora patch - keeps newTab.properties unchanged Given that there is a different fix coming for 42, let's get this into 40 and 41 and verify there. I don't know whether this UI will be triggered for the audience of these channels. We should verify the fix independently. Beta+ Aurora+

(In reply to Lawrence Mandel [:lmandel] (use needinfo) from comment #61) > I don't know whether this UI will be triggered for > the audience of these channels. Maxim confirms that the UI will be triggered for all users.

NOTE to release drivers: Working on merging aurora and beta patches cleanly and starting try server on them. Will reattach new patches and try server links for each branch shortly.

(In reply to Matej Novak [:matej] from comment #57) > I got a little lost in the different versions here. Can someone clarify > which one I should be looking at? Thanks. Matej, sorry for the confusion. Since we're going to use the an existing copy for 40 and 41, we cannot change the text there. For, 42, below is the copy we're going to use and we'd like your review: Now when you open New Tab, you'll also see sites we think might be interesting to you. Some sites may be suggested by Mozilla and may be sponsored by a Mozilla partner. In order to provide this service, some data is automatically sent to Mozilla in accordance with our Privacy Notice. You can turn off this service by unchecking the option under the gear icon (⚙).

Thanks. Just a couple of small edits: Now when you open New Tab, you'll also see sites we think might be interesting to you. Some may be suggested by Mozilla or sponsored by one of our partners. In order to provide this service, some data is automatically sent back to us in accordance with our Privacy Notice. You can turn this off by unchecking the option under the gear icon (⚙).

tests are passing in beta tree on my local machine. try server run started: https://treeherder.mozilla.org/#/jobs?repo=try&revision=6fe6d9231fc1

tests are passing on my local aurora branch try server started: https://treeherder.mozilla.org/#/jobs?repo=try&revision=73cf164a9585

Tested on Windows 10 64-bit, Windows 7 64-bit, Mac OS X 10.10.4 and Ubuntu 12.04 32-bit. The onboarding is changed as it follows: *On 42.0a1 Nightly (build ID: 20150726030217), the implementation is the following: http://i.imgur.com/rR5Qiqq.png *On 41.0a2 Aurora (20150726004010) and Firefox 40.0b7 (20150723165742): http://i.imgur.com/yEOvmhB.png However, the visual styles does not match the design from https://bug1183932.bmoattachments.org/attachment.cgi?id=8635387. Is this design outdated? Will the current implementation remain as it is or should I fill a follow-up bug?

It is my understanding that nightly implementation is final. Design spec was matched as much as possible, however, we had to account for possible changes caused by translated strings which is why it's slightly modified. Also, there were last minute changes from matej that altered the original text.

It this case, I consider it safe to mark this issue as verified, based on the above comments. Thanks!

https://hg.mozilla.org/mozilla-central/diff/39cfcd483236/browser/locales/en-US/chrome/browser/newTab.properties > +# LOCALIZATION NOTE(newtab.intro1.paragraph2): %1$S will be replaced inline by > +# an active link using string newtab.privacy.link as text. %2$S will be replaced > +# inline by the gear icon used to customize the new tab window. > +newtab.intro1.paragraph2=In order to provide this service, some data is automatically sent back to us in accordance with our %1$S. You can turn this off by unchecking the option under the gear icon (%2$S) "the option"?

Stefan - The option, specifically, is to check/un-check the box next to "Include suggested sites".