Closed Bug 1357005 Opened 7 years ago Closed 7 years ago

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

Categories

(Firefox :: General, enhancement, P1)

Product:
Firefox
Component:
General
Type:
enhancement

Tracking

()

Status:
VERIFIED FIXED
Milestone:
Firefox 56
Tracking Flags:
Tracking Status
firefox55 --- verified

People

(Reporter: Fischer, Assigned: rexboy)

References

Details

(Whiteboard: [photon-onboarding])

Attachments

(1 file)

Bug 1357005 - Create onboarding icon which toggles a first-time use dialog on net newtab.
59 bytes, text/x-review-board-request
mossop
: review+
Details 
Should be able to toggle the onBoarding overlay by clicking the overlay fox icon
Whiteboard: [photon-onboarding]
Target Milestone: --- → Firefox 55
Assignee: nobody → gasolin
Status: NEW → ASSIGNED
The toggle prototype has done in 
https://github.com/gasolin/onboarding.js
Flags: qe-verify+
QA Contact: jwilliams
Priority: -- → P1
Target Milestone: Firefox 55 → Firefox 56
Hi Michael, do you have assets for the fox icon now? We have only icons inside the overlay.
Flags: needinfo?(mverdi)
Thank you Michael!

There'll be a skeleton patch for making it system add-on with injecting the icon and empty overlay later.
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 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: gasolin → rexboy
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 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 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 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)
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!
Flags: needinfo?(dtownsend)
(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)
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 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 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.
ok, now it loads framescript from bootstrap.js! and I addressed changes mentioned above.
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)
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.
(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 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 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 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+
Thank you Mossop!

Tagging checkin-needed. I'll move on to work on patches for the contents inside the overlay.
Keywords: checkin-needed
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)
Didn't notice the conflict. Rebased.
Keywords: checkin-needed
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)
Backout by cbook@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/d01f0ffd5bc1
Backed out changeset bab63755b35d for eslint failure
Sorry, forgot to run lint. 
I've got those the errors above fixed.
Flags: needinfo?(rexboy)
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
https://hg.mozilla.org/mozilla-central/rev/283ed0ad8bf4
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
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
Blocks: 1369296
Depends on: 1369794
Are there any prefs or specs that will be helpful for me to verify this issue? Thanks
Flags: needinfo?(fliu)
(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
Depends on: 1372444
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: