Closed Bug 1357020 Opened 7 years ago Closed 7 years ago

Should hide the onboarding tour if user explicitly checked the hide-the-tour checkbox

Categories

(Firefox :: General, enhancement, P1)

Product:
Firefox
Component:
General
Type:
enhancement

Tracking

()

Status:
VERIFIED FIXED
Milestone:
Firefox 56
Tracking Flags:
Tracking Status
firefox56 --- fixed

People

(Reporter: Fischer, Assigned: Fischer)

References

Details

(Whiteboard: [photon-onboarding] )

Attachments

(1 file)

Bug 1357020 - Should hide the onboarding tour if user explicitly checked the hide-the-tour checkbox,
59 bytes, text/x-review-board-request
mossop
: review+
gasolin
: review+
rexboy
: review+
Details
Whiteboard: [photon-onboarding]
What we want is that the user can complete all of the tour steps (has a checkmark for all six items) and the Fox icon will remain on the new tab page so that they can revisit the tour when they want. If the user then checks the box to remove the tips menu, we should remove the fox icon from the new tab page. In other words, the tour is only "complete" when the user marks it as complete.
Hi Cindy,
As the Comment 1 from Verdi, the criteria to hide the fox icon are
  - user completes all 6 tours
  - user *explicitly* check the hiding-tips-menu checkbox

Please update the user story as well: https://bugzilla.mozilla.org/show_bug.cgi?id=1356175#c0

Thank you
Flags: needinfo?(chsiang)
(In reply to Fischer [:Fischer] from comment #2)
> Hi Cindy,
> As the Comment 1 from Verdi, the criteria to hide the fox icon are
>   - user completes all 6 tours
>   - user *explicitly* check the hiding-tips-menu checkbox
> 
> Please update the user story as well:
> https://bugzilla.mozilla.org/show_bug.cgi?id=1356175#c0
> 
> Thank you
Sorry, got the criteria wrong. In fact, the completions of 6 tours don't matter.
The only criterion to hide the fox icon is user *explicitly* checks the hiding-tips-menu checkbox.
Summary: Should not display the onBoarding fox icon when all the onBoarding tours are completed → Should not display the onBoarding fox icon if user explicitly checked the hiding-tips-menu checkbox
Updated!
Flags: needinfo?(chsiang)
Target Milestone: --- → Firefox 55
Assignee: nobody → rexboy
Status: NEW → ASSIGNED
I looked the specification again and there's only one checkbox labeled "Mark all as complete and hide the menu".
Does that mean bug 1357021 is just a duplicate of this one rather than another checkbox alone?

https://mozilla.invisionapp.com/share/2HB9YE939#/screens
And seems we don't have a scenario for showing the fox icon again, so actually what really matters is just hiding the icon.
I just saw bug 1356180 talking about popping up future/different tours again. So maybe we still need to do "mark all and hide" to distinguish from future tours.
Flags: qe-verify+
QA Contact: jwilliams
Priority: -- → P1
Priority: P1 → P2
Target Milestone: Firefox 55 → Firefox 57
Assignee: rexboy → fliu
Priority: P2 → P1
(In reply to Fischer [:Fischer] from comment #8)
> Created attachment 8875681 [details]
> Bug 1357020 - Should not display the onBoarding fox icon if user explicitly
> checked the hiding-tips-menu checkbox
> 
> Review commit: https://reviewboard.mozilla.org/r/147110/diff/#index_header
> See other reviews: https://reviewboard.mozilla.org/r/147110/
Right now this wip patch has built a message channel to let the chrome process receive and execute the request of setting prefs from the framescript.
(In reply to Fischer [:Fischer] from comment #0)
> Should not display the onBoarding fox icon when all the onBoarding tours are completed
Update the correct bug description:
Should not display the onBoarding fox icon if user explicitly checked the hiding-tips-menu checkbox
Comment on attachment 8875681 [details]
Bug 1357020 - Should hide the onboarding tour if user explicitly checked the hide-the-tour checkbox,

https://reviewboard.mozilla.org/r/147110/#review151632

::: browser/app/profile/firefox.js:1682
(Diff revision 1)
>  
>  pref("browser.suppress_first_window_animation", true);
>  
>  // Preferences for Photon onboarding system extension
>  pref("browser.onboarding.enabled", true);
> +pref("browser.onboarding.alwaysHidden", false);

actually its not `always hidden` because we will force to show user tours in several conditions, I suggest use `userPreferHidden` or `preferHidden` instead

::: browser/extensions/onboarding/bootstrap.js:12
(Diff revision 1)
>  const {classes: Cc, interfaces: Ci, utils: Cu} = Components;
>  Cu.import("resource://gre/modules/Services.jsm");
>  
> +/**
> + * Set pref, The reason why there is no `getPrefs` function is
> + * because of the priviledge level, we cannot set prefs inside a freamscript,

nit:　freamscript -> framescript

And the description can be polished a bit:
```
Because of the priviledge level, we can get prefs  inside a framescript but cannot set prefs. We still read pref inside the framescript for efficiency but do setPref here
```

::: browser/extensions/onboarding/content/onboarding.js:63
(Diff revision 1)
>    }
>  
>    toggleOverlay() {
> +    let hiddenCheckbox = this._window.document.querySelector("#onboarding-tour-hidden-checkbox");
> +    if (hiddenCheckbox.checked) {
> +      this.destroy();

put destroy as the last call

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

Since we expect to add more conditions here, we can wrap all test cases into a function `shouldRenderOverlay` and return true/false here
Comment on attachment 8875681 [details]
Bug 1357020 - Should hide the onboarding tour if user explicitly checked the hide-the-tour checkbox,

https://reviewboard.mozilla.org/r/147110/#review151672

::: browser/extensions/onboarding/content/onboarding.css:92
(Diff revision 1)
>    grid-column: dialog-start / tour-end;
> +  font-size: 13px;
>  }
> +
> +#onboarding-tour-hidden-checkbox {
> +  margin-left: 27px;

let's use margin-inline-start/margin-inline-end for rtl compatibility
Comment on attachment 8875681 [details]
Bug 1357020 - Should hide the onboarding tour if user explicitly checked the hide-the-tour checkbox,

https://reviewboard.mozilla.org/r/147110/#review151686

::: browser/app/profile/firefox.js:1682
(Diff revision 1)
>  
>  pref("browser.suppress_first_window_animation", true);
>  
>  // Preferences for Photon onboarding system extension
>  pref("browser.onboarding.enabled", true);
> +pref("browser.onboarding.alwaysHidden", false);

In what condition we should force to show the tour again?

::: browser/extensions/onboarding/bootstrap.js:12
(Diff revision 1)
>  const {classes: Cc, interfaces: Ci, utils: Cu} = Components;
>  Cu.import("resource://gre/modules/Services.jsm");
>  
> +/**
> + * Set pref, The reason why there is no `getPrefs` function is
> + * because of the priviledge level, we cannot set prefs inside a freamscript,

Thanks, ok.

::: browser/extensions/onboarding/content/onboarding.css:92
(Diff revision 1)
>    grid-column: dialog-start / tour-end;
> +  font-size: 13px;
>  }
> +
> +#onboarding-tour-hidden-checkbox {
> +  margin-left: 27px;

Thanks ok

::: browser/extensions/onboarding/content/onboarding.js:63
(Diff revision 1)
>    }
>  
>    toggleOverlay() {
> +    let hiddenCheckbox = this._window.document.querySelector("#onboarding-tour-hidden-checkbox");
> +    if (hiddenCheckbox.checked) {
> +      this.destroy();

Thanks ok

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

Let's handle this part in Bug 1367696 - show new user / updating user tour because we will have to decide to show the tour or not as well when classifying user as New user tour or Updating user tour
Comment on attachment 8875681 [details]
Bug 1357020 - Should hide the onboarding tour if user explicitly checked the hide-the-tour checkbox,

https://reviewboard.mozilla.org/r/147110/#review151632

> actually its not `always hidden` because we will force to show user tours in several conditions, I suggest use `userPreferHidden` or `preferHidden` instead

We may need to re-show the tour if there is a new version of tour.
Just for the semantic reason:
```
if (userPreferHidden) {
  // This is a bit vague. User prefers but it dosen't mean we should hide the tour.
}
```
If "alwaysHidden" is too strong, we could use just "browser.onboarding.hidden".
> > +pref("browser.onboarding.alwaysHidden", false);
> 
> In what condition we should force to show the tour again?

Not show the same tour again, but for following cases:
* if user prefer hidden(as a new user) but we still need to show if there's an update tour
* if user prefer hidden(as a update user) but we have new version of update tour, we still need to show it

So maybe we should store the intent-to-hide new/update tour version instead of a single hidden pref?
Then we can decide if we want to show the overlay by comparing if the (new/update hide pref) is smaller than the current new/update tour version
(In reply to Fred Lin [:gasolin] from comment #15)
> > > +pref("browser.onboarding.alwaysHidden", false);
> > 
> > In what condition we should force to show the tour again?
> 
> Not show the same tour again, but for following cases:
> * if user prefer hidden(as a new user) but we still need to show if there's
> an update tour
> * if user prefer hidden(as a update user) but we have new version of update
> tour, we still need to show it
> 
> So maybe we should store the intent-to-hide new/update tour version instead
> of a single hidden pref?
> Then we can decide if we want to show the overlay by comparing if the
> (new/update hide pref) is smaller than the current new/update tour version
Yes, so like my comment 14, maybe we could use "browser.onboarding.hidden" which is not so strong.
And when doing the version checking we flip the pref, such as:
```
if (isThereNewTourVersion()) {
  Services.prefs.setBooldPref("browser.onboarding.hidden", false);
}
if (Services.prefs.getBooldPref("browser.onboarding.hidden")) {
  return; // Do show the tour
}
```
Comment on attachment 8875681 [details]
Bug 1357020 - Should hide the onboarding tour if user explicitly checked the hide-the-tour checkbox,

https://reviewboard.mozilla.org/r/147110/#review151726

::: browser/extensions/onboarding/content/onboarding.js:62
(Diff revision 1)
>      this._overlay.remove();
>    }
>  
>    toggleOverlay() {
> +    let hiddenCheckbox = this._window.document.querySelector("#onboarding-tour-hidden-checkbox");
> +    if (hiddenCheckbox.checked) {

Examining just in toggleOvleay means a user need to both check the checkbox and close the overlay before it really takes effect. this may not be what user expect  if they open another tab or just closed the current tab directly. Have you considered listning to onchange of the checkbox?
Comment on attachment 8875681 [details]
Bug 1357020 - Should hide the onboarding tour if user explicitly checked the hide-the-tour checkbox,

https://reviewboard.mozilla.org/r/147110/#review151726

> Examining just in toggleOvleay means a user need to both check the checkbox and close the overlay before it really takes effect. this may not be what user expect  if they open another tab or just closed the current tab directly. Have you considered listning to onchange of the checkbox?

Checked with UX. The desire behavior is "user needs to both check the checkbox and close the overlay before it really takes effect". User may want to check and uncheck, just thinking between un/checking. Taking effect right away is a bit too abrupt.
Comment on attachment 8875681 [details]
Bug 1357020 - Should hide the onboarding tour if user explicitly checked the hide-the-tour checkbox,

https://reviewboard.mozilla.org/r/147110/#review152270

::: browser/app/profile/firefox.js:1682
(Diff revision 2)
>  
>  pref("browser.suppress_first_window_animation", true);
>  
>  // Preferences for Photon onboarding system extension
>  pref("browser.onboarding.enabled", true);
> +pref("browser.onboarding.hidden", false);

let's use `browser.onboarding.newtour.hidden` since we will have `updateTour` very soon

::: browser/app/profile/firefox.js:1683
(Diff revision 2)
>  pref("browser.suppress_first_window_animation", true);
>  
>  // Preferences for Photon onboarding system extension
>  pref("browser.onboarding.enabled", true);
> +pref("browser.onboarding.hidden", false);
> +// On the Activity-Stream page, the snippet's position overlaps with our notificaition.

nit: notificaition->notification

::: browser/extensions/onboarding/bootstrap.js:32
(Diff revision 2)
> +/**
> + * @param {String} action the action to ask the content to do
> + * @param {Array} params the parameters for the action
> + */
> +function broadcastMessageToContent(action, params) {
> +  Services.mm.broadcastAsyncMessage("Onboarding:onChromeMessage", {

The last time I set review to gijs and he said all Async message string should be capitaled after `:`. ex: "Onboarding:OnChromeMessage" 
http://searchfox.org/mozilla-central/search?q=broadcastAsyncMessage&path=

::: browser/extensions/onboarding/content/onboarding.js:152
(Diff revision 2)
> +  }
> +
> +  handlePrefUpdates(prefs) {
> +    for (let { name, value } of prefs) {
> +      switch (name) {
> +        case "browser.onboarding.hidden":

Is there any case that we need to act after the value is updated from outside of this script?

::: browser/extensions/onboarding/content/onboarding.js:190
(Diff revision 2)
>        // Lazy loading until first toggle.
>        this._loadTours(onboardingTours);
>      }
>  
> +    let hiddenCheckbox = this._window.document.querySelector("#onboarding-tour-hidden-checkbox");
> +    if (hiddenCheckbox.checked) {

The UX flow looks strange to me.

I think the flow will be 

* checked the checkbox -> close the overlay and it never shown again

But not 

* checked the checkbox -> the overlay is closed immediately and not able to uncheck


We can set pref in onChanged state, since set the pref right away doesn't means close the overlay itself.

::: browser/extensions/onboarding/content/onboarding.js:203
(Diff revision 2)
> +  hideOverlay() {
> +    // To clear the listener first is important
> +    // because we only want to hide and destory,
> +    // not to react to the following prefs updates.
> +    this._clearChromeMessageListener();
> +    this.sendMessageToChrome("set-prefs", [

we should also support uncheck to set these value back
Attachment #8875681 - Flags: review?(gasolin)
Comment on attachment 8875681 [details]
Bug 1357020 - Should hide the onboarding tour if user explicitly checked the hide-the-tour checkbox,

https://reviewboard.mozilla.org/r/147110/#review152270

> let's use `browser.onboarding.newtour.hidden` since we will have `updateTour` very soon

Thanks. Per the offline discussion, let's keep "browser.onboarding.hidden still and put the version checking in the bug 1367696.

> nit: notificaition->notification

Thanks updated.

> The last time I set review to gijs and he said all Async message string should be capitaled after `:`. ex: "Onboarding:OnChromeMessage" 
> http://searchfox.org/mozilla-central/search?q=broadcastAsyncMessage&path=

Thanks will update.

> Is there any case that we need to act after the value is updated from outside of this script?

This is for the case that more than 2 about:newtab or about:home pages opened at the same time. So one page hides our onboarding tours, other page would receieve the message and hide the onboarding tours as well.

> The UX flow looks strange to me.
> 
> I think the flow will be 
> 
> * checked the checkbox -> close the overlay and it never shown again
> 
> But not 
> 
> * checked the checkbox -> the overlay is closed immediately and not able to uncheck
> 
> 
> We can set pref in onChanged state, since set the pref right away doesn't means close the overlay itself.

Thanks per offline discussion, yes the current actual behavior is "checked the checkbox -> close the overlay and it never shown again" already.
This hiding call happens when toggling tour overlay.
Since the effect really takes effect when toggling tour overlay, not checking the checkbox. setting pref when toggling tour overlay would be less complicated.

> we should also support uncheck to set these value back

Thanks per the offline discussion, since the actiual behavior is "checked the checkbox -> close the overlay and it never shown again", there should be no need to save these value back. And the current specs there is no other button or checkbox to let user re-show the tours either.
Comment on attachment 8875681 [details]
Bug 1357020 - Should hide the onboarding tour if user explicitly checked the hide-the-tour checkbox,

https://reviewboard.mozilla.org/r/147110/#review152476

What is the plan for automated testing of this UI?

::: browser/extensions/onboarding/bootstrap.js:25
(Diff revision 3)
> + **/
> +function setPrefs(prefs) {
> +  prefs.forEach(pref => {
> +    Preferences.set(pref.name, pref.value);
> +  });
> +}

This opens up a security hole where a compromised child process would be able to set any preference it liked. Please check pref.name against a list of prefs you want to allow the child to set here.

::: browser/extensions/onboarding/bootstrap.js:47
(Diff revision 3)
> +        break;
> +    }
> +  });
> +}
> +
> +function initPrefObservers() {

Preference observers are meant to work in the child process, is this necessary?

::: browser/extensions/onboarding/content/onboarding.js:191
(Diff revision 3)
>        this._loadTours(onboardingTours);
>      }
>  
> +    let hiddenCheckbox = this._window.document.querySelector("#onboarding-tour-hidden-checkbox");
> +    if (hiddenCheckbox.checked) {
> +      this.hideOverlay();

You should just set the prefs here and let the pref observer take care of hiding the icon.

::: browser/extensions/onboarding/content/onboarding.js:198
(Diff revision 3)
> +    }
> +
>      this._overlay.classList.toggle("onboarding-opened");
>    }
>  
> +  hideOverlay() {

This really should be called hideOverlayIcon
Attachment #8875681 - Flags: review?(dtownsend) → review-
Attachment #8875681 - Flags: review?(rexboy)
Attachment #8875681 - Flags: review?(gasolin)
Summary: Should not display the onBoarding fox icon if user explicitly checked the hiding-tips-menu checkbox → Should hide the onboarding tour if user explicitly checked the hide-the-tour checkbox
Comment on attachment 8875681 [details]
Bug 1357020 - Should hide the onboarding tour if user explicitly checked the hide-the-tour checkbox,

https://reviewboard.mozilla.org/r/147110/#review152476

An browser_onboarding_hide_tours.js test is added, thank you.

> This opens up a security hole where a compromised child process would be able to set any preference it liked. Please check pref.name against a list of prefs you want to allow the child to set here.

Thanks for reminding this, updated

> Preference observers are meant to work in the child process, is this necessary?

OK, moved to the child process

> You should just set the prefs here and let the pref observer take care of hiding the icon.

Updated, now this function only takes care of related prefs.

> This really should be called hideOverlayIcon

Maybe the bug title dosen't cover enough what this bug dose. In fact this bug is to destroy the onboarding tour and set the `browser.onboarding.hidden = true` so as to hide the tour. Just updated the bug title, the commit message and the function name to make it clearer.
Thank you.
(In reply to Fischer [:Fischer] from comment #25)
> Comment on attachment 8875681 [details]
> Bug 1357020 - Should hide the onboarding tour if user explicitly checked the
> hide-the-tour checkbox,
> 
> Review request updated; see interdiff:
> https://reviewboard.mozilla.org/r/147110/diff/3-4/
TRY: https://treeherder.mozilla.org/#/jobs?repo=try&revision=6c286523a58b8b443358b41eb73bdf5e231fb81e
Comment on attachment 8875681 [details]
Bug 1357020 - Should hide the onboarding tour if user explicitly checked the hide-the-tour checkbox,

https://reviewboard.mozilla.org/r/147110/#review153112

This is pretty close now, just want to see one last pass.

::: browser/extensions/onboarding/bootstrap.js:20
(Diff revision 4)
> +];
> +
> +/**
> + * Set pref. Why no `getPrefs` function is due to the priviledge level.
> + * We cannot set prefs inside a framescript but can read.
> + * For the simplicity and effeciency, we still read pref inside the freamscript.

Grammar nit: "For simplicity and effeciency, we still read prefs inside the framescript."

::: browser/extensions/onboarding/content/onboarding.js:55
(Diff revision 4)
> +    this.prefObserver = {
> +      observe: (subject, topic, data) => {

This can just be a function, no need for the surrounding object.

::: browser/extensions/onboarding/content/onboarding.js:192
(Diff revision 4)
>  
> -  let window = evt.target.defaultView;
> +    let window = evt.target.defaultView;
> -  // Load onboarding module only when we enable it.
> -  if ((window.location.href == ABOUT_NEWTAB_URL ||
> +    if ((window.location.href == ABOUT_NEWTAB_URL ||
> -       window.location.href == ABOUT_HOME_URL) &&
> +         window.location.href == ABOUT_HOME_URL) &&
> -      Services.prefs.getBoolPref("browser.onboarding.enabled", false)) {
> +        !Services.prefs.getBoolPref("browser.onboarding.hidden")) {

Please pass a default argument here.

::: browser/extensions/onboarding/test/browser/.eslintrc.js:5
(Diff revision 4)
> +"use strict";
> +
> +module.exports = {
> +  "extends": [
> +    "plugin:mozilla/mochitest-test"

You want "plugin:mozilla/browser-test" here and then you probably won't need the additional global definitions you've put in the other test files.

::: browser/extensions/onboarding/test/browser/browser_onboarding_hide_tours.js:38
(Diff revision 4)
> +  let expectedPrefUpdates = [
> +    promisePrefUpdated("browser.onboarding.hidden", true),
> +    promisePrefUpdated("browser.onboarding.notification.finished", true)
> +  ];
> +  await promiseClickElement(hometab.linkedBrowser, "#onboarding-overlay-icon");
> +  await promiseClickElement(hometab.linkedBrowser, "#onboarding-tour-hidden-checkbox");

Can you add a check in here that the onboarding overlay became visible?

::: browser/extensions/onboarding/test/browser/head.js:29
(Diff revision 4)
> +    100,
> +    30
> +  );
> +}
> +
> +function promiseClickElement(browser, selector) {

Can you use BrowserTestUtils.synthesizeMouseAtCenter here?
Attachment #8875681 - Flags: review?(dtownsend) → review-
Comment on attachment 8875681 [details]
Bug 1357020 - Should hide the onboarding tour if user explicitly checked the hide-the-tour checkbox,

https://reviewboard.mozilla.org/r/147110/#review153244

::: browser/extensions/onboarding/content/onboarding.js:48
(Diff revision 4)
> +
> +    this._initPrefObserver();
> +  }
> +
> +  _initPrefObserver() {
> +    if (this.prefObserver) {

I think we don't have situation of being initialized twice?

::: browser/extensions/onboarding/content/onboarding.js:112
(Diff revision 4)
>      this._overlay.remove();
>    }
>  
>    toggleOverlay() {
>      this._overlay.classList.toggle("opened");
> +    let hiddenCheckbox = this._window.document.querySelector("#onboarding-tour-hidden-checkbox");

Use getElementById() when possible to get better performance.

::: browser/extensions/onboarding/content/onboarding.js:192
(Diff revision 4)
>  
> -  let window = evt.target.defaultView;
> +    let window = evt.target.defaultView;
> -  // Load onboarding module only when we enable it.
> -  if ((window.location.href == ABOUT_NEWTAB_URL ||
> +    if ((window.location.href == ABOUT_NEWTAB_URL ||
> -       window.location.href == ABOUT_HOME_URL) &&
> +         window.location.href == ABOUT_HOME_URL) &&
> -      Services.prefs.getBoolPref("browser.onboarding.enabled", false)) {
> +        !Services.prefs.getBoolPref("browser.onboarding.hidden")) {

Should we move this pref check to the outmost if() too?
Attachment #8875681 - Flags: review?(rexboy)
Comment on attachment 8875681 [details]
Bug 1357020 - Should hide the onboarding tour if user explicitly checked the hide-the-tour checkbox,

https://reviewboard.mozilla.org/r/147110/#review153244

> Use getElementById() when possible to get better performance.

Thanks, updated.

> Should we move this pref check to the outmost if() too?

Thanks updated.
Comment on attachment 8875681 [details]
Bug 1357020 - Should hide the onboarding tour if user explicitly checked the hide-the-tour checkbox,

https://reviewboard.mozilla.org/r/147110/#review153244

> I think we don't have situation of being initialized twice?

Thanks per the offline talk, let's keep this check in case
Comment on attachment 8875681 [details]
Bug 1357020 - Should hide the onboarding tour if user explicitly checked the hide-the-tour checkbox,

https://reviewboard.mozilla.org/r/147110/#review153112

> Grammar nit: "For simplicity and effeciency, we still read prefs inside the framescript."

Thank you for reminding this, updated

> This can just be a function, no need for the surrounding object.

Thanks updated.
Just explain more about why originally went for observer object and now switched to a Map object approach.
In Preferences.jsm [1], if callback was a function, it would be invoked with only updated pref value, so unable to know which pref was updated inside callback.
And if callback was a observer object, it would be called as `this.callback.observe(subject, topic, data)` so able to know which pref was updated from `data` param.
Originally we though that prefs could share one observer then be distinguished with `switch...case`.
The updated approach uses a Map to store each pref being observed and its corresponding callback. This would need to generate more functions but no `switch...case` and can directly have pref value passed-in which reduces complexity.
Please let us know if any could be improved.


[1] https://dxr.mozilla.org/mozilla-central/rev/91134c95d68cbcfe984211fa3cbd28d610361ef1/toolkit/modules/Preferences.jsm#411

> Please pass a default argument here.

OK, updated.

> You want "plugin:mozilla/browser-test" here and then you probably won't need the additional global definitions you've put in the other test files.

Thanks updated.

> Can you add a check in here that the onboarding overlay became visible?

Thanks, added
(In reply to Fischer [:Fischer] from comment #32)
> Comment on attachment 8875681 [details]
> Bug 1357020 - Should hide the onboarding tour if user explicitly checked the
> hide-the-tour checkbox,
> 
> Review request updated; see interdiff:
> https://reviewboard.mozilla.org/r/147110/diff/4-5/
TRY: https://treeherder.mozilla.org/#/jobs?repo=try&revision=d1d5e4d388f739a89ee8a325d373796a52da0a8e
Comment on attachment 8875681 [details]
Bug 1357020 - Should hide the onboarding tour if user explicitly checked the hide-the-tour checkbox,

https://reviewboard.mozilla.org/r/147110/#review153112

> Can you use BrowserTestUtils.synthesizeMouseAtCenter here?

It seems not. If calling `BrowserTestUtils.synthesizeMouseAtCenter` inside the spawned content task, it would result error of finding no `BrowserTestUtils`.
Please let us know if there is a better way, thanks
Comment on attachment 8875681 [details]
Bug 1357020 - Should hide the onboarding tour if user explicitly checked the hide-the-tour checkbox,

https://reviewboard.mozilla.org/r/147110/#review153360

I have one issue not very sure in test; If you can confirmed with that I can r+ it.

::: browser/extensions/onboarding/bootstrap.js:36
(Diff revision 5)
> +    }
> +  });
> +}
> +
> +function initContentMessageListener() {
> +  Services.mm.addMessageListener("Onboarding:OnContentMessage", msg => {

nit: Onboarding:onContentMessage

::: browser/extensions/onboarding/content/onboarding.js:183
(Diff revision 5)
> -    return;
> +      return;
> -  }
> +    }
> -  removeEventListener("load", onLoad);
> +    removeEventListener("load", onLoad);
>  
> -  let window = evt.target.defaultView;
> +    let window = evt.target.defaultView;
> -  // Load onboarding module only when we enable it.
> +    let location = window.location.href

add ; at the end of line

::: browser/extensions/onboarding/test/browser/browser_onboarding_hide_tours.js:40
(Diff revision 5)
> +  ];
> +  await promiseClickElement(hometab.linkedBrowser, "#onboarding-overlay-icon");
> +  await promiseOnboardingOverlayOpened(hometab.linkedBrowser);
> +  await promiseClickElement(hometab.linkedBrowser, "#onboarding-tour-hidden-checkbox");
> +  await promiseClickElement(hometab.linkedBrowser, "#onboarding-overlay-close-btn");
> +  await Promise.all(expectedPrefUpdates);

I'm not very sure but can we guarantee the observer in promisePrefUpdated is called after the observer in Onboarding.js?
Attachment #8875681 - Flags: review?(rexboy)
(In reply to Fischer [:Fischer] from comment #31)
> Comment on attachment 8875681 [details]
> Bug 1357020 - Should hide the onboarding tour if user explicitly checked the
> hide-the-tour checkbox,
> 
> https://reviewboard.mozilla.org/r/147110/#review153112
> 
> > Grammar nit: "For simplicity and effeciency, we still read prefs inside the framescript."
> 
> Thank you for reminding this, updated
> 
> > This can just be a function, no need for the surrounding object.
> 
> Thanks updated.
> Just explain more about why originally went for observer object and now
> switched to a Map object approach.
> In Preferences.jsm [1], if callback was a function, it would be invoked with
> only updated pref value, so unable to know which pref was updated inside
> callback.
> And if callback was a observer object, it would be called as
> `this.callback.observe(subject, topic, data)` so able to know which pref was
> updated from `data` param.
> Originally we though that prefs could share one observer then be
> distinguished with `switch...case`.
> The updated approach uses a Map to store each pref being observed and its
> corresponding callback. This would need to generate more functions but no
> `switch...case` and can directly have pref value passed-in which reduces
> complexity.
> Please let us know if any could be improved.

Ah I didn't spot that Preferences.jsm observers behave differently to the default pref service. The new approach looks fine.
(In reply to Fischer [:Fischer] from comment #34)
> Comment on attachment 8875681 [details]
> Bug 1357020 - Should hide the onboarding tour if user explicitly checked the
> hide-the-tour checkbox,
> 
> https://reviewboard.mozilla.org/r/147110/#review153112
> 
> > Can you use BrowserTestUtils.synthesizeMouseAtCenter here?
> 
> It seems not. If calling `BrowserTestUtils.synthesizeMouseAtCenter` inside
> the spawned content task, it would result error of finding no
> `BrowserTestUtils`.
> Please let us know if there is a better way, thanks

You don't need the spawned content task at all, you just call BrowserTestUtils.synthesizeMouseAtCenter from the parent process, it handles spawning a content task to do the work.
Comment on attachment 8875681 [details]
Bug 1357020 - Should hide the onboarding tour if user explicitly checked the hide-the-tour checkbox,

https://reviewboard.mozilla.org/r/147110/#review153606

::: browser/extensions/onboarding/bootstrap.js:36
(Diff revision 5)
> +    }
> +  });
> +}
> +
> +function initContentMessageListener() {
> +  Services.mm.addMessageListener("Onboarding:OnContentMessage", msg => {

No, OnContentMessage is correct, See comment 20.
Attachment #8875681 - Flags: review?(dtownsend) → review+
Comment on attachment 8875681 [details]
Bug 1357020 - Should hide the onboarding tour if user explicitly checked the hide-the-tour checkbox,

https://reviewboard.mozilla.org/r/147110/#review153804
Attachment #8875681 - Flags: review?(gasolin) → review+
(In reply to Dave Townsend [:mossop] from comment #37)
> (In reply to Fischer [:Fischer] from comment #34)
> > Comment on attachment 8875681 [details]
> > Bug 1357020 - Should hide the onboarding tour if user explicitly checked the
> > hide-the-tour checkbox,
> > 
> > https://reviewboard.mozilla.org/r/147110/#review153112
> > 
> > > Can you use BrowserTestUtils.synthesizeMouseAtCenter here?
> > 
> > It seems not. If calling `BrowserTestUtils.synthesizeMouseAtCenter` inside
> > the spawned content task, it would result error of finding no
> > `BrowserTestUtils`.
> > Please let us know if there is a better way, thanks
> 
> You don't need the spawned content task at all, you just call
> BrowserTestUtils.synthesizeMouseAtCenter from the parent process, it handles pawning a content task to do the work.
Thanks for this advise. The test was updated and the try results are good.
TRY: https://treeherder.mozilla.org/#/jobs?repo=try&revision=9f027a169c76eff6101212b2f153779c9f35ae34
(In reply to KM Lee [:rexboy] from comment #35)
> ::: browser/extensions/onboarding/content/onboarding.js:183
> (Diff revision 5)
> > -    return;
> > +      return;
> > -  }
> > +    }
> > -  removeEventListener("load", onLoad);
> > +    removeEventListener("load", onLoad);
> >  
> > -  let window = evt.target.defaultView;
> > +    let window = evt.target.defaultView;
> > -  // Load onboarding module only when we enable it.
> > +    let location = window.location.href
> 
> add ; at the end of line
> 
Thanks for spotting this. updated.

> :::
> browser/extensions/onboarding/test/browser/browser_onboarding_hide_tours.js:
> 40
> (Diff revision 5)
> > +  ];
> > +  await promiseClickElement(hometab.linkedBrowser, "#onboarding-overlay-icon");
> > +  await promiseOnboardingOverlayOpened(hometab.linkedBrowser);
> > +  await promiseClickElement(hometab.linkedBrowser, "#onboarding-tour-hidden-checkbox");
> > +  await promiseClickElement(hometab.linkedBrowser, "#onboarding-overlay-close-btn");
> > +  await Promise.all(expectedPrefUpdates);
> 
> I'm not very sure but can we guarantee the observer in promisePrefUpdated is called after the observer in Onboarding.js?
Per the offline discussion, the order is not guaranteed. However, the following check of `assertOnboardingDestroyed` would be scheduled behind the promise resolving and as well as behind the spawned content task, so it should happen quite late.
The test results in comment 33 and comment 26 are passed.
I also triggered 10x try runs across Linux, Mac, Window (see comment 41), the results are good as well.
Thank you
Comment on attachment 8875681 [details]
Bug 1357020 - Should hide the onboarding tour if user explicitly checked the hide-the-tour checkbox,

https://reviewboard.mozilla.org/r/147110/#review153810

Looks good to me. Thanks for the work!
Attachment #8875681 - Flags: review?(rexboy) → review+
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 dbad771a36de -d 8c18e90f0774: rebasing 401866:dbad771a36de "Bug 1357020 - Should hide the onboarding tour if user explicitly checked the hide-the-tour checkbox, r=gasolin,mossop,rexboy" (tip)
merging browser/app/profile/firefox.js
merging browser/extensions/onboarding/content/onboarding.css
merging browser/extensions/onboarding/content/onboarding.js
merging browser/extensions/onboarding/locales/en-US/onboarding.properties
warning: conflicts while merging browser/extensions/onboarding/content/onboarding.css! (edit, then use 'hg resolve --mark')
warning: conflicts while merging browser/extensions/onboarding/content/onboarding.js! (edit, then use 'hg resolve --mark')
warning: conflicts while merging browser/extensions/onboarding/locales/en-US/onboarding.properties! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)
(In reply to Fischer [:Fischer] from comment #45)
> Comment on attachment 8875681 [details]
> Bug 1357020 - Should hide the onboarding tour if user explicitly checked the
> hide-the-tour checkbox,
> 
> Review request updated; see interdiff:
> https://reviewboard.mozilla.org/r/147110/diff/6-7/
TRY with rebased on Autoland: https://treeherder.mozilla.org/#/jobs?repo=try&revision=896b3da767d99719fcbdc0b7d53e04533d29e91e
Pushed by rchien@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/985bc548d68b
Should hide the onboarding tour if user explicitly checked the hide-the-tour checkbox, r=gasolin,mossop,rexboy
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/985bc548d68b
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox56: --- → fixed
Resolution: --- → FIXED
I have tested this feature in latest nightly 56.0a1 on Deepin OS 15.4 (64 bit).
It successfully hides the onboarding tour if user explicitly checks the hide-the-tour checkbox.

But I found some other problems:

1.

(In reply to Verdi [:verdi] from comment #1)
> user can complete all of the tour steps (has a checkmark for all six items) 
> and the Fox icon will remain on the new tab
> page so that they can revisit the tour when they want. 

But, there are only five items.

2.

(In reply to Verdi [:verdi] from comment #1)
> the tour is only "complete" when the user
> marks it as complete.

But there's no option to mark it as complete. 


Build ID 	20170616100254
User Agent 	Mozilla/5.0 (X11; Linux x86_64; rv:56.0) Gecko/20100101 Firefox/56.0

[bugday-20170614]
The issue is no longer reproducible on Firefox 56.0a1. Tests were performed under Windows 10x64.
Once the checkbox is activated it hides the tour. I tried with not syncing with any profile. After it hides the tour, I again tried syncing with my profile but now also its in hide mode in both about:newtab and about:home. 
Is there any option to get back the tour on about:newtab and about:home?
Does this happens intentionally?

Build ID: 20170628030206
[bugday-20170628]
(In reply to Fahima Zulfath from comment #50)
> The issue is no longer reproducible on Firefox 56.0a1. Tests were performed
> under Windows 10x64.
> Once the checkbox is activated it hides the tour. I tried with not syncing
> with any profile. After it hides the tour, I again tried syncing with my
> profile but now also its in hide mode in both about:newtab and about:home. 
> Is there any option to get back the tour on about:newtab and about:home?
> Does this happens intentionally?
> 
> Build ID: 20170628030206
> [bugday-20170628]
Open about:config and set the below prefs:
- "browser.onboarding.notification.finished" = false
- "browser.onboarding.hidden" = false

Then reopen about:newtab or about:home.
You should find the onboarding tours back
(In reply to Meraj Kazi from comment #49)
> I have tested this feature in latest nightly 56.0a1 on Deepin OS 15.4 (64
> bit).
> It successfully hides the onboarding tour if user explicitly checks the
> hide-the-tour checkbox.
> 
> But I found some other problems:
> 
> 1.
> 
> (In reply to Verdi [:verdi] from comment #1)
> > user can complete all of the tour steps (has a checkmark for all six items) 
> > and the Fox icon will remain on the new tab
> > page so that they can revisit the tour when they want. 
> 
> But, there are only five items.
> 
The 6th tour is done in Bug 1357023 which will land today.

> 2.
> 
> (In reply to Verdi [:verdi] from comment #1)
> > the tour is only "complete" when the user
> > marks it as complete.
> 
> But there's no option to mark it as complete. 
> 
This is done in Bug 1357021 which will land today or tomorrow.
Target Milestone: Firefox 57 → Firefox 56
This issue has been verified on Win 10 x64.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: