1074932 - Desktop client user can access product tour from gears menu

Status: VERIFIED FIXED

(Hello (Loop) :: Client, defect, P1)

Comment 1

[u279076]

Are we planning to ship UI Tour in 34? If so, I think this should block uplift.

Comment 2

[Romain Testard [:RT]]

No, in Fx35, marking as a Fx35 item.

Comment 3

[Matthew N. [:MattN]]

I don't see how this is related to rooms. Can we remove the whiteboard and blocking bug?

Comment 4

[Matthew N. [:MattN]]

We will likely want the URL to be slightly different (e.g. an additional query parameter) than the one launched from the "Get Started" button.

Comment 5

[Romain Testard [:RT]]

(In reply to Matthew N. [:MattN] (behind on reviews - focused on Loop) from comment #3)
> I don't see how this is related to rooms. Can we remove the whiteboard and
> blocking bug?
This is in scope for the Fx35 release, I thought the point of the [rooms] tag is to identify the bugs released with the initial rooms release in Fx35.
Am I getting this wrong Shell?

Comment 6

[Matthew N. [:MattN]]

(In reply to Romain Testard [:RT] from comment #5)
> This is in scope for the Fx35 release, I thought the point of the [rooms]
> tag is to identify the bugs released with the initial rooms release in Fx35.

OK, I wasn't aware of that and it's non-obvious. We already have "blocking-loop: Fx35", the target milestone, and status-firefox35: affected which more clearly say "Firefox 35" so I wish we wouldn't invent new ways to mark bugs.

Comment 7

[Maire Reavy [:mreavy]]

To clarify the blocking-loop Fx35+ flag determines the targeted release. "[rooms]" in the whiteboard is specifically for bugs that are needed for the Rooms feature.

Comment 8

[Jared Wein [:jaws] (please needinfo? me)]

Hey Marco, can you add this to the current iteration?

Comment 9

[Marco Mucci [:MarcoM]]

Added to IT 36.3

Comment 10

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Patch

Cory, what kind of extra URL parameter do you want added to the patch to track that it came from the Gear menu? I've appended ?source=settingsMenu to the URL when it comes from the settings/gear menu. Is that enough for you? Do you have a specification/requirement for the names?

Michael, do you have an icon for this menu item? Right now it is the only menu item in the settings menu without an icon. The other ones are Sign in/Sign out and My Account (http://screencast.com/t/e6FANNf2MiNT)

Comment 11

[Cory Price [:ckprice] (bugmail disabled, NI me!)]

Hi Jared,

Question: is it an option to have multiple parameters appended to the URL?

e.g. ?utm_campaign=abc&utm_source=def&utm_medium=ghi

Thanks

Comment 12

[Matthew N. [:MattN]]

Comment on attachment 8524846 [details] [diff] [review]
Patch

Review of attachment 8524846 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with fixes

::: browser/components/loop/MozLoopService.jsm
@@ +1412,2 @@
>     */
> +  openGettingStartedTour: Task.async(function(aSrc) {

Denote that it's optional by providing the default value in the argument list e.g.:
aSrc = null

@@ +1414,4 @@
>      try {
>        let url = Services.prefs.getCharPref("loop.gettingStarted.url");
> +      if (aSrc) {
> +        url += (url.contains("?") ? "&" : "?") + "source=" + aSrc;

It would be good to just use a DOM URL object here instead of implementing it ourselves.
url.searchParams.set("source", aSrc) is easier to read IMO.

::: browser/components/loop/content/js/panel.jsx
@@ +287,5 @@
>      },
>  
> +    openGettingStartedTour: function() {
> +      navigator.mozLoop.openGettingStartedTour("settingsMenu");
> +      this.hideDropdownMenu();

Shouldn't hideDropdownMenu be instead called upon the click of every SettingsDropdownEntry? I mean that it can probably be done inside the SettingsDropdownEntry implementation.

@@ +314,5 @@
>                                     icon="account"
>                                     displayed={this._isSignedIn()} />
> +            <SettingsDropdownEntry label={mozL10n.get("tour_label")}
> +                                   onClick={this.openGettingStartedTour}
> +                                   displayed={true} />

I believe @displayed isn't necessary

Comment 13

[Matthew N. [:MattN]]

Comment on attachment 8524846 [details] [diff] [review]
Patch

Review of attachment 8524846 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with fixes

::: browser/components/loop/MozLoopService.jsm
@@ +1412,2 @@
>     */
> +  openGettingStartedTour: Task.async(function(aSrc) {

Denote that it's optional by providing the default value in the argument list e.g.:
aSrc = null

@@ +1414,4 @@
>      try {
>        let url = Services.prefs.getCharPref("loop.gettingStarted.url");
> +      if (aSrc) {
> +        url += (url.contains("?") ? "&" : "?") + "source=" + aSrc;

It would be good to just use a DOM URL object here instead of implementing it ourselves.
url.searchParams.set("source", aSrc) is easier to read IMO.

::: browser/components/loop/content/js/panel.jsx
@@ +287,5 @@
>      },
>  
> +    openGettingStartedTour: function() {
> +      navigator.mozLoop.openGettingStartedTour("settingsMenu");
> +      this.hideDropdownMenu();

Shouldn't hideDropdownMenu be instead called upon the click of every SettingsDropdownEntry? I mean that it can probably be done inside the SettingsDropdownEntry implementation.

@@ +314,5 @@
>                                     icon="account"
>                                     displayed={this._isSignedIn()} />
> +            <SettingsDropdownEntry label={mozL10n.get("tour_label")}
> +                                   onClick={this.openGettingStartedTour}
> +                                   displayed={true} />

I believe @displayed isn't necessary

Comment 14

[Jared Wein [:jaws] (please needinfo? me)]

(In reply to Cory Price [:ckprice] from comment #11)
> Question: is it an option to have multiple parameters appended to the URL?
> 
> e.g. ?utm_campaign=abc&utm_source=def&utm_medium=ghi

Yeah, we can put multiple parameters on it. What are you looking for? Should this be handled in a follow-up bug (in other words, will this need more discussions)?

Comment 15

[Cory Price [:ckprice] (bugmail disabled, NI me!)]

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #14)
> Yeah, we can put multiple parameters on it. What are you looking for? Should
> this be handled in a follow-up bug (in other words, will this need more
> discussions)?

Yeah, if this bug is ready, please feel free to close with placeholder parameters.

I've opened bug 1101286 to track the URL parameter. We should have both URL questions answered in the next couple of days.

Comment 16

[Jared Wein [:jaws] (please needinfo? me)]

(In reply to Matthew N. [:MattN] (UTC+1 until Nov. 22; away Nov. 14-19) from comment #13)
> > +    openGettingStartedTour: function() {
> > +      navigator.mozLoop.openGettingStartedTour("settingsMenu");
> > +      this.hideDropdownMenu();
> 
> Shouldn't hideDropdownMenu be instead called upon the click of every
> SettingsDropdownEntry? I mean that it can probably be done inside the
> SettingsDropdownEntry implementation.

This was here in error, as I had accidentally started working on this in the availability dropdown menu code. I didn't notice this remnant since it was called at the end of the event handler and all of the behavior matched the other menuitems. With more digging I filed bug 1101705, since none of the menuitems close the panel.

Thanks for the review, landed as https://hg.mozilla.org/integration/fx-team/rev/5f82b789efa3

Comment 17

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/5f82b789efa3

Comment 18

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 8524846 [details] [diff] [review]
Patch

Approval Request Comment
[Feature/regressing bug #]: N/A
[User impact if declined]: Hard to re-access First Time Experience tour
[Describe test coverage new/current, TBPL]: on m-c
[String/UUID change made/needed]: none

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/7ca685a7deb7

Comment 20

[Paul Silaghi, QA [:pauly]]

In FF 35b5, the 'Tour' gear menu button has no icon.
In FF 36, 37 it has the icon.
Thoughts ?

Comment 21

[Randell Jesup [:jesup] (needinfo me)]

(In reply to Paul Silaghi, QA [:pauly] from comment #20)
> In FF 35b5, the 'Tour' gear menu button has no icon.
> In FF 36, 37 it has the icon.
> Thoughts ?

That's bug 1106991, which I'm about to land on beta in a few minutes.

Comment 22

[Paul Silaghi, QA [:pauly]]

Verified fixed FF 35b5, 36.0a2(2014-12-19) Win 7.
Note that the 'Help' button from the gear menu opens the same http://mzl.la/1p3JqIy link.