1432743 - Move cookie settings to the Site Data section

Status: VERIFIED FIXED

(Firefox :: Settings UI, enhancement, P1)

Description

[Johann Hofmann [:johannh]]

As per our UI concept in bug 1421690 and (finally) following the spec guidelines (https://www.w3.org/TR/webstorage/#privacy) we'd like to merge the cookies settings into the "Site Data" section and move them to top-level.

This also means removing individual cookie management from about:preferences.

Comment 1

[Johann Hofmann [:johannh]]

Attachment: Bug 1432743 - Move cookie settings to the Site Data section and remove the cookies dialog from preferences.

After including cookies in the site data manager in bug 1421737, we would
like to move the cookies settings to the site data section to give them
more visibility and to unify site data and cookies.

Our UR has shown that our differentiation between site data and cookies
is not helpful to users and that they struggle with discovering the
cookie settings that are hidden in the "custom history" menu.

This grouping is also recommended by the webstorage spec:
https://www.w3.org/TR/webstorage/#privacy

The cookies dialog is not removed yet, because it is still accessible from
Page Info, but bug 1348223 will likely remove it entirely.

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

Comment 2

[Johann Hofmann [:johannh]]

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

Comment 3

[Johann Hofmann [:johannh]]

Comment on attachment 8951213 [details]
Bug 1432743 - Move cookie settings to the Site Data section and remove the cookies dialog from preferences.

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

Comment 4

[:Gijs (out for now; he/him)]

Comment on attachment 8951213 [details]
Bug 1432743 - Move cookie settings to the Site Data section and remove the cookies dialog from preferences.

https://reviewboard.mozilla.org/r/220474/#review228234

::: commit-message-27fd0:10
(Diff revision 2)
> +more visibility and to unify site data and cookies.
> +
> +Our UR has shown that our differentiation between site data and cookies
> +is not helpful to users and that they struggle with discovering the
> +cookie settings that are hidden in the "custom history" menu.
> +

The commit message needs to mention that you're actually changing the control types for some of the cookie controls here, and why/how. And that some strings are changing.

::: browser/components/preferences/in-content/privacy.js:824
(Diff revision 2)
>      var menu = document.getElementById("keepCookiesUntil");
>  
>      // enable the rest of the UI for anything other than "disable all cookies"
>      var acceptCookies = (pref.value != 2);
>  
> +    accept.value = pref.value;

So... I may be misunderstanding this (in my defense: this pref is silly-hairy!) but it seems like both the third party cookie control and (now) this radio group are affected by the same preference value.

This is confusing because the third-party cookie control recognizes 4 different values for the preference (0-3 inclusive).

So it seems here we can write e.g. 3. But the radiogroup only has 2 values (0 and 2). As you noted on IRC, it also seems like we should be returning the int (stringified, perhaps) instead of the boolean.

::: browser/components/preferences/in-content/privacy.xul:110
(Diff revision 2)
> -          <description>&rememberActions.pre.label;<label
> +          <description>&dontrememberActions.pre.label;<label
>            class="text-link" id="historyRememberClear"
> -          >&rememberActions.clearHistory.label;</label>&rememberActions.middle.label;<label
> +          >&dontrememberActions.clearHistory.label;</label>&dontrememberActions.post.label;</description>

Please check with flod if reusing the dontrememberActions strings here is OK. I realize that in English the strings are exactly the same, and they're in a separate <description>, but they relate to the previous sentence, which is different here from in the other case where we use these labels. In some language that may make a difference (e.g. whether the previous sentence had a negation or not). I know there's been other cases where we couldn't reuse a string for this reason.

Comment 5

[Johann Hofmann [:johannh]]

Comment on attachment 8951213 [details]
Bug 1432743 - Move cookie settings to the Site Data section and remove the cookies dialog from preferences.

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

Comment 6

[Johann Hofmann [:johannh]]

Comment on attachment 8951213 [details]
Bug 1432743 - Move cookie settings to the Site Data section and remove the cookies dialog from preferences.

https://reviewboard.mozilla.org/r/220474/#review228234

> The commit message needs to mention that you're actually changing the control types for some of the cookie controls here, and why/how. And that some strings are changing.

Good idea.

> So... I may be misunderstanding this (in my defense: this pref is silly-hairy!) but it seems like both the third party cookie control and (now) this radio group are affected by the same preference value.
> 
> This is confusing because the third-party cookie control recognizes 4 different values for the preference (0-3 inclusive).
> 
> So it seems here we can write e.g. 3. But the radiogroup only has 2 values (0 and 2). As you noted on IRC, it also seems like we should be returning the int (stringified, perhaps) instead of the boolean.

I agree on the silly hairy part, but I don't think we're changing anything except turning a 2-value checkbox into a 2-value radiogroup, so I'm not sure what's wrong about this (except from the correct nit that we should return the value).

The third-party menulist indeed changes the pref more granularly but it should not affect the visual state because it can not change it to 2 (always block).

> Please check with flod if reusing the dontrememberActions strings here is OK. I realize that in English the strings are exactly the same, and they're in a separate <description>, but they relate to the previous sentence, which is different here from in the other case where we use these labels. In some language that may make a difference (e.g. whether the previous sentence had a negation or not). I know there's been other cases where we couldn't reuse a string for this reason.

This will be removed in bug 1436568, so I don't think it matters much. I'm going to drop the issue if it's okay for you.

Comment 7

[:Gijs (out for now; he/him)]

(In reply to Johann Hofmann [:johannh] from comment #6)
> > So... I may be misunderstanding this (in my defense: this pref is silly-hairy!) but it seems like both the third party cookie control and (now) this radio group are affected by the same preference value.
> > 
> > This is confusing because the third-party cookie control recognizes 4 different values for the preference (0-3 inclusive).
> > 
> > So it seems here we can write e.g. 3. But the radiogroup only has 2 values (0 and 2). As you noted on IRC, it also seems like we should be returning the int (stringified, perhaps) instead of the boolean.
> 
> I agree on the silly hairy part, but I don't think we're changing anything
> except turning a 2-value checkbox into a 2-value radiogroup, so I'm not sure
> what's wrong about this (except from the correct nit that we should return
> the value).
> 
> The third-party menulist indeed changes the pref more granularly but it
> should not affect the visual state because it can not change it to 2 (always
> block).

Sorry, I should have been more explicit. AIUI, we call readAcceptCookies for onsyncfrompreference. It returns the pref value raw, and then the preferences implementation takes that value and assigns it to the radio group's value property and/or attribute (depends on whether the xbl binding is bound or not).

That might work in practice, but it's not really 'right' - the correct value is either 0 or 1 (the value attribute on the individual <radio> elements), and instead we're assigning some value between 0 and 3. Apparently validation in XBL / XUL is making this still do the right thing, but we shouldn't be relying on that. This is why the function used to return the boolean - which then got assigned into the checked property/attribute.

Comment 8

[Johann Hofmann [:johannh]]

Yeah, that makes sense. I didn't really consider it a problem but it's better to be explicit about it. I'll update the patch. Thanks for being thorough!

Comment 9

[Johann Hofmann [:johannh]]

Comment on attachment 8951213 [details]
Bug 1432743 - Move cookie settings to the Site Data section and remove the cookies dialog from preferences.

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

Comment 10

[:Gijs (out for now; he/him)]

Comment on attachment 8951213 [details]
Bug 1432743 - Move cookie settings to the Site Data section and remove the cookies dialog from preferences.

https://reviewboard.mozilla.org/r/220474/#review228566

Thanks!

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/36be9b83bcc9
Move cookie settings to the Site Data section and remove the cookies dialog from preferences. r=Gijs

Comment 12

[Cristina Coroiu [:ccoroiu]]

https://hg.mozilla.org/mozilla-central/rev/36be9b83bcc9

Comment 14

[roxleitan]

This bug was verified during Site Storage UI Redesign feature testing.
I will mark as Verified fixed.