1175689 - Group the existing site identity URL bar icon with the tracking protection shield when TP is enabled

Status: VERIFIED FIXED

(Firefox :: Address Bar, defect, P1)

Description

[Brian Grinstead [:bgrins]]

When tracking protection is enabled and a site has tracking elements, the TP shield should show up to the left of the 'globe' icon.  That shield's state should depend on whether the site has been whitelisted and TP is blocking elements.

Comment 1

[Brian Grinstead [:bgrins]]

Attachment: Mockup

Screenshot from Bug 1174986

Comment 2

[:Paolo Amadini]

Attachment: Hover state with notifications and site label visible

Comment 3

[:Paolo Amadini]

Attachment: Basic state without other notifications

Comment 4

[:Paolo Amadini]

Attachment: Notifications but interaction with identity block disabled

Comment 5

[:Paolo Amadini]

Attachment: MozReview Request: Bug 1175689 - Group the existing site identity URL bar icon with the tracking protection shield when TP is enabled. r=ttaubert

Bug 1175689 - Group the existing site identity URL bar icon with the tracking protection shield when TP is enabled. r=ttaubert

Comment 6

[:Paolo Amadini]

https://reviewboard.mozilla.org/r/12829/#review11399

::: browser/themes/shared/identity-block.inc.css:78
(Diff revision 1)
> +#tracking-protection-icon:not([block-disabled]):not([block-active]) {
> +  display: none;
> +}

Wouldn't it be simpler and less code to use a single attribute rather than two mutually exclusive attributes, for example state="blocked-tracking-content", state="loaded-tracking-content", and no state attribute? I could refactor the existing code to use one attribute only.

Asking only for feedback now, tests in the next version.

Comment 7

[Tim Taubert [:ttaubert] (inactive)]

(In reply to :Paolo Amadini from comment #6)
> > +#tracking-protection-icon:not([block-disabled]):not([block-active]) {
> > +  display: none;
> > +}
> 
> Wouldn't it be simpler and less code to use a single attribute rather than
> two mutually exclusive attributes, for example
> state="blocked-tracking-content", state="loaded-tracking-content", and no
> state attribute? I could refactor the existing code to use one attribute
> only.

Yeah, go for it.

Comment 8

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 8631047 [details]
MozReview Request: Bug 1175689 - Group the existing site identity URL bar icon with the tracking protection shield when TP is enabled. r=ttaubert

https://reviewboard.mozilla.org/r/12829/#review11521

::: browser/themes/linux/jar.mn:79
(Diff revision 1)
> +  skin/classic/browser/tracking-protection-16.svg           (../shared/tracking-protection-16.svg)
> +  skin/classic/browser/tracking-protection-disabled-16.svg  (../shared/tracking-protection-disabled-16.svg)

Please move those to themes/shared/identity-block/. I just landed the patch to create that today so you couldn't know about it :)

::: browser/themes/shared/tracking-protection-16.svg:2
(Diff revision 1)
> +<!DOCTYPE svg PUBLIC "-//W3C//DTD SVG 1.1//EN" "http://www.w3.org/Graphics/SVG/1.1/DTD/svg11.dtd">

Please replace this with:

<!-- 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/. -->

::: browser/themes/shared/tracking-protection-16.svg:4
(Diff revision 1)
> +<svg version="1.1"

Please remove the attribute tag.

Same for the other SVG. I was told this is how SVG's should look like and we've done this with all the other new SVGs we landed.

::: browser/themes/shared/tracking-protection-16.svg:30
(Diff revision 1)
> +  <use xlink:href="#shape-shield-outer" mask="url(#mask-shield-cutout)" class="icon-default" />

Please replace

class="icon-default"

... with

fill="808080"

... and remove the <style> element.

(Same for the other SVG.)

Comment 9

[:Paolo Amadini]

Comment on attachment 8631047 [details]
MozReview Request: Bug 1175689 - Group the existing site identity URL bar icon with the tracking protection shield when TP is enabled. r=ttaubert

Bug 1175689 - Group the existing site identity URL bar icon with the tracking protection shield when TP is enabled. r=ttaubert

Comment 10

[:Paolo Amadini]

(In reply to Tim Taubert [:ttaubert] from comment #8)
> Please replace
> class="icon-default"

This actually makes the two new SVGs consistent with the ones already in the folder, even if the style is used only once in one of the SVGs, so I'd keep it.

Comment 11

[:Paolo Amadini]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=2d2ff18501cb

Comment 12

[Tim Taubert [:ttaubert] (inactive)]

(In reply to :Paolo Amadini from comment #10)
> (In reply to Tim Taubert [:ttaubert] from comment #8)
> > Please replace
> > class="icon-default"
> 
> This actually makes the two new SVGs consistent with the ones already in the
> folder, even if the style is used only once in one of the SVGs, so I'd keep
> it.

I wouldn't mind having a follow-up to fix those then. Smaller and simpler SVG files are probably a good thing.

Comment 13

[:Paolo Amadini]

(In reply to Tim Taubert [:ttaubert] from comment #12)
> I wouldn't mind having a follow-up to fix those then. Smaller and simpler
> SVG files are probably a good thing.

I mean, if you look at all the SVGs in the folder after applying this patch, all except one do use the style as a way to avoid repeating the color value. I'm not sure repeating the color every time is strictly better than using the style.

Another way to reduce and simplify the SVGs would be to have multiple icons per file, in which case they could reuse the same style declarations, and changing a color would change it consistently for all the icon variants. But again I'm not sure if this would work given the workflow for obtaining new assets from the visual design team, if that involves creating individual files.

Comment 14

[Brian Grinstead [:bgrins]]

https://reviewboard.mozilla.org/r/12829/#review11579

::: browser/themes/shared/identity-block/identity-block.inc.css:71
(Diff revision 2)
> +  list-style-image: url(chrome://browser/skin/tracking-protection-16.svg);

Could you use chrome://browser/skin/controlcenter/tracking-protection.svg and chrome://browser/skin/controlcenter/tracking-protection-disabled.svg instead of adding the new icons?  I'm guesing these are the same assets that were added for the CC bug

Comment 15

[:Paolo Amadini]

(In reply to Brian Grinstead [:bgrins] from comment #14)
> Could you use chrome://browser/skin/controlcenter/tracking-protection.svg
> and chrome://browser/skin/controlcenter/tracking-protection-disabled.svg
> instead of adding the new icons?  I'm guesing these are the same assets that
> were added for the CC bug

Hm, I assumed tracking-protection-16 and tracking-protection-24 in the provided assets archive had different shapes, but apparently that's not the case as we used the 16px version for the larger icon?

Tim, if we reuse the SVG maybe we should move the current one to a different folder, or unify the folders?

Comment 16

[Brian Grinstead [:bgrins]]

(In reply to :Paolo Amadini from comment #15)
> (In reply to Brian Grinstead [:bgrins] from comment #14)
> > Could you use chrome://browser/skin/controlcenter/tracking-protection.svg
> > and chrome://browser/skin/controlcenter/tracking-protection-disabled.svg
> > instead of adding the new icons?  I'm guesing these are the same assets that
> > were added for the CC bug
> 
> Hm, I assumed tracking-protection-16 and tracking-protection-24 in the
> provided assets archive had different shapes, but apparently that's not the
> case as we used the 16px version for the larger icon?

From what I gathered they were the same icons, and the size didn't really matter since it was a vector

Comment 17

[:Paolo Amadini]

Comment on attachment 8631047 [details]
MozReview Request: Bug 1175689 - Group the existing site identity URL bar icon with the tracking protection shield when TP is enabled. r=ttaubert

Bug 1175689 - Group the existing site identity URL bar icon with the tracking protection shield when TP is enabled. r=ttaubert

Comment 18

[Tim Taubert [:ttaubert] (inactive)]

(In reply to :Paolo Amadini from comment #13)
> (In reply to Tim Taubert [:ttaubert] from comment #12)
> > I wouldn't mind having a follow-up to fix those then. Smaller and simpler
> > SVG files are probably a good thing.
> 
> I mean, if you look at all the SVGs in the folder after applying this patch,
> all except one do use the style as a way to avoid repeating the color value.
> I'm not sure repeating the color every time is strictly better than using
> the style.

I wasn't talking about repeating color values. If the color value / class name is used only once we should replace it. If the class is used multiple times we should obviously keep it.

Comment 19

[Tim Taubert [:ttaubert] (inactive)]

(In reply to :Paolo Amadini from comment #15)
> (In reply to Brian Grinstead [:bgrins] from comment #14)
> > Could you use chrome://browser/skin/controlcenter/tracking-protection.svg
> > and chrome://browser/skin/controlcenter/tracking-protection-disabled.svg
> > instead of adding the new icons?  I'm guesing these are the same assets that
> > were added for the CC bug
> 
> Hm, I assumed tracking-protection-16 and tracking-protection-24 in the
> provided assets archive had different shapes, but apparently that's not the
> case as we used the 16px version for the larger icon?

Please use the 16px versions. We should file another bug to fix the icons in the control panel and use the 24px version.

Comment 20

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 8631047 [details]
MozReview Request: Bug 1175689 - Group the existing site identity URL bar icon with the tracking protection shield when TP is enabled. r=ttaubert

https://reviewboard.mozilla.org/r/12829/#review11713

Please re-include the correct 16px versions of the icon. The existing ones used in the control center are accidentally 16px but should be the 24px version.

(The rest of the patch looks great!)

Comment 21

[Tim Taubert [:ttaubert] (inactive)]

(In reply to Tim Taubert [:ttaubert] from comment #19)
> Please use the 16px versions. We should file another bug to fix the icons in
> the control panel and use the 24px version.

Filed bug 1183079.

Comment 22

[Brian Grinstead [:bgrins]]

(In reply to Tim Taubert [:ttaubert] from comment #20)
> Comment on attachment 8631047 [details]
> MozReview Request: Bug 1175689 - Group the existing site identity URL bar
> icon with the tracking protection shield when TP is enabled. r=ttaubert
> 
> https://reviewboard.mozilla.org/r/12829/#review11713
> 
> Please re-include the correct 16px versions of the icon. The existing ones
> used in the control center are accidentally 16px but should be the 24px
> version.

Is there a difference in how these two end up looking?  I assumed that it was the same shape but just scaled up from 16 to 24 and since it was a vector it wouldn't affect what it would look like.  So thought we could save adding two extra files by just reusing the one asset.  Doesn't really matter to me either way, just curious if this will have an effect.

Comment 23

[Tim Taubert [:ttaubert] (inactive)]

(In reply to Brian Grinstead [:bgrins] from comment #22)
> Is there a difference in how these two end up looking?  I assumed that it
> was the same shape but just scaled up from 16 to 24 and since it was a
> vector it wouldn't affect what it would look like.  So thought we could save
> adding two extra files by just reusing the one asset.  Doesn't really matter
> to me either way, just curious if this will have an effect.

The 24px version seems to have a thicker stroke, that's what I remember from a quick look - maybe there are more changes. So it should look slightly different. AFAIK icons at those tiny sizes are usually adjusted for every size because they don't scale well.

Comment 24

[:Paolo Amadini]

Comment on attachment 8631047 [details]
MozReview Request: Bug 1175689 - Group the existing site identity URL bar icon with the tracking protection shield when TP is enabled. r=ttaubert

Bug 1175689 - Group the existing site identity URL bar icon with the tracking protection shield when TP is enabled. r=ttaubert

Comment 25

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 8631047 [details]
MozReview Request: Bug 1175689 - Group the existing site identity URL bar icon with the tracking protection shield when TP is enabled. r=ttaubert

https://reviewboard.mozilla.org/r/12829/#review11807

Ship It!

Comment 26

[:Paolo Amadini]

https://hg.mozilla.org/integration/fx-team/rev/b92c7e0e9789

Comment 27

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/b92c7e0e9789

Comment 28

[Catalin Varga [QA][:VarCat]]

Verified as fixed using:

Win 7 x64, Mac Os X 10.8.5, Ubuntu 12.04 x86
FF 42
Build Id: 20150722030205