Update Tracking Protection section in the identity popup

VERIFIED FIXED in Firefox 62

Status

()

P1
normal
VERIFIED FIXED
10 months ago
4 months ago

People

(Reporter: johannh, Assigned: johannh)

Tracking

(Blocks: 1 bug, {feature})

unspecified
Firefox 62
feature
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(relnote-firefox -, firefox-esr52 unaffected, firefox-esr60 unaffected, firefox60 unaffected, firefox61 unaffected, firefox62+ verified)

Details

Attachments

(6 attachments)

(Assignee)

Description

10 months ago
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
(Assignee)

Updated

10 months ago
Assignee: nobody → jhofmann
Status: NEW → ASSIGNED
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 4

10 months ago
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 6

10 months ago
mozreview-review
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...
Attachment #8980242 - Flags: review?(paolo.mozmail)

Comment 7

10 months ago
mozreview-review
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
Attachment #8980243 - Flags: review?(paolo.mozmail)

Comment 8

10 months ago
mozreview-review
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
Attachment #8980241 - Flags: review?(ehsan) → review+
(Assignee)

Comment 9

10 months ago
mozreview-review-reply
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 hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 17

10 months ago
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?
Flags: needinfo?(jhofmann)
(Assignee)

Updated

10 months ago
Blocks: 1466235
(Assignee)

Comment 18

10 months ago
(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!
(Assignee)

Updated

10 months ago
Flags: needinfo?(jhofmann)
(Assignee)

Updated

10 months ago
Blocks: 1462471

Comment 19

10 months ago
mozreview-review
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.
Attachment #8982340 - Flags: review?(paolo.mozmail) → review+

Comment 20

10 months ago
mozreview-review
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.
Attachment #8982339 - Flags: review?(paolo.mozmail) → review+

Comment 21

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

https://reviewboard.mozilla.org/r/248286/#review255080
Attachment #8982338 - Flags: review?(paolo.mozmail) → review+

Comment 22

10 months ago
mozreview-review
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.
Attachment #8980243 - Flags: review?(paolo.mozmail) → review+

Comment 23

10 months ago
mozreview-review
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.
Attachment #8980242 - Flags: review?(paolo.mozmail)

Comment 24

10 months ago
mozreview-review
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
Attachment #8980242 - Flags: review+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 31

10 months ago
(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.
(Assignee)

Comment 32

10 months ago
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!
Attachment #8980241 - Flags: review+ → review?(ehsan)

Comment 34

9 months ago
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...
Attachment #8980241 - Flags: review?(ehsan) → review+
(Assignee)

Comment 35

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

Comment 36

9 months ago
mozreview-review-reply
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

9 months ago
Pushed by jhofmann@mozilla.com:
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
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
status-firefox60: --- → unaffected
status-firefox61: --- → unaffected
status-firefox-esr52: --- → unaffected
status-firefox-esr60: --- → unaffected
tracking-firefox62: --- → +
Flags: needinfo?(jhofmann)
Keywords: feature
(Assignee)

Updated

9 months ago
Blocks: 1468316
Depends on: 1468671
(Assignee)

Comment 40

9 months ago
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.
relnote-firefox: --- → ?
Flags: needinfo?(jhofmann)
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.
Status: RESOLVED → VERIFIED
status-firefox62: fixed → verified

Updated

7 months ago
Blocks: 1484585

Updated

6 months ago
Depends on: 1493185
(Assignee)

Updated

6 months ago
No longer depends on: 1493185
This didn't make the Fx62 relnotes.
relnote-firefox: ? → -
You need to log in before you can comment on or make changes to this bug.