Closed Bug 1175689 Opened 5 years ago Closed 5 years ago

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

Categories

(Firefox :: Address Bar, defect, P1)

defect
Points:
2

Tracking

()

VERIFIED FIXED
Firefox 42
Iteration:
42.2 - Jul 27
Tracking Status
firefox41 --- affected
firefox42 --- verified

People

(Reporter: bgrins, Assigned: Paolo)

References

Details

(Whiteboard: [fxprivacy] [campaign])

Attachments

(5 files)

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.
Flags: firefox-backlog?
Flags: firefox-backlog? → firefox-backlog+
See Also: → 1175858
Rank: 2
Attached image Mockup
Screenshot from Bug 1174986
Assignee: nobody → paolo.mozmail
Points: --- → 5
Flags: qe-verify+
Status: NEW → ASSIGNED
Iteration: --- → 42.1 - Jul 13
Attachment #8626244 - Attachment description: grouped-icon.png → Mockup
QA Contact: mwobensmith
Depends on: 1180202
Blocks: 1180213
Points: 5 → 2
Component: General → Location Bar
Bug 1175689 - Group the existing site identity URL bar icon with the tracking protection shield when TP is enabled. r=ttaubert
Attachment #8631047 - Flags: review?(ttaubert)
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.
Blocks: 1175858
(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.
Attachment #8631047 - Flags: review?(ttaubert)
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 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
Attachment #8631047 - Flags: review?(ttaubert)
(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.
(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.
(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.
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
(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?
(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 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
(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.
(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.
Attachment #8631047 - Flags: review?(ttaubert)
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!)
(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.
(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.
Flags: needinfo?(ttaubert)
(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.
Flags: needinfo?(ttaubert)
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
Attachment #8631047 - Flags: review?(ttaubert)
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!
Attachment #8631047 - Flags: review?(ttaubert) → review+
Iteration: 42.1 - Jul 13 → 42.2 - Jul 27
Depends on: 1183774
No longer depends on: 1183774
https://hg.mozilla.org/mozilla-central/rev/b92c7e0e9789
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 42
Depends on: 1184445
Whiteboard: [fxprivacy] → [fxprivacy] [campaign]
Verified as fixed using:

Win 7 x64, Mac Os X 10.8.5, Ubuntu 12.04 x86
FF 42
Build Id: 20150722030205
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.