1354046 - [meta] Implement the OnBoarding overlay

Status: RESOLVED FIXED

(Firefox :: New Tab Page, enhancement, P1)

Description

[Evan Tseng [:evanxd]]

Implement the OnBoarding overlay.

And we have a rough idea document here https://mozilla.invisionapp.com/share/PXAICZSHU#

Comment 1

[Evan Tseng [:evanxd]]

In the engineering plan, we have (at least) two questions need to figure out.
1. Do we build the OnBoarding overlay as an (system) Add-on or build-in feature in Firefox?
2. What do we need to do for the OnBoarding Overlay Funnelcake test in engineering work? For instance, just adding a config flag to help build the Funnelcake build or we need to do more?

Comment 2

[Fischer [:Fischer]]

Attachment: 0001-This-is-a-try-of-onBoarding.png

Comment 3

[Fischer [:Fischer]]

Attachment: 0001-This-is-a-try-of-onBoarding.patch

Comment 4

[Fischer [:Fischer]]

For the onBoarding overlay, we have to make it not dependent on about:newtab or activity-stream, however, also have to let about:newtab or activity-stream able to display the onBoarding overlay on page load.

After discussion with Tim, thanks Tim's input, here is the current plan to achieve the above goal:

1. Build the onBoarding as one components, locating under toolkit/components/onBoarding

2. In the about:newtab or Activity-stream, they include our onBoarding resources and call our onBoarding.init() at the proper moment

3. Inside the onBoarding, we check if
     - this is a new user (currently see no profile as new user)
     - not all the tours are completed
   then we really init the onBoarding codes to do things like appending the overlay into page, controlling the tour, or else just return

A test patch[1] has been done. The test patch will make
  - Have onBoarding.js included in the about:newtab
  - Once the about:newtab is loaded, call `onBoarding.init(window)`
  - onBoarding.js would create an overlay then append and display that overlay into page
  - Clicking outside the overlay would hide the overlay

See 0001-This-is-a-try-of-onBoarding.png[2] for the try screenshot.

[1] attachment 8858263 [details] [diff] [review]: 0001-This-is-a-try-of-onBoarding.patch
[2] attachment 8858262 [details]: 0001-This-is-a-try-of-onBoarding.png

Comment 5

[Fischer [:Fischer]]

Update the product prototype: https://mozilla.invisionapp.com/share/2HB9YE939#/screens/228699838

Comment 7

[[e-mail]]

I saw fisher has put the entrypoint patch here and I'd like to discuss further about how to organize those OnBoarding tasks.

I made a prototype about how to organize tasks, which separate the tasks definition from the basic onboarding flow.
https://jsfiddle.net/gasolin/mLpp1dL5/

The benefits are

* Separate concern of basic flow and the task specific scripts
  * developer do not have to deal with the basic flow if he just want to add a new task.
  * make it more testable
* The verify function is async, can support simple or multiple pref condition check.
* Auto detect if we need to show the UI
  * verify each task's condition instead of write a big check function
  * its flexible if a new task is introduced or an old task is removed
* Support `Set all tasks as complete` toggle by default
* hook UI with documentFragmenet for performance

The main API is

onboard.init(hook, rules, width, height);
- hook: pass the placeholder element for onboarding UI
- rules: define each task(achievement) content and verify conditions

Rules is an array of task definition. Here is how a item in Rules array looks like:

```
{
    id: "task1",
    // title, badge, ...
    task: { title: "task 1" },
    // content about this task,
    // return an element for append to the content
    page: () => {
      let page = document.createElement("div");
      let txt = document.createTextNode("task 1 content");
      page.appendChild(txt);
      return page;
    },
    // script to deal with this page
    pageActoin: function() {},
    // check if this task is done, return a promise
    verify: async function() {
      return true;
    },
    // set this task as done (called by setAllComplete)
    setComplete: async function() {
      alert("set task1 as done");
    },
},
```

Comment 8

[[e-mail]]

Structure mentioned in comment 7 is hosted at github (with some changes)

https://github.com/gasolin/onboarding.js

The demo page
http://gasolin.github.io/onboarding.js/

Comment 9

[Fischer [:Fischer]]

Attachment: Bug 1354046 - Implement the onBoarding overlay architecture

Review commit: https://reviewboard.mozilla.org/r/133776/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/133776/

Comment 10

[Fischer [:Fischer]]

Attachment: test_onBoarding_overlay.png

Comment 11

[[e-mail]]

Comment on attachment 8861778 [details]
Bug 1354046 - Implement the onBoarding overlay architecture

https://reviewboard.mozilla.org/r/133776/#review136722

::: toolkit/components/onBoarding/content/onBoarding.js:32
(Diff revision 1)
> +    id: "sync",
> +    title: "Firefox Sync",
> +    iconSrc: "chrome://global/content/onBoarding/img/dummy-nav-icon.png ",
> +    page() {
> +      let div = win.document.createElement("div");
> +      div.innerHTML = `

I prefer create and append page with `document.createElement` than innerHTML. Like 
https://github.com/gasolin/onboarding.js/blob/gh-pages/customize-firefox.js#L8

The content is relatively simple, we can add specific button listener with uitour there.

::: toolkit/components/onBoarding/content/onBoarding.js:241
(Diff revision 1)
> +    // Although we add the uitour for about:newtab in the permission, it requires to compile.
> +    // Quite inconvienet, for testing purpose, simply set a test origin. Will remove in the formal patch.
> +    Services.prefs.setCharPref("browser.uitour.testingOrigins", "about:newtab");
> +
> +    if (Services.prefs.getBoolPref("browser.onBoarding.disabled", false) ||
> +        Services.prefs.getBoolPref("browser.onBoarding.alwaysHidden", false)

The proper condition might be check these 2 conditions `plus` each tour's complete state. Since we should show the overlay when ther is a new tour added.

::: toolkit/components/onBoarding/content/onBoarding.js:245
(Diff revision 1)
> +    if (Services.prefs.getBoolPref("browser.onBoarding.disabled", false) ||
> +        Services.prefs.getBoolPref("browser.onBoarding.alwaysHidden", false)
> +    ) {
> +      return;
> +    }
> +    if (!this._isNewOrReturningUser()) {

we may not need to implement this for the first run testing. And we should import this function since the same check could be reused for auto migration

::: toolkit/components/onBoarding/content/onBoarding.js:255
(Diff revision 1)
> +    // no flash of style changes and no additional reflow.
> +    await this._loadCSS();
> +    this._foxIcon = this._renderFoxIcon();
> +    this._overlay = this._renderOverlay();
> +    this._loadTours(this._overlay, tours);
> +    this.gotoTour(tours[0].id);

can moved into _loadTours

::: toolkit/components/onBoarding/content/onBoarding.js:256
(Diff revision 1)
> +    await this._loadCSS();
> +    this._foxIcon = this._renderFoxIcon();
> +    this._overlay = this._renderOverlay();
> +    this._loadTours(this._overlay, tours);
> +    this.gotoTour(tours[0].id);
> +    let docFrag = win.document.createDocumentFragment();

how about name `docFrag` as `fragment`?

::: toolkit/components/onBoarding/content/onBoarding.js:259
(Diff revision 1)
> +    this._loadTours(this._overlay, tours);
> +    this.gotoTour(tours[0].id);
> +    let docFrag = win.document.createDocumentFragment();
> +    docFrag.appendChild(this._foxIcon);
> +    docFrag.appendChild(this._overlay);
> +    win.document.body.appendChild(docFrag);

we can put all render related process into renderUI, therefore we can have shorten init()

::: toolkit/components/onBoarding/content/onBoarding.js:287
(Diff revision 1)
> +    // Detroy on unload. This is to ensure we remove all the stuff we left.
> +    // No any leak out there.
> +    win.document.addEventListener("unload", () => this.destroy());
> +
> +    this._initPrefsObserver();
> +    this._loadJSLib();

We have to make sure the lib is ready before doing click event

Comment 12

[Fischer [:Fischer]]

(In reply to Fred Lin [:gasolin] from comment #11)
> Comment on attachment 8861778 [details]
> Bug 1354046 - Implement the onBoarding overlay architecture
> 
> https://reviewboard.mozilla.org/r/133776/#review136722
> 
> ::: toolkit/components/onBoarding/content/onBoarding.js:32
> (Diff revision 1)
> > +    id: "sync",
> > +    title: "Firefox Sync",
> > +    iconSrc: "chrome://global/content/onBoarding/img/dummy-nav-icon.png ",
> > +    page() {
> > +      let div = win.document.createElement("div");
> > +      div.innerHTML = `
> 
> I prefer create and append page with `document.createElement` than
> innerHTML. Like 
> https://github.com/gasolin/onboarding.js/blob/gh-pages/customize-firefox.
> js#L8
> 
Maybe use the innerHTML would be more reading friendly.

> The content is relatively simple, we can add specific button listener with
> uitour there.
> 
> ::: toolkit/components/onBoarding/content/onBoarding.js:241
> (Diff revision 1)
> > +    // Although we add the uitour for about:newtab in the permission, it requires to compile.
> > +    // Quite inconvienet, for testing purpose, simply set a test origin. Will remove in the formal patch.
> > +    Services.prefs.setCharPref("browser.uitour.testingOrigins", "about:newtab");
> > +
> > +    if (Services.prefs.getBoolPref("browser.onBoarding.disabled", false) ||
> > +        Services.prefs.getBoolPref("browser.onBoarding.alwaysHidden", false)
> 
> The proper condition might be check these 2 conditions `plus` each tour's
> complete state. Since we should show the overlay when ther is a new tour
> added.
> 
Each tour's completion state doesn't matter. Even all tours completed, the overlay would still be there until user explicitly check the hiding-tips-menu checkobox.  

> ::: toolkit/components/onBoarding/content/onBoarding.js:245
> (Diff revision 1)
> > +    if (Services.prefs.getBoolPref("browser.onBoarding.disabled", false) ||
> > +        Services.prefs.getBoolPref("browser.onBoarding.alwaysHidden", false)
> > +    ) {
> > +      return;
> > +    }
> > +    if (!this._isNewOrReturningUser()) {
> 
> we may not need to implement this for the first run testing. And we should
> import this function since the same check could be reused for auto migration
> 
Yes, that is a future work.

> ::: toolkit/components/onBoarding/content/onBoarding.js:255
> (Diff revision 1)
> > +    // no flash of style changes and no additional reflow.
> > +    await this._loadCSS();
> > +    this._foxIcon = this._renderFoxIcon();
> > +    this._overlay = this._renderOverlay();
> > +    this._loadTours(this._overlay, tours);
> > +    this.gotoTour(tours[0].id);
> 
> can moved into _loadTours
> 
OK, thanks
> ::: toolkit/components/onBoarding/content/onBoarding.js:256
> (Diff revision 1)
> > +    await this._loadCSS();
> > +    this._foxIcon = this._renderFoxIcon();
> > +    this._overlay = this._renderOverlay();
> > +    this._loadTours(this._overlay, tours);
> > +    this.gotoTour(tours[0].id);
> > +    let docFrag = win.document.createDocumentFragment();
> 
> how about name `docFrag` as `fragment`?
> 
OK, thanks
> ::: toolkit/components/onBoarding/content/onBoarding.js:259
> (Diff revision 1)
> > +    this._loadTours(this._overlay, tours);
> > +    this.gotoTour(tours[0].id);
> > +    let docFrag = win.document.createDocumentFragment();
> > +    docFrag.appendChild(this._foxIcon);
> > +    docFrag.appendChild(this._overlay);
> > +    win.document.body.appendChild(docFrag);
> 
> we can put all render related process into renderUI, therefore we can have
> shorten init()
> 
Maybe we could do this in the future once the codes get bigger. Right now, not much UI to handle so one less `renderUI` probably be ok.

> ::: toolkit/components/onBoarding/content/onBoarding.js:287
> (Diff revision 1)
> > +    // Detroy on unload. This is to ensure we remove all the stuff we left.
> > +    // No any leak out there.
> > +    win.document.addEventListener("unload", () => this.destroy());
> > +
> > +    this._initPrefsObserver();
> > +    this._loadJSLib();
> 
> We have to make sure the lib is ready before doing click event
OK, thanks

Comment 13

[Fischer [:Fischer]]

Comment on attachment 8861778 [details]
Bug 1354046 - Implement the onBoarding overlay architecture

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/133776/diff/1-2/

Comment 14

[Fischer [:Fischer]]

Comment on attachment 8861778 [details]
Bug 1354046 - Implement the onBoarding overlay architecture

Hi Matt,

We are working on the Photon onBoarding tour feature. This tour would present a tour overlay to guide user explore Firefox features. Please see the specs[1].
We are targeting the Jun 1 to uplift this pref-controled feature to Beta for the funnelcake testing and will ship for Firefox 57.
Before submitting the review patches, we would like to discus with you about our approach so as to ensure the current direction is right and a smoother reviews later.

# The requirement for this onBoarding feature:
Because uncertain of the landing page, we have to make it not dependent on about:newtab or activity-stream,
however, also have to let about:newtab or activity-stream able to display the onBoarding overlay on page load.

# Our approach:
1. Create the onBoarding component under toolkit/components/onBoarding

2. In the about:newtab, include onBoarding.js and call the `onBoarding.init()` at the proper moment.

3. During init, onBoarding.js would check if should be hidden or not.

4. onBoarding.js would dynamically include the css and UITour-lib.js

5. onBoarding.js would dynamically create the related elements and append into the DOM.

6. The architecture overview:
   - one onBoarding.js in charge of managing tours
   - one onBoarding.css in charge of styles
   - one `onBoarding` object would be exposed to `window` for outside's use
   - for some tour completion condiftion. It needs to interact with other page.
     For example, the addons tour would be marked as completed after user visited the addons page.

This patch implemented the current approach and the key functionalities for your feedbacks.
That would be greate you could feedback us on
- how is the architecture?
- is the file location proper?
- if some security or performance concern there like about our injection, including external resources etc?
- or you see some issue?

p.s You will see some dummy UI images, this is because we haven't received the visual assets.
We definitely will polish the UI when sending review requests.

[1] Spec: https://mozilla.invisionapp.com/share/4MBDUMS5W#/screens/228511972_Overview
[2] UX demo page: https://dl.dropboxusercontent.com/u/105549/tour1.framer/index.html

Thank you

Comment 15

[Fischer [:Fischer]]

Comment on attachment 8861778 [details]
Bug 1354046 - Implement the onBoarding overlay architecture

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/133776/diff/2-3/

Comment 16

[[e-mail]]

Comment on attachment 8861778 [details]
Bug 1354046 - Implement the onBoarding overlay architecture

https://reviewboard.mozilla.org/r/133776/#review137226

::: toolkit/components/onBoarding/content/onBoarding.js:55
(Diff revision 3)
> +    isCompleted() {
> +      return Services.prefs.getBoolPref("browser.onBoarding.tour.sync.completed");
> +    },
> +    setCompleted() {
> +      Services.prefs.setBoolPref("browser.onBoarding.tour.sync.completed", true);
> +    },

we can define related `prefs` in each tour as well, so we can do prefs.addObserver at once and handle register/unregister handlers(isComplete) automatically with onboarding.js

Comment 17

[KM Lee [:rexboy] (inactivate after Sep. 22)]

After clicking on the highlighted items it opens the page on the current tab, but on the specification seems it is opened on another tab.

Comment 18

[KM Lee [:rexboy] (inactivate after Sep. 22)]

And I guess the situation above is the same to all buttons that use UITour. (e.g. searching on the searching bar will redirect the current tab to search engine) we may need to check if it's acceptable to our scenario.

Comment 19

[KM Lee [:rexboy] (inactivate after Sep. 22)]

Hmmm, after some trying it sometimes does open on a new page but sometimes it doesn't. Not very sure about the reason yet.

Comment 20

[Ekanan Ketunuti]

Target Milestone tracks when the patch landed on m-c and is set automatically by bugherder.

Comment 21

[Matthew N. [:MattN]]

So after our discussions about the implementation options below, which ones are preferred (and why) by the development team?

a) System Add-on
** Local frame or remote frame?
** Probably not a webextension since we need to inject in about:newtab
b) Remote/Web hosted frame(s)
** Consider the overlay and notification bar
** Deal with interaction with snippets for the notification bar
c) Fully built-in
d) Use snippets for the notification bar content with UITour APIs to open the overlay and add the fox icon to the page?
e) Something else?

Comment 22

[Matthew N. [:MattN]]

Comment on attachment 8861778 [details]
Bug 1354046 - Implement the onBoarding overlay architecture

Clearing review for now until we get an answer on comment 21.

Comment 23

[KM Lee [:rexboy] (inactivate after Sep. 22)]

Hello Matt:

We think using system add-on may be a better plan. Something like changing default browser, and listening to push event of firefox sync login, may need a more powerful permission to do it.

There was a discussion about how to put on the frame. If we inject it by listen to onload event of gBrowser and inject it by contentDocument.body.appendChild() to about:newtab, we wonder using a global listener creates global pollution to gBrowser. And the similar case if we inject it into tabpanel (maybe need to listen on visibilitychange? and show/hide the overlay correspondingly?).

So how about just put necessary files in add-on directory, and still import them from newTab.xhtml to show the overlay? This way we can update necessary things flexibly while avoiding global hooking.

Comment 24

[KM Lee [:rexboy] (inactivate after Sep. 22)]

Address some possible consideration:

- Our possible way for preference-off may be letting Newtab to load the script, and determine the preference value inside the script to disable the overlay icon. The point is not to create error upon loading newtab page. If loading time matters we may need to do perf-check inside newtab. The script is not going to be a big one so it may be okay.

- For css, we may make each rule under .overlay to prevent pollution outside overlay.

Comment 25

[KM Lee [:rexboy] (inactivate after Sep. 22)]

Attachment: Patch for making it a system add-on

So here's the patch for making it a system add-on based on the patch attachment 8861778 [details]; posted here for reference and further discussion. (With a few testing code for fxaccount login things that can be ignored for now)

Comment 26

[KM Lee [:rexboy] (inactivate after Sep. 22)]

We're just going to discuss it before having a specific assignee.

Comment 27

[Matthew N. [:MattN]]

Clearing needinfo since the questions were discussed in a meeting

Comment 28

[Fischer [:Fischer]]

Hi Verdi,
Do we only show New and Returning users the onboarding overlay and the notification bar? Thanks.

Comment 29

[Verdi [:verdi] Best to slack me]

(In reply to Fischer [:Fischer] from comment #28)
> Hi Verdi,
> Do we only show New and Returning users the onboarding overlay and the
> notification bar? Thanks.

Normally, new users and existing users on a new device will see the overlay and the notifications. When we fix Bug 1357666, we'll turn some returning users into new users and they will also see the overlay and notifications.

For special updates like Firefox 57, we'll show the overlay and notifications to updating users but it will have different content - an update tour about what's new or different in Firefox. That is Bug 1360378.

Comment 30

[[e-mail]]

Michael, I'm a bit confused between these UX bugs,

Based on UX bug 1360474, I guess its option 1:

1. Should display overlay when there's any uncomplete tours after tours replacement 
(v56: 6 tours in current spec
 v57: ? tours in current spec + some new tours for 57)



Or do you mean the option 2:

2. Should display different tour sets overlay in different condition
(v56: 6 tours in current spec
 v57: 6 tours in current spec  + extra tours for 57)

Comment 31

[Verdi [:verdi] Best to slack me]

Attachment: which-tour-do-we-show.jpg

(In reply to Fred Lin [:gasolin] from comment #30)
> Michael, I'm a bit confused between these UX bugs,

Hi Fred, I made this flowchart to help explain. Let me know if this makes sense. We can talk about it over video too if you'd like.

Comment 32

[[e-mail]]

Thanks for the flowchart diagram, based on our discussion, the overlay should support 2 sets of tours and showing them by condition:
* the new user tour-set
* the updating user tour-set

Some tour in those tour-set maybe overlapped.
And we should carry the complete state if user already completed that tour.


ex:

new user tour-set
{
 tour1: undone,
 tour2: undone,
 tour3: done,
}

updating user tour-set
{
 tour3: done,
 tour4: undone,
 tour5: undone,
}

Comment 34

[Emma Humphries ☕️🎸🧞‍♀️✨ (she/they) [:emceeaich] (Pacific Time) use needinfo]

This is a P1 bug without an assignee. 

P1 are bugs which are being worked on for the current release cycle/iteration/sprint. 

If the bug is not assigned by Monday, 28 August, the bug's priority will be reset to '--'.