Add a checkbox to the in-content Privacy preferences to toggle TP in Private Browsing

VERIFIED FIXED in Firefox 42

Status

()

Firefox
General
P1
normal
Rank:
9
VERIFIED FIXED
3 years ago
2 years ago

People

(Reporter: bgrins, Assigned: past)

Tracking

Trunk
Firefox 42
Points:
2
Dependency tree / graph
Bug Flags:
firefox-backlog +
qe-verify +

Firefox Tracking Flags

(firefox41 affected, firefox42 verified)

Details

(Whiteboard: [fxprivacy] [campaign])

Attachments

(4 attachments, 2 obsolete attachments)

(Reporter)

Description

3 years ago
Created attachment 8624241 [details]
pref.jpg

When toggling the feature off, TP no longer runs on new pages.  I'm assuming that pages that have already been blocked will still require a manual reload to remove TP.

When toggling the feature on, TP runs on non-exempt pages.  I'm assuming that pages that have already been loaded (without blocking) will still require a manual reload to get TP to work.
Flags: firefox-backlog?
(Reporter)

Updated

3 years ago
Blocks: 1175922

Updated

3 years ago
Flags: firefox-backlog? → firefox-backlog+

Updated

3 years ago
Rank: 9

Updated

2 years ago
Points: --- → 2
Flags: qe-verify+

Updated

2 years ago
Assignee: nobody → past
Status: NEW → ASSIGNED
Iteration: --- → 42.1 - Jul 13
(Assignee)

Comment 1

2 years ago
Created attachment 8628327 [details] [diff] [review]
WIP

This is missing the new shield icon (need to figure out if we have it yet) and has the shield in the end of the label, not the beginning, just like it is in Nightly. I think it's going to be a pain to find a way to put the icon first and maintain the standard behavior where clicking the label toggles the checkbox. Is this something that we really need?
(Reporter)

Comment 2

2 years ago
(In reply to Panos Astithas [:past] from comment #1)
> Created attachment 8628327 [details] [diff] [review]
> WIP
> 
> This is missing the new shield icon (need to figure out if we have it yet)

These are now available at chrome://browser/skin/controlcenter/tracking-protection.svg and chrome://browser/skin/controlcenter/tracking-protection-disabled.svg (landed in Bug 1175327).

Updated

2 years ago
QA Contact: mwobensmith
(Assignee)

Updated

2 years ago
Duplicate of this bug: 1175922

Updated

2 years ago
No longer blocks: 1175922
(Assignee)

Comment 4

2 years ago
Created attachment 8628968 [details] [diff] [review]
Patch v2

We discussed the shield placement in the daily meeting and the decision was to take it out for now and perhaps try to make it appear in front of the label later. This patch is working fine in my testing and includes the "Learn More" link for a SUMO article, which it expects to be at:

https://support.mozilla.org/1/firefox/%VERSION%/%OS%/%LOCALE%/tracking-protection-pbm

Try run:

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

Jared, I've only modified in-content prefs per our discussion in Whistler.
Attachment #8628327 - Attachment is obsolete: true
(Assignee)

Updated

2 years ago
Attachment #8628968 - Flags: review?(jaws)
Comment on attachment 8628968 [details] [diff] [review]
Patch v2

Review of attachment 8628968 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/components/preferences/in-content/privacy.xul
@@ -88,5 @@
> -      <checkbox id="trackingProtection"
> -                preference="privacy.trackingprotection.enabled"
> -                accesskey="&trackingProtection.accesskey;"
> -                label="&trackingProtection.label;" />
> -      <image id="trackingProtectionImage"/>

Was it intentional to get rid of the trackingProtectionImage here? This seems orthogonal to the private browsing mode changes.

@@ +89,4 @@
>    <vbox>
>      <hbox align="center">
>        <checkbox id="privacyDoNotTrackCheckbox"
> +                label="&dntTrackingNotOkay.label3;"

Please revert this change, a later review note below explains why.

As a side note when making a change like this, please use &dntTrackingNotOkay3.label and then update &dntTrackingNotOkay.accesskey to &dntTrackingNotOkay3.accesskey. It is preferred by l10n to keep the prefix the same for labels and accesskeys, while also using `label` and `accesskey` as the suffix. Note that it is also good to "touch" the accesskey if the label is updated, and vice versa due to the association between the two.

::: browser/locales/en-US/chrome/browser/preferences/privacy.dtd
@@ +8,2 @@
>  <!ENTITY dntTrackingNotOkay.accesskey   "n">
> +<!ENTITY trackingProtection.label2       "Prevent sites from tracking me.">

Please revert these changes. I don't see periods at the end of any other checkbox within en-US in the Preferences.

@@ +10,4 @@
>  <!ENTITY trackingProtection.accesskey   "m">
>  <!ENTITY trackingProtectionLearnMore.label "Learn more">
> +<!ENTITY trackingProtectionPBM.label       		"Prevent sites from tracking my online activity in Private Windows.">
> +<!ENTITY trackingProtectionPBM.accesskey	    "m">

Replace the tabs with spaces and remove the period at the end of trackingProtectionPBM.label.
Attachment #8628968 - Flags: review?(jaws) → review+
(Assignee)

Comment 6

2 years ago
(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #5)
> Was it intentional to get rid of the trackingProtectionImage here? This
> seems orthogonal to the private browsing mode changes.

The icon has changed, but I thought changing a style that was no longer used would be worse overall than simply removing it. Happy to do that however if you prefer it.

> Note that it is also good to "touch" the accesskey if the
> label is updated, and vice versa due to the association between the two.

For future reference, does this hold true even for minor label updates like adding a period?

> ::: browser/locales/en-US/chrome/browser/preferences/privacy.dtd
> @@ +8,2 @@
> >  <!ENTITY dntTrackingNotOkay.accesskey   "n">
> > +<!ENTITY trackingProtection.label2       "Prevent sites from tracking me.">
> 
> Please revert these changes. I don't see periods at the end of any other
> checkbox within en-US in the Preferences.

I assumed it was an intentional change, but I'll ask Aislinn to confirm.
Flags: needinfo?(agrigas)
(Assignee)

Comment 7

2 years ago
> I assumed it was an intentional change

Presumably to better separate the label from the "Learn More" link that moved to the same line.
(Assignee)

Comment 8

2 years ago
(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #5)
> Was it intentional to get rid of the trackingProtectionImage here? This
> seems orthogonal to the private browsing mode changes.

It occurred to me that by "here" perhaps you refer to the normal mode TP, in which case I did it for consistency with PBM TP. Aislinn commented that it looks weird having the shield between the label and "Learn More" in the new design.

Comment 9

2 years ago
(In reply to Panos Astithas [:past] from comment #8)
> (In reply to Jared Wein [:jaws] (please needinfo? me) from comment #5)
> > Was it intentional to get rid of the trackingProtectionImage here? This
> > seems orthogonal to the private browsing mode changes.
> 
> It occurred to me that by "here" perhaps you refer to the normal mode TP, in
> which case I did it for consistency with PBM TP. Aislinn commented that it
> looks weird having the shield between the label and "Learn More" in the new
> design.

Panos is correct, design decided to remove the shield image as it was causing layout issues and its not essential for V1. There are more visual entry points (mainly the url bar and onboarding) that people will become familiar with this feature. Please remove the shield image. The period can be removed as well for consistency with the other pref labels.
Flags: needinfo?(agrigas)
(Assignee)

Comment 10

2 years ago
Created attachment 8629982 [details] [diff] [review]
Patch v3

Updated patch per previous comments.
Attachment #8628968 - Attachment is obsolete: true
(Assignee)

Comment 11

2 years ago
Comment on attachment 8629982 [details] [diff] [review]
Patch v3

Carrying over r+
Attachment #8629982 - Flags: review+
(Assignee)

Comment 12

2 years ago
Green try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=6d1d7a46b6e3

Comment 13

2 years ago
https://hg.mozilla.org/integration/fx-team/rev/044631883a28
https://hg.mozilla.org/mozilla-central/rev/044631883a28
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox42: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 42
Is the accesskey conflict on "m" expected or overlooked?
(In reply to Francesco Lodolo [:flod] from comment #15)
> Is the accesskey conflict on "m" expected or overlooked?

Overlooked on my part, not sure if Panos noticed it. Panos, we should change the accesskey here to not have a conflict.
Flags: needinfo?(past)
(Assignee)

Comment 17

2 years ago
Oops, copy & paste fail. I have it fixed locally as part of bug 1177085 that I'm working on and I expect to ask you to review in the next few days.
Flags: needinfo?(past)
(Assignee)

Updated

2 years ago
Depends on: 1182541
(Assignee)

Comment 18

2 years ago
Plans changed, so I'm fixing the accesskey in bug 1182541.
Created attachment 8632742 [details]
screenshot

(In reply to agrigas from comment #9)
> Panos is correct, design decided to remove the shield image as it was
> causing layout issues and its not essential for V1. There are more visual
> entry points (mainly the url bar and onboarding) that people will become
> familiar with this feature. Please remove the shield image. The period can
> be removed as well for consistency with the other pref labels.

Without a period this doesn't look good in en-US and for sure it is an error in Polish.

Didn't check if this is direct result of this bug but also the two other learn more links appear now inline with checkbox labels making it similar to the above.

Is this intentional, will stay that way and I should update Polish localization?
Flags: needinfo?(agrigas)
Before change screenshot: https://ffp4g1ylyit3jdyti1hqcvtb-wpengine.netdna-ssl.com/ux/files/2015/07/Untitled2-600x261.png (from https://blog.mozilla.org/ux/2015/07/user-study-of-tracking-protection-in-firefox-nightly/).

It is really unfortunate (for me as localizer) to learn about such changes by complete accident :(

Comment 21

2 years ago
Not sure why there is a needinfo for me. The no period decision was to be consistent with what we currently use in Prefs. If we change this rule for strings more than X # of words, then its fine to add a period in that case but I am not aware of such a rule.
Flags: needinfo?(agrigas)
(In reply to agrigas from comment #21)
> Not sure why there is a needinfo for me. The no period decision was to be
> consistent with what we currently use in Prefs. If we change this rule for
> strings more than X # of words, then its fine to add a period in that case
> but I am not aware of such a rule.

Compare the current Privacy panel (where the "Learn more" link seems part of the sentence) to, for example, Advanced->Data Choices. I find the current layout (with basically no margin between the two elements) extremely confusing.

Comment 23

2 years ago
(In reply to Francesco Lodolo [:flod] from comment #22)
> (In reply to agrigas from comment #21)
> > Not sure why there is a needinfo for me. The no period decision was to be
> > consistent with what we currently use in Prefs. If we change this rule for
> > strings more than X # of words, then its fine to add a period in that case
> > but I am not aware of such a rule.
> 
> Compare the current Privacy panel (where the "Learn more" link seems part of
> the sentence) to, for example, Advanced->Data Choices. I find the current
> layout (with basically no margin between the two elements) extremely
> confusing.

I don't think that looks better. It shows up pretty misaligned on the row for me 
https://www.dropbox.com/s/fcl6pd38rorty6d/Screenshot%202015-07-13%2010.38.17.png?dl=0

A single space between the end of the label and the link is sufficient.For example, look at chrome - https://www.dropbox.com/s/7lfhs8cwt87us0r/Screenshot%202015-07-13%2010.40.01.png?dl=0 - same as our solution.
Created attachment 8634011 [details]
screenshot

(In reply to Stefan Plewako [:stef] from comment #19)
> (In reply to agrigas from comment #9)
> > Panos is correct, design decided to remove the shield image as it was
> > causing layout issues and its not essential for V1. There are more visual
> > entry points (mainly the url bar and onboarding) that people will become
> > familiar with this feature. Please remove the shield image. The period can
> > be removed as well for consistency with the other pref labels.
> 
> Without a period this doesn't look good in en-US and for sure it is an error
> in Polish.
> 
> Didn't check if this is direct result of this bug but also the two other
> learn more links appear now inline with checkbox labels making it similar to
> the above.
> 
> Is this intentional, will stay that way and I should update Polish
> localization?

(In reply to agrigas from comment #21)
> Not sure why there is a needinfo for me.

See question in comment #9

> A single space between the end of the label and the link is sufficient.For
> example, look at chrome -
> https://www.dropbox.com/s/7lfhs8cwt87us0r/Screenshot%202015-07-13%2010.40.01.
> png?dl=0 - same as our solution.

No, it is not the same, there is no period at the end of old checkbox label sentence in Firefox before learn more  link and the string context change makes it look that there is an error (missing period) in some locales at least - please review attached screenshot from latest en-US nightly.
Flags: needinfo?(agrigas)
(In reply to Stefan Plewako [:stef] from comment #24)
> See question in comment #9

Should be comment #19

Comment 26

2 years ago
Let's use a period then at the end of each. Thanks!
Flags: needinfo?(agrigas)

Updated

2 years ago
Depends on: 1184188

Updated

2 years ago
Whiteboard: [fxprivacy] → [fxprivacy] [campaign]
Verified checkbox in Nightly Fx42.0a1, 2015-07-20.

Obviously not dependent on pending changes in related bug 1184188.
Status: RESOLVED → VERIFIED
status-firefox42: fixed → verified
You need to log in before you can comment on or make changes to this bug.