Move cookie settings to the Site Data section

VERIFIED FIXED in Firefox 60

Status

()

enhancement
P1
normal
VERIFIED FIXED
a year ago
10 months ago

People

(Reporter: johannh, Assigned: johannh)

Tracking

(Depends on 1 bug, Blocks 1 bug)

60 Branch
Firefox 60
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox60 verified)

Details

(Whiteboard: [storage-v2])

Attachments

(1 attachment)

Assignee

Description

a year ago
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.
Flags: qe-verify+
Assignee

Updated

a year ago
Blocks: 569627
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Assignee

Updated

a year ago
Blocks: 1436568
Assignee

Updated

a year ago
Attachment #8951213 - Flags: review?(jaws) → review?(gijskruitbosch+bugs)

Comment 4

a year ago
mozreview-review
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.
Attachment #8951213 - Flags: review?(gijskruitbosch+bugs)
Comment hidden (mozreview-request)
Assignee

Comment 6

a year ago
mozreview-review-reply
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

a year ago
(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.
Flags: needinfo?(jhofmann)
Assignee

Comment 8

a year ago
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!
Flags: needinfo?(jhofmann)
Comment hidden (mozreview-request)

Comment 10

a year ago
mozreview-review
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!
Attachment #8951213 - Flags: review?(gijskruitbosch+bugs) → review+

Comment 11

a year ago
Pushed by jhofmann@mozilla.com:
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

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/36be9b83bcc9
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 60
Assignee

Updated

a year ago
Depends on: 1441138
Assignee

Updated

a year ago
Depends on: 1441766
Duplicate of this bug: 1441861
Assignee

Updated

a year ago
Depends on: 1442993
Assignee

Updated

a year ago
No longer depends on: 1442993
This bug was verified during Site Storage UI Redesign feature testing.
I will mark as Verified fixed.
Status: RESOLVED → VERIFIED
Flags: qe-verify+

Updated

a year ago
Depends on: 1457180

Updated

a year ago
No longer depends on: 1457180

Updated

10 months ago
Depends on: 1480175
You need to log in before you can comment on or make changes to this bug.