Closed Bug 854077 Opened 7 years ago Closed 6 years ago

Change - Use radio buttons for "Do Not Track" options and add a neutral option

Categories

(Firefox for Metro Graveyard :: Flyouts, defect, P1)

All
Windows 8.1
defect

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 23

People

(Reporter: ywang, Assigned: rsilveira)

References

Details

(Whiteboard: feature=change c=Settings_pane_options_and_about u=metro_firefox_user p=1)

Attachments

(2 files)

Privacy is pushing "Do No Track" to have 3 exclusive mutual options instead of 2 options(On/Off).

We need to use a drop-down menu instead.

Title: Do Not Track

Options:
1. Do not tell sites my tracking preferences [Default]

2. Tell sites that I do not want to be tracked

3. Tell sites that I want to be tracked
Assignee: nobody → mbrubeck
Blocks: 852236, 765398
Hardware: x86 → All
Summary: Change - Use drop-down menu for "Do Not Track" options → Change - Use drop-down menu for "Do Not Track" options and add "okay to track me" option
I had proposed both the three radios and a drop down in bug 765398, but dolske suggested the three radios approach in bug 765398 comment 3.  

(other mock-up: [obsolete] attachment 633705 [details] on bug 765398)

I'm a little concerned that a drop-down with just the three options listed above in the description would make it such that the other two options are not discoverable; it's not clearly obvious when reading "Do not tell sites my tracking preference" that the other options exist and are different.

Just my two cents... I'll defer to the UX folks for design and such, but wanted to weigh in a little since I've spent lots of time on this.
(In reply to Sid Stamm [:geekboy or :sstamm] from comment #1)
> I had proposed both the three radios and a drop down in bug 765398, but
> dolske suggested the three radios approach in bug 765398 comment 3.  
> 
> (other mock-up: [obsolete] attachment 633705 [details] on bug 765398)
> 
> I'm a little concerned that a drop-down with just the three options listed
> above in the description would make it such that the other two options are
> not discoverable; it's not clearly obvious when reading "Do not tell sites
> my tracking preference" that the other options exist and are different.
> 
> Just my two cents... I'll defer to the UX folks for design and such, but
> wanted to weigh in a little since I've spent lots of time on this.

Thanks a lot for the feedback, Sid. 
To understand the context, I went through the discussions you guys have had in Bug 765398. Now I think using Radio buttons make better sense here. 

Windows 8 apps often prefer drop-down menus over radio buttons, in order to keep the flyout clean and simple. But in this particular case, to make a sound decision, the users often need to read through 3 options first. Using radio buttons to show 3 options together could provide the users more clarity. 


I have a question regarding to attachment 633705 [details] you posted:
The order on this mockup is [Yes, No, Neutral]. But in the comments Bug 765398, Matej was using the order [Yes, Neutral, No] all the time. 
Could you please clarify the rationale of that change?

In my opinion, since "Yes" and "No" both starts with "Tell sites that I..", placing them together might make it harder to scan and tell the difference. 
Regardless of the order, I think at least for Windows 8 FX, we would like to list the options under one phrase: "Tell sites". 

Tell sites:
- I don't want to be tracked
- Nothing about my tracking preferences
- I want to be tracked

Let me know if you have other preference on the wording of the neutral option.
Summary: Change - Use drop-down menu for "Do Not Track" options and add "okay to track me" option → Change - Use radio buttons for "Do Not Track" options and add a neutral option
If it is clear what the "default" is, I am not too sure order matters, though I'm willing to bet that top-to-bottom scan order will bias choice of the first option.  (This is, I think, why the "send nothing" option is default in the patch to mainstream Firefox.)

I think as long as the wording is reasonably honest that this feature is just "telling sites" things, it's up to you, Matej, and the other wordsmiths out there to make it clear.  :)  Your proposal above sounds pretty good.  The one thing I ask is that "do not" remain separate and not contracted to "don't", since it will sound more like the feature's name ("do not track") this way.
it5?
Blocks: 831958
Flags: needinfo?(mmucci)
Blocks: metrov1it5
No longer blocks: metrov1triage
Moving to Iteration #5 for consideration as a Change.
Flags: needinfo?(mmucci)
Priority: -- → P1
QA Contact: jbecerra
Whiteboard: feature=change c=tbd u=tbd p=tbd
Blocks: 831965
Whiteboard: feature=change c=tbd u=tbd p=tbd → feature=change c=Settings_pane_options_and_about u=metro_firefox_user p=tbd
Being placed into Story Backlog based on Asa's review.
Blocks: metrov1backlog
No longer blocks: metrov1it5
Blocks: metrov1planning
No longer blocks: metrov1backlog
Blocks: metrov1defect&change
No longer blocks: metrov1planning
Priority: P1 → P5
Assignee: mbrubeck → rsilveira
Whiteboard: feature=change c=Settings_pane_options_and_about u=metro_firefox_user p=tbd → feature=change c=Settings_pane_options_and_about u=metro_firefox_user p=1
For future consideration: p=1.

Defect Stories will be taking priority over Change Stories.
Whiteboard: feature=change c=Settings_pane_options_and_about u=metro_firefox_user p=1 → feature=change c=Settings_pane_options_and_about u=metro_firefox_user p=0
Blocks: metrov1it6
No longer blocks: metrov1defect&change
Priority: P5 → P1
Whiteboard: feature=change c=Settings_pane_options_and_about u=metro_firefox_user p=0 → feature=change c=Settings_pane_options_and_about u=metro_firefox_user p=1
Attachment #737196 - Flags: review?(ally)
Comment on attachment 737196 [details] [diff] [review]
Patch v1

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

looks pretty good overall; a couple of nits and a follow up question.

::: browser/metro/base/content/bindings/toggleswitch.xml
@@ +20,5 @@
>          <xul:description anonid="onlabel" class="onlabel" value="&checkbox.on.label;" xbl:inherits="value=onlabel"/>
>          <xul:description anonid="offlabel" class="offlabel" value="&checkbox.off.label;" xbl:inherits="value=offlabel"/>
>          <xul:radiogroup anonid="group" xbl:inherits="disabled">
> +          <xul:radio type="toggle" anonid="on" class="checkbox-radio-on"/>
> +          <xul:radio type="toggle" anonid="off" class="checkbox-radio-off"/>

Limitation of reviewer's brain: Why was this change necessary?

::: browser/metro/base/content/preferences.js
@@ +15,5 @@
> +  },
> +  onDNTPreferenceChanged: function onDNTPreferenceChanged() {
> +    let dntValue = document.getElementById("prefs-dnt-options").selectedItem.value;
> +
> +    Services.prefs.setBoolPref("privacy.donottrackheader.enabled", dntValue !== "-1");

is there any way we can get around using a magic number? (-1)?

::: browser/metro/base/tests/mochitest/browser_sanitize_ui.js
@@ +98,2 @@
>  
> +function checkDNTPrefs(aExpectedEnabled, aExpectedValue) {

Thank you so much for slogging through getting a test going.

@@ +100,5 @@
> +  let currentEnabled = Services.prefs.getBoolPref("privacy.donottrackheader.enabled");
> +  let currentValue = Services.prefs.getIntPref("privacy.donottrackheader.value");
> +
> +  ok(aExpectedEnabled === currentEnabled,
> +    "testing privacy.donottrackheader.enabled, expected " + aExpectedEnabled + " got " + currentEnabled);

nit: Can we break out the long strings into their own variables?
let enabledTestMsg =  "testing privacy.donottrackheader.enabled, expected " + aExpectedEnabled + " got " + currentEnabled;
ok(aExpectedEnabled === currentEnabled, enabledTestMsg);

@@ +119,3 @@
>      yield promise;
>  
> +    noPref.click();

quick nit: could you add a comment indicating where the magic numbers are set? That way if someone who isn't us breaks this test when we are not around, they can find things faster.
Attachment #737196 - Flags: review?(ally) → review+
(In reply to :Ally Naaktgeboren from comment #10)

Thanks for the review!

> ::: browser/metro/base/content/bindings/toggleswitch.xml
> @@ +20,5 @@
> >          <xul:description anonid="onlabel" class="onlabel" value="&checkbox.on.label;" xbl:inherits="value=onlabel"/>
> >          <xul:description anonid="offlabel" class="offlabel" value="&checkbox.off.label;" xbl:inherits="value=offlabel"/>
> >          <xul:radiogroup anonid="group" xbl:inherits="disabled">
> > +          <xul:radio type="toggle" anonid="on" class="checkbox-radio-on"/>
> > +          <xul:radio type="toggle" anonid="off" class="checkbox-radio-off"/>
> 
> Limitation of reviewer's brain: Why was this change necessary?
> 

Radiogroups were not rendering the radio circle, that was because the metro toggle switch (the on/off) uses radio but doesn't want to render the circles. I've added this new attribute to distinguish them when adding the binding at content/browser.css

> ::: browser/metro/base/content/preferences.js
> @@ +15,5 @@
> > +  },
> > +  onDNTPreferenceChanged: function onDNTPreferenceChanged() {
> > +    let dntValue = document.getElementById("prefs-dnt-options").selectedItem.value;
> > +
> > +    Services.prefs.setBoolPref("privacy.donottrackheader.enabled", dntValue !== "-1");
> 
> is there any way we can get around using a magic number? (-1)?
> 

The values of the radio items are automatically sent to the appropriate pref by the setting tag, so it makes things easier to have them match what the pref expects. I'll change this code to see if the radio item with id "prefs-dnt-nopref" is selected instead of checking the value, that would make it clearer.

> ::: browser/metro/base/tests/mochitest/browser_sanitize_ui.js
> @@ +98,2 @@
> >  
> > +function checkDNTPrefs(aExpectedEnabled, aExpectedValue) {
> 
> Thank you so much for slogging through getting a test going.
> 
> @@ +100,5 @@
> > +  let currentEnabled = Services.prefs.getBoolPref("privacy.donottrackheader.enabled");
> > +  let currentValue = Services.prefs.getIntPref("privacy.donottrackheader.value");
> > +
> > +  ok(aExpectedEnabled === currentEnabled,
> > +    "testing privacy.donottrackheader.enabled, expected " + aExpectedEnabled + " got " + currentEnabled);
> 
> nit: Can we break out the long strings into their own variables?
> let enabledTestMsg =  "testing privacy.donottrackheader.enabled, expected "
> + aExpectedEnabled + " got " + currentEnabled;
> ok(aExpectedEnabled === currentEnabled, enabledTestMsg);
> 

Done.

> @@ +119,3 @@
> >      yield promise;
> >  
> > +    noPref.click();
> 
> quick nit: could you add a comment indicating where the magic numbers are
> set? That way if someone who isn't us breaks this test when we are not
> around, they can find things faster.

Sure.
Status: NEW → ASSIGNED
https://hg.mozilla.org/mozilla-central/rev/0536fbab2e8c
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 23
For Kamil to test and verify.
Flags: needinfo?(kamiljoz)
Used the following build to verify and test the following "Change":

http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2013-04-18-03-10-48-mozilla-central/

Went through the following test cases:

- Ensured that the feature in Firefox Metro matches the mockup that has been attached
- Ensured that the selections are radio buttons as mentioned in Comment 2
- Ensured that the correct order appears in "Options" as mentioned in Comment 2
- Ensured that toggling DNT on the Firefox Desktop doesn't affect Firefox Metro
- Installed Firefox Metro several times and ensured that "I do not want to be tracked" was set as [Default] every single time
- Ensured that all three options worked without any issues (checking using http://donottrack.us/)
- Ensured that you can tap & click the selections without any problems (radio button selecting the correct item)
- Ensured that selecting the back button under "Options" went back to the "Settings" charm
- Ensured that the radio buttons worked without any issues when in Filled View
- Ensured that the "Do Not Track" area aligned with the rest of the options
Status: RESOLVED → VERIFIED
Flags: needinfo?(kamiljoz)
Verified for it8
Went through the following "Change" for iteration #9 testing without any issues. Used the following build:

http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2013-06-27-03-10-27-mozilla-central/

- Went through the original attached story without any issues
- Went through the test cases added in comment 15 without any issues
Depends on: 898641
Depends on: 898643
No longer depends on: 898643
No longer depends on: 898641
User Agent: Mozilla/5.0 (Windows NT 6.2; WOW64; rv:26.0) Gecko/20100101 Firefox/26.0
Build ID: 20130826074752
Built from http://hg.mozilla.org/mozilla-central/rev/14b1e8c2957e

WFM
Tested on windows 8 using latest nightly for iteration-12. I saw radio buttons and neutral option for tracking preferences.
We need to adapt the recent changes for DNT on Android and Firefox OS to stay consistent. 

Notes from Alina's privacy review: https://bugzilla.mozilla.org/show_bug.cgi?id=885129#c13

* DNT three-state (pg 6) - We should make this consistent with how DNT is presented on Fennec and Fx OS v1.2. Here is the copy for the three-state setting in the order in which it should be presented (which is the same across Desktop, Fennec and Fx OS):
--
O Tell websites that I do not want to be tracked.
O Tell websites that I do want to be tracked.
O Do not tell websites anything about my tracking preferences.
--
The default above should be set to the last ('Do not tell...'). 
A screenshot of Firefox OS DNT:
https://bug885129.bugzilla.mozilla.org/attachment.cgi?id=805689&t=BUfdYvdXSK

*If there is no room to include copy about 'How does Do Not Track work', you can include a "Learn More" link to https://www.mozilla.org/en-US/dnt/ under the three options.
Status: VERIFIED → REOPENED
Resolution: FIXED → ---
Hi Yuan, please file a new follow up bug for Comment #19.
Status: REOPENED → RESOLVED
Closed: 7 years ago6 years ago
Flags: needinfo?(ywang)
Resolution: --- → FIXED
Filed a new follow up bug at Bug 917422
Flags: needinfo?(ywang)
Went through the following "Change" for iteration #16 testing without any issues. Used the following build:
http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2013-10-22-03-02-02-mozilla-central/

- Went through the original attached story without any issues
- Went through the test cases added in comment 15 without any issues
- Also went through Bug 917422 as it basically also tests this feature (changing the wording, adding a link)
OS: Windows 8 Metro → Windows 8.1
You need to log in before you can comment on or make changes to this bug.