Opened 23 months ago

Closed 22 months ago

Last modified 22 months ago

#6042 closed change (fixed)

Introduce ui_warn_tracking boolean preference and updated adbockplusui dependency to #bb9b1b67aedf9

Reported by: saroyanm Assignee: saroyanm
Priority: P3 Milestone: Adblock-Plus-3.0.2-for-Firefox
Module: Platform Keywords:
Cc: wspee, greiner, kzar, sebastian Blocked By:
Blocking: #6031 Platform: Unknown / Cross platform
Ready: yes Confidential: no
Tester: Ross Verified working: yes
Review URL(s):

https://codereview.adblockplus.org/29613772/

Description (last modified by kzar)

Background

If a user has both EasyPrivacy and Acceptable Ads enable at the same time it's important we explain the inherent conflict between those lists to them (#6031). Whitelisting filters take precendence over blacklisting ones, and so the user might be subject to some tracking that way.

Once the user has seen the explanation we need a way to remember not to keep displaying it, which will be the ui_warn_tracking preference.

What to change

Add ui_warn_tracking to the preferences, make it default to true:

/**
 * Whether to show tracking warning in options page when both Acceptable Ads and subscription of type "Privacy" are enabled
 * @type {boolean}
 */
defaults.ui_warn_tracking = true;

And update adblockplusui dependency to bb9b1b67aedf9 in order to include the changes below:

  • #5872 - Style changes
  • #5990 - Chrome broken icons fix
  • #6031 - Acceptable Ads Notification

Hint for testers

  • The icons should not misbehave anymore for Chrome
    • Hovering Trash/Close/Gear icons should change the color
    • Toggle icon in the advanced tab filter lists table should change it state when clicked
    • Acceptable Ads checkbox should change the state on click
  • Acceptable Ads notification should behave as following
    • When Acceptable Ads and Block additional tracking are both selected show the notification
    • When user click on X or OK, got it button, this notification shouldn't be shown anymore.

Note: The styles are slightly different, but that's just a cosmetic change, to reflect new colors and styles.

Change History (19)

comment:1 Changed 23 months ago by saroyanm

  • Blocking 6031 added

comment:2 follow-up: Changed 22 months ago by greiner

@saroyanm The JSDoc type says that this is a boolean but the default value you specify in the ticket description is a string. Based on the implementation in #6031 the default value should be changed to true.

comment:3 in reply to: ↑ 2 Changed 22 months ago by saroyanm

  • Description modified (diff)

Replying to greiner:

@saroyanm The JSDoc type says that this is a boolean but the default value you specify in the ticket description is a string. Based on the implementation in #6031 the default value should be changed to true.

Thanks for noticing, was a bad copy-past.
Fixed.

Last edited 22 months ago by saroyanm (previous) (diff)

comment:4 Changed 22 months ago by saroyanm

  • Cc kzar sebastian added

@Kzar, @Sebastian: Can you please have a look and mark this as ready if applicable.

I can also provide a patch after that.

comment:5 follow-up: Changed 22 months ago by kzar

  • Description modified (diff)
  • Owner set to saroyanm
  • Priority changed from Unknown to P3
  • Ready set

Perhaps do the dependency update to include the adblockplusui changes at the same time as a part of this issue?

comment:6 Changed 22 months ago by kzar

  • Description modified (diff)

comment:7 in reply to: ↑ 5 Changed 22 months ago by saroyanm

Replying to kzar:

Perhaps do the dependency update to include the adblockplusui changes at the same time as a part of this issue?

Okey, I think the other review should be ready soon as well.

comment:8 Changed 22 months ago by saroyanm

  • Description modified (diff)

Changed notification to warning in the description, as we reference to this as a warning currently.

comment:9 Changed 22 months ago by saroyanm

  • Description modified (diff)

I updated the description in order more properly describe the usage, as it's in general for the subscription of type="privacy" we are using/recommending.

comment:10 Changed 22 months ago by saroyanm

  • Review URL(s) modified (diff)
  • Status changed from new to reviewing

comment:11 Changed 22 months ago by saroyanm

  • Description modified (diff)

comment:12 Changed 22 months ago by saroyanm

  • Description modified (diff)
  • Summary changed from Introduce ui_warn_tracking boolean preference to Introduce ui_warn_tracking boolean preference and updated adbockplusui dependency to #bb9b1b67aedf9

comment:13 Changed 22 months ago by saroyanm

Updated the description in order to provide hint to the testers and include dependency update as well

comment:14 Changed 22 months ago by kzar

  • Description modified (diff)

comment:16 Changed 22 months ago by saroyanm

  • Resolution set to fixed
  • Status changed from reviewing to closed

comment:17 Changed 22 months ago by kzar

  • Milestone set to Adblock-Plus-for-Chrome-Opera-Firefox-next

comment:18 Changed 22 months ago by saroyanm

Filled an issue, as a regression to this, but it's easy to fix #6074

comment:19 Changed 22 months ago by Ross

  • Tester changed from Unknown to Ross
  • Verified working set

Done. Works as expected.

ABP 3.0.1.1939
Firefox 51 / 57 / Windows 7
Chrome 52 / 62 / Windows 7
Opera 38 / 49 / Windows 7

Last edited 22 months ago by Ross (previous) (diff)
Note: See TracTickets for help on using tickets.