Closed Bug 1470020 Opened 2 years ago Closed 2 years ago

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

Categories

(Firefox :: Site Identity, enhancement, P1)

enhancement

Tracking

()

VERIFIED FIXED
Firefox 62
Tracking Status
firefox62 --- verified

People

(Reporter: johannh, Assigned: johannh)

References

(Blocks 1 open bug)

Details

Attachments

(4 files)

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 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? :-)
Attachment #8986620 - Flags: review?(florian) → review+
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: ?
Attachment #8986719 - Flags: review?(florian) → review+
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
Attachment #8986633 - Flags: review?(florian) → review+
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.
Attachment #8986634 - Flags: review?(florian) → review+
Pushed by jhofmann@mozilla.com:
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
Depends on: 1473685
I can confirm this issue is fixed. I verified using Windows 10 x64, Ubuntu 18.04 LTS x64 and mac OS X 10.13.
Status: RESOLVED → VERIFIED
Depends on: 1476317
You need to log in before you can comment on or make changes to this bug.