Update onboarding for Fx 40 with single slide

VERIFIED FIXED in Firefox 40

Status

()

Firefox
New Tab Page
P1
normal
VERIFIED FIXED
3 years ago
a year ago

People

(Reporter: Kevin Ghim, Assigned: maxim zhilyaev)

Tracking

(Blocks: 1 bug)

Trunk
Firefox 42
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +
qe-verify +

Firefox Tracking Flags

(firefox39 unaffected, firefox40+ verified, firefox41 verified, firefox42 verified, firefox-esr38 unaffected)

Details

(Whiteboard: .001)

Attachments

(8 attachments, 10 obsolete attachments)

184.93 KB, image/png
Details
980.80 KB, image/png
Details
237.08 KB, application/pdf
Details
1.07 MB, image/png
Details
1.05 MB, image/png
Details
24.71 KB, patch
Details | Diff | Splinter Review
24.59 KB, patch
Details | Diff | Splinter Review
28.88 KB, patch
Details | Diff | Splinter Review
(Reporter)

Description

3 years ago
Created attachment 8633829 [details]
NT Privacy Notice.png

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.
(Reporter)

Updated

3 years ago
status-firefox42: affected → ---
Whiteboard: .001

Comment 1

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

Comment 2

3 years ago
which other flow are you thinking?
Flags: needinfo?(edilee)

Comment 3

3 years ago
There's the new user onboarding and existing user onboarding.
Flags: needinfo?(edilee)
Is this bug replacing bug 1176985? Or is this a temporary solution?
(Reporter)

Comment 5

3 years ago
(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.

Updated

3 years ago
Blocks: 1184288
(Reporter)

Comment 6

3 years ago
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.

Comment 7

2 years ago
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.

Comment 10

2 years ago
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.

Comment 11

2 years ago
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?
Flags: needinfo?(athornburgh)
(Assignee)

Comment 12

2 years ago
Created attachment 8634967 [details]
current look of on boarding into page

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.
(Assignee)

Updated

2 years ago
Attachment #8634967 - Attachment description: current look of onbarding into page → current look of on boarding into page
(Assignee)

Comment 13

2 years ago
Created attachment 8634968 [details] [diff] [review]
v1.  aurora patch - first cut
Attachment #8634968 - Flags: review?(msamuel)
(Assignee)

Comment 14

2 years ago
(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

Comment 15

2 years ago
Max, I will provide further guidance with a light Style Guide later tomorrow.

You're very, very close! Thank you :)

Comment 16

2 years ago
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?
Flags: needinfo?(athornburgh)

Comment 17

2 years ago
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.

Comment 18

2 years ago
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
Attachment #8634968 - Flags: feedback-

Updated

2 years ago
Assignee: nobody → mzhilyaev
(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.
Flags: needinfo?(kghim)
(Reporter)

Comment 21

2 years ago
We had legal copy review done. Let's have Matej review marketing copy.
Flags: needinfo?(kghim) → needinfo?(matej)
(Assignee)

Comment 22

2 years ago
(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
Flags: needinfo?(athornburgh)

Comment 23

2 years ago
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.

Comment 24

2 years ago
Max, don't worry about the "X" or clicking outside window to close for this round. We'll address those in the next version.
Flags: needinfo?(athornburgh)
(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?
Flags: needinfo?(kghim)
(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
Flags: needinfo?(athornburgh)

Comment 28

2 years ago
(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.

Comment 29

2 years ago
Created attachment 8635387 [details]
NT-Privacy_Notice_SG-V1.pdf

Max, here's a Style Guide for the Privacy Notice
Flags: needinfo?(athornburgh)

Updated

2 years ago
Attachment #8635387 - Attachment mime type: application/octet → application/pdf
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.
(Assignee)

Comment 31

2 years ago
Created attachment 8635489 [details] [diff] [review]
v2.  re-using current property strings and fixing look and feel

this patch contains modifications to newTab.properties files and is suitable for nightly
Attachment #8634968 - Attachment is obsolete: true
Attachment #8634968 - Flags: review?(msamuel)
Attachment #8635489 - Flags: review?(msamuel)
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
Attachment #8635489 - Flags: review?(msamuel) → review+
> @@ +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 34

2 years ago
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.
Attachment #8635489 - Flags: feedback?(francesco.lodolo)

Comment 35

2 years ago
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
(Assignee)

Comment 36

2 years ago
Created attachment 8635530 [details] [diff] [review]
v3.  fixed reviewer comments
Attachment #8635489 - Attachment is obsolete: true
Attachment #8635489 - Flags: feedback?(francesco.lodolo)
(Assignee)

Comment 37

2 years ago
Created attachment 8635559 [details] [diff] [review]
v4.  reviewer comments

Finalized nightly patch: intro.js concatenates existing strings from newTab.properties to avoid strings modifications. Ready to land upon positive feedback from flod
Attachment #8635530 - Attachment is obsolete: true
Attachment #8635559 - Flags: feedback?(francesco.lodolo)

Comment 38

2 years ago
Are we only reusing strings for aurora and beta -> new strings on nightly? Or reused strings everywhere?
(Assignee)

Comment 39

2 years ago
Created attachment 8635580 [details] [diff] [review]
v5.  missed reviewer comments
Attachment #8635559 - Attachment is obsolete: true
Attachment #8635559 - Flags: feedback?(francesco.lodolo)
Attachment #8635580 - Flags: feedback?(francesco.lodolo)
(Assignee)

Comment 40

2 years ago
(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
(Assignee)

Comment 41

2 years ago
Created attachment 8635607 [details] [diff] [review]
v1.  aurora patch - no changes to newTab.properties

Aurora patch - identical to nightly patch, but does not have changes to newTab.properties
Attachment #8635607 - Flags: feedback?(francesco.lodolo)

Comment 42

2 years ago
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
(Assignee)

Comment 43

2 years ago
Created attachment 8635664 [details] [diff] [review]
v6. remove unsued pref
Attachment #8635580 - Attachment is obsolete: true
Attachment #8635580 - Flags: feedback?(francesco.lodolo)
Attachment #8635664 - Flags: feedback?(francesco.lodolo)
(Assignee)

Comment 44

2 years ago
Created attachment 8635665 [details] [diff] [review]
v2. aurora patch - keeps newTab.properties unchanged
Attachment #8635607 - Attachment is obsolete: true
Attachment #8635607 - Flags: feedback?(francesco.lodolo)
Attachment #8635665 - Flags: feedback?(francesco.lodolo)
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.
Attachment #8635665 - Flags: feedback?(francesco.lodolo) → feedback+
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
Attachment #8635664 - Flags: feedback?(francesco.lodolo) → feedback-
Created attachment 8635793 [details]
italian.png
Created attachment 8635794 [details]
german.png
> 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.
(Assignee)

Comment 51

2 years ago
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?
Flags: needinfo?(francesco.lodolo)
(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).
Flags: needinfo?(francesco.lodolo)

Updated

2 years ago
Attachment #8635665 - Flags: review+
(Assignee)

Comment 53

2 years ago
Created attachment 8635832 [details] [diff] [review]
v7.  fixed newTab.properties issues and removed concatanation in intro.js
Attachment #8635664 - Attachment is obsolete: true
Attachment #8635832 - Flags: feedback?(francesco.lodolo)
(Assignee)

Updated

2 years ago
Attachment #8635665 - Attachment description: v2. aurora patch - same as nightly, but newTab.properties unchanged → v2. aurora patch - keeps newTab.properties unchanged
(Assignee)

Comment 54

2 years ago
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
Attachment #8635665 - Flags: approval-mozilla-aurora?
(Assignee)

Comment 55

2 years ago
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
Attachment #8635665 - Flags: approval-mozilla-beta?
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
Attachment #8635832 - Flags: feedback?(francesco.lodolo) → feedback+
I got a little lost in the different versions here. Can someone clarify which one I should be looking at? Thanks.
Flags: needinfo?(matej)
(Assignee)

Comment 58

2 years ago
(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.
status-firefox39: --- → unaffected
status-firefox40: --- → affected
status-firefox41: --- → affected
status-firefox42: --- → affected
status-firefox-esr38: --- → unaffected
tracking-firefox40: --- → +
Flags: needinfo?(mzhilyaev)
(Assignee)

Comment 60

2 years ago
(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+
Attachment #8635665 - Flags: approval-mozilla-beta?
Attachment #8635665 - Flags: approval-mozilla-beta+
Attachment #8635665 - Flags: approval-mozilla-aurora?
Attachment #8635665 - Flags: approval-mozilla-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.
Flags: needinfo?(mzhilyaev)
(Assignee)

Comment 63

2 years ago
Created attachment 8636119 [details] [diff] [review]
v8.  Implement originally-requested intro design and fix flod's comments
Attachment #8635832 - Attachment is obsolete: true
(Assignee)

Comment 64

2 years ago
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.
(Reporter)

Comment 65

2 years ago
(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 (⚙).
Flags: needinfo?(kghim)
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 (⚙).
(Assignee)

Comment 67

2 years ago
Created attachment 8636219 [details] [diff] [review]
v1.  beta patch

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

Comment 68

2 years ago
Created attachment 8636221 [details] [diff] [review]
v3. aurora patch

tests are passing on my local aurora branch
try server started:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=73cf164a9585
Attachment #8635665 - Attachment is obsolete: true
(Assignee)

Comment 69

2 years ago
Created attachment 8636325 [details] [diff] [review]
v9. incorporating matej changes
Attachment #8636119 - Attachment is obsolete: true

Comment 70

2 years ago
https://hg.mozilla.org/integration/fx-team/rev/39cfcd483236
https://hg.mozilla.org/mozilla-central/rev/39cfcd483236
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox42: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 42
https://hg.mozilla.org/releases/mozilla-aurora/rev/92dd75b29646
status-firefox41: affected → fixed
Flags: in-testsuite+
https://hg.mozilla.org/releases/mozilla-beta/rev/fee5d27ef355
status-firefox40: affected → fixed
Flags: qe-verify+
QA Contact: cornel.ionce
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?
Flags: needinfo?(mzhilyaev)
(Assignee)

Comment 75

2 years ago
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!
Status: RESOLVED → VERIFIED
status-firefox40: fixed → verified
status-firefox41: fixed → verified
status-firefox42: fixed → verified
Flags: needinfo?(mzhilyaev)
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"?

Comment 78

2 years ago
Stefan - The option, specifically, is to check/un-check the box next to "Include suggested sites".
Depends on: 1319432
You need to log in before you can comment on or make changes to this bug.