1462470 - Update Tracking Protection section in the identity popup

Status: VERIFIED FIXED

(Firefox :: Site Identity, enhancement, P1)

Description

[Johann Hofmann [:johannh]]

We're planning some incremental updates to the tracking protection section, the first iteration can be seen here: https://mozilla.invisionapp.com/share/RSIY1B8GMC2#/screens/297824891

Comment 1

[Johann Hofmann [:johannh]]

Attachment: Bug 1462470 - Part 1 - Notify frontend of loaded tracking content in annotation-only mode.

In order to notify the user that there is tracking content on the page
which could have been blocked by Tracking Protection, we need to mark
the document as having tracking content even when TP is off.

Part 2 will update the desktop frontend to handle this correctly and
show a different message when TP is on vs. off. Fennec should not be
affected by this change since it already prevents loaded state from
showing if TP is disabled:

https://searchfox.org/mozilla-central/rev/dc6d85680539cb7e5fe2843d38a30a0581bfefe1/mobile/android/chrome/content/browser.js#5669
https://searchfox.org/mozilla-central/rev/dc6d85680539cb7e5fe2843d38a30a0581bfefe1/mobile/android/modules/geckoview/GeckoViewProgress.jsm#112

Review commit: https://reviewboard.mozilla.org/r/246402/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/246402/

Comment 2

[Johann Hofmann [:johannh]]

Attachment: Bug 1462470 - Part 2 - Alert about trackers when Tracking Protection is off in the identity popup.

This commit changes the tracking protection UI to always be present in the
identity popup (control center) UI. Following the UI spec in
https://mozilla.invisionapp.com/share/RSIY1B8GMC2#/screens/297824891,
we are now warning about trackers even when TP is disabled and alter the
button that shows up in this case to open the TP settings in about:preferences.

Review commit: https://reviewboard.mozilla.org/r/246404/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/246404/

Comment 3

[Johann Hofmann [:johannh]]

Attachment: Bug 1462470 - Part 3 - Show a "reload this page" warning in the control center when trackers are loaded while TP is on.

Part 2 exposed an edge case that would allow the user to have TP enabled but
still have tracking content on a page, bringing our UI into a confused state.

This adds a new element that shows when TP is enabled and no exception is set,
but tracking content is still loaded, and recommends reloading the page.

Review commit: https://reviewboard.mozilla.org/r/246406/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/246406/

Comment 4

[Johann Hofmann [:johannh]]

This had a lot more edge cases to cover than I'd expected at first. This is a first attempt at solving all of them. I haven't added tests yet, I'll push more commits later, I wanted to get this out first to be able to collect some feedback and see if we have to make any fundamental changes.

Paolo, do you have time to take a look at this? If not let me know and I'll ask someone else :)

(I know I still owe you two reviews which I'm going to get at shortly)

Thanks!

Comment 5

[Johann Hofmann [:johannh]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=024cb48478c0b7741b945da5af028d0e629fc8c4

Comment 6

[:Paolo Amadini]

Comment on attachment 8980242 [details]
Bug 1462470 - Part 2 - Alert about trackers when Tracking Protection is off in the identity popup.

https://reviewboard.mozilla.org/r/246404/#review252910

This is a pretty straightforward implementation, looks good to me. The tests will help with ensuring that it does what it's expected to :-)

::: browser/base/content/browser-trackingprotection.js:18
(Diff revision 1)
>    content: null,
>    icon: null,
>    activeTooltipText: null,
>    disabledTooltipText: null,
>  
> +  get _normalizedCurrentURI() {

"normalized" seems like a misnomer. Maybe baseUriForChannelClassifier?

::: browser/locales/en-US/chrome/browser/browser.dtd:902
(Diff revision 1)
>  <!ENTITY getUserMedia.allWindowsShared.message "All visible windows on your screen will be shared.">
>  
> -<!ENTITY trackingProtection.title "Tracking Protection">
> +<!ENTITY trackingProtection.on "Tracking Protection: ON">
> +<!ENTITY trackingProtection.off "Tracking Protection: OFF">
>  <!ENTITY trackingProtection.detectedBlocked3 "&brandShortName; is blocking parts of the page that may track your browsing.">
> -<!ENTITY trackingProtection.detectedNotBlocked3 "This site includes elements that may track your browsing. You have disabled protection.">
> +<!ENTITY trackingProtection.detectedNotBlocked4 "Several trackers have been detected on this webpage.">

Also this may be a UI issue, but the word "several" might not be correct...

Comment 7

[:Paolo Amadini]

Comment on attachment 8980243 [details]
Bug 1462470 - Part 3 - Show a "reload this page" warning in the control center when trackers are loaded while TP is on.

https://reviewboard.mozilla.org/r/246406/#review252914

Comment 8

[(no longer active)]

Comment on attachment 8980241 [details]
Bug 1462470 - Part 1 - Notify frontend of loaded tracking content in annotation-only mode.

https://reviewboard.mozilla.org/r/246402/#review253730

Comment 9

[Johann Hofmann [:johannh]]

Comment on attachment 8980242 [details]
Bug 1462470 - Part 2 - Alert about trackers when Tracking Protection is off in the identity popup.

https://reviewboard.mozilla.org/r/246404/#review252910

> Also this may be a UI issue, but the word "several" might not be correct...

I agree that it's a bit weird when it's incorrect. I dropped the "several" for now and will talk to Bryan about it. I think this copy might also not be super final yet.

Comment 10

[Johann Hofmann [:johannh]]

Attachment: Bug 1462470 - Part 4 - Update TP shield icon in the control center.

Review commit: https://reviewboard.mozilla.org/r/248286/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/248286/

Comment 11

[Johann Hofmann [:johannh]]

Attachment: Bug 1462470 - Part 5 - Show a preferences button in the Tracking Protection section of the identity popup.

Review commit: https://reviewboard.mozilla.org/r/248288/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/248288/

Comment 12

[Johann Hofmann [:johannh]]

Attachment: Bug 1462470 - Part 6 - Add tests for the updated tracking protection UI in the identity popup.

This adds tests and moves the browser_trackingUI_1 test to the more general
trackingUI directory. The remaining TP tests will be moved in bug 1451307.

Review commit: https://reviewboard.mozilla.org/r/248290/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/248290/

Comment 13

[Johann Hofmann [:johannh]]

Comment on attachment 8980241 [details]
Bug 1462470 - Part 1 - Notify frontend of loaded tracking content in annotation-only mode.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/246402/diff/1-2/

Comment 14

[Johann Hofmann [:johannh]]

Comment on attachment 8980242 [details]
Bug 1462470 - Part 2 - Alert about trackers when Tracking Protection is off in the identity popup.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/246404/diff/1-2/

Comment 15

[Johann Hofmann [:johannh]]

Comment on attachment 8980243 [details]
Bug 1462470 - Part 3 - Show a "reload this page" warning in the control center when trackers are loaded while TP is on.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/246406/diff/1-2/

Comment 16

[Johann Hofmann [:johannh]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=be5849ddc3d48cca0fd9fabffcdc1399a75ff913

Comment 17

[:Paolo Amadini]

During some initial testing I noticed that step 3 of the UI Tour still refers to the button as "Disable protection for this site". I guess this string is set by the "mozilla.org" site, and it should either be made version-dependent or the button text should be removed from it. Is there a bug on file for this?

Comment 18

[Johann Hofmann [:johannh]]

(In reply to :Paolo Amadini from comment #17)
> During some initial testing I noticed that step 3 of the UI Tour still
> refers to the button as "Disable protection for this site". I guess this
> string is set by the "mozilla.org" site, and it should either be made
> version-dependent or the button text should be removed from it. Is there a
> bug on file for this?

Thanks for noticing this, filed bug 1466235!

Comment 19

[:Paolo Amadini]

Comment on attachment 8982340 [details]
Bug 1462470 - Part 6 - Add tests for the updated tracking protection UI in the identity popup.

https://reviewboard.mozilla.org/r/248290/#review255068

::: browser/base/content/test/trackingUI/browser_trackingUI_state.js:145
(Diff revision 1)
> +  // We can't use BENIGN_PAGE here because, in mochitest,
> +  // the third party URI check done for subresources unfortunately
> +  // does not work. example.com does not load any subresources.
> +  await promiseTabLoadEvent(tab, "http://example.com/");

I don't understand the issue entirely, but is this something that will eventually be fixed? If so, you could reference a bug number here.

::: browser/base/content/test/trackingUI/browser_trackingUI_open_preferences.js:17
(Diff revision 1)
> +  let display = win.getComputedStyle(el).getPropertyValue("display", null);
> +  let opacity = win.getComputedStyle(el).getPropertyValue("opacity", null);
> +  return display === "none" || opacity === "0";
> +}
> +
> +async function assertPreferencesShown() {

nit: waitAndAssertPreferencesShown

::: browser/base/content/test/trackingUI/browser_trackingUI_open_preferences.js:32
(Diff revision 1)
> +  });
> +
> +  BrowserTestUtils.removeTab(gBrowser.selectedTab);
> +}
> +
> +// Tests that pressing the  "Enable Tracking Protection" button in the

nit: double space

::: browser/base/content/test/trackingUI/browser_trackingUI_reload_hint.js:44
(Diff revision 1)
> +    let reloaded = BrowserTestUtils.browserLoaded(gBrowser.selectedBrowser, false, TRACKING_PAGE);
> +    reloadButton.click();
> +    await reloaded;

Manual testing showed that this doesn't close the site identity panel, while I think it should, like the other Tracking Protection buttons do. This can be tested here.

In fact, the panel height may be incorrect after the reload if it's not closed, because the interface changes but doesn't re-apply the description height workaround.

Comment 20

[:Paolo Amadini]

Comment on attachment 8982339 [details]
Bug 1462470 - Part 5 - Show a preferences button in the Tracking Protection section of the identity popup.

https://reviewboard.mozilla.org/r/248288/#review255074

::: browser/themes/shared/controlcenter/panel.inc.css:138
(Diff revision 1)
> +.identity-popup-preferences-button-container {
> +  display: flex;
> +  justify-content: space-between;
> +}

Can we easily use box layout instead of flex? In general, I'd prefer if we reduced the number of cases where we mix these display modes.

Comment 21

[:Paolo Amadini]

Comment on attachment 8982338 [details]
Bug 1462470 - Part 4 - Update TP shield icon in the control center.

https://reviewboard.mozilla.org/r/248286/#review255080

Comment 22

[:Paolo Amadini]

Comment on attachment 8980243 [details]
Bug 1462470 - Part 3 - Show a "reload this page" warning in the control center when trackers are loaded while TP is on.

https://reviewboard.mozilla.org/r/246406/#review255088

::: browser/components/controlcenter/content/panel.inc.xul:82
(Diff revision 2)
>  
> +          <button id="tracking-action-reload"
> +                  class="tracking-protection-button"
> +                  label="&trackingProtection.reload.label;"
> +                  accesskey="&trackingProtection.reload.accesskey;"
> +                  oncommand="BrowserReload();" />

You can add a method that calls BrowserReload and then closes the panel. No need to ask for review again.

Comment 23

[:Paolo Amadini]

Comment on attachment 8980242 [details]
Bug 1462470 - Part 2 - Alert about trackers when Tracking Protection is off in the identity popup.

https://reviewboard.mozilla.org/r/246404/#review255092

::: browser/themes/shared/controlcenter/panel.inc.css:307
(Diff revision 2)
> +}
> +
> +#tracking-protection-label-on,
> +#tracking-protection-content[enabled="false"] > #tracking-protection-label-off,
> +#tracking-protection-content[state="loaded-tracking-content"] > #tracking-protection-label-off {
> +  display: block;

Shouldn't this be "display: -moz-box;"?

::: toolkit/modules/PrivateBrowsingUtils.jsm:63
(Diff revision 2)
> +    let pbmtpWhitelist = Cc["@mozilla.org/pbm-tp-whitelist;1"]
> +                           .getService(Ci.nsIPrivateBrowsingTrackingProtectionWhitelist);

nit: may be a good time to refactor this to be a lazy service getter.

Comment 24

[:Paolo Amadini]

Comment on attachment 8980242 [details]
Bug 1462470 - Part 2 - Alert about trackers when Tracking Protection is off in the identity popup.

https://reviewboard.mozilla.org/r/246404/#review255096

Comment 25

[Johann Hofmann [:johannh]]

Comment on attachment 8980241 [details]
Bug 1462470 - Part 1 - Notify frontend of loaded tracking content in annotation-only mode.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/246402/diff/2-3/

Comment 26

[Johann Hofmann [:johannh]]

Comment on attachment 8980242 [details]
Bug 1462470 - Part 2 - Alert about trackers when Tracking Protection is off in the identity popup.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/246404/diff/2-3/

Comment 27

[Johann Hofmann [:johannh]]

Comment on attachment 8980243 [details]
Bug 1462470 - Part 3 - Show a "reload this page" warning in the control center when trackers are loaded while TP is on.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/246406/diff/2-3/

Comment 28

[Johann Hofmann [:johannh]]

Comment on attachment 8982338 [details]
Bug 1462470 - Part 4 - Update TP shield icon in the control center.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/248286/diff/1-2/

Comment 29

[Johann Hofmann [:johannh]]

Comment on attachment 8982339 [details]
Bug 1462470 - Part 5 - Show a preferences button in the Tracking Protection section of the identity popup.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/248288/diff/1-2/

Comment 30

[Johann Hofmann [:johannh]]

Comment on attachment 8982340 [details]
Bug 1462470 - Part 6 - Add tests for the updated tracking protection UI in the identity popup.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/248290/diff/1-2/

Comment 31

[Johann Hofmann [:johannh]]

(In reply to :Paolo Amadini from comment #19)
> Comment on attachment 8982340 [details]
> Bug 1462470 - Part 6 - Add tests for the updated tracking protection UI in
> the identity popup.
> 
> https://reviewboard.mozilla.org/r/248290/#review255068
> 
> ::: browser/base/content/test/trackingUI/browser_trackingUI_state.js:145
> (Diff revision 1)
> > +  // We can't use BENIGN_PAGE here because, in mochitest,
> > +  // the third party URI check done for subresources unfortunately
> > +  // does not work. example.com does not load any subresources.
> > +  await promiseTabLoadEvent(tab, "http://example.com/");
> 
> I don't understand the issue entirely, but is this something that will
> eventually be fixed? If so, you could reference a bug number here.

So while sitting on a beach and pondering this issue I had the realization that this does not only occur in mochitests (when debugging a mochitest I had previously assumed that this is happening because topWinURI is null here: https://searchfox.org/mozilla-central/source/netwerk/base/nsChannelClassifier.cpp#347), but rather that this is a symptom of a bug in my implementation. I noticed that "trackers blocked" notifications were sent for any page with subresources, even if it did not contain trackers. The reason is that I had put the NotifyTrackingProtectionDisabled in the wrong place. Correcting that makes this test work with BENIGN_PAGE and works great in my manual testing, too, which I think means success!

Going back to my last day of vacation now, with one less problem to keep my mind busy about.

Comment 32

[Johann Hofmann [:johannh]]

Comment on attachment 8980241 [details]
Bug 1462470 - Part 1 - Notify frontend of loaded tracking content in annotation-only mode.

Ehsan, since I moved this code to a different place, can you give it another quick sanity check? Thanks!

Comment 33

[Johann Hofmann [:johannh]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=23c723417b93a9ac691dd1c447d0086d3cae27e3

Comment 34

[(no longer active)]

Comment on attachment 8980241 [details]
Bug 1462470 - Part 1 - Notify frontend of loaded tracking content in annotation-only mode.

LGTM.  I couldn't figure out how to r+ this in MozReview, so marking this in Bugzilla instead, hope this is Doing The Right Thing...

Comment 35

[Johann Hofmann [:johannh]]

Thanks! Yeah, I couldn't figure out how to re-request in reviewboard and just asked in bugzilla... :)

Comment 36

[Johann Hofmann [:johannh]]

Comment on attachment 8982339 [details]
Bug 1462470 - Part 5 - Show a preferences button in the Tracking Protection section of the identity popup.

https://reviewboard.mozilla.org/r/248288/#review255074

> Can we easily use box layout instead of flex? In general, I'd prefer if we reduced the number of cases where we mix these display modes.

I tried, but I wouldn't know how to do this with box layout... :/

Comment 37

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e659c87f7267
Part 1 - Notify frontend of loaded tracking content in annotation-only mode. r=Ehsan
https://hg.mozilla.org/integration/autoland/rev/a242cf779e39
Part 2 - Alert about trackers when Tracking Protection is off in the identity popup. r=Paolo
https://hg.mozilla.org/integration/autoland/rev/ae1a4273a19b
Part 3 - Show a "reload this page" warning in the control center when trackers are loaded while TP is on. r=Paolo
https://hg.mozilla.org/integration/autoland/rev/c6d81707f23c
Part 4 - Update TP shield icon in the control center. r=Paolo
https://hg.mozilla.org/integration/autoland/rev/10b9a650f28c
Part 5 - Show a preferences button in the Tracking Protection section of the identity popup. r=Paolo
https://hg.mozilla.org/integration/autoland/rev/1ad660199a87
Part 6 - Add tests for the updated tracking protection UI in the identity popup. r=Paolo

Comment 38

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/e659c87f7267
https://hg.mozilla.org/mozilla-central/rev/a242cf779e39
https://hg.mozilla.org/mozilla-central/rev/ae1a4273a19b
https://hg.mozilla.org/mozilla-central/rev/c6d81707f23c
https://hg.mozilla.org/mozilla-central/rev/10b9a650f28c
https://hg.mozilla.org/mozilla-central/rev/1ad660199a87

Comment 39

[Pascal Chevrel:pascalc]

Johann, could you do a release note request addition for this feature please? Thanks
https://wiki.mozilla.org/Release_Management/Release_Notes#How_to_nominate_a_bug_for_release_notes_addition.3F

Comment 40

[Johann Hofmann [:johannh]]

Release Note Request (optional, but appreciated)
[Why is this notable]: We now tell the user that there are trackers on the site even if Tracking Protection is off (and some other minor improvements to the TP UI).
[Affects Firefox for Android]: No
[Suggested wording]: Even with Tracking Protection off, you can now use the site identity popup to find out whether the current page contains trackers.
[Links (documentation, blog post, etc)]: Nothing yet, but this is part of the larger anti-tracking work so stay tuned.

Comment 41

[Cristian Comorasu, QA [:ccomorasu], Releae Desktop QA - Please contact [e-mail]]

I can confirm this issue is fixed. I verified using Windows 10 x64, Ubuntu 18.04 LTS x64 and mac OS X 10.13, on Fx 62.0b13.

Comment 42

[Ryan VanderMeulen [:RyanVM]]

This didn't make the Fx62 relnotes.