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)
Firefox
General
Tracking
()
VERIFIED
FIXED
Firefox 56
Tracking | Status | |
---|---|---|
firefox56 | --- | verified |
People
(Reporter: Fischer, Assigned: Fischer)
References
Details
(Whiteboard: [photon-onboarding] )
Attachments
(4 files)
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
Assignee | ||
Updated•6 years ago
|
Whiteboard: [photon-onboarding]
Assignee | ||
Comment 1•6 years ago
|
||
(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.
Updated•6 years ago
|
Priority: -- → P2
Updated•6 years ago
|
Flags: qe-verify+
QA Contact: jwilliams
Updated•6 years ago
|
Assignee: nobody → fliu
Status: NEW → ASSIGNED
Updated•6 years ago
|
Priority: P2 → P1
Updated•6 years ago
|
Target Milestone: --- → Firefox 56
Comment hidden (mozreview-request) |
Assignee | ||
Comment 3•6 years ago
|
||
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)
Assignee | ||
Comment 4•6 years ago
|
||
(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 5•6 years ago
|
||
mozreview-review |
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
Updated•6 years ago
|
Attachment #8877430 -
Flags: feedback?(gasolin) → feedback+
Comment 6•6 years ago
|
||
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+
Assignee | ||
Comment 7•6 years ago
|
||
Assignee | ||
Comment 8•6 years ago
|
||
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 11•6 years ago
|
||
mozreview-review |
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.
Assignee | ||
Comment 12•6 years ago
|
||
(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 15•6 years ago
|
||
mozreview-review |
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 hidden (mozreview-request) |
Comment 17•6 years ago
|
||
mozreview-review |
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 18•6 years ago
|
||
mozreview-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-
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 21•6 years ago
|
||
(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 22•6 years ago
|
||
mozreview-review |
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
Assignee | ||
Comment 23•6 years ago
|
||
(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.
Assignee | ||
Comment 24•6 years ago
|
||
(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.
Assignee | ||
Comment 25•6 years ago
|
||
(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 hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 28•6 years ago
|
||
mozreview-review-reply |
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 29•6 years ago
|
||
mozreview-review |
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+
Assignee | ||
Comment 30•6 years ago
|
||
(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 hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 35•6 years ago
|
||
mozreview-review |
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 36•6 years ago
|
||
mozreview-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+
Assignee | ||
Comment 37•6 years ago
|
||
mozreview-review-reply |
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
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 40•6 years ago
|
||
(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
Assignee | ||
Comment 41•6 years ago
|
||
mozreview-review-reply |
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 42•6 years ago
|
||
mozreview-review |
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
Assignee | ||
Comment 43•6 years ago
|
||
(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 44•6 years ago
|
||
mozreview-review |
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); } ```
Comment 45•6 years ago
|
||
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
Comment 46•6 years ago
|
||
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
Comment hidden (mozreview-request) |
Assignee | ||
Comment 48•6 years ago
|
||
(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
Comment hidden (mozreview-request) |
Assignee | ||
Comment 50•6 years ago
|
||
(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
Comment 51•6 years ago
|
||
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
Comment 52•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/3eb9c03bd0cd https://hg.mozilla.org/mozilla-central/rev/ad8486907154
Comment 53•6 years ago
|
||
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
Comment 54•6 years ago
|
||
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 → ---
Assignee | ||
Comment 55•6 years ago
|
||
(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.
Comment 57•6 years ago
|
||
(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 58•6 years ago
|
||
mozreview-review |
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.
Assignee | ||
Comment 59•6 years ago
|
||
mozreview-review-reply |
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.
Assignee | ||
Comment 60•6 years ago
|
||
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
Comment 61•6 years ago
|
||
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
Comment 62•6 years ago
|
||
bugherder |
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
Assignee | ||
Comment 63•6 years ago
|
||
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)
Comment 64•6 years ago
|
||
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]
Comment 65•6 years ago
|
||
will address comment 58 in bug 1376521
Comment 66•6 years ago
|
||
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!
Comment 68•6 years ago
|
||
The issue is no longer reproducible on Firefox 56.0a1. Build ID: 20170629030206 Tests were performed under Windows 10x64. [bugday-20170628]
Comment 70•6 years ago
|
||
Screenshots! (with a bit of post All-Hands delay) https://screenshots.mattn.ca/compare/?oldProject=mozilla-central&oldRev=4c6668cbaccb1c207ced3bba18b81cf2be8ca495&newProject=mozilla-central&newRev=b1b9129838ade91684574f42219b2010928d7db4
Updated•6 years ago
|
Comment 71•6 years ago
|
||
(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.
Updated•6 years ago
|
Flags: needinfo?(khudson)
You need to log in
before you can comment on or make changes to this bug.
Description
•