Closed
Bug 1404193
Opened 7 years ago
Closed 7 years ago
make Onboarding more configurable
Categories
(Firefox :: New Tab Page, enhancement)
Tracking
()
RESOLVED
FIXED
Firefox 58
People
(Reporter: gasolin, Assigned: gasolin)
References
Details
(Whiteboard: [onboarding-modulize])
Attachments
(1 file, 2 obsolete files)
To enable A/B test more combination of Onboarding Tours, such as * Change fox Logo * Hide skip tour button * Change the string in the speech bubble we can define more pref and only set via shield study. * Save icon url as pref to support change icon via shield study. * Put show/hide skip tour button as pref to support hide the skip tour button * Save (new and update) speech bubble string id as pref to support showing different welcome strings
Assignee | ||
Updated•7 years ago
|
Whiteboard: [photon-onboarding][triage][photon-onboarding-newui]
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 3•7 years ago
|
||
The first patch document current support pref for customization. The 2nd PR do the actual change. Note those change does not affect current onboarding behavior and not set any extra pref(by load the default value when those pref does not exist) for now. We just define some pref as entrypoint to allow set those prefs via shield study.
Updated•7 years ago
|
Whiteboard: [photon-onboarding][triage][photon-onboarding-newui]
Comment 4•7 years ago
|
||
mozreview-review |
Comment on attachment 8914158 [details] Bug 1404193 - enable customizable logo, speech bubble string, and hide the skip tour button; https://reviewboard.mozilla.org/r/185484/#review190978 ::: browser/extensions/onboarding/README.md:77 (Diff revision 1) > +| `browser.onboarding.enabled` | disable onboarding experience entirely | true > +| `browser.onboarding.notification.finished` | Decide if we want to hide the notification permanently. | false > +| `browser.onboarding.notification.mute-duration-on-first-session-ms` |Notification mute duration. It also effect when the speech bubble is hidden and turned into the blue dot | 300000 (5 Min) > +| `browser.onboarding.notification.max-life-time-all-tours-ms` | Notification tours will all hide after this period | 1209600000 (10 Days) > +| `browser.onboarding.notification.max-life-time-per-tours-ms` | Per Notification tours will hide and show the next tour after this period | 432000000 (5 Days) > +| `browser.onboarding.notification.max-prompt-count-per-tour` | Each tour can only show the specific times in notification bar if the action button is not clicked. | 8 Should say: "Each tour can only show the specific times in notification bar if user didn't interact with the tour notification" because we also have the close button (beside the action button) there for users to interact with.
Attachment #8914158 -
Flags: review?(fliu)
Comment 5•7 years ago
|
||
mozreview-review |
Comment on attachment 8914159 [details] Bug 1404193 - enable customize logo, speech bubble string, and hide the skip button; https://reviewboard.mozilla.org/r/185486/#review190980 ::: browser/extensions/onboarding/README.md:79 (Diff revision 1) > | `browser.onboarding.notification.mute-duration-on-first-session-ms` |Notification mute duration. It also effect when the speech bubble is hidden and turned into the blue dot | 300000 (5 Min) > | `browser.onboarding.notification.max-life-time-all-tours-ms` | Notification tours will all hide after this period | 1209600000 (10 Days) > | `browser.onboarding.notification.max-life-time-per-tours-ms` | Per Notification tours will hide and show the next tour after this period | 432000000 (5 Days) > | `browser.onboarding.notification.max-prompt-count-per-tour` | Each tour can only show the specific times in notification bar if the action button is not clicked. | 8 > | `browser.onboarding.newtour` | The tourset of new user tour. | performance,private,screenshots,addons,customize,default > +| `browser.onboarding.newtour.tooltip` | The string id which is shown in the new user tour's speech bubble. The preffered length is 2 lines.| `onboarding.overlay-icon-tooltip2` The "document support prefs for customize" commit is short and sort of related to this commit. Please simply combine these 2 commits. ::: browser/extensions/onboarding/content/onboarding.js:1054 (Diff revision 1) > } > > _renderOverlay() { > let div = this._window.document.createElement("div"); > div.id = "onboarding-overlay"; > + let skipButtonStr = Services.prefs.getBoolPref("browser.onboarding.skip-tour-button.hide", false) ? `skipButtonStr` is not holding string but the html markup. The innerHTML markup is fine. Please make the enabling/disabling "skip-tour-button" clear, for exmaple ``` skipButtonMarkup = Services.prefs.getBoolPref("browser.onboarding.skip-tour-button.hide", false) ? "" : `<button id="onboarding-skip-tour-button" class="onboarding-action-button">SKIP BUTTON TITLE</button>` ``` ::: browser/extensions/onboarding/content/onboarding.js:1090 (Diff revision 1) > } > > _renderOverlayButton() { > let button = this._window.document.createElement("button"); > let tooltipStringId = this._tourType === "new" ? > - "onboarding.overlay-icon-tooltip2" : "onboarding.overlay-icon-tooltip-updated2"; > + Services.prefs.getStringPref("browser.onboarding.newtour.tooltip", What if the customized pref was set to some unknow id? Would it throw at `this._bundle.formatStringFromName(tooltipStringId, [BRAND_SHORT_NAME], 1);`? ::: browser/extensions/onboarding/content/onboarding.js:1094 (Diff revision 1) > let tooltipStringId = this._tourType === "new" ? > - "onboarding.overlay-icon-tooltip2" : "onboarding.overlay-icon-tooltip-updated2"; > + Services.prefs.getStringPref("browser.onboarding.newtour.tooltip", > + "onboarding.overlay-icon-tooltip2") : > + Services.prefs.getStringPref("browser.onboarding.updatetour.tooltip", > + "onboarding.overlay-icon-tooltip-updated2"); > let tooltip = this._bundle.formatStringFromName(tooltipStringId, [BRAND_SHORT_NAME], 1); What if the customzed pref'ed string didn't contain the brand short name?
Attachment #8914159 -
Flags: review?(fliu)
Assignee | ||
Updated•7 years ago
|
Assignee: nobody → gasolin
Assignee | ||
Comment 7•7 years ago
|
||
Confirmed with Cindy that we don't need this in v57
Flags: needinfo?(gasolin)
Assignee | ||
Updated•7 years ago
|
Version: 57 Branch → 58 Branch
Updated•7 years ago
|
status-firefox57:
--- → wontfix
status-firefox58:
--- → affected
Assignee | ||
Comment 8•7 years ago
|
||
Instead of hardcode in source, we can also define a property in tourSet to denote this tour would be marked as complete instantly after user switched to this tour.
Assignee | ||
Comment 9•7 years ago
|
||
mozreview-review |
Comment on attachment 8914159 [details] Bug 1404193 - enable customize logo, speech bubble string, and hide the skip button; https://reviewboard.mozilla.org/r/185486/#review196130 Thanks for review. The patch is keep in origin scope, will do comment 8 in separate bug ::: browser/extensions/onboarding/content/onboarding.js:1054 (Diff revision 1) > } > > _renderOverlay() { > let div = this._window.document.createElement("div"); > div.id = "onboarding-overlay"; > + let skipButtonStr = Services.prefs.getBoolPref("browser.onboarding.skip-tour-button.hide", false) ? updated ::: browser/extensions/onboarding/content/onboarding.js:1090 (Diff revision 1) > } > > _renderOverlayButton() { > let button = this._window.document.createElement("button"); > let tooltipStringId = this._tourType === "new" ? > - "onboarding.overlay-icon-tooltip2" : "onboarding.overlay-icon-tooltip-updated2"; > + Services.prefs.getStringPref("browser.onboarding.newtour.tooltip", add try catch around the `formatStringFromName`. If it doesn't work, print some log and fallback to current implementation. ::: browser/extensions/onboarding/content/onboarding.js:1094 (Diff revision 1) > let tooltipStringId = this._tourType === "new" ? > - "onboarding.overlay-icon-tooltip2" : "onboarding.overlay-icon-tooltip-updated2"; > + Services.prefs.getStringPref("browser.onboarding.newtour.tooltip", > + "onboarding.overlay-icon-tooltip2") : > + Services.prefs.getStringPref("browser.onboarding.updatetour.tooltip", > + "onboarding.overlay-icon-tooltip-updated2"); > let tooltip = this._bundle.formatStringFromName(tooltipStringId, [BRAND_SHORT_NAME], 1); It's fine if the string doesn't contain any variable. If the string contains more variables than we provide, the previous try/catch clause with catch that and fallback to current implementation
Comment hidden (mozreview-request) |
Assignee | ||
Updated•7 years ago
|
Attachment #8914159 -
Attachment is obsolete: true
Comment hidden (mozreview-request) |
Comment 12•7 years ago
|
||
mozreview-review |
Comment on attachment 8914158 [details] Bug 1404193 - enable customizable logo, speech bubble string, and hide the skip tour button; https://reviewboard.mozilla.org/r/185484/#review196626 ::: commit-message-e6c32:1 (Diff revision 3) > +Bug 1404193 - enable customize logo, speech bubble string, and hide the skip button;r=fischer nit: s/customize/customizable ::: browser/extensions/onboarding/README.md:83 (Diff revision 3) > +| `browser.onboarding.notification.mute-duration-on-first-session-ms` |Notification mute duration. It also effect when the speech bubble is hidden and turned into the blue dot | 300000 (5 Min) > +| `browser.onboarding.notification.max-life-time-all-tours-ms` | Notification tours will all hide after this period | 1209600000 (10 Days) > +| `browser.onboarding.notification.max-life-time-per-tours-ms` | Per Notification tours will hide and show the next tour after this period | 432000000 (5 Days) > +| `browser.onboarding.notification.max-prompt-count-per-tour` | Each tour can only show the specific times in notification bar if user didn't interact with the tour notification. | 8 > +| `browser.onboarding.newtour` | The tourset of new user tour. | performance,private,screenshots,addons,customize,default > +| `browser.onboarding.newtour.tooltip` | The string id which is shown in the new user tour's speech bubble. The preffered length is 2 lines.| `onboarding.overlay-icon-tooltip2` Since we are documenting this, also remark maximum one brand short name is expected ::: browser/extensions/onboarding/content/onboarding.js:1134 (Diff revision 3) > } > > _renderOverlay() { > let div = this._window.document.createElement("div"); > div.id = "onboarding-overlay"; > + let skipButtonMarkup = Services.prefs.getBoolPref("browser.onboarding.skip-tour-button.hide", false) ? I was thinking: Could we do ``` let skipButtonMarkup = ""; if (!Services.prefs.getBoolPref("browser.onboarding.skip-tour-button.hide", false)) { let label = this._bundle.GetStringFromName("onboarding.skip-tour-button-label"); skipButtonMarkup = `<button id="onboarding-skip-tour-button" class="onboarding-action-button">${label}</button>`; } ``` so no another `div.querySelector("#onboarding-skip-tour-button");` and `if (skipBtn) ...` ::: browser/extensions/onboarding/content/onboarding.js:1157 (Diff revision 3) > this._dialog.id = ONBOARDING_DIALOG_ID; > > - div.querySelector("#onboarding-skip-tour-button").textContent = > + let skipBtn = div.querySelector("#onboarding-skip-tour-button"); > + if (skipBtn) { > + skipBtn.textContent = > this._bundle.GetStringFromName("onboarding.skip-tour-button-label"); nit: indentation ::: browser/extensions/onboarding/content/onboarding.js:1177 (Diff revision 3) > - let tooltip = this._bundle.formatStringFromName(tooltipStringId, [BRAND_SHORT_NAME], 1); > + Services.prefs.getStringPref("browser.onboarding.updatetour.tooltip", SPEECH_BUBBLE_UPDATETOUR_STRING_ID); > + let tooltip = ""; > + try { > + tooltip = this._bundle.formatStringFromName(tooltipStringId, [BRAND_SHORT_NAME], 1); > + } catch (e) { > + Cu.reportError("the provided string is in wrong format ", e); Report what is the wrong string so a better debgging trace so: ``` let tooltipStringPrefId = ""; if (this._tourType === "new") { tooltipStringId = SPEECH_BUBBLE_NEWTOUR_STRING_ID; tooltipStringPrefId = "browser.onboarding.newtour.tooltip"; } else { tooltipStringId = SPEECH_BUBBLE_UPDATETOUR_STRING_ID; tooltipStringPrefId = "browser.onboarding.updatetour.tooltip"; } Cu.reportError(`the provided ${tooltipStringPrefId} string is in wrong format `, e); ```
Attachment #8914158 -
Flags: review?(fliu)
Assignee | ||
Comment 13•7 years ago
|
||
mozreview-review |
Comment on attachment 8914158 [details] Bug 1404193 - enable customizable logo, speech bubble string, and hide the skip tour button; https://reviewboard.mozilla.org/r/185484/#review197040 ::: browser/extensions/onboarding/README.md:83 (Diff revision 3) > +| `browser.onboarding.notification.mute-duration-on-first-session-ms` |Notification mute duration. It also effect when the speech bubble is hidden and turned into the blue dot | 300000 (5 Min) > +| `browser.onboarding.notification.max-life-time-all-tours-ms` | Notification tours will all hide after this period | 1209600000 (10 Days) > +| `browser.onboarding.notification.max-life-time-per-tours-ms` | Per Notification tours will hide and show the next tour after this period | 432000000 (5 Days) > +| `browser.onboarding.notification.max-prompt-count-per-tour` | Each tour can only show the specific times in notification bar if user didn't interact with the tour notification. | 8 > +| `browser.onboarding.newtour` | The tourset of new user tour. | performance,private,screenshots,addons,customize,default > +| `browser.onboarding.newtour.tooltip` | The string id which is shown in the new user tour's speech bubble. The preffered length is 2 lines.| `onboarding.overlay-icon-tooltip2` I'll add `Should use `%S` to denote Firefox (brand short name) in string, or use `%1$S` if the name shows more than 1 time.` ::: browser/extensions/onboarding/content/onboarding.js:1177 (Diff revision 3) > - let tooltip = this._bundle.formatStringFromName(tooltipStringId, [BRAND_SHORT_NAME], 1); > + Services.prefs.getStringPref("browser.onboarding.updatetour.tooltip", SPEECH_BUBBLE_UPDATETOUR_STRING_ID); > + let tooltip = ""; > + try { > + tooltip = this._bundle.formatStringFromName(tooltipStringId, [BRAND_SHORT_NAME], 1); > + } catch (e) { > + Cu.reportError("the provided string is in wrong format ", e); updated
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 16•7 years ago
|
||
mozreview-review |
Comment on attachment 8914158 [details] Bug 1404193 - enable customizable logo, speech bubble string, and hide the skip tour button; https://reviewboard.mozilla.org/r/185484/#review197142 ::: browser/extensions/onboarding/README.md:83 (Diff revision 5) > +| `browser.onboarding.notification.mute-duration-on-first-session-ms` |Notification mute duration. It also effect when the speech bubble is hidden and turned into the blue dot | 300000 (5 Min) > +| `browser.onboarding.notification.max-life-time-all-tours-ms` | Notification tours will all hide after this period | 1209600000 (10 Days) > +| `browser.onboarding.notification.max-life-time-per-tours-ms` | Per Notification tours will hide and show the next tour after this period | 432000000 (5 Days) > +| `browser.onboarding.notification.max-prompt-count-per-tour` | Each tour can only show the specific times in notification bar if user didn't interact with the tour notification. | 8 > +| `browser.onboarding.newtour` | The tourset of new user tour. | performance,private,screenshots,addons,customize,default > +| `browser.onboarding.newtour.tooltip` | The string id which is shown in the new user tour's speech bubble. The preffered length is 2 lines. Should use `%S` to denote Firefox (brand short name) in string, or use `%1$S` if the name shows more than 1 time. | `onboarding.overlay-icon-tooltip2` I tried New to %1S?\nLet’s get started %1S, %1S. but got "New to Nightly? Let’s get started (null), (null).". Did you see this too?
Attachment #8914158 -
Flags: review?(fliu)
Comment 17•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8914158 [details] Bug 1404193 - enable customizable logo, speech bubble string, and hide the skip tour button; https://reviewboard.mozilla.org/r/185484/#review197142 > I tried New to %1S?\nLet’s get started %1S, %1S. but got "New to Nightly? > Let’s get started (null), (null).". Did you see this too? Wait, looks some typo in my trials
Comment 18•7 years ago
|
||
mozreview-review |
Comment on attachment 8914158 [details] Bug 1404193 - enable customizable logo, speech bubble string, and hide the skip tour button; https://reviewboard.mozilla.org/r/185484/#review197146
Attachment #8914158 -
Flags: review+
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Updated•7 years ago
|
Attachment #8921319 -
Attachment is obsolete: true
Comment 21•7 years ago
|
||
Pushed by flin@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/0c69682ddb67 enable customizable logo, speech bubble string, and hide the skip button;r=Fischer
Assignee | ||
Comment 22•7 years ago
|
||
test passed, thanks
Comment 23•7 years ago
|
||
Backed out for eslint failure at browser/extensions/onboarding/content/onboarding.js:1141: Unsafe assignment to innerHTML: https://hg.mozilla.org/integration/autoland/rev/9c968cad8e6ae47aec9ec71d5ace795c2468a82b Push with failure: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=0c69682ddb674300d182e1a4c63c88c437c789fa&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=139094949&repo=autoland TEST-UNEXPECTED-ERROR | /builds/worker/checkouts/gecko/browser/extensions/onboarding/content/onboarding.js:1141:5 | Unsafe assignment to innerHTML (no-unsanitized/property)
Flags: needinfo?(gasolin)
Comment hidden (mozreview-request) |
Assignee | ||
Comment 25•7 years ago
|
||
Thanks for catching, I append dom element instead of embed variable with innerHTML. Fischer here's the diff if you like to double check https://reviewboard.mozilla.org/r/185482/diff/7-8/
Flags: needinfo?(gasolin) → needinfo?(fliu)
Comment 27•7 years ago
|
||
Pushed by flin@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/7eafcf419a63 enable customizable logo, speech bubble string, and hide the skip tour button;r=Fischer
Assignee | ||
Updated•7 years ago
|
Status: NEW → ASSIGNED
Comment 28•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/7eafcf419a63
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 58
Assignee | ||
Updated•7 years ago
|
Whiteboard: [onboarding-modulize]
You need to log in
before you can comment on or make changes to this bug.
Description
•