Closed Bug 1476217 Opened 6 years ago Closed 6 years ago

Update Tracking Protection preferences for Content Blocking

Categories

(Firefox :: Settings UI, enhancement, P1)

Product:
Firefox
Component:
Settings UI
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Firefox 63
Tracking Flags:
Tracking Status
firefox63 --- fixed

People

(Reporter: johannh, Assigned: johannh)

References

(Blocks 1 open bug)

Details

Attachments

(5 files)

Bug 1476217 - Part 3 - Add FastBlock to Content Blocking categories in about:preferences.
59 bytes, text/x-review-board-request
jaws
: review+
Details
Bug 1476217 - Part 1 - Update Tracking Protection preferences for Content Blocking.
59 bytes, text/x-review-board-request
jaws
: review+
Details
Bug 1476217 - Part 2 - Add a "Restore Defaults" button to Content Blocking preferences.
59 bytes, text/x-review-board-request
jaws
: review+
Details
Bug 1476217 - Part 4 - Add tests for content blocking.
59 bytes, text/x-review-board-request
jaws
: review+
Details
Bug 1476217 - Part 5 - Disable content blocking sub-controls when content blocking is disabled.
59 bytes, text/x-review-board-request
jaws
: review+
Details 
Design spec is here:

https://mozilla.invisionapp.com/share/VSMVD99JK6D#/screens/301705296

63 is introducing a new "content blocking" mode that takes the place of tracking protection and demotes it to a category of content blocking. This will be behind a pref for now, which means the UI has to be able to fit both for the old TP content as well as the new Content Blocking.

In practice this probably means, turning the content blocking UI pref off, will:

- Change the "Content Blocking" title to "Tracking Protection"
- Change the global toggle to flip the tracking protection pref instead of the  content blocking pref
- Hide the "Choose what to block" section

All pref names are TBD
Assignee: nobody → jhofmann
Status: NEW → ASSIGNED
Priority: P2 → P1
Depends on: 1476879
Comment on attachment 8994317 [details]
Bug 1476217 - Part 2 - Add a "Restore Defaults" button to Content Blocking preferences.

https://reviewboard.mozilla.org/r/258912/#review266386
Attachment #8994317 - Flags: review?(jaws) → review+
Comment on attachment 8993708 [details]
Bug 1476217 - Part 3 - Add FastBlock to Content Blocking categories in about:preferences.

https://reviewboard.mozilla.org/r/258390/#review266390

::: browser/app/profile/firefox.js:1505
(Diff revision 2)
>  pref("browser.ping-centre.production.endpoint", "https://tiles.services.mozilla.com/v3/links/ping-centre");
>  
>  // Enable GMP support in the addon manager.
>  pref("media.gmp-provider.enabled", true);
>  
> +// TODO: this will be removed in the final patch and set by bug 1474280.

Bug 1474280 is now marked as fixed so this likely needs updating. Should the pref get set to true now by default? I see it is false by default on m-c.

::: browser/themes/shared/controlcenter/slowtrackers-16.svg:4
(Diff revision 2)
> +<!-- This Source Code Form is subject to the terms of the Mozilla Public
> +   - License, v. 2.0. If a copy of the MPL was not distributed with this
> +   - file, You can obtain one at http://mozilla.org/MPL/2.0/. -->
> +<svg xmlns="http://www.w3.org/2000/svg" width="16" height="16" viewBox="0 0 16 16" fill="context-fill" fill-opacity="context-fill-opacity">

Please remove the -16 suffix since this is scalable and the other SVG files don't use a size suffix.
Attachment #8993708 - Flags: review?(jaws) → review+
Comment on attachment 8994318 [details]
Bug 1476217 - Part 4 - Add tests for content blocking.

https://reviewboard.mozilla.org/r/258914/#review266392

::: browser/components/preferences/in-content/tests/browser_contentblocking.js:32
(Diff revision 1)
> +  is(contentBlockingToggle.getAttribute("aria-pressed"), "true", "toggle button has correct aria attribute");
> +
> +  Services.prefs.setBoolPref(CB_PREF, false);
> +  await TestUtils.waitForCondition(() => contentBlockingToggleLabel.textContent == "OFF", "toggle label is correct");
> +
> +  ok(!contentBlockingCheckbox.checked, "Checkbox is checked not when CB is off");

"Checkbox is not checked when CB is off"
Attachment #8994318 - Flags: review?(jaws) → review+
Comment on attachment 8994316 [details]
Bug 1476217 - Part 1 - Update Tracking Protection preferences for Content Blocking.

https://reviewboard.mozilla.org/r/258910/#review266380

I understand the desire to use a slider here is to match the Firefox menu, but this is the only place that we use a slider in the preferences and I don't see how it offers any advantage (other than consistency with the Firefox menu) over a checkbox. Can we just use a checkbox to mark as on or off?

Also, when Content Blocking is off I would expect the "Choose what to block" section should be disabled. It's currently interactive and that is confusing since the feature is supposed to be off and this might lead users to think that it is still doing something or that they will need to set both options to "Never block" to fully disable it.

::: browser/themes/shared/controlcenter/trackers-16.svg:4
(Diff revision 1)
> +<!-- This Source Code Form is subject to the terms of the Mozilla Public
> +   - License, v. 2.0. If a copy of the MPL was not distributed with this
> +   - file, You can obtain one at http://mozilla.org/MPL/2.0/. -->
> +<svg xmlns="http://www.w3.org/2000/svg" width="16" height="16" viewBox="0 0 16 16" fill="context-fill" fill-opacity="context-fill-opacity">

Because SVG is scalable, we should drop the "-16" from the filename. No other files in browser/themes/shared/controlcenter have this suffix.

::: browser/themes/shared/incontentprefs/privacy.css:54
(Diff revision 1)
> +
> +#contentBlockingCheckbox {
> +  visibility: collapse;
> +}
> +
> +#contentBlockingToggle > hbox {

Can you set a className on the hbox so the selector won't need to use the generic 'hbox' tag name?

::: browser/themes/shared/incontentprefs/privacy.css:58
(Diff revision 1)
> +
> +#contentBlockingToggle > hbox {
> +  display: none;
> +}
> +
> +#contentBlockingToggle {

Can some of this toggle styling be moved to common.inc.css, and use the colors that are defined there?

::: browser/themes/shared/incontentprefs/privacy.css:100
(Diff revision 1)
> +  border: 1px solid #b1b1b3;
> +}
> +
> +#contentBlockingCheckbox[checked] + #contentBlockingToggle:hover,
> +#contentBlockingCheckbox[checked] + #contentBlockingToggle:-moz-focusring {
> +  background-color: #45a1ff;

Please use 2292d0 to match the checkboxes,
https://searchfox.org/mozilla-central/source/toolkit/themes/shared/in-content/common.inc.css#568,575
Attachment #8994316 - Flags: review?(jaws) → review-
(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #12)
> Comment on attachment 8994316 [details]
> Bug 1476217 - Part 1 - Update Tracking Protection preferences for Content
> Blocking.
> 
> https://reviewboard.mozilla.org/r/258910/#review266380
> 
> I understand the desire to use a slider here is to match the Firefox menu,
> but this is the only place that we use a slider in the preferences and I
> don't see how it offers any advantage (other than consistency with the
> Firefox menu) over a checkbox. Can we just use a checkbox to mark as on or
> off?

Thanks for the reviews, needinfo Bryan to answer this question.

(Not back from PTO yet, just want to make sure this gets resolved quickly)
Flags: needinfo?(bbell)
(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #12)
> Comment on attachment 8994316 [details]
> I understand the desire to use a slider here is to match the Firefox menu,
> but this is the only place that we use a slider in the preferences and I
> don't see how it offers any advantage (other than consistency with the
> Firefox menu) over a checkbox. Can we just use a checkbox to mark as on or off?

There is a desire to visually match this switch to the one found in the Application Menu to give users a clear context-clue it and the menu item are one in the same. 

Also while there is no precedent for a switch in Preferences, there is also no precedent for this type of control in the Application Menu; rarity doesn't mean it's not appropriate.  The switch in preferences is being used to set this feature apart from other for two reasons. One is that it's key to the marketability of Firefox and drives attention to an important feature, and two it is to clarify to the user that this control does a more than your average boolean.  Changing this control changes how Firefox interacts with the web, so it's a general good to give the user something a unique control to highlight a unique feature.   




> section should be disabled. It's currently interactive and that is confusing
> since the feature is supposed to be off and this might lead users to think
> that it is still doing something or that they will need to set both options
> to "Never block" to fully disable it.

Agreed. If Content Blocking is off, then the subordinate controls should be disabled, but still visible.  We need to make sure that happens before it reaches the public, but is ok to push this out to Nightly for testing purposes.  



> Because SVG is scalable, we should drop the "-16" from the filename. No
> other files in browser/themes/shared/controlcenter have this suffix.

"-16" denotes the SVG's predetermined size and baked in pixel grid size. 
It's a misnomer that because SVG's can scale that they should be used at any size. Icons are hand tooled for specific sizes. Vector lines need to resolve to the very edge to the displayed pixels for icons to look sharp; this means that an icon intended for use at 16-pixel should not be used in a 48-pixel application because some pixels will appear to be "fuzzy."  This is why we deliver icons with a -16 or -32 or -48 suffix. So that future developers and designers will know at what scales these icons are meant to be displayed.
Flags: needinfo?(bbell)
Comment on attachment 8993708 [details]
Bug 1476217 - Part 3 - Add FastBlock to Content Blocking categories in about:preferences.

https://reviewboard.mozilla.org/r/258390/#review266390

> Bug 1474280 is now marked as fixed so this likely needs updating. Should the pref get set to true now by default? I see it is false by default on m-c.

I'll remove it from the patch now and talk to the team about how to proceed here. I think it's best to make a separate bug for turning it on.

> Please remove the -16 suffix since this is scalable and the other SVG files don't use a size suffix.

I don't feel strongly about this and I think Bryan has a point that it's intended to be only used in appropriate context, but I'm going to go with your suggestion as reviewer (also considering that we're actually blowing these up to 20px in about:preferences to match other icon sizes right now).

Thanks!
Comment on attachment 8994316 [details]
Bug 1476217 - Part 1 - Update Tracking Protection preferences for Content Blocking.

https://reviewboard.mozilla.org/r/258910/#review266380

I think based on what Bryan says that the blue slider toggle thingy has pretty significant buy-in from our UX team. I totally understand your concern (and I'm usually the one fighting against inconsistencies like this when reviewing other people's code) but in this case I think the answer is "no, let's use a special slider".

> Can some of this toggle styling be moved to common.inc.css, and use the colors that are defined there?

So, since we have this button in three places now (preferences, main menu, about:privatebrowsing), it seems like a bit of code debt to be duplicating styles for all three. The issue is that with the way we're doing this button right now all the numbers and colors are quite different in every implementation and there's not enough overlap IMO to easily share the styles in this patch.

As for using the colors in common.inc.css, I think the blue here might be a brand color we want to keep independent of the theme, though that would have to be discussed with UX.

For that reason I'd prefer to keep it like this and make a follow-up bug for cleaning up and consolidating the toggle styles. Does that sound acceptable to you?
Comment on attachment 8994316 [details]
Bug 1476217 - Part 1 - Update Tracking Protection preferences for Content Blocking.

https://reviewboard.mozilla.org/r/258910/#review267828

r=me with the below changes.

These shades of blue are so close that I don't see users getting confused by them, and they appear on different background colors as well. The menu panel's background color on Windows is #fff, but the preferences background color is #f9f9fa.

::: browser/themes/shared/incontentprefs/privacy.css:90
(Diff revisions 1 - 2)
>    background-color: #0a84ff;
>    border: 1px solid #0a84ff;

#2292d0 is used for the checkmark check, while #0a84ff is used for the checkmark hover border.

Can you please use #2292d0 here, and then use #0a84ff on the [checked] :hover and :-moz-focusring rules below?
Attachment #8994316 - Flags: review?(jaws) → review+
Comment on attachment 8994316 [details]
Bug 1476217 - Part 1 - Update Tracking Protection preferences for Content Blocking.

https://reviewboard.mozilla.org/r/258910/#review266380

> So, since we have this button in three places now (preferences, main menu, about:privatebrowsing), it seems like a bit of code debt to be duplicating styles for all three. The issue is that with the way we're doing this button right now all the numbers and colors are quite different in every implementation and there's not enough overlap IMO to easily share the styles in this patch.
> 
> As for using the colors in common.inc.css, I think the blue here might be a brand color we want to keep independent of the theme, though that would have to be discussed with UX.
> 
> For that reason I'd prefer to keep it like this and make a follow-up bug for cleaning up and consolidating the toggle styles. Does that sound acceptable to you?

Yeah, a follow-up would be fine.
Comment on attachment 8996700 [details]
Bug 1476217 - Part 5 - Disable content blocking sub-controls when content blocking is disabled.

https://reviewboard.mozilla.org/r/260778/#review267868
Attachment #8996700 - Flags: review?(jaws) → review+
hg error in cmd: hg push -r tip ssh://hg.mozilla.org/integration/autoland: pushing to ssh://hg.mozilla.org/integration/autoland
searching for changes
remote: adding changesets
remote: adding manifests
remote: adding file changes
remote: added 5 changesets with 25 changes to 13 files
remote: 
remote: ************************ ERROR *************************
remote: You are trying to commit a change to an FTL file.
remote: At the moment modifying FTL files requires a review from
remote: one of the L10n Drivers.
remote: Please, request review from either:
remote:     - Francesco Lodolo (:flod)
remote:     - Zibi Braniecki (:gandalf)
remote:     - Axel Hecht (:pike)
remote:     - Stas Malolepszy (:stas)
remote: ********************************************************
remote: 
remote: transaction abort!
remote: rollback completed
remote: pretxnchangegroup.mozhooks hook failed
abort: push failed on remote
Attachment #8994316 - Flags: review?(francesco.lodolo)
Attachment #8994317 - Flags: review?(francesco.lodolo)
Attachment #8993708 - Flags: review?(francesco.lodolo)
Comment on attachment 8994316 [details]
Bug 1476217 - Part 1 - Update Tracking Protection preferences for Content Blocking.

https://reviewboard.mozilla.org/r/258910/#review267948
Attachment #8994316 - Flags: review?(francesco.lodolo) → review+
Comment on attachment 8994317 [details]
Bug 1476217 - Part 2 - Add a "Restore Defaults" button to Content Blocking preferences.

https://reviewboard.mozilla.org/r/258912/#review267950
Attachment #8994317 - Flags: review?(francesco.lodolo) → review+
Comment on attachment 8993708 [details]
Bug 1476217 - Part 3 - Add FastBlock to Content Blocking categories in about:preferences.

https://reviewboard.mozilla.org/r/258390/#review267946

r+ with a comment added (and capitalization clarified, even if not a blocker)

::: browser/locales/en-US/browser/preferences/preferences.ftl:815
(Diff revision 4)
>  content-blocking-toggle-label-off = OFF
>    .accesskey = O
>  
>  content-blocking-category-label = Choose what to block
>  
> +content-blocking-fastblock-label = Slow Tracking Elements

I believe this needs a short comment. Does this mean "Tracking Elements that are slow to load"?

The description doesn't include "tracking", which makes things more confusing.

::: browser/locales/en-US/browser/preferences/preferences.ftl:819
(Diff revision 4)
>  
> +content-blocking-fastblock-label = Slow Tracking Elements
> +  .accesskey = S
> +content-blocking-fastblock-description = Blocks third-party content that takes longer than 5 seconds to load.
> +content-blocking-fastblock-option-enabled =
> +  .label = Always Block

Is Title Case expected for these? I can't spot this on Invision, and the code makes me think they're used in a dropdown list.
Attachment #8993708 - Flags: review?(francesco.lodolo) → review+
Comment on attachment 8993708 [details]
Bug 1476217 - Part 3 - Add FastBlock to Content Blocking categories in about:preferences.

https://reviewboard.mozilla.org/r/258390/#review267946

> Is Title Case expected for these? I can't spot this on Invision, and the code makes me think they're used in a dropdown list.

You're right, thanks!
Pushed by jhofmann@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/a1aade7a6cc2
Part 1 - Update Tracking Protection preferences for Content Blocking. r=flod,jaws
https://hg.mozilla.org/integration/autoland/rev/8d9cb12c3cd7
Part 2 - Add a "Restore Defaults" button to Content Blocking preferences. r=flod,jaws
https://hg.mozilla.org/integration/autoland/rev/33040e6da136
Part 3 - Add FastBlock to Content Blocking categories in about:preferences. r=flod,jaws
https://hg.mozilla.org/integration/autoland/rev/2575008d5807
Part 4 - Add tests for content blocking. r=jaws
https://hg.mozilla.org/integration/autoland/rev/c20648606d7c
Part 5 - Disable content blocking sub-controls when content blocking is disabled. r=jaws
Blocks: 1480441
Depends on: 1480667
Depends on: 1480809
Blocks: 1481233
Depends on: 1482281
Depends on: 1482282
Depends on: 1482285
Depends on: 1487380
Depends on: 1495081
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: