1177162 - Show an info panel on the tracking protection doorhanger anchor the first time tracking elements are blocked

Status: VERIFIED FIXED

(Firefox :: Tours, defect, P1)

Description

[Matthew N. [:MattN]]

The first time tracking protection blocks an element, show an info panel explaining what the feature is about and provide a link/button to start the tour.

Comment 1

[Matthew N. [:MattN]]

We can keep track whether the user has already seen this panel in a preference. For privacy reasons, is it okay to store this information in a pref for the initial launch in Private Browsing. If we don't store this somewhere, the panel will be quite annoying in every new PB session.

Comment 2

[Matthew N. [:MattN]]

I will work on this but will need in-product strings since this appears before the www.mozilla.org tour.

Comment 3

[Matthew N. [:MattN]]

Attachment: MozReview Request: Bug 1177162 - Show an info panel on the tracking protection doorhanger anchor the first time tracking elements are blocked. r=bgrins,ttaubert

Bug 1177162 - WIP: Show an info panel on the tracking protection doorhanger anchor the first time tracking elements are blocked.

Comment 4

[Matthew N. [:MattN]]

Alex/Cory, what should the URL for the tracking protection FTU tour be?
Example:
https://www.mozilla.org/%LOCALE%/firefox/%VERSION%/trackingprotection/tour/#step2

#step2 would be appended when going from step 1B directly to step 2. See http://bit.ly/1KQ3v5e

We may want to add utm arguments to track the difference in engagement between the 2 entry points (though I don't think the page will have Google Analytics as the feature is to reduce tracking).

Comment 5

[Matthew N. [:MattN]]

Comment on attachment 8630884 [details]
MozReview Request: Bug 1177162 - Show an info panel on the tracking protection doorhanger anchor the first time tracking elements are blocked. r=bgrins,ttaubert

Bug 1177162 - WIP: Show an info panel on the tracking protection doorhanger anchor the first time tracking elements are blocked.

Comment 6

[Alex Gibson [:agibson]]

(In reply to Matthew N. [:MattN] from comment #4)
> Alex/Cory, what should the URL for the tracking protection FTU tour be?
> Example:
> https://www.mozilla.org/%LOCALE%/firefox/%VERSION%/trackingprotection/tour/
> #step2
> 
> #step2 would be appended when going from step 1B directly to step 2. See
> http://bit.ly/1KQ3v5e
> 
> We may want to add utm arguments to track the difference in engagement
> between the 2 entry points (though I don't think the page will have Google
> Analytics as the feature is to reduce tracking).

A couple of questions re the tour URL:

- Do we envisage this is something that we will need to update as time goes on (e.g. like the Hello TFU). If so, I think we should definitely include the version number in the URL like suggested.

- do we need /trackingprotection/tour/, or could we just use /trackingprotection/?

The bit.ly URL doesn't seem to work for me, but I imagine adding a hash to the URL should pose any problem (I don't have access to these wireframes yet, so can't really remember how everything is set to work sorry).

Cory, any thoughts on the URL proposed? Sounds ok to me other than the couple of questions above.

Comment 7

[Matthew N. [:MattN]]

(In reply to Alex Gibson [:agibson] from comment #6)
> (In reply to Matthew N. [:MattN] from comment #4)
> > Alex/Cory, what should the URL for the tracking protection FTU tour be?
> > Example:
> > https://www.mozilla.org/%LOCALE%/firefox/%VERSION%/trackingprotection/tour/
> > #step2
> > 
> > #step2 would be appended when going from step 1B directly to step 2. See
> > http://bit.ly/1KQ3v5e
> > 
> > We may want to add utm arguments to track the difference in engagement
> > between the 2 entry points (though I don't think the page will have Google
> > Analytics as the feature is to reduce tracking).
> 
> A couple of questions re the tour URL:
> 
> - Do we envisage this is something that we will need to update as time goes
> on (e.g. like the Hello TFU). If so, I think we should definitely include
> the version number in the URL like suggested.

I think it's possible we will want to update it e.g. if we add additional tracking protection lists in the future or we make it easier to enable it outside of PB. I figured it's probably not much more work to handle the versions since we normally do that.

> - do we need /trackingprotection/tour/, or could we just use
> /trackingprotection/?

I don't think this tour works as a landing/product/marketing page so that's why I figured we would want some subdirectory for it.

> The bit.ly URL doesn't seem to work for me, but I imagine adding a hash to
> the URL should pose any problem (I don't have access to these wireframes
> yet, so can't really remember how everything is set to work sorry).

Try this: https://www.dropbox.com/s/gob4upfg6n2gku2/Tp%20Onboarding%20Spec%20v3.png?dl=0

Comment 8

[Matthew N. [:MattN]]

Comment on attachment 8630884 [details]
MozReview Request: Bug 1177162 - Show an info panel on the tracking protection doorhanger anchor the first time tracking elements are blocked. r=bgrins,ttaubert

Bug 1177162 - WIP: Show an info panel on the tracking protection doorhanger anchor the first time tracking elements are blocked.

Comment 9

[Matthew N. [:MattN]]

Comment on attachment 8630884 [details]
MozReview Request: Bug 1177162 - Show an info panel on the tracking protection doorhanger anchor the first time tracking elements are blocked. r=bgrins,ttaubert

Bug 1177162 - Show an info panel on the tracking protection doorhanger anchor the first time tracking elements are blocked. r=bgrins,ttaubert

Comment 10

[Matthew N. [:MattN]]

Attachment: MozReview Request: Bug 1177162 - Don't delete the UITour variable from the browser window as it breaks later tests. r=bgrins

Bug 1177162 - Don't delete the UITour variable from the browser window as it breaks later tests. r=bgrins

Comment 11

[Matthew N. [:MattN]]

https://reviewboard.mozilla.org/r/13037/#review11627

::: browser/components/uitour/test/browser_openSearchPanel.js
(Diff revision 1)
> -Components.utils.import("resource:///modules/UITour.jsm");
> -Components.utils.import("resource://gre/modules/Services.jsm");

UITour.jsm was already lazily loaded in head.js and the import this way leaks the property.

Services.jsm was already in scope by default.

::: browser/components/uitour/test/head.js
(Diff revision 1)
> -    delete window.UITour;
> -    delete window.UITourMetricsProvider;

We don't need to delete these since they won't show as leaks anymore.

Comment 12

[Matthew N. [:MattN]]

https://reviewboard.mozilla.org/r/12809/#review11631

::: browser/components/uitour/UITour.jsm:509
(Diff revision 4)
> -                  callbackID: buttonData.callbackID,
> +                  callback: event => {
> +                    this.sendPageCallback(messageManager, callback);
> +                  },

Here I'm making it easier for internal brower consumers to use UITour.showInfo by doing the callbackID => JS function conversion before calling showInfo. This way internal consumers don't have to do weird workarounds with callback IDs and the message manager.

::: browser/components/uitour/UITour.jsm:741
(Diff revision 4)
> +  initForBrowser(aBrowser) {
> +    let window = aBrowser.ownerDocument.defaultView;
> +
> +    window.gBrowser.tabContainer.addEventListener("TabSelect", this);

This is needed so UITour properly tears down e.g. for TabSelect when not driven by a content webpage.

::: browser/components/uitour/UITour.jsm:1413
(Diff revision 4)
>        for (let button of aButtons) {
> -        let el = document.createElement("button");
> -        el.setAttribute("label", button.label);
> +        let isButton = button.style == "text" ? false : true;
> +        let el = document.createElement(isButton ? "button" : "label");
> +        el.setAttribute(isButton ? "label" : "value", button.label);

This adds support for `button.style = "text"` which isn't clickable and doesn't support images. It's used for the "1 of N" text.

Comment 13

[Brian Grinstead [:bgrins]]

Comment on attachment 8632322 [details]
MozReview Request: Bug 1177162 - Don't delete the UITour variable from the browser window as it breaks later tests. r=bgrins

https://reviewboard.mozilla.org/r/13037/#review11633

Ship It!

Comment 14

[Brian Grinstead [:bgrins]]

Comment on attachment 8630884 [details]
MozReview Request: Bug 1177162 - Show an info panel on the tracking protection doorhanger anchor the first time tracking elements are blocked. r=bgrins,ttaubert

https://reviewboard.mozilla.org/r/12809/#review11629

Just did a quick pass through, mostly focused on the browser.js and browser-trackingprotection.js changes.  I can take a closer look next week

::: browser/base/content/browser.js:6872
(Diff revision 4)
> +        gPrefService.savePrefFile(null);

Why would this pref need to be saved even in the case of an abrupt shutdown? I see a lot of prefs being set with gPrefService in this file and only one other calls savePrefFile, but it has a comment explaining that it's related to the shutdown somehow so it needs to be persisted no matter what.

::: browser/base/content/browser-trackingprotection.js:122
(Diff revision 4)
> +      gPrefService.savePrefFile(null);

Same comment about savePrefFile as below

::: browser/base/content/browser-trackingprotection.js:6
(Diff revision 4)
> +  MAX_INTROS: 1,

Maybe when we land this stuff (but before steps 2 and beyond are ready) we should temporarily set MAX_INTROS to 0 so people don't see a halfway working tour and it will still show up for them once we fully enable things

::: browser/base/content/browser-trackingprotection.js:112
(Diff revision 4)
> +  showIntroPanel: Task.async(function* TrackingProtection_showIntroPanel() {

Nit: the `TrackingProtection_showIntroPanel` name is no longer needed for debugging (devtools are now smart enough to infer `showIntroPanel` as the function name.

So this is enough:

showIntroPanel: Task.async(function*() { });

::: browser/base/content/browser.js:6867
(Diff revision 4)
> +    // Open the tracking protection introduction panel, if applicable, after showing the anchor.

I'm guessing we only want to show this when the following things are true?

TrackingProtection.enabledInPrivateWindows && PrivateBrowsingUtils.isWindowPrivate(window)

As in, we don't want to show the tour if TP is disabled or enabled but in a non-private win.  If so we should probably extract that bit from the TP.enabled getter into a new getter on TP and then use it in both places.

::: browser/base/content/browser.js:6866
(Diff revision 4)
> +

I feel that we should (at least eventually) move this logic out of gIdentityHandler and directly into TrackingProtection.onSecurityChange.  It's a little tied up in the progress of Bugs 1175702 and 1175689 but after those are done no shield will be triggered by gIdentityHandler.showBadContentDoorhanger and will rather be shown based on an attribute on TrackingProtection.icon which is updated in onSecurityChange (if you look at the WIP patch in Bug 1175689 you will see what I mean).   I'm also interested in Tim's and your opinion about that, but it seems like that's the right place to put this logic.

Comment 15

[Matthew N. [:MattN]]

https://reviewboard.mozilla.org/r/12809/#review11629

> I'm guessing we only want to show this when the following things are true?
> 
> TrackingProtection.enabledInPrivateWindows && PrivateBrowsingUtils.isWindowPrivate(window)
> 
> As in, we don't want to show the tour if TP is disabled or enabled but in a non-private win.  If so we should probably extract that bit from the TP.enabled getter into a new getter on TP and then use it in both places.

This tour panel isn't related to PB and it's helpful for people who manually flip the pref so it was intentional that I did this. It also adds some plausible deniability so that the intro pref being set doesn't necessarily imply that PB was used in the profile. We're still discussing the privacy aspect via email.

> Why would this pref need to be saved even in the case of an abrupt shutdown? I see a lot of prefs being set with gPrefService in this file and only one other calls savePrefFile, but it has a comment explaining that it's related to the shutdown somehow so it needs to be persisted no matter what.

It's to avoid constant nagging for people who have shutdown hangs or crashes and nothing else in the session saving prefs. https://groups.google.com/d/topic/mozilla.dev.platform/XYSCvVypY7s/discussion

> Nit: the `TrackingProtection_showIntroPanel` name is no longer needed for debugging (devtools are now smart enough to infer `showIntroPanel` as the function name.
> 
> So this is enough:
> 
> showIntroPanel: Task.async(function*() { });

That's more magic than I expected since there is a Task.async in between the function* and the property. Are you sure about this?

> Maybe when we land this stuff (but before steps 2 and beyond are ready) we should temporarily set MAX_INTROS to 0 so people don't see a halfway working tour and it will still show up for them once we fully enable things

I'm pointing to the SUMO page so we don't need to delay testing of this panel. I guess there could be some confusion about the "1 of 3" portion but I would rather hide that for now than not get any panel testing on Nightly and Aurora. Tour development normally doesn't happen until Beta.

Comment 16

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/339a04f0e082

Comment 17

[Brian Grinstead [:bgrins]]

(In reply to Matthew N. [:MattN] from comment #15)
> > Nit: the `TrackingProtection_showIntroPanel` name is no longer needed for debugging (devtools are now smart enough to infer `showIntroPanel` as the function name.
> > 
> > So this is enough:
> > 
> > showIntroPanel: Task.async(function*() { });
> 
> That's more magic than I expected since there is a Task.async in between the
> function* and the property. Are you sure about this?

Yeah, I remember when this got fixed and just confirmed with fitzgen that this is the case.  The only time you would need to give it a name is if you want to reference the name from within the function scope.  Try it out -> add a console.trace() inside the function with and without the name and you'll see the difference.

Comment 18

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/339a04f0e082

Comment 19

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

(In reply to Alex Gibson [:agibson] from comment #6)
> Cory, any thoughts on the URL proposed? Sounds ok to me other than the
> couple of questions above.

* For Hello, we used 'start' as the subdirectory. We also included 'firefox' in the string. So: /lang/firefox/version/trackingprotection/start/

* Should we use any delimiter in the URL? e.g. tracking-protection

* Given the discussion today, I would assume no UTM parameters in the URL.

Comment 20

[Alex Gibson [:agibson]]

(In reply to Cory Price [:ckprice] from comment #19)
> * Should we use any delimiter in the URL? e.g. tracking-protection

We do use delimiters elsewhere, so makes sense to do it here too?

https://www.mozilla.org/%LOCALE%/firefox/%VERSION%/tracking-protection/start/#step2

It sounds like the second-entry point to the tour is a bit up in the air. Potentially we might not need the hash #step2 if it gets dropped?

Comment 21

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 8630884 [details]
MozReview Request: Bug 1177162 - Show an info panel on the tracking protection doorhanger anchor the first time tracking elements are blocked. r=bgrins,ttaubert

https://reviewboard.mozilla.org/r/12809/#review11975

The panel itself isn't quite wide enough so that the headline wraps. Is that something to be addressed in a follow-up?

::: browser/components/uitour/UITour.jsm:1414
(Diff revision 4)
> -        let el = document.createElement("button");
> +        let isButton = button.style == "text" ? false : true;

Maybe: let isButton = button.style != "text";

::: browser/base/content/browser-trackingprotection.js:6
(Diff revision 4)
> +  MAX_INTROS: 1,

We're not really landing the final panel though. Nightly folks wouldn't see that panel again when we update it and so we get no testing for that later version, right?

::: browser/base/content/browser.js:6866
(Diff revision 4)
> +

Yeah, totally agree with Brian. This method is hopefully going away soon and moving this code to TrackingProtection.onSecurityChange() is the right thing to do in my opinion too.

Comment 22

[Matthew N. [:MattN]]

https://reviewboard.mozilla.org/r/12809/#review11975

> We're not really landing the final panel though. Nightly folks wouldn't see that panel again when we update it and so we get no testing for that later version, right?

My idea was to bump the number again when the real tour is ready. That doesn't need to affect the number that we ship to later channels with. With the current privacy concerns I'm going to land with a value of 0 now instead until that's sorted out.

Comment 23

[Matthew N. [:MattN]]

https://reviewboard.mozilla.org/r/12809/#review11975

I would recommend that since there are bugs to change the margin/padding of the panel. I don't actually think it's a big problem though since it's going to wrap in some locales anyways.

Comment 24

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/628e80194537

Comment 25

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/142992f32215

Comment 26

[Ryan VanderMeulen [:RyanVM]]

Backed out for frequent Linux debug/ASAN browser_trackingProtection.js failures.
https://treeherder.mozilla.org/logviewer.html#?job_id=3782801&repo=fx-team

https://hg.mozilla.org/integration/fx-team/rev/a5b19d05fcdb

Comment 27

[Marco Mucci [:MarcoM]]

Temporarily removed from IT 42.2.  Work will continue for fixing test failures.  Decision next Tuesday will determine if removed from the Backlog.

Comment 28

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/d253511f5765

Comment 29

[Matthew N. [:MattN]]

Note to sheriffs: If the intermittent comes back (didn't happen with ~20 runs on try), please just disable the test on Linux instead of backing out.

Btw. I landed with a SUMO URL for now.

Comment 31

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/5ca20b9b0955

Comment 32

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/0de836e73be4

Comment 33

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/mozilla-central/rev/d253511f5765
https://hg.mozilla.org/mozilla-central/rev/5ca20b9b0955
https://hg.mozilla.org/mozilla-central/rev/0de836e73be4

Comment 34

[Matt Wobensmith [:mwobensmith][:matt:]]

Verified Fx42.0a1, 2015-07-30.

Comment 35

[Alex Gibson [:agibson]]

Matt, testing in the latest Nightly it seems that the target `trackingProtection` is always in the list of availableTargets when calling getConfiguration. This seems to be true irrespective of whether I have tracking protection enabled, or if trackers have been found and blocked on the page. Is this intentional?

Comment 36

[Alex Gibson [:agibson]]

Clearing old needsinfo