Allow visitors to choose which cookie categories they give their consent to

Setting issue status to Needs review to trigger feedback on the "first attempt" patch.

@Dozz, can you take a look at the report and fix what appears to be some coding style issues?

If you have time, it would be great if you can do a patch for that.

Updating status with last patch AUTHORED BY Dozz

Thanks for the patch, it is an amazing work!
I found 4 bugs on #16 to make it fully functional:
- Categories are not saved on "Save Preferences" action (tested in chrome).
- All categories are saved on "accept" action, instead of the current settings (tested in chrome).
- Whitelisted Scripts are not loaded on save preferences or accept action (tested in chrome).
- let and const statements that are can cause issues on old browsers.

Here is a patch solving this issues. Hope to help.

Updated patch to set all categories as checked by default. Added interdiff with #16 and #18

Thanks for the patch, akalam. I will take a look once the bug reports have been sorted out. Maybe you can find somebody to do a review meanwhile?

Sorry that we don't have tests :)

Thanks, Dozz.
I started reviewing your patch this week. Any chance you can help port it to Drupal 7?

This patch is looking promising, and goes a good way to being a feature that can be added to the module.

Some comments first on the labels for the new option added:

I'm not sure about the grammar of "Mark first consent as checked and readonly.", I would assume you're marking a checkbox as checked and read only and not the consent itself. Maybe we should have a native English speaker validate the strings added by this patch.

I would suggest: "Check the first checkbox and mark it read only"

I'd also like to move the new option to follow immediately after "Opt-in", since these two options are similar, and then label the new option "Opt-in categories" rather than just "Categories".

Now to the functionality:

1) There's now an additional button to "Save preferences", which I think many users will find confusing. It's not clear whether I need to click both buttons, and what will happen when I dismiss the banner clicking "OK I agree", will I accept all options, none, or just the ones I have checked? Can you make it so the "OK" button saves the preferences and remove the "Save preferences" button? These banners add already quite some annoyance for users, so we should aim to make the experience as easy and streamlined as possible.

2) We need default css for the new features. The ideal patch that we can accept and merge is when the site builders can install the module, choose a background and foreground color and then have an "ok"-ish appearance that fits well with the color scheme of their site without having to code a css file to position the checkboxes on the same line.

3) Do we really need "Descriptions"? If an option says "Advertising" or "Statistics" isn't that label clear enough in itself?

4) Option 2 and onwards are also checked by default, even though there's no interface option or advise on this. Do we want to have another check box in the preferences to "Check all options by default"? I can see that some site owner could want to offer unchecked checkboxes for the remaining options after the first one. I've seen that pattern on a few sites.

5) The check boxes need to be present in the "Privacy options" tray so that the users can change the settings if they change their mind.

6) We need a port to Drupal 7 before we can add this feature to the module.

I haven't yet reviewed all the code added for code style issues or bugs, will do that once the patch addresses these issues.

Thanks for following up, and looking for time to improve the patch. If you need to bounce any ideas, you can typically find me on Drupal Slack.

So that is why there is a main 'Accept all cookies' button that stands out next to a smaller 'Save preferences' button (or link).
Maybe we can make the button as well as checking all checkboxes optional.

1) Yes, that's a good idea. Let's have a button, and a preference for whether to display it.

3) Can we do a compromise: Display the Description as a hover tooltip? Or at least make it optional. Right now it seems like a description is required, which just adds clutter to the screen if it's to be added for every category.

Can we do the same for Drupal 7 module?

I have reviewed the patch. Here's my feedback:

1. +++ b/css/eu_cookie_compliance.css
   @@ -63,7 +72,8 @@
   +.eu-cookie-compliance-save-preferences-button{
   

   Missing a space before curly brace.

2. +++ b/eu_cookie_compliance.module
   @@ -549,12 +671,10 @@ function eu_cookie_compliance_module_set_weight() {
   +function _eu_cookie_compliance_convert_relative_uri(&$element) {
   

   Thanks for removing $key, which isn't used. Though it would have been better if it was handled in a separate issue, since it's unrelated to this work.

3. +++ b/js/eu_cookie_compliance.js
   @@ -165,11 +201,13 @@
   +      .addClass('sliding-popup-top clearfix')
   

   Please try not to shift formatting as part of your patch, it gets difficult to see what was changed.

4. +++ b/js/eu_cookie_compliance.js
   @@ -179,11 +217,13 @@
   +      .addClass('sliding-popup-bottom')
   

   Same here

5. +++ b/js/eu_cookie_compliance.js
   @@ -454,6 +658,12 @@
   +          // Some jQuery Cookie versions don't remove cookies well.  Try again
   

   Good catch - thanks!

6. +++ b/src/Form/EuCookieComplianceConfigForm.php
   @@ -193,11 +193,64 @@ class EuCookieComplianceConfigForm extends ConfigFormBase {
   +        'categories' => $this->t('Categories. Let visitors choose which cookie categories they want to opt-in for.'),
   

   I think we should call it "Opt-in with categories" to make it very clear that the method works like "Opt-in".

7. +++ b/src/Form/EuCookieComplianceConfigForm.php
   @@ -236,7 +289,8 @@ class EuCookieComplianceConfigForm extends ConfigFormBase {
   -      '#description' => $this->t("Include the full path of JavaScripts, each on a separate line. When using the opt-in or opt-out consent options, you can block certain JavaScript files from being loaded when consent isn't given. The on-site JavaScripts should be written as root relative paths <strong>without the leading slash</strong>, you can use public://path/to/file.js and private://path/to/file.js, and off-site JavaScripts should be written as complete URLs <strong>with the leading http(s)://</strong>. Note that after the user gives consent, the scripts will be executed in the order you enter here."),
   

   We just used the | character for a different feature. Let's rather use colon for the separation in this patch.

I also think we should make some improvements to the HTML markup, as some users may want to display the options horizontally rather than vertically. Instead of the current

<div id="eu-cookie-compliance-categories" class="eu-cookie-compliance-categories">
  <div>
    <input type="checkbox" name="cookie-categories" id="cookie-category-key" value="key" checked="" disabled="">
    <label for="cookie-category-key">Performance cookies</label> 
  </div>
  <div class="eu-cookie-compliance-category-description">
    Description1
  </div>
  <div>
    <input type="checkbox" name="cookie-categories" id="cookie-category-key2" value="key2">
    <label for="cookie-category-key2">Marketing cookies</label> 
  </div>
  <div class="eu-cookie-compliance-category-description">
    Description2
  </div>
  <div class="eu-cookie-compliance-categories-buttons">
    <button type="button" class="eu-cookie-compliance-save-preferences-button">Save preferences</button> 
  </div>
</div>

I think we should have an additional outer div and also add a class to that div:

<div id="eu-cookie-compliance-categories" class="eu-cookie-compliance-categories">
  <div class="eu-cookie-compliance-category">
    <div>
      <input type="checkbox" name="cookie-categories" id="cookie-category-key" value="key" checked="" disabled="">
      <label for="cookie-category-key">Performance cookies</label> 
    </div>
    <div class="eu-cookie-compliance-category-description">
      Description1 
    </div>
  </div>
  <div class="eu-cookie-compliance-category">
    <div>
      <input type="checkbox" name="cookie-categories" id="cookie-category-key2" value="key2">
      <label for="cookie-category-key2">Marketing cookies</label> 
    </div>
    <div class="eu-cookie-compliance-category-description">
      Description2 
    </div>
    <div class="eu-cookie-compliance-categories-buttons">
      <button type="button" class="eu-cookie-compliance-save-preferences-button">Save preferences</button> 
    </div>
  </div>
</div>

Attached is a patch that addresses the comments in #49, adds an update hook and fixes some small issues here and there.

It would be awesome if @akalam or @Dozz can take the patch for a spin to do an RTBC, then we can get this issue merged and start porting it to D7.

Unfortunately no interdiff as the patch in #44 doesn't apply nicely to the latest commit of 1.x-dev.

I've tested the patch from #51 against latest dev and it is working fine for me.
Two minor things I noticed:

1. Maybe it's better for consistency to use the colon for whitelisted cookies too: So "category:cookie_name" instead of "category|cookie_name"
2. The method is now listed everywhere as "Opt-in with Categories", but shouldn't categories be without a capital?

Thank you for the review. Would you say it's good to go as RTBC?

1. Good idea. I was contemplating the same while changing the patch, but it escaped me.
2. Well spotted. I have a bad habit of using title case in English labels.

The thing that cause me to hold back on using colon for whitelisted cookies was that maybe somebody would put colons in the cookie names, but I guess we can just say we don't support cookies with colon in the name? Would colon be more likely to be used in a cookie name than the vertical bar?

Ah yes that is tricky, but I think colon should be fine.
I've looked it up and according to this answer on StackOverflow, colon should be forbidden in cookie names, but nothing is ever enforced.

I think we should just stick to RFC-6265 and use colon as separator.

I've attached a patch for both changes.

I also just realised that colons/pipes in cookie names would not be an issue, since the JS code checks for the first index.
That means that the only part that can't contain the separator is the category name.

Thanks for the patch. Looks good.

Now on to porting this to Drupal 7..

Here's a preliminary patch for 7.x. Would be great if somebody can test it and RTBC. I've done some testing, but there was a lot of code to port so some D8 codes may have slipped through.

I also found a few issues with the 8.x version. Here's an updated patch as well as an interdiff.

Hi svenryen.
Regarding some testing I have done to the updated code with the patch applied I have to mention 2 issues:

1. file sites/all/modules/eu_cookie_compliance/theme/eu-cookie-compliance-popup-info.tpl.php at line 62 is missing a closing ">" for the input element, which makes the label-checkbox clicking functionality to not work
2. I made some changes in file sites/all/modules/eu_cookie_compliance/eu_cookie_compliance.module because some things seemed to not work for me, I haven't tested it thoroughly

Line 374, from:

$disabled_javascripts = _eu_cookie_compliance_explode_multiple_lines($disabled_javascripts);

to:

$disabled_javascripts = _eu_cookie_compliance_explode_multiple_lines($disabled_javascripts, FALSE);

and in line 378, from:

$parts = explode('%3A', $script);

to:

$parts = explode(':', $script);

I hope I explained it clearly.

Hi svenryen.
Regarding some testing I have done to the updated code with the patch applied I have to mention 2 issues:

1. file sites/all/modules/eu_cookie_compliance/theme/eu-cookie-compliance-popup-info.tpl.php at line 62 is missing a closing ">" for the input element, which makes the label-checkbox clicking functionality to not work
2. I made some changes in file sites/all/modules/eu_cookie_compliance/eu_cookie_compliance.module because some things seemed to not work for me, I haven't tested it thoroughly

Line 374, from:

$disabled_javascripts = _eu_cookie_compliance_explode_multiple_lines($disabled_javascripts);

to:

$disabled_javascripts = _eu_cookie_compliance_explode_multiple_lines($disabled_javascripts, FALSE);

and in line 378, from:

$parts = explode('%3A', $script);

to:

$parts = explode(':', $script);

I hope I explained it clearly.

Use the colon to separate the category and path to the scripts (category:path/to/the/script.js), give an error with the absolute URLs returning http(s) as the path to the script.
This patch will solve these cases.

Testbot report test fail because there are no tests. Moving back to Needs Review.

Thanks for the additional patch. I ported it to D7. This went under my radar, so unfortunately the patch from #71 wasn't in the tagged release that contained the new feature. I'm also not sure how the issue credit system works when there's already a committed patch previously in the issue, hopefully you guys get your credits for this work.

I'm testing with the 8.x-1.6 version in combination with the categories. The new functionality is great! And also thanks for all the hard work put into this module!

But there is one thing that seems to be missing: the possibility to change the preferences, per category, after a first save. Can you add this option? Or is this option already present?

See https://www.drupal.org/project/eu_cookie_compliance/issues/2989038#comme...

5) The check boxes need to be present in the "Privacy options" tray so that the users can change the settings if they change their mind.

I can't find this setting. I can enable a "privacy settings" floating tray but there is only the option to "withdraw consent".

I have the same question as #75
How can someone change their preferences after a first save? i can only choose "withdraw consent"?

I'm testing with the 8.x-1.9 version in combination with the categories.

jan@sevendays there will be a new feature in 1.10 that lets the user change their preferences at any time.