Closed Bug 1404193 Opened 7 years ago Closed 7 years ago

make Onboarding more configurable

Categories

(Firefox :: New Tab Page, enhancement)

58 Branch
enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 58
Tracking Status
firefox57 --- wontfix
firefox58 --- fixed

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
Whiteboard: [photon-onboarding][triage][photon-onboarding-newui]
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.
Whiteboard: [photon-onboarding][triage][photon-onboarding-newui]
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 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: nobody → gasolin
Fred, does this need to be uplifted to 57?
Flags: needinfo?(gasolin)
Confirmed with Cindy that we don't need this in v57
Flags: needinfo?(gasolin)
Version: 57 Branch → 58 Branch
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.
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
Attachment #8914159 - Attachment is obsolete: true
Blocks: 1409977
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)
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 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 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 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+
Attachment #8921319 - Attachment is obsolete: true
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
test passed, thanks
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)
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)
try & local test fine, land it
Flags: needinfo?(fliu)
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
Status: NEW → ASSIGNED
https://hg.mozilla.org/mozilla-central/rev/7eafcf419a63
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 58
Blocks: 1416171
Whiteboard: [onboarding-modulize]
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: