1470020 - Apply some minor UX/copy improvements to the TP section of the identity popup

Status: VERIFIED FIXED

(Firefox :: Site Identity, enhancement, P1)

Description

[Johann Hofmann [:johannh]]

See https://mozilla.invisionapp.com/share/KJLE1CMNGE5#/304090827_Phase_I_States

Comment 1

[Johann Hofmann [:johannh]]

Attachment: Bug 1470020 - Part 3 - Updates to TP copy in the identity popup.

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

Comment 2

[Johann Hofmann [:johannh]]

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

Comment 3

[Johann Hofmann [:johannh]]

Attachment: Bug 1470020 - Part 1 - Don't show the "Enable Protection" button when it links to the TP preferences.

UX found this confusing and unnecessary, after all.

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

Comment 4

[Johann Hofmann [:johannh]]

Attachment: Bug 1470020 - Part 2 - Add specific states for when the user has added a TP exception to the TP section of the identity popup.

We used to give this all the same "tracking-loaded" state, but now we want to differentiate between:

- Tracking loaded because TP is off
- Tracking loaded because of an exception
- Tracking not loaded but the site has an exception, which we want to allow the user to remove (if TP is on)

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

Comment 5

[Johann Hofmann [:johannh]]

Comment on attachment 8986620 [details]
Bug 1470020 - Part 3 - Updates to TP copy in the identity popup.

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

Comment 6

[Johann Hofmann [:johannh]]

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

Comment 7

[Johann Hofmann [:johannh]]

Attachment: Bug 1470020 - Part 4 - Don't handle Tracking Protection UI on file: and about: URIs.

... because these URIs are incompatible with TP.

We now show it on moz-extension: instead, which was forgotten previously.

This should probably have been in its own separate bug, but the changes in bug 1470020
surfaced this issue by throwing a lot of console errors when the baseURI was accessed,
so I didn't want to defer the fix.

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

Comment 8

[Johann Hofmann [:johannh]]

Comment on attachment 8986634 [details]
Bug 1470020 - Part 2 - Add specific states for when the user has added a TP exception to the TP section of the identity popup.

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

Comment 9

[Johann Hofmann [:johannh]]

Comment on attachment 8986620 [details]
Bug 1470020 - Part 3 - Updates to TP copy in the identity popup.

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

Comment 10

[Johann Hofmann [:johannh]]

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

Comment 11

[Johann Hofmann [:johannh]]

Comment on attachment 8986634 [details]
Bug 1470020 - Part 2 - Add specific states for when the user has added a TP exception to the TP section of the identity popup.

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

Comment 12

[Johann Hofmann [:johannh]]

Comment on attachment 8986620 [details]
Bug 1470020 - Part 3 - Updates to TP copy in the identity popup.

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

Comment 13

[Johann Hofmann [:johannh]]

Comment on attachment 8986719 [details]
Bug 1470020 - Part 4 - Don't handle Tracking Protection UI on file: and about: URIs.

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

Comment 14

[Johann Hofmann [:johannh]]

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

Comment 15

[Johann Hofmann [:johannh]]

Comment on attachment 8986633 [details]
Bug 1470020 - Part 1 - Don't show the "Enable Protection" button when it links to the TP preferences.

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

Comment 16

[Johann Hofmann [:johannh]]

Comment on attachment 8986634 [details]
Bug 1470020 - Part 2 - Add specific states for when the user has added a TP exception to the TP section of the identity popup.

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

Comment 17

[Johann Hofmann [:johannh]]

Comment on attachment 8986620 [details]
Bug 1470020 - Part 3 - Updates to TP copy in the identity popup.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/251924/diff/4-5/

Comment 18

[Johann Hofmann [:johannh]]

Comment on attachment 8986719 [details]
Bug 1470020 - Part 4 - Don't handle Tracking Protection UI on file: and about: URIs.

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

Comment 19

[Johann Hofmann [:johannh]]

Was chatting about this with Florian and he offered to steal the patches. Of course I'm happy to get input from everyone! :)

Thanks!

Comment 20

[Florian Quèze [:florian]]

Comment on attachment 8986620 [details]
Bug 1470020 - Part 3 - Updates to TP copy in the identity popup.

https://reviewboard.mozilla.org/r/251924/#review258540

Fifth time's the charm? :-)

Comment 21

[Florian Quèze [:florian]]

Comment on attachment 8986719 [details]
Bug 1470020 - Part 4 - Don't handle Tracking Protection UI on file: and about: URIs.

https://reviewboard.mozilla.org/r/252014/#review258544

::: browser/base/content/browser-trackingprotection.js:25
(Diff revision 3)
>      // nsChannelClassifier::ShouldEnableTrackingProtection.
>      // Any scheme turned into https is correct.
> +    try {
> -    return Services.io.newURI("https://" + gBrowser.selectedBrowser.currentURI.hostPort);
> +      return Services.io.newURI("https://" + gBrowser.selectedBrowser.currentURI.hostPort);
> +    } catch (e) {
> +      return null;

Add a comment here saying this is catching about: and file: ?

Comment 22

[Florian Quèze [:florian]]

Comment on attachment 8986633 [details]
Bug 1470020 - Part 1 - Don't show the "Enable Protection" button when it links to the TP preferences.

https://reviewboard.mozilla.org/r/251938/#review258546

Comment 23

[Florian Quèze [:florian]]

Comment on attachment 8986634 [details]
Bug 1470020 - Part 2 - Add specific states for when the user has added a TP exception to the TP section of the identity popup.

https://reviewboard.mozilla.org/r/251940/#review258536

r=me. Note: these reviews were done by code inspection only, you have enough test coverage here that I trust this must be working as expected. Let me know if you expected me to also test the patches locally.

::: browser/base/content/test/trackingUI/browser_trackingUI_state.js:77
(Diff revision 4)
> +  ok(!TrackingProtection.icon.hasAttribute("state"), "icon: no state");
> +  ok(!TrackingProtection.icon.hasAttribute("tooltiptext"), "icon: no tooltip");
> +
> +  ok(hidden("#tracking-protection-icon"), "icon is hidden");
> +  is(!hidden("#tracking-action-block"), TrackingProtection.enabled,
> +    "blockButton is visible if TP is on");

nit: you have one missing space of indent on most your 'is' calls in this file.

Comment 24

[Johann Hofmann [:johannh]]

Comment on attachment 8986634 [details]
Bug 1470020 - Part 2 - Add specific states for when the user has added a TP exception to the TP section of the identity popup.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/251940/diff/4-5/

Comment 25

[Johann Hofmann [:johannh]]

Comment on attachment 8986620 [details]
Bug 1470020 - Part 3 - Updates to TP copy in the identity popup.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/251924/diff/5-6/

Comment 26

[Johann Hofmann [:johannh]]

Comment on attachment 8986719 [details]
Bug 1470020 - Part 4 - Don't handle Tracking Protection UI on file: and about: URIs.

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

Comment 27

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b7af8264a336
Part 1 - Don't show the "Enable Protection" button when it links to the TP preferences. r=florian
https://hg.mozilla.org/integration/autoland/rev/74475cd5a233
Part 2 - Add specific states for when the user has added a TP exception to the TP section of the identity popup. r=florian
https://hg.mozilla.org/integration/autoland/rev/00c94772a661
Part 3 - Updates to TP copy in the identity popup. r=florian
https://hg.mozilla.org/integration/autoland/rev/6e0fc38940b4
Part 4 - Don't handle Tracking Protection UI on file: and about: URIs. r=florian

Comment 28

[Cristina Coroiu [:ccoroiu]]

https://hg.mozilla.org/mozilla-central/rev/b7af8264a336
https://hg.mozilla.org/mozilla-central/rev/74475cd5a233
https://hg.mozilla.org/mozilla-central/rev/00c94772a661
https://hg.mozilla.org/mozilla-central/rev/6e0fc38940b4

Comment 29

[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.