Closed Bug 1357641 Opened 6 years ago Closed 6 years ago

Should show the notification bar to notify user there is onBoarding tour to learn

Categories

(Firefox :: General, defect, P1)

Product:
Firefox
Component:
General
Type:
defect

Tracking

()

Status:
VERIFIED FIXED
Milestone:
Firefox 56
Tracking Flags:
Tracking Status
firefox56 --- verified

People

(Reporter: Fischer, Assigned: Fischer)

References

Details

(Whiteboard: [photon-onboarding] )

Attachments

(4 files)

Bug 1357641 - Part 1: Add onboarding tour notification,
59 bytes, text/x-review-board-request
mossop
: review+
flod
: review+
Details
tour_notification_on_aboutHome.png
444.34 KB, image/png
Details
tour_notification_on_aboutNewTab.png
701.08 KB, image/png
Details
Bug 1357641 - Part 2: Add the browser_onboarding_notification.js test,
59 bytes, text/x-review-board-request
mossop
: review+
Details 
Should show the notification bar to notify user there is onBoarding tour to learn and when clicking the notification bar, will open the onBoarding overlay
Whiteboard: [photon-onboarding]
(In reply to Fischer [:Fischer] from comment #0)
> Should show the notification bar to notify user there is onBoarding tour to
> learn and when clicking the notification bar, will open the onBoarding
> overlay
Currently there will be 6 types of tours to notify: Sync, Add-ons, Search, Customize Firefox, Private browsing, Default browser.
For the completed tour, should not display notification bar.
Priority: -- → P2
Flags: qe-verify+
QA Contact: jwilliams
Assignee: nobody → fliu
Status: NEW → ASSIGNED
Priority: P2 → P1
Target Milestone: --- → Firefox 56
Comment on attachment 8877430 [details]
Bug 1357641 - Part 1: Add onboarding tour notification,

Hi Fred, Rex,

This patch:
- shows tour notification turn by turn
- won't show notification for completed tour

Thanks
Attachment #8877430 - Flags: feedback?(rexboy)
Attachment #8877430 - Flags: feedback?(gasolin)
(In reply to Fischer [:Fischer] from comment #3)
> Comment on attachment 8877430 [details]
> Bug 1357641 - Should show the notification bar to notify user there is
> onBoarding tour to learn
> 
> Hi Fred, Rex,
> 
> This patch:
> - shows tour notification turn by turn
> - won't show notification for completed tour
  - open specific tour for corresponding tour notification
> 
> Thanks
Comment on attachment 8877430 [details]
Bug 1357641 - Part 1: Add onboarding tour notification,

https://reviewboard.mozilla.org/r/148836/#review153368

looks good, some nits

::: browser/extensions/onboarding/content/onboarding.js:27
(Diff revision 1)
>   *   // The string id of tour name which would be displayed on the navigation bar
>   *   tourNameId: "onboarding.tour-addon",
> + *   // The object holding the ids of strings used on tour notification
> + *   notification:
> + *       // The string id of tour notification title
> + *     - titleId: "onboarding.notification.tour-search.title",

we use addons as tour example, so let's keep use it for consistency

::: browser/extensions/onboarding/content/onboarding.js:113
(Diff revision 1)
>  
>      this._overlayIcon.addEventListener("click", this);
>      this._overlay.addEventListener("click", this);
>      // Destroy on unload. This is to ensure we remove all the stuff we left.
>      // No any leak out there.
>      this._window.addEventListener("unload", () => this.destroy());

add unload handler after we inited everything

::: browser/extensions/onboarding/content/onboarding.js:179
(Diff revision 1)
>        // that means clicking outside the tour content area.
>        // Let's toggle the overlay.
>        case "onboarding-overlay":
>          this.toggleOverlay();
> -        break;
> +        return;
> +

no line between cases
Attachment #8877430 - Flags: feedback?(gasolin) → feedback+
Comment on attachment 8877430 [details]
Bug 1357641 - Part 1: Add onboarding tour notification,

Overall looks good to me.
If we can find a way to get rid of the timeout() for animation begin that would be good but for me it's not a blocker for now.
Attachment #8877430 - Flags: feedback?(rexboy) → feedback+

    
    

    
      
  


  

    
    

    
      
  

    
    

    
  
Comment on attachment 8878811 [details]
Bug 1357641 - Part 2: Add the browser_onboarding_notification.js test,

https://reviewboard.mozilla.org/r/150090/#review154770

::: browser/extensions/onboarding/test/browser/browser_onboarding_notification.js:68
(Diff revision 1)
> +  await promiseTourNotificationOpened(tab.linkedBrowser);
> +  let targetTourId = await getCurrentNotificationTargetTourId(tab.linkedBrowser);
> +
> +  // There is 0.8s notiificaiton sliding-up tranisition so must wait a bit here
> +  // or `synthesizeMouseAtCenter` wouldn't be able to click on the final correct position of the action button
> +  await new Promise(resolve => setTimeout(resolve, 1000));

The reason not listening to the css transition end event is because it is possible that by the time we added event listener the event might be already finished. This possibility could result in intermittent timeout failure. Since our goal is to test clicking the action button so a simple `setTimeout` is used.

    
    

    
  
(In reply to Fischer [:Fischer] from comment #9)
> Created attachment 8878811 [details]
> Bug 1357641 - Part 2: Add browser_onboarding_notification.js test,
> 
> Review commit: https://reviewboard.mozilla.org/r/150090/diff/#index_header
> See other reviews: https://reviewboard.mozilla.org/r/150090/
Hi Mossop,

Please see [1][2] for the UI screeshots.
TRY: https://treeherder.mozilla.org/#/jobs?repo=try&revision=dfc8cb903f1cf8c8b4f3f933218616c210fa1611

[1] attachment 8878804 [details]: tour_notification_on_aboutHome.png
[2] attachment 8878805 [details]: tour_notification_on_aboutNewTab.png

Thank you

    
    

    
  
Comment on attachment 8877430 [details]
Bug 1357641 - Part 1: Add onboarding tour notification,

https://reviewboard.mozilla.org/r/148836/#review154852

::: browser/extensions/onboarding/content/onboarding.js:33
(Diff revision 2)
>   *   // The string id of tour name which would be displayed on the navigation bar
>   *   tourNameId: "onboarding.tour-addon",
> + *   // The object holding the ids of strings used on tour notification
> + *   notification:
> + *       // The string id of tour notification title
> + *     - titleId: "onboarding.notification.onboarding-tour-addon.title",

nit: `addons`

::: browser/extensions/onboarding/content/onboarding.js:35
(Diff revision 2)
> + *   // The object holding the ids of strings used on tour notification
> + *   notification:
> + *       // The string id of tour notification title
> + *     - titleId: "onboarding.notification.onboarding-tour-addon.title",
> + *       // The string id of tour notification message
> + *     - messageId: "onboarding.notification.onboarding-tour-addon.body"

`addons`

::: browser/extensions/onboarding/locales/en-US/onboarding.properties:49
(Diff revision 2)
>  # LOCALIZATION NOTE(onboarding.tour-default-browser.win7.button): Label for a button to directly set the default browser (Windows 7). %S is brandShortName
>  onboarding.tour-default-browser.win7.button=Make %S Your Default Browser
> +
> +onboarding.learnMore=Learn More
> +onboarding.notification-icon-tool-tip=New to Firefox?
> +onboarding.notification.onboarding-tour-private-browsing.title=Browse by yourself.

we can put related notification strings below each tour strings

    
    

    
      
  

    
    

    
  
Comment on attachment 8877430 [details]
Bug 1357641 - Part 1: Add onboarding tour notification,

https://reviewboard.mozilla.org/r/148836/#review155358

::: browser/extensions/onboarding/content/onboarding.js:53
(Diff revision 2)
>      id: "onboarding-tour-private-browsing",
>      tourNameId: "onboarding.tour-private-browsing",
> +    notification: {
> +      titleId: "onboarding.notification.onboarding-tour-private-browsing.title",
> +      messageId: "onboarding.notification.onboarding-tour-private-browsing.message",
> +      buttonId: "onboarding.learnMore",

Please have l10n weigh in on whether re-using this text in so many places is ok

::: browser/extensions/onboarding/content/onboarding.js:216
(Diff revision 2)
> +  _initNotification() {
> +    let doc = this._window.document;
> +    if (doc.hidden) {
> +      // When the preloaded-browser feature is on,
> +      // it would preload an hidden about:newtab in the background.
> +      // We don't wnat to show notification in that hidden state.

Why not?

::: browser/extensions/onboarding/content/onboarding.js:223
(Diff revision 2)
> +        if (!doc.hidden) {
> +          doc.removeEventListener("visibilitychange", onVisible);
> +          this.showNotification();
> +        }
> +      };
> +      doc.addEventListener("visibilitychange", onVisible);

Use the once option here

::: browser/extensions/onboarding/content/onboarding.js:332
(Diff revision 2)
>  
> +  isTourCompleted(tourId) {
> +    return Preferences.get(`browser.onboarding.tour.${tourId}.completed`, false);
> +  }
> +
> +  getUncompletedTours() {

getIncompleteTours

::: browser/extensions/onboarding/content/onboarding.js:342
(Diff revision 2)
> +    if (Preferences.get("browser.onboarding.notification.finished", false)) {
> +      return;
> +    }
> +
> +    if (this._notificationBar && this._notificationBar.classList.contains("onboarding-opened")) {
> +      return; // Don't show twice if there was already one out there.

When could this happen?

::: browser/extensions/onboarding/content/onboarding.js:359
(Diff revision 2)
> +    // Pick out the next tour to show
> +    let nextTourIndex = 0;
> +    let lastTourId = Preferences.get("browser.onboarding.notification.lastPrompted", "");
> +    if (lastTourId) {
> +      let lastTourIndex = toursToNotify.findIndex(tour => tour.id == lastTourId);
> +      if (lastTourIndex >= 0) {

What if it is -1?

::: browser/extensions/onboarding/content/onboarding.js:363
(Diff revision 2)
> +    if (nextTourIndex > toursToNotify.length - 1) {
> +      nextTourIndex = 0;
> +    }

nextTourIndex %= toursToNotify.length

::: browser/extensions/onboarding/content/onboarding.js:369
(Diff revision 2)
> +      nextTourIndex = 0;
> +    }
> +    let targetTour = toursToNotify[nextTourIndex];
> +
> +    // Show the target tour notification
> +    if (!this._notificationBar) {

When is this not the case?

        Attachment #8877430 -
        Flags: review?(dtownsend) → review-

    
    

    
  
Comment on attachment 8878811 [details]
Bug 1357641 - Part 2: Add the browser_onboarding_notification.js test,

https://reviewboard.mozilla.org/r/150090/#review155366

::: browser/extensions/onboarding/test/browser/browser_onboarding_notification.js:56
(Diff revision 2)
> +  await promiseTourNotificationOpened(tab.linkedBrowser);
> +  let targetTourId = await getCurrentNotificationTargetTourId(tab.linkedBrowser);
> +
> +  // There is 0.8s notiificaiton sliding-up tranisition so must wait a bit here
> +  // or `synthesizeMouseAtCenter` wouldn't be able to click on the final correct position of the action button
> +  await new Promise(resolve => setTimeout(resolve, 1000));

Use the transition events to get the end of the transition rather than a timeout please. I'd recommend rolling it into the promiseTourNotificationOpened function.

::: browser/extensions/onboarding/test/browser/browser_onboarding_notification.js:66
(Diff revision 2)
> +  is(targetTourId, activeNavItemId, "Should navigate to the target tour item.");
> +  is(`${targetTourId}-page`, activePageId, "Should display the target tour page.");
> +  await BrowserTestUtils.removeTab(tab);
> +});
> +
> +add_task(async function test_not_show_notification_for_completed_tour() {

Also add a test that individual completed tours are skipped.

::: browser/extensions/onboarding/test/browser/head.js:4
(Diff revision 2)
> -Cu.import("resource://gre/modules/Preferences.jsm");
> +let scope = {};
> +Cu.import("resource://gre/modules/Preferences.jsm", scope);
> +let { Preferences } = scope;

let { Preferences } = Cu.import("...", {});

::: browser/extensions/onboarding/test/browser/head.js:118
(Diff revision 2)
> +}
> +
> +function getCurrentActiveTour(browser) {
> +  return ContentTask.spawn(browser, {}, function() {
> +    let list = content.document.querySelector("#onboarding-tour-list");
> +    let items = list.querySelectorAll("li");

Use a more precise selector than this, .onboarding-tour-item or something

        Attachment #8878811 -
        Flags: review?(dtownsend) → review-

    
    

    
      
  

    
    

    
      
  

    
    

    
  
(In reply to Fischer [:Fischer] from comment #19)
> Comment on attachment 8877430 [details]
> Bug 1357641 - Part 1: Add onboarding tour notification,
> y 
> Review request updated; see interdiff:
> https://reviewboard.mozilla.org/r/148836/diff/2-3/
Hi Flod,
Could you please have a look at browser/extensions/onboarding/locales/en-US/onboarding.properties?
Note: `onboarding.learnMore=Learn More` is shared in many places.
Thanks

    
    

    
  
Comment on attachment 8877430 [details]
Bug 1357641 - Part 1: Add onboarding tour notification,

https://reviewboard.mozilla.org/r/148836/#review155546

Do you have specs for this? What's the difference between notifications and tour?

::: browser/extensions/onboarding/locales/en-US/onboarding.properties:48
(Diff revision 3)
>  
>  # LOCALIZATION NOTE(onboarding.tour-default-browser.win7.button): Label for a button to directly set the default browser (Windows 7). %S is brandShortName
>  onboarding.tour-default-browser.win7.button=Make %S Your Default Browser
> +
> +onboarding.learnMore=Learn More
> +onboarding.notification-icon-tool-tip=New to Firefox?

Don't hardcode Firefox

::: browser/extensions/onboarding/locales/en-US/onboarding.properties:53
(Diff revision 3)
> +onboarding.notification-icon-tool-tip=New to Firefox?
> +onboarding.notification.onboarding-tour-private-browsing.title=Browse by yourself.
> +onboarding.notification.onboarding-tour-private-browsing.message=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.notification.onboarding-tour-addons.title=Get more done.
> +onboarding.notification.onboarding-tour-addons.message=Add-ons are small apps you can add to Firefox that do lots of things — from managing to-do lists, to downloading videos, to changing the look of your browser.

Don't hardcode Firefox

::: browser/extensions/onboarding/locales/en-US/onboarding.properties:56
(Diff revision 3)
> +
> +onboarding.notification.onboarding-tour-addons.title=Get more done.
> +onboarding.notification.onboarding-tour-addons.message=Add-ons are small apps you can add to Firefox that do lots of things — from managing to-do lists, to downloading videos, to changing the look of your browser.
> +
> +onboarding.notification.onboarding-tour-customize.title=Rearrange your toolbar.
> +onboarding.notification.onboarding-tour-customize.message=Put the tools you use most right at your fingertips. Add more options to your toolbar. Or select a theme to make Firefox reflect your personality.

Don't hardcode Firefox

::: browser/extensions/onboarding/locales/en-US/onboarding.properties:61
(Diff revision 3)
> +onboarding.notification.onboarding-tour-customize.message=Put the tools you use most right at your fingertips. Add more options to your toolbar. Or select a theme to make Firefox reflect your personality.
> +
> +onboarding.notification.onboarding-tour-search.title=Find it faster.
> +onboarding.notification.onboarding-tour-search.message=Access all of your favorite search engines with a click. Search the whole Web or just one website right from the search box.
> +
> +onboarding.notification.onboarding-tour-default-browser.title=Make Firefox your go-to browser.

Don't hardcode Firefox

::: browser/extensions/onboarding/locales/en-US/onboarding.properties:62
(Diff revision 3)
> +
> +onboarding.notification.onboarding-tour-search.title=Find it faster.
> +onboarding.notification.onboarding-tour-search.message=Access all of your favorite search engines with a click. Search the whole Web or just one website right from the search box.
> +
> +onboarding.notification.onboarding-tour-default-browser.title=Make Firefox your go-to browser.
> +onboarding.notification.onboarding-tour-default-browser.message=It doesn’t take much to get the most from Firefox. Just set Firefox as your default browser and put control, customization, and protection on autopilot.

Don't hardcode Firefox

    
    

    
  
(In reply to Dave Townsend [:mossop] from comment #17)
> Comment on attachment 8877430 [details]
> Bug 1357641 - Part 1: Add onboarding tour notification,
> 
> https://reviewboard.mozilla.org/r/148836/#review155358
> 
> ::: browser/extensions/onboarding/content/onboarding.js:216
> (Diff revision 2)
> > +  _initNotification() {
> > +    let doc = this._window.document;
> > +    if (doc.hidden) {
> > +      // When the preloaded-browser feature is on,
> > +      // it would preload an hidden about:newtab in the background.
> > +      // We don't wnat to show notification in that hidden state.
> 
> Why not?
> 
Consider the case as below
There are Tour #0 ~ #5.
(1) Open an about:newtab #A and #0 notification is prompted, so The last prompted is #0.
(2) A preloaded hidden about:newtab #B would open after Step (1). Another prompt happens, so The last prompted is #1.
(3) Reload about:newtab #A, the #2 notification would show because #1 is prompted in the background in Step (2).
We would miss #1 notification in the background.

> ::: browser/extensions/onboarding/content/onboarding.js:223
> (Diff revision 2)
> > +        if (!doc.hidden) {
> > +          doc.removeEventListener("visibilitychange", onVisible);
> > +          this.showNotification();
> > +        }
> > +      };
> > +      doc.addEventListener("visibilitychange", onVisible);
> 
> Use the once option here
> 
The `document.visibilityState` may have up to 4 states ("visible", "hidden", "prerender", "unloaded").
If `visibilityState` = "visible", `doc.hidden` is ture, otherwise false.
So this is to make sure we show at the right moment then remove the listener.

> ::: browser/extensions/onboarding/content/onboarding.js:342
> (Diff revision 2)
> > +    if (Preferences.get("browser.onboarding.notification.finished", false)) {
> > +      return;
> > +    }
> > +
> > +    if (this._notificationBar && this._notificationBar.classList.contains("onboarding-opened")) {
> > +      return; // Don't show twice if there was already one out there.
> 
> When could this happen?
> 
No at the moment, this shouldn't happen. Remove this redundant check here at this moment, thanks.

> ::: browser/extensions/onboarding/content/onboarding.js:359
> (Diff revision 2)
> > +    // Pick out the next tour to show
> > +    let nextTourIndex = 0;
> > +    let lastTourId = Preferences.get("browser.onboarding.notification.lastPrompted", "");
> > +    if (lastTourId) {
> > +      let lastTourIndex = toursToNotify.findIndex(tour => tour.id == lastTourId);
> > +      if (lastTourIndex >= 0) {
> 
> What if it is -1?
> 
Thanks for reminding this. Updated this part.
We should still consider the completed tours for the right prompting order.
For example. Tour #0 ~ #5, if Tour #3 is completed and last prompted, the next one should be #4.

> ::: browser/extensions/onboarding/content/onboarding.js:363
> (Diff revision 2)
> > +    if (nextTourIndex > toursToNotify.length - 1) {
> > +      nextTourIndex = 0;
> > +    }
> 
> nextTourIndex %= toursToNotify.length
> 
Thanks, updated.

> ::: browser/extensions/onboarding/content/onboarding.js:369
> (Diff revision 2)
> > +      nextTourIndex = 0;
> > +    }
> > +    let targetTour = toursToNotify[nextTourIndex];
> > +
> > +    // Show the target tour notification
> > +    if (!this._notificationBar) {
> 
> When is this not the case?
No at the moment, this shouldn't happen. Remove this redundant check here at this moment, thanks.

    
    

    
  
(In reply to Dave Townsend [:mossop] from comment #18)

Hi mossop,

Now the test is not using `SpecialPowers.pushPrefEnv` but directly setting the tours' completed prefs.
This is because after adding one more test_skip_notification_for_completed_tour test, after all the test cases are passed and closed, the test running would not terminate. 

It seems after doing multiple `SpecialPowers.pushPrefEnv` on the same prefs with different value, it would have issue in restoring prefs. I haven't digged into this test framework issue because of the schedule and since we are reseting the default state for every test, here just directly set prefs.

Thanks

> Comment on attachment 8878811 [details]
> Bug 1357641 - Part 2: Add browser_onboarding_notification.js test,
> 
> https://reviewboard.mozilla.org/r/150090/#review155366
> 
> :::
> browser/extensions/onboarding/test/browser/browser_onboarding_notification.
> js:56
> (Diff revision 2)
> > +  await promiseTourNotificationOpened(tab.linkedBrowser);
> > +  let targetTourId = await getCurrentNotificationTargetTourId(tab.linkedBrowser);
> > +
> > +  // There is 0.8s notiificaiton sliding-up tranisition so must wait a bit here
> > +  // or `synthesizeMouseAtCenter` wouldn't be able to click on the final correct position of the action button
> > +  await new Promise(resolve => setTimeout(resolve, 1000));
> 
> Use the transition events to get the end of the transition rather than a
> timeout please. I'd recommend rolling it into the promiseTourNotificationOpened function.
> 
Thanks updated. Because by the time we added the transitionend event onto the notification from the test, the event was finished already, I added `data-css-transition=end` in the script and utilize that attribute to track the open of notification.

> :::
> browser/extensions/onboarding/test/browser/browser_onboarding_notification.
> js:66
> (Diff revision 2)
> > +  is(targetTourId, activeNavItemId, "Should navigate to the target tour item.");
> > +  is(`${targetTourId}-page`, activePageId, "Should display the target tour page.");
> > +  await BrowserTestUtils.removeTab(tab);
> > +});
> > +
> > +add_task(async function test_not_show_notification_for_completed_tour() {
> 
> Also add a test that individual completed tours are skipped.
> 
Thanks added one test_skip_notification_for_completed_tour test to test skipping completed tour.

> ::: browser/extensions/onboarding/test/browser/head.js:4
> (Diff revision 2)
> > -Cu.import("resource://gre/modules/Preferences.jsm");
> > +let scope = {};
> > +Cu.import("resource://gre/modules/Preferences.jsm", scope);
> > +let { Preferences } = scope;
> 
> let { Preferences } = Cu.import("...", {});
> 
> ::: browser/extensions/onboarding/test/browser/head.js:118
> (Diff revision 2)
> > +}
> > +
> > +function getCurrentActiveTour(browser) {
> > +  return ContentTask.spawn(browser, {}, function() {
> > +    let list = content.document.querySelector("#onboarding-tour-list");
> > +    let items = list.querySelectorAll("li");
> 
> Use a more precise selector than this, .onboarding-tour-item or something
Thanks updated.

    
    

    
  
(In reply to Francesco Lodolo [:flod] from comment #22)
> Comment on attachment 8877430 [details]
> Bug 1357641 - Part 1: Add onboarding tour notification,
> 
> https://reviewboard.mozilla.org/r/148836/#review155546
> 
> Do you have specs for this? What's the difference between notifications and
> tour?
> 
Hi Flod,
The tour notification is a notification bar prompted in the page bottom to notify users there are tours to explore.
Please see [1][2] for the UI screenshot.
And see [3] for the strings from the UX

[1] attachment 8878804 [details]: tour_notification_on_aboutHome.png
[2] attachment 8878805 [details]: tour_notification_on_aboutNewTab.png
[3] https://bugzilla.mozilla.org/show_bug.cgi?id=1357668#c4

Thanks

    
    

    
      
  

    
    

    
      
  

    
    

    
  
Comment on attachment 8877430 [details]
Bug 1357641 - Part 1: Add onboarding tour notification,

https://reviewboard.mozilla.org/r/148836/#review155546

> Don't hardcode Firefox

Thanks updated

    
    

    
  
Comment on attachment 8877430 [details]
Bug 1357641 - Part 1: Add onboarding tour notification,

https://reviewboard.mozilla.org/r/148836/#review155576

One nit, but strings look good.

Looking at the UI screenshots, I can't help asking: How good is the "New to Firefox?" badge at coping with very long strings?

::: browser/extensions/onboarding/locales/en-US/onboarding.properties:47
(Diff revision 4)
>  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
>  onboarding.tour-default-browser.win7.button=Make %S Your Default Browser
> +
> +onboarding.learnMore=Learn More

Based on the UI screenshot, I would change this to make sure it's clear this is a button label, e.g. onboarding.button.learnMore or similar.

Maybe

# LOCALIZATION NOTE(onboarding.button.learnMore): this string is used as a
# button label, displayed near the message, and shared across all the
# onboarding notifications.
onboarding.button.learnMore=Learn More

        Attachment #8877430 -
        Flags: review?(francesco.lodolo) → review+

    
    

    
  
(In reply to Francesco Lodolo [:flod] from comment #29)
> Comment on attachment 8877430 [details]
> Bug 1357641 - Part 1: Add onboarding tour notification,
> 
> https://reviewboard.mozilla.org/r/148836/#review155576
> 
> One nit, but strings look good.
> 
> Looking at the UI screenshots, I can't help asking: How good is the "New to Firefox?" badge at coping with very long strings?
> 
Thanks, it would wrap into the next line or it would overlay with the right-side descriptions which is not what we want.

> ::: browser/extensions/onboarding/locales/en-US/onboarding.properties:47
> > +onboarding.learnMore=Learn More
> 
> Based on the UI screenshot, I would change this to make sure it's clear this is a button label, e.g. onboarding.button.learnMore or similar.
> 
> Maybe
> 
> # LOCALIZATION NOTE(onboarding.button.learnMore): this string is used as a
> # button label, displayed near the message, and shared across all the
> # onboarding notifications.
> onboarding.button.learnMore=Learn More
Thanks updated to `onboarding.button.learnMore` to make sure clear

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
  
Comment on attachment 8877430 [details]
Bug 1357641 - Part 1: Add onboarding tour notification,

https://reviewboard.mozilla.org/r/148836/#review155940

::: browser/extensions/onboarding/content/onboarding.js:361
(Diff revision 6)
> +      // or the tour version has been updated so have an new tours set.
> +      // Take the last tour as the last prompted so would start from the 1st one below.
> +      lastTourIndex = onboardingTours.length - 1;
> +    }
> +
> +    // Form tours to notifify into the order we want.

*notify

::: browser/extensions/onboarding/content/onboarding.js:367
(Diff revision 6)
> +    for (let tour of toursToNotify) {
> +      if (!this.isTourCompleted(tour.id)) {
> +        targetTour = tour;
> +        break;
> +      }
> +    }

You can use toursToNotify.find to simplify this a little.

        Attachment #8877430 -
        Flags: review?(dtownsend) → review+

    
    

    
  
Comment on attachment 8878811 [details]
Bug 1357641 - Part 2: Add the browser_onboarding_notification.js test,

https://reviewboard.mozilla.org/r/150090/#review155946

        Attachment #8878811 -
        Flags: review?(dtownsend) → review+

    
    

    
  
Comment on attachment 8877430 [details]
Bug 1357641 - Part 1: Add onboarding tour notification,

https://reviewboard.mozilla.org/r/148836/#review155940

> *notify

Thanks updated

> You can use toursToNotify.find to simplify this a little.

Thanks updated

    
    

    
      
  

    
    

    
      
  

    
    

    
  
(In reply to Fischer [:Fischer] from comment #38)
> Comment on attachment 8877430 [details]
> Bug 1357641 - Part 1: Add onboarding tour notification,
> 
> Review request updated; see interdiff:
> https://reviewboard.mozilla.org/r/148836/diff/6-7/
TRY: https://treeherder.mozilla.org/#/jobs?repo=try&revision=ba2085ae5de282c3b9ddee9e2759d1463ed3fb4a
Keywords: checkin-needed

    
    

    
  
Comment on attachment 8877430 [details]
Bug 1357641 - Part 1: Add onboarding tour notification,

https://reviewboard.mozilla.org/r/148836/#review154852

> we can put related notification strings below each tour strings

Thanks updated

    
    

    
  
Comment on attachment 8878811 [details]
Bug 1357641 - Part 2: Add the browser_onboarding_notification.js test,

https://reviewboard.mozilla.org/r/150090/#review156096

::: browser/extensions/onboarding/test/browser/browser_onboarding_hide_tours.js
(Diff revision 7)
>   * http://creativecommons.org/publicdomain/zero/1.0/ */
>  
>  "use strict";
>  
> -const ABOUT_HOME_URL = "about:home";
> -const ABOUT_NEWTAB_URL = "about:newtab";

need add /* globals ABOUT_HOME_URL, ABOUT_NEWTAB_URL */ for lint

::: browser/extensions/onboarding/test/browser/browser_onboarding_notification.js:4
(Diff revision 7)
> +/* Any copyright is dedicated to the Public Domain.
> + * http://creativecommons.org/publicdomain/zero/1.0/ */
> +
> +"use strict";

need add /* globals ABOUT_NEWTAB_URL */ for lint

    
    

    
  
(In reply to Fred Lin [:gasolin] from comment #42)
> Comment on attachment 8878811 [details]
> Bug 1357641 - Part 2: Add the browser_onboarding_notification.js test,
> 
> https://reviewboard.mozilla.org/r/150090/#review156096
> 
> :::
> browser/extensions/onboarding/test/browser/browser_onboarding_hide_tours.js
> (Diff revision 7)
> >   * http://creativecommons.org/publicdomain/zero/1.0/ */
> >  
> >  "use strict";
> >  
> > -const ABOUT_HOME_URL = "about:home";
> > -const ABOUT_NEWTAB_URL = "about:newtab";
> 
> need add /* globals ABOUT_HOME_URL, ABOUT_NEWTAB_URL */ for lint
> 
> :::
> browser/extensions/onboarding/test/browser/browser_onboarding_notification.
> js:4
> (Diff revision 7)
> > +/* Any copyright is dedicated to the Public Domain.
> > + * http://creativecommons.org/publicdomain/zero/1.0/ */
> > +
> > +"use strict";
> 
> need add /* globals ABOUT_NEWTAB_URL */ for lint
No this is unnecessary because our eslint rule is "plugin:mozilla/browser-test" and you can find the lint test on TRY is green too in comment 40.

    
    

    
  
Comment on attachment 8878811 [details]
Bug 1357641 - Part 2: Add the browser_onboarding_notification.js test,

https://reviewboard.mozilla.org/r/150090/#review156132

::: browser/extensions/onboarding/test/browser/browser_onboarding_notification.js:23
(Diff revision 7)
> +      reloadPromise = BrowserTestUtils.browserLoaded(tab.linkedBrowser);
> +      tab.linkedBrowser.reload();
> +      await reloadPromise;
> +    } else {
> +      tab = await BrowserTestUtils.openNewForegroundTab(gBrowser);
> +      await BrowserTestUtils.loadURI(tab.linkedBrowser, ABOUT_NEWTAB_URL);

We can test both newtab and home url with the small change

```
let urls = [ABOUT_NEWTAB_URL, ABOUT_HOME_URL];
for (let url of urls) {
  let tab = await BrowserTestUtils.openNewForegroundTab(gBrowser);
  await BrowserTestUtils.loadURI(tab.linkedBrowser, url);
}
```

    
    

    
  
Pushed by rchien@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/9c86446d0a75
Part 1: Add onboarding tour notification, r=flod,mossop
https://hg.mozilla.org/integration/autoland/rev/eb0d8a0d77e7
Part 2: Add the browser_onboarding_notification.js test, r=mossop
Keywords: checkin-needed

    
    

    
      
  

    
    

    
  
(In reply to Phil Ringnalda (:philor) from comment #46)
> Backed out in https://hg.mozilla.org/integration/autoland/rev/6d631ae7ee67
> for Windows opt failures in browser_newtab_focus.js,
> https://treeherder.mozilla.org/logviewer.
> html#?job_id=109088479&repo=autoland and
> https://treeherder.mozilla.org/logviewer.html#?job_id=109082265&repo=autoland
Found the failure reason. It is simply that the focus count on about:newtab would increase with the onboarding tour notification.
Running TRY: https://treeherder.mozilla.org/#/jobs?repo=try&revision=3b992742564a6257a915964b92cd51669dea3210
Keywords: checkin-needed

    
    

    
      
  

    
    

    
  
(In reply to Fischer [:Fischer] from comment #49)
> Comment on attachment 8878811 [details]
> Bug 1357641 - Part 2: Add the browser_onboarding_notification.js test,
> 
> Review request updated; see interdiff:
> https://reviewboard.mozilla.org/r/150090/diff/8-9/
The lint issue in comment 48 is fixed

    
    

    
  
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/3eb9c03bd0cd
Part 1: Add onboarding tour notification, r=flod,mossop
https://hg.mozilla.org/integration/autoland/rev/ad8486907154
Part 2: Add the browser_onboarding_notification.js test, r=mossop
Keywords: checkin-needed

    
    

    
  
Backout by philringnalda@gmail.com:
https://hg.mozilla.org/mozilla-central/rev/7455c74d833a
Backed out 2 changesets for OS X near-permaorange in browser_permissionsPromptDeny.js

    
    

    
  
Which is odd, but also true.

https://treeherder.mozilla.org/#/jobs?repo=autoland&fromchange=1c8abc6019d9faffe2e533869a311062e123e7f6&group_state=expanded&filter-searchStr=6e9887a16f819bdbab200d4574932cbabac5ec58&tochange=9192d865160dd91999fa7a45a0978a31557fe2f5&selectedJob=109438753 from the second landing, https://treeherder.mozilla.org/#/jobs?repo=autoland&noautoclassify&fromchange=dc422ed66f91e7ee67b3f36ca59cbf3e0c38cfd9&tochange=6d631ae7ee67&filter-searchStr=6e9887a16f819bdbab200d4574932cbabac5ec58&selectedJob=109073086 from the first landing.

If nothing else works out, bug 1324163 does have a patch, you might just have to wait for that to land (after trying to figure out why this would break that, and if it doesn't make sense that it would, pushing this and that patch to try to see if that fixes it for you).
Status: RESOLVED → REOPENED

          status-firefox56:
          fixed → ---
Resolution: FIXED → ---
Target Milestone: Firefox 56 → ---

    
    

    
  
(In reply to Phil Ringnalda (:philor) from comment #54)
> Which is odd, but also true.
> 
> https://treeherder.mozilla.org/#/
> jobs?repo=autoland&fromchange=1c8abc6019d9faffe2e533869a311062e123e7f6&group_
> state=expanded&filter-
> searchStr=6e9887a16f819bdbab200d4574932cbabac5ec58&tochange=9192d865160dd9199
> 9fa7a45a0978a31557fe2f5&selectedJob=109438753 from the second landing,
> https://treeherder.mozilla.org/#/
> jobs?repo=autoland&noautoclassify&fromchange=dc422ed66f91e7ee67b3f36ca59cbf3e
> 0c38cfd9&tochange=6d631ae7ee67&filter-
> searchStr=6e9887a16f819bdbab200d4574932cbabac5ec58&selectedJob=109073086
> from the first landing.
> 
> If nothing else works out, bug 1324163 does have a patch, you might just
> have to wait for that to land (after trying to figure out why this would
> break that, and if it doesn't make sense that it would, pushing this and
> that patch to try to see if that fixes it for you).
OK, this is really odd.
Just run TRY: https://treeherder.mozilla.org/#/jobs?repo=try&revision=4222dbdb874351082b6f72184dbbcdc379ed54bd against browser_permissionsPromptDeny.js.
The reasult seems good.
I will reach Shwan who is handling bug 1324163 to see if any could be found.

    
    

    
  
good news, this patch has a talos improvement:
== Change summary for alert #7488 (as of June 22 2017 13:35 UTC) ==

Improvements:

  7%  tart summary linux64 pgo e10s     4.67 -> 4.33
  7%  tart summary linux64 opt e10s     5.51 -> 5.13

For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=7488

and the backout has a regression:
== Change summary for alert #7499 (as of June 23 2017 04:07 UTC) ==

Regressions:

  9%  tart summary linux64 pgo e10s     4.33 -> 4.73
  9%  tart summary linux64 opt e10s     5.13 -> 5.59

For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=7499

looking forward to this landing again.

    
    

    
  
(In reply to Joel Maher ( :jmaher) from comment #56)
> good news, this patch has a talos improvement:

That's quite surprising since this patch does extra work for new tabs.

    
    

    
  
Comment on attachment 8877430 [details]
Bug 1357641 - Part 1: Add onboarding tour notification,

https://reviewboard.mozilla.org/r/148836/#review157332

::: browser/extensions/onboarding/content/onboarding.js:224
(Diff revision 7)
> +  _initNotification() {
> +    let doc = this._window.document;
> +    if (doc.hidden) {
> +      // When the preloaded-browser feature is on,
> +      // it would preload an hidden about:newtab in the background.
> +      // We don't wnat to show notification in that hidden state.

nit: wnat -> want

::: browser/extensions/onboarding/content/onboarding.js:282
(Diff revision 7)
>        // that means clicking outside the tour content area.
>        // Let's toggle the overlay.
>        case "onboarding-overlay":
>          this.toggleOverlay();
>          break;
> +

nit: don't need the blank lines between switch cases

::: browser/extensions/onboarding/content/onboarding.js:313
(Diff revision 7)
>      if (this._tourItems.length == 0) {
>        // Lazy loading until first toggle.
>        this._loadTours(onboardingTours);
>      }
>  
> -    this._overlay.classList.toggle("opened");
> +    this.hideNotification();

doh ><

::: browser/extensions/onboarding/content/onboarding.js:419
(Diff revision 7)
> +  }
> +
> +  _renderNotificationBar() {
> +    let div = this._window.document.createElement("div");
> +    div.id = "onboarding-notification-bar";
> +    // Here we use `innerHTML` is for more friendly reading.

// We use `innerHTML` for more friendly reading.

::: browser/extensions/onboarding/content/onboarding.js:454
(Diff revision 7)
>    }
>  
>    _renderOverlay() {
>      let div = this._window.document.createElement("div");
>      div.id = "onboarding-overlay";
>      // Here we use `innerHTML` is for more friendly reading.

// We use `innerHTML` for more friendly reading.

::: browser/extensions/onboarding/locales/en-US/onboarding.properties:30
(Diff revision 7)
> +onboarding.notification.onboarding-tour-private-browsing.message=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-addons=Add-ons
>  onboarding.tour-addons.title=Add more functionality.
> +onboarding.notification.onboarding-tour-addons.title=Get more done.
> +# LOCALIZATION NOTE(onboarding.notification.onboarding-tour-addons.message): %S is brandShortName.

let's add some explaination when there's a comment: This string will be used in the notification message for the add-ons tour. %S is brandShortName.

::: browser/extensions/onboarding/locales/en-US/onboarding.properties:42
(Diff revision 7)
> -
>  # LOCALIZATION NOTE(onboarding.tour-customize.description): This string will be used in the customize tour description. %S is brandShortName
>  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.notification.onboarding-tour-customize.title=Rearrange your toolbar.
> +# LOCALIZATION NOTE(onboarding.notification.onboarding-tour-customize.message): %S is brandShortName.

This string will be used in the notification message for Customize tour. %S is brandShortName.

::: browser/extensions/onboarding/locales/en-US/onboarding.properties:42
(Diff revision 7)
> -
>  # LOCALIZATION NOTE(onboarding.tour-customize.description): This string will be used in the customize tour description. %S is brandShortName
>  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.notification.onboarding-tour-customize.title=Rearrange your toolbar.
> +# LOCALIZATION NOTE(onboarding.notification.onboarding-tour-customize.message): %S is brandShortName.

This string will be used in the notification message for the customize tour. %S is brandShortName.

::: browser/extensions/onboarding/locales/en-US/onboarding.properties:53
(Diff revision 7)
>  # 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
>  onboarding.tour-default-browser.win7.button=Make %S Your Default Browser
> +# LOCALIZATION NOTE(onboarding.notification.onboarding-tour-default-browser.title): %S is brandShortName.

This string will be used in the notification message for the default browser tour. %S is brandShortName.

::: browser/extensions/onboarding/locales/en-US/onboarding.properties:57
(Diff revision 7)
>  onboarding.tour-default-browser.win7.button=Make %S Your Default Browser
> +# LOCALIZATION NOTE(onboarding.notification.onboarding-tour-default-browser.title): %S is brandShortName.
> +onboarding.notification.onboarding-tour-default-browser.title=Make %S your go-to browser.
> +# LOCALIZATION NOTE(onboarding.notification.onboarding-tour-default-browser.message): %1$S is brandShortName
> +onboarding.notification.onboarding-tour-default-browser.message=It doesn’t take much to get the most from %1$S. Just set %1$S as your default browser and put control, customization, and protection on autopilot.
> +

we should use a consistent way to add blank lines. Previously in .properties, we put a blank line before each comment. Let's stick on that.

::: browser/extensions/onboarding/locales/en-US/onboarding.properties:63
(Diff revision 7)
> +onboarding.hidden-checkbox-label=Hide the tour
> +
> +#LOCALIZATION NOTE(onboarding.button.learnMore): this string is used as a button label, displayed near the message, and shared across all the onboarding notifications.
> +onboarding.button.learnMore=Learn More
> +
> +# LOCALIZATION NOTE(onboarding.notification-icon-tool-tip): %S is brandShortName.

This string will be used to show the tooltip alongside the notification icon. %S is brandShortName.

    
    

    
  
Comment on attachment 8877430 [details]
Bug 1357641 - Part 1: Add onboarding tour notification,

https://reviewboard.mozilla.org/r/148836/#review157332

> This string will be used to show the tooltip alongside the notification icon. %S is brandShortName.

For adding more coomments or arrange newlines, myabe it is not propery to do at this moment. And could discuss afterwards. To keep thing simple, right now should focus on having the patch landed or other bugs are being blocked.

    
    

    
  
Since bug 1324163 is skipped on OS X now, let's try to land this patch again.
[1] https://bugzilla.mozilla.org/show_bug.cgi?id=1324163#c45
Keywords: checkin-needed

    
  
Depends on: 1324163

    
    

    
  
Pushed by rchien@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/886e2e0b90a7
Part 1: Add onboarding tour notification, r=flod,mossop
https://hg.mozilla.org/integration/autoland/rev/36dfc734a106
Part 2: Add the browser_onboarding_notification.js test, r=mossop
Keywords: checkin-needed

    
    

    
  
https://hg.mozilla.org/mozilla-central/rev/886e2e0b90a7
https://hg.mozilla.org/mozilla-central/rev/36dfc734a106
Status: REOPENED → RESOLVED
Closed: 6 years ago → 6 years ago

          status-firefox56:
          --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 56

    
  
Blocks: 1375775

    
    

    
  
Hi Kate,
The Photon Onboarding tour notification has been landed.
Once no more notification to prompt, we would set "browser.onboarding.notification.finished"=true[1].
Please check out that pref to see if it is ok to prompt the snippet, thanks
[1] https://dxr.mozilla.org/mozilla-central/rev/53477d584130945864c4491632f88da437353356/browser/app/profile/firefox.js#1709
Flags: needinfo?(khudson)

    
  
Depends on: 1377129

    
    

    
  
I have reproduced this bug with Nightly 55.0a1 (2017-04-18) on Windows 10 , 64 Bit ! 

This bug's fix is Verified with latest Nightly 56.0a1 !

Build   ID    20170629030206
User Agent    Mozilla/5.0 (Windows NT 10.0; WOW64; rv:56.0) Gecko/20100101 Firefox/56.0
QA Whiteboard: [bugday-20170628]

    
    

    
  
will address comment 58 in bug 1376521

    
    

    
  
I have reproduced this bug with Nightly 55.0a1 (2017-04-18) (64-bit) on Ubuntu 16.04 LTS!

This bug's fix is verified with latest Nightly!

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

[bugday-20170628]

    
    

    
  
when this landed talos found a performance improvement:
== Change summary for alert #7570 (as of June 27 2017 22:10 UTC) ==

Improvements:

  8%  tart summary linux64 opt e10s     5.50 -> 5.05
  8%  tart summary linux64 pgo e10s     4.66 -> 4.29

For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=7570

thanks!

    
  
Depends on: 1377347

    
    

    
  
The issue is no longer reproducible on Firefox 56.0a1. 
Build ID: 20170629030206
Tests were performed under Windows 10x64.
[bugday-20170628]

          status-firefox56:
          fixed → verified

    
    

    
  
This issue has been verified on Win 10 x64.
Status: RESOLVED → VERIFIED

    
  
Depends on: 1377496
Depends on: 1377323
Depends on: 1378168
Depends on: 1378685

    
  
Blocks: 1377239

    
  
Blocks: 1379042

    
  
No longer blocks: 1379042
Depends on: 1379042

    
  
Blocks: 1377433

    
    

    
  
(In reply to Joel Maher ( :jmaher) (UTC-8) from comment #67)
> when this landed talos found a performance improvement:
> == Change summary for alert #7570 (as of June 27 2017 22:10 UTC) ==
> 
> Improvements:
> 
>   8%  tart summary linux64 opt e10s     5.50 -> 5.05
>   8%  tart summary linux64 pgo e10s     4.66 -> 4.29
> 
> For up to date results, see:
> https://treeherder.mozilla.org/perf.html#/alerts?id=7570
> 
> thanks!

Note that the updated alert includes performance wins on Windows 7 and 10.
Flags: needinfo?(khudson)

          You need to log in
          before you can comment on or make changes to this bug.