Should be able to toggle the onBoarding overlay by clicking the overlay fox icon

VERIFIED FIXED in Firefox 55

Status

()

Firefox
General
P1
normal
VERIFIED FIXED
4 months ago
2 months ago

People

(Reporter: Fischer, Assigned: rexboy)

Tracking

unspecified
Firefox 56
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox55 verified)

Details

(Whiteboard: [photon-onboarding])

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

(Reporter)

Description

4 months ago
Should be able to toggle the onBoarding overlay by clicking the overlay fox icon
(Reporter)

Updated

4 months ago
Whiteboard: [photon-onboarding]

Updated

4 months ago
Target Milestone: --- → Firefox 55

Updated

4 months ago
Assignee: nobody → gasolin
Status: NEW → ASSIGNED

Comment 1

4 months ago
The toggle prototype has done in 
https://github.com/gasolin/onboarding.js

Updated

4 months ago
Flags: qe-verify+
QA Contact: jwilliams

Updated

4 months ago
Priority: -- → P1
(Assignee)

Updated

3 months ago
Target Milestone: Firefox 55 → Firefox 56
(Assignee)

Comment 2

3 months ago
Hi Michael, do you have assets for the fox icon now? We have only icons inside the overlay.
Flags: needinfo?(mverdi)

Comment 3

3 months ago
Sorry - forgot to do this yesterday. https://drive.google.com/a/mozilla.com/file/d/0B2y0OSD97CR6SlBuTkFYdmp0UjQ/view?usp=sharing
Flags: needinfo?(mverdi)
(Assignee)

Comment 4

3 months ago
Thank you Michael!

There'll be a skeleton patch for making it system add-on with injecting the icon and empty overlay later.
Comment hidden (mozreview-request)
(Assignee)

Comment 6

3 months ago
Comment on attachment 8868896 [details]
Bug 1357005 - Create onboarding icon which toggles a first-time use dialog on net newtab.

Hi Matt:
We just brokedown the code from WIP in bug 1354046 to smaller pieces; This patch contains mainly just skeleton of system-addon so maybe it's easier for transferring reviewer to Mossop if he's available.

It contains mainly the following features:
- Put necessary file as system add-on
- Add an icon to newtab
- Clicking the icon toggles an overlay with empty dialog; clicking [X] or outside dialog closes it.
Attachment #8868896 - Flags: review?(MattN+bmo)

Comment 7

3 months ago
mozreview-review
Comment on attachment 8868896 [details]
Bug 1357005 - Create onboarding icon which toggles a first-time use dialog on net newtab.

https://reviewboard.mozilla.org/r/140522/#review143950

::: browser/base/content/newtab/page.js:45
(Diff revision 1)
> +    // If Onboarding module is turned on, load it.
> +    if (!Services.prefs.getBoolPref("browser.onboarding.disabled", false)) {
> +      Components.classes["@mozilla.org/moz/jssubscript-loader;1"]
> +        .getService(Components.interfaces.mozIJSSubScriptLoader)
> +        .loadSubScript("resource://onboarding/onboarding.js", window);
> +        window.onboarding.init();

since onboarding ui does not need to block the newtab ui, we can wrap it inside of `requestIdleCallback` function and let browser decide when to execute it.

::: browser/extensions/moz.build:36
(Diff revision 1)
>  # Nightly-only system add-ons
>  if CONFIG['NIGHTLY_BUILD']:
>      DIRS += [
>          'activity-stream',
>          'webcompat-reporter',
> +        'onboarding',

it will be nice to apply the alphabet ordering (put onboarding before webcompact-)

::: browser/extensions/onboarding/content/onboarding.css:60
(Diff revision 1)
> +}
> +
> +#onboarding-overlay-dialog > header {
> +  grid-row: dialog-start / page-start;
> +  grid-column: dialog-start / tour-end;
> +  margin: 36px 0 0 36px;

Not sure if we need to support rtl, but using `margin-inline-start` instead of `margin-right` here seems no harm.
(Assignee)

Updated

3 months ago
Assignee: gasolin → rexboy
(Reporter)

Comment 8

3 months ago
mozreview-review
Comment on attachment 8868896 [details]
Bug 1357005 - Create onboarding icon which toggles a first-time use dialog on net newtab.

https://reviewboard.mozilla.org/r/140522/#review143988

::: browser/extensions/onboarding/content/onboarding.js:13
(Diff revision 1)
> +let onboarding = {
> +
> +  _overlayIcon: null,
> +  _overlay: null,
> +
> +  async init() {

Thanks for the work.
Looks like we are having outside user to check browser.onboarding.disabled to decide if should call `onboarding.init`. Just plase add comments like "Please check browser.onboarding.disabled before calling onboarding.init" so in the future other when people see the onboarding.js, will know in fact the onboarding system add-on is behind a pref.
Comment hidden (mozreview-request)
(Assignee)

Comment 10

3 months ago
Comment on attachment 8868896 [details]
Bug 1357005 - Create onboarding icon which toggles a first-time use dialog on net newtab.

I did some update based on the advices above. Matt please see my comment 6, thanks!
Attachment #8868896 - Flags: review?(MattN+bmo)

Comment 11

3 months ago
mozreview-review
Comment on attachment 8868896 [details]
Bug 1357005 - Create onboarding icon which toggles a first-time use dialog on net newtab.

https://reviewboard.mozilla.org/r/140522/#review144434

::: browser/extensions/onboarding/content/img/overlay-icon.svg:7
(Diff revision 2)
> +<svg width="36px" height="29px" viewBox="0 0 36 29" version="1.1" xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink">
> +    <!-- Generator: Sketch 44 (41411) - http://www.bohemiancoding.com/sketch -->
> +    <title>overlayfox</title>
> +    <desc>Created with Sketch.</desc>
> +    <defs>
> +        <polygon id="path-1" points="0.00197734509 0.057704922 35.9549691 0.057704922 35.9549691 27.9413306 0.00197734509 27.9413306"></polygon>

the svg looks like have not been optimized, should be optimized by inkscape or if there's any script tool available

::: browser/extensions/onboarding/content/onboarding.css:29
(Diff revision 2)
> +#onboarding-overlay-icon {
> +  width: 52px;
> +  height: 40px;
> +  position: absolute;
> +  top: 30px;
> +  left: 30px;

should confirm with UX if we need put the icon at right in rtl mode

::: browser/extensions/onboarding/content/onboarding.css:40
(Diff revision 2)
> +}
> +
> +#onboarding-tour-close-btn {
> +  position: absolute;
> +  top: 15px;
> +  right: 15px;

need UX confirm as well, we can deal with it later anyway
Attachment #8868896 - Flags: review?(MattN+bmo) → review?(dtownsend)
Hi KM, I discussed with Mossop and he will be doing the reviews for the tour now so I've redirected the review to him.

Comment 13

3 months ago
mozreview-review
Comment on attachment 8868896 [details]
Bug 1357005 - Create onboarding icon which toggles a first-time use dialog on net newtab.

https://reviewboard.mozilla.org/r/140522/#review144744

::: browser/base/content/newtab/page.js:48
(Diff revision 2)
> +        Components.classes["@mozilla.org/moz/jssubscript-loader;1"]
> +          .getService(Components.interfaces.mozIJSSubScriptLoader)
> +          .loadSubScript("resource://onboarding/onboarding.js", window);
> +          window.onboarding.init();
> +      });
> +    }

Perhaps this was dicussed elsewhere but I'm new to the project. Generally system add-ons shouldn't rely on code in the main application. Why wouldn't the system add-on detect when the about:newtab page loads and inject itself into it? This would allow us to deploy the add-on to any version of Firefox we chose.
Attachment #8868896 - Flags: review?(dtownsend)
(Assignee)

Comment 14

3 months ago
mozreview-review-reply
Comment on attachment 8868896 [details]
Bug 1357005 - Create onboarding icon which toggles a first-time use dialog on net newtab.

https://reviewboard.mozilla.org/r/140522/#review144744

> Perhaps this was dicussed elsewhere but I'm new to the project. Generally system add-ons shouldn't rely on code in the main application. Why wouldn't the system add-on detect when the about:newtab page loads and inject itself into it? This would allow us to deploy the add-on to any version of Firefox we chose.

One of the reason is noted in bug 1354046 comment 23:
> 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?).

And I'm afraid whether making a check on every page load slows down the page load; since it is only used by about:newtab (Or activity-stream in the future) this may make things easier; Loading from js code rather than from newpage.xhtml is to avoid loading the script (or error) when pref-off.
How do you think?

And thanks for helping on review!
(Assignee)

Updated

3 months ago
Flags: needinfo?(dtownsend)
Comment hidden (mozreview-request)
(In reply to KM Lee [:rexboy] from comment #14)
> Comment on attachment 8868896 [details]
> Bug 1357005 - Create onboarding icon which toggles a first-time use dialog
> on net newtab.
> 
> https://reviewboard.mozilla.org/r/140522/#review144744
> 
> > Perhaps this was dicussed elsewhere but I'm new to the project. Generally system add-ons shouldn't rely on code in the main application. Why wouldn't the system add-on detect when the about:newtab page loads and inject itself into it? This would allow us to deploy the add-on to any version of Firefox we chose.
> 
> One of the reason is noted in bug 1354046 comment 23:
> > 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?).

Where you choose to inject the frame isn't at question here (though I'd recommend injecting it into the new tab page itself). I'm asking about how you trigger the injection in the first place.

> And I'm afraid whether making a check on every page load slows down the page
> load; since it is only used by about:newtab (Or activity-stream in the
> future) this may make things easier; Loading from js code rather than from
> newpage.xhtml is to avoid loading the script (or error) when pref-off.
> How do you think?

I guess this makes me question whether doing this as a system add-on is the right choice. The benefits of a system add-on are that we can quickly update it and release it for multiple versions of Firefox, but if we rely on Firefox itself having hooks to start the add-on then we run the risk of not being able to use the system add-on for certain versions of Firefox.

The cost of listening for page loads is very small and we can make it even cheaper by only doing the real check and load on idle (as you are in the newtab page here) so I don't think it is worth worrying about.
Flags: needinfo?(dtownsend)
(Assignee)

Comment 17

3 months ago
Hmm, actually Activity-stream is doing quite similar things in NewTabService:
http://searchfox.org/mozilla-central/source/browser/components/newtab/aboutNewTabService.js#119
But they've been stabilized it for a while and need to replace the whole page anyway, so it's somewhat different with our case.
I think we just want to serve the hook as an entry point; and nothing will be checked into newTab after this bug, so the risk of the hook is not working may be relative low? 

I also just had a quick discussion with team, and we actually feel okay to use injection inside add-on if that doesn’t hurt performance much. So you may decide which one is more suitable, thanks!
Flags: needinfo?(dtownsend)
(In reply to KM Lee [:rexboy] from comment #17)
> Hmm, actually Activity-stream is doing quite similar things in NewTabService:
> http://searchfox.org/mozilla-central/source/browser/components/newtab/
> aboutNewTabService.js#119
> But they've been stabilized it for a while and need to replace the whole
> page anyway, so it's somewhat different with our case.
> I think we just want to serve the hook as an entry point; and nothing will
> be checked into newTab after this bug, so the risk of the hook is not
> working may be relative low? 
>
> I also just had a quick discussion with team, and we actually feel okay to
> use injection inside add-on if that doesn’t hurt performance much. So you
> may decide which one is more suitable, thanks!

It's a relatively low risk but I think regardless we're better off architecting on the assumption that the system add-on is standalone and doesn't rely on browser hooks where we can. Activity Stream requires much deeper hooks. If we find performance to be a concern it would be a simple change to switch to using such a hook in the future.

We have to support activity stream which runs in the content process right? In which case the simplest route is going to be registering a framescript in bootstrap.js, have it listen for load events from the content window for the frame and check the url of the window for newtab or activity stream and then running the injection code which can probably all live in the frame script. That will work for both chrome process and content process documents.
Flags: needinfo?(dtownsend)

Comment 19

3 months ago
mozreview-review
Comment on attachment 8868896 [details]
Bug 1357005 - Create onboarding icon which toggles a first-time use dialog on net newtab.

https://reviewboard.mozilla.org/r/140522/#review145692

::: browser/extensions/onboarding/content/onboarding.js:24
(Diff revision 3)
> +    // We want to create and append elements after CSS is loaded so
> +    // no flash of style changes and no additional reflow.
> +    await this._loadCSS();
> +    this._overlayIcon = this._renderOverlayIcon();
> +    this._overlay = this._renderOverlay();
> +    let fragment = win.document.createDocumentFragment();

Why use the fragment?

::: browser/extensions/onboarding/content/onboarding.js:50
(Diff revision 3)
> +    win.document.body.removeChild(this._overlayIcon);
> +    win.document.body.removeChild(this._overlay);

I believe that element.remove() works now.

::: browser/extensions/onboarding/content/onboarding.js:85
(Diff revision 3)
> +    img.id = "onboarding-overlay-icon";
> +    return img;
> +  },
> +
> +  _loadCSS() {
> +    return new Promise(resolve => {

Why does this need to be a promise?
Attachment #8868896 - Flags: review?(dtownsend)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 22

3 months ago
mozreview-review-reply
Comment on attachment 8868896 [details]
Bug 1357005 - Create onboarding icon which toggles a first-time use dialog on net newtab.

https://reviewboard.mozilla.org/r/140522/#review145692

> Why does this need to be a promise?

We tries to load css completely before appending things into DOM to avoid additional reflow. I added some comments for that.
(Assignee)

Comment 23

3 months ago
ok, now it loads framescript from bootstrap.js! and I addressed changes mentioned above.

Comment 24

3 months ago
mozreview-review
Comment on attachment 8868896 [details]
Bug 1357005 - Create onboarding icon which toggles a first-time use dialog on net newtab.

https://reviewboard.mozilla.org/r/140522/#review146154

The code here looks mostly fine however I think there is an architectural change we should make. Currently you reload the onboarding script everytime a new tab page is opened clobbering the old onboarding instance. Instead you should move the code into the framescript and only call it when the pref says so. This will save us costs on subsequent loads.

In order to be able to support multiple new tab pages at a time rather than onboarding being an object you should make it a class (Onboarding) and create a new instance of it for each page that we see the load event for.

::: browser/app/profile/firefox.js:1664
(Diff revision 5)
>  #endif
>  
>  pref("browser.suppress_first_window_animation", true);
> +
> +// Preferences for Photon onboarding system extension
> +pref("browser.onboarding.disabled", false);

This is unnecessary since you provide a default when getting the pref anyway. Unless we want this to be easily togglable by users?

::: browser/extensions/onboarding/content/onboarding.css:14
(Diff revision 5)
> +  position: fixed;
> +  top: 0;
> +  left: 0;
> +  right: 0;
> +  bottom: 0;
> +  z-index: 999;

If I recall my CSS correctly this isn't necessary since the element comes at the end of the DOM tree.
Attachment #8868896 - Flags: review?(dtownsend)
(Assignee)

Comment 25

3 months ago
mozreview-review-reply
Comment on attachment 8868896 [details]
Bug 1357005 - Create onboarding icon which toggles a first-time use dialog on net newtab.

https://reviewboard.mozilla.org/r/140522/#review146154

I changed it but didn't understand very well about clobbering. Does that mean if I load onboarding.js from framescript using SubScriptLoader, it would be cleaned-up and reloaded each time I open a new tab; but not the case if I load onboarding.js itself by mm.loadFrameScript()?

> If I recall my CSS correctly this isn't necessary since the element comes at the end of the DOM tree.

For now activity-stream has a search icon set to z-index=2, so I'd just left it for some case like that.
I added some comments for it.
Comment hidden (mozreview-request)
(In reply to KM Lee [:rexboy] from comment #25)
> Comment on attachment 8868896 [details]
> Bug 1357005 - Create onboarding icon which toggles a first-time use dialog
> on net newtab.
> 
> https://reviewboard.mozilla.org/r/140522/#review146154
> 
> I changed it but didn't understand very well about clobbering. Does that
> mean if I load onboarding.js from framescript using SubScriptLoader, it
> would be cleaned-up and reloaded each time I open a new tab; but not the
> case if I load onboarding.js itself by mm.loadFrameScript()?

Each time you call loadSubScript it reruns the script you give it. Since the end of the script assigns to this.onboarding you get a new object each time. Technically in your original patch I don't think that would have been an issue but it is confusing and as we build on this it might have become a problem.

Comment 28

3 months ago
mozreview-review
Comment on attachment 8868896 [details]
Bug 1357005 - Create onboarding icon which toggles a first-time use dialog on net newtab.

https://reviewboard.mozilla.org/r/140522/#review146578

As I mentioned in my previous review you need to make onboarding a class so you can instantiate it once for each new tab page or things won't work correctly when multiple new tab pages are open. I've included comments here showing how to do that.

::: browser/extensions/onboarding/content/onboarding.js:14
(Diff revision 6)
> +
> +/**
> + * The script won't be initialized if we turned off onboarding by
> + * setting "browser.onboarding.disabled" to true.
> + */
> +(function(exports) {

There's no need to wrap this in a function.

::: browser/extensions/onboarding/content/onboarding.js:18
(Diff revision 6)
> + */
> +(function(exports) {
> +const ONBOARDING_CSS_URL = "resource://onboarding/onboarding.css";
> +const ABOUT_NEWTAB_URL = "about:newtab";
> +
> +let onboarding = {

class Onboarding {
      constructor(contentWindow) {
        this.init(contentWindow);
      }

Needed because constructors can't be async

::: browser/extensions/onboarding/content/onboarding.js:109
(Diff revision 6)
> +  // Load onboarding module only when we enable it.
> +  if (content.location.href == ABOUT_NEWTAB_URL &&
> +      !Services.prefs.getBoolPref("browser.onboarding.disabled")) {
> +
> +    content.requestIdleCallback(() => {
> +      onboarding.init(content);

new Onboarding(content);

::: browser/extensions/onboarding/content/onboarding.js:114
(Diff revision 6)
> +      onboarding.init(content);
> +    });
> +  };
> +}, true);
> +
> +exports.onboarding = onboarding;

No need for this.
Attachment #8868896 - Flags: review?(dtownsend)
Comment hidden (mozreview-request)
(Assignee)

Comment 30

3 months ago
mozreview-review-reply
Comment on attachment 8868896 [details]
Bug 1357005 - Create onboarding icon which toggles a first-time use dialog on net newtab.

https://reviewboard.mozilla.org/r/140522/#review146578

Ok, I think I got you. So if I understand correctly the issue is making assignment each time rather than doing class/function declarction, which actually don't rerun although the script is loaded into each frame; but no additional load or computing resource is taken for the class if it don't instinitiate. Is that correct?
And I just modified the patch.

Comment 31

3 months ago
mozreview-review
Comment on attachment 8868896 [details]
Bug 1357005 - Create onboarding icon which toggles a first-time use dialog on net newtab.

https://reviewboard.mozilla.org/r/140522/#review147010

This looks good, thanks for making the changes.
Attachment #8868896 - Flags: review?(dtownsend) → review+
(Assignee)

Comment 32

3 months ago
Thank you Mossop!

Tagging checkin-needed. I'll move on to work on patches for the contents inside the overlay.
Keywords: checkin-needed

Comment 33

3 months ago
We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s 89a40b368712 -d 45aed7ca18ba: rebasing 398989:89a40b368712 "Bug 1357005 - Create onboarding icon which toggles a first-time use dialog on net newtab. r=mossop" (tip)
merging browser/app/profile/firefox.js
merging browser/extensions/moz.build
warning: conflicts while merging browser/extensions/moz.build! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)
Keywords: checkin-needed
Comment hidden (mozreview-request)
(Assignee)

Comment 35

3 months ago
Didn't notice the conflict. Rebased.
Keywords: checkin-needed

Comment 36

3 months ago
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/bab63755b35d
Create onboarding icon which toggles a first-time use dialog on net newtab. r=mossop
Keywords: checkin-needed
backed out for eslint failure - https://treeherder.mozilla.org/logviewer.html#?job_id=103296456&repo=autoland
Flags: needinfo?(rexboy)

Comment 38

3 months ago
Backout by cbook@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/d01f0ffd5bc1
Backed out changeset bab63755b35d for eslint failure
also it causes https://treeherder.mozilla.org/logviewer.html#?job_id=103309421&repo=autoland
Comment hidden (mozreview-request)
(Assignee)

Comment 41

3 months ago
Sorry, forgot to run lint. 
I've got those the errors above fixed.
Flags: needinfo?(rexboy)
(Assignee)

Comment 42

3 months ago
try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=4a57f109f447
(Assignee)

Updated

3 months ago
Keywords: checkin-needed

Comment 43

3 months ago
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/283ed0ad8bf4
Create onboarding icon which toggles a first-time use dialog on net newtab. r=mossop
Keywords: checkin-needed

Comment 44

3 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/283ed0ad8bf4
Status: ASSIGNED → RESOLVED
Last Resolved: 3 months ago
status-firefox55: --- → fixed
Resolution: --- → FIXED

Comment 45

3 months ago
mozreview-review
Comment on attachment 8868896 [details]
Bug 1357005 - Create onboarding icon which toggles a first-time use dialog on net newtab.

https://reviewboard.mozilla.org/r/140522/#review148646

::: browser/extensions/onboarding/content/onboarding.js:50
(Diff revision 9)
> +      // If the clicking target is directly on the outer-most overlay,
> +      // that means clicking outside the tour content area.
> +      // Let's toggle the overlay.
> +      case "onboarding-overlay":
> +        this.toggleOverlay();
> +      break;

wrong intent

::: browser/extensions/onboarding/content/onboarding.js:106
(Diff revision 9)
> +  }
> +}
> +
> +addEventListener("load", function(evt) {
> +  // Load onboarding module only when we enable it.
> +  if (content.location.href == ABOUT_NEWTAB_URL &&

If this patch is not landed yet, we can add about:Home URL as well based on PM's new request, or its fine to file a new bug for this

Updated

3 months ago
Duplicate of this bug: 1357015

Updated

3 months ago
Duplicate of this bug: 1357014

Updated

3 months ago
Duplicate of this bug: 1365547

Updated

3 months ago
Blocks: 1369296

Updated

3 months ago
Depends on: 1369794
Are there any prefs or specs that will be helpful for me to verify this issue? Thanks
Flags: needinfo?(fliu)
(Reporter)

Comment 50

3 months ago
(In reply to Justin [:JW_SoftvisionQA] from comment #49)
> Are there any prefs or specs that will be helpful for me to verify this
> issue? Thanks
Hi,
Specs is here: https://mozilla.invisionapp.com/share/4MBDUMS5W#/screens/228511972_Overview
In the specs, you will see the little fox icon sitting on the top-left corner of the page (not the bottom one).
An overlay should open by clicking that fox icon.
Please notice the content of the overlay doesn't matter (even empty or lack of enough tours).
This bug should focus on testing the open/close the overlay. Other bugs will deal with the overlay content, thanks.
Flags: needinfo?(fliu)
I have tested this bug and it works as expected.
Status: RESOLVED → VERIFIED

Updated

2 months ago
Depends on: 1372444
Based on comment 51 I am updating the flags.
status-firefox55: fixed → verified
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.