Issue 5973 - broken icons on Firefox v50-53

Issue 5973 - broken icons on Firefox v50-53

[saroyanm, 2017-11-03 18:02:27]

@Thomas, @Wladimir: Would be great if somebody from you can have a chance to
review this.

https://codereview.adblockplus.org/29596678/diff/29596679/skin/icons/checkmar...
File skin/icons/checkmark.svg (right):

https://codereview.adblockplus.org/29596678/diff/29596679/skin/icons/checkmar...
skin/icons/checkmark.svg:1: <svg xmlns="http://www.w3.org/2000/svg">
This Icon is also being used in the mobile version.

https://codereview.adblockplus.org/29596678/diff/29596679/skin/icons/trash.svg
File skin/icons/trash.svg (right):

https://codereview.adblockplus.org/29596678/diff/29596679/skin/icons/trash.sv...
skin/icons/trash.svg:1: <svg xmlns="http://www.w3.org/2000/svg">
This Icon is being used in mobile version as well.

[saroyanm, 2017-11-03 18:28:30]

https://codereview.adblockplus.org/29596678/diff/29596679/skin/icons/checkmar...
File skin/icons/checkmark.svg (right):

https://codereview.adblockplus.org/29596678/diff/29596679/skin/icons/checkmar...
skin/icons/checkmark.svg:1: <svg xmlns="http://www.w3.org/2000/svg">
On 2017/11/03 18:02:26, saroyanm wrote:
> This Icon is also being used in the mobile version.

Updated mobile-options.html to use "ID".

https://codereview.adblockplus.org/29596678/diff/29596679/skin/icons/trash.svg
File skin/icons/trash.svg (right):

https://codereview.adblockplus.org/29596678/diff/29596679/skin/icons/trash.sv...
skin/icons/trash.svg:1: <svg xmlns="http://www.w3.org/2000/svg">
On 2017/11/03 18:02:26, saroyanm wrote:
> This Icon is being used in mobile version as well.

Updated mobile-options.html to use "ID".

[Thomas Greiner, 2017-11-03 19:04:11]

As a quick fix this should hopefully do the job so most my comments are
regarding the colors we use. Later on we should, however, investigate ways how
we can tackle this in a more elegant way because there's quite a bit of
duplication in there.

https://codereview.adblockplus.org/29596678/diff/29596695/skin/icons/checkmar...
File skin/icons/checkmark.svg (right):

https://codereview.adblockplus.org/29596678/diff/29596695/skin/icons/checkmar...
skin/icons/checkmark.svg:5: <view id="approve" viewBox="10 40 30 30"/>
Detail: I'd suggest calling it "approved" since "approve" is a verb, not an
adjective which would be more fitting for describing a state.

https://codereview.adblockplus.org/29596678/diff/29596695/skin/icons/checkmar...
skin/icons/checkmark.svg:6: <polygon points="20.49 31.17 13.29 23.98 14.71 22.56
20.49 28.34 33.29 15.54 34.71 16.95 20.49 31.17" transform="translate(0, 30)"
fill="#008000"/>
Detail: According to the options page style guide, the color green is specified
as "#43A548".

https://codereview.adblockplus.org/29596678/diff/29596695/skin/icons/delete.svg
File skin/icons/delete.svg (right):

https://codereview.adblockplus.org/29596678/diff/29596695/skin/icons/delete.s...
skin/icons/delete.svg:3: <polygon points="42.71 6.71 41.29 5.29 24 22.59 6.71
5.29 5.29 6.71 22.59 24 5.29 41.29 6.71 42.71 24 25.41 41.29 42.71 42.71 41.29
25.41 24 42.71 6.71" fill="#FFF"/>
Detail: Is this color meant to be completely white or `#F3F3F3` as specified in
the options page style guide?

https://codereview.adblockplus.org/29596678/diff/29596695/skin/icons/gear.svg
File skin/icons/gear.svg (right):

https://codereview.adblockplus.org/29596678/diff/29596695/skin/icons/gear.svg...
skin/icons/gear.svg:3: <path
d="M42.3,26.4c0-.72.25-1.68.25-2.4a8,8,0,0,0-.25-2.4L47.48,18a1.1,1.1,0,0,0,.25-1.68l-4.94-8.4a1.11,1.11,0,0,0-1.48-.72L35.39,9.84a15.85,15.85,0,0,0-4.44-2.4L30.21,1.2A1.15,1.15,0,0,0,29,0H19.1a1.15,1.15,0,0,0-1.23,1.2l-.74,6a19.11,19.11,0,0,0-4.44,2.4L6.76,7.2a1.2,1.2,0,0,0-1.48.48L.34,16.08c-.49.72-.49,1.44.25,1.68L5.77,21.6c0,.72-.25,1.68-.25,2.4a8,8,0,0,0,.25,2.4L.59,30a1.1,1.1,0,0,0-.25,1.68l4.94,8.4a1.11,1.11,0,0,0,1.48.72l5.92-2.64a15.85,15.85,0,0,0,4.44,2.4l.74,6.24A1.15,1.15,0,0,0,19.1,48H29a1.15,1.15,0,0,0,1.23-1.2l.74-6.24a19.11,19.11,0,0,0,4.44-2.4l5.92,2.64a1.2,1.2,0,0,0,1.48-.48l4.94-8.4a1.45,1.45,0,0,0-.25-1.68ZM24,34A10,10,0,1,1,34,24,10,10,0,0,1,24,34Z"
fill="#099DD1"/>
Detail: Where is this color coming from? I can't find it in the options page
style guide color palette.

I'm also seeing it being used in a bunch of other SVGs.

https://codereview.adblockplus.org/29596678/diff/29596695/skin/icons/tooltip.svg
File skin/icons/tooltip.svg (right):

https://codereview.adblockplus.org/29596678/diff/29596695/skin/icons/tooltip....
skin/icons/tooltip.svg:1: <svg id="8c46c7bc-3457-406a-9a79-62c8849fc98c"
data-name="artwork" xmlns="http://www.w3.org/2000/svg" viewBox="0 0 48 48"
fill="#099DD1">
Doesn't `<svg fill="...">` apply the color to the entire SVG rather than to the
`<path>` elements? I assume not since you probably tested it but it still seems
counter-intuitive.

https://codereview.adblockplus.org/29596678/diff/29596695/skin/icons/tooltip....
skin/icons/tooltip.svg:1: <svg id="8c46c7bc-3457-406a-9a79-62c8849fc98c"
data-name="artwork" xmlns="http://www.w3.org/2000/svg" viewBox="0 0 48 48"
fill="#099DD1">
Detail: You've been cleaning up the SVG but I noticed that there's still some
meta data attached to the `<svg>` tag so we could clean that up as well.

https://codereview.adblockplus.org/29596678/diff/29596695/skin/icons/trash.svg
File skin/icons/trash.svg (right):

https://codereview.adblockplus.org/29596678/diff/29596695/skin/icons/trash.sv...
skin/icons/trash.svg:8: <view id="hover" viewBox="0 48 48 48"/>
Detail: Let's add a comment above to explain why we need this workaround so that
we can get rid of it as soon as we drop support for older Firefox versions which
don't support the "mask" property.

This also applies to other SVG files with this workaround.

https://codereview.adblockplus.org/29596678/diff/29596695/skin/icons/trash.sv...
skin/icons/trash.svg:12: <path
d="M44,7H33V5a5,5,0,0,0-5-5H20a5,5,0,0,0-5,5V7H4V9H6.68V39a9,9,0,0,0,9,9H32.23a9,9,0,0,0,9-9V9H44ZM17,5a3,3,0,0,1,3-3h8a3,3,0,0,1,3,3V7H17ZM39.23,39a7,7,0,0,1-7,7H15.68a7,7,0,0,1-7-7V9H39.23Z"
transform="translate(0, 48)" fill="#5CBCE1"/>
Suggestion: You can avoid duplicating the "transform" attribute for each
element, you can group all them inside a `<g>` element and add it to this one
instead.

[saroyanm, 2017-11-03 19:18:17]

https://codereview.adblockplus.org/29596678/diff/29596695/skin/icons/checkmar...
File skin/icons/checkmark.svg (right):

https://codereview.adblockplus.org/29596678/diff/29596695/skin/icons/checkmar...
skin/icons/checkmark.svg:5: <view id="approve" viewBox="10 40 30 30"/>
On 2017/11/03 19:04:10, Thomas Greiner wrote:
> Detail: I'd suggest calling it "approved" since "approve" is a verb, not an
> adjective which would be more fitting for describing a state.

agree

https://codereview.adblockplus.org/29596678/diff/29596695/skin/icons/checkmar...
skin/icons/checkmark.svg:6: <polygon points="20.49 31.17 13.29 23.98 14.71 22.56
20.49 28.34 33.29 15.54 34.71 16.95 20.49 31.17" transform="translate(0, 30)"
fill="#008000"/>
On 2017/11/03 19:04:10, Thomas Greiner wrote:
> Detail: According to the options page style guide, the color green is
specified
> as "#43A548".

I can't see it specified, but I'd rather keep the colors as they used to be
before this change and address color change separately.

https://codereview.adblockplus.org/29596678/diff/29596695/skin/icons/delete.svg
File skin/icons/delete.svg (right):

https://codereview.adblockplus.org/29596678/diff/29596695/skin/icons/delete.s...
skin/icons/delete.svg:3: <polygon points="42.71 6.71 41.29 5.29 24 22.59 6.71
5.29 5.29 6.71 22.59 24 5.29 41.29 6.71 42.71 24 25.41 41.29 42.71 42.71 41.29
25.41 24 42.71 6.71" fill="#FFF"/>
On 2017/11/03 19:04:10, Thomas Greiner wrote:
> Detail: Is this color meant to be completely white or `#F3F3F3` as specified
in
> the options page style guide?
I can't see this color being specified, but I'd rather keep current color for
now, as it has been used beforehand as well.

https://codereview.adblockplus.org/29596678/diff/29596695/skin/icons/gear.svg
File skin/icons/gear.svg (right):

https://codereview.adblockplus.org/29596678/diff/29596695/skin/icons/gear.svg...
skin/icons/gear.svg:3: <path
d="M42.3,26.4c0-.72.25-1.68.25-2.4a8,8,0,0,0-.25-2.4L47.48,18a1.1,1.1,0,0,0,.25-1.68l-4.94-8.4a1.11,1.11,0,0,0-1.48-.72L35.39,9.84a15.85,15.85,0,0,0-4.44-2.4L30.21,1.2A1.15,1.15,0,0,0,29,0H19.1a1.15,1.15,0,0,0-1.23,1.2l-.74,6a19.11,19.11,0,0,0-4.44,2.4L6.76,7.2a1.2,1.2,0,0,0-1.48.48L.34,16.08c-.49.72-.49,1.44.25,1.68L5.77,21.6c0,.72-.25,1.68-.25,2.4a8,8,0,0,0,.25,2.4L.59,30a1.1,1.1,0,0,0-.25,1.68l4.94,8.4a1.11,1.11,0,0,0,1.48.72l5.92-2.64a15.85,15.85,0,0,0,4.44,2.4l.74,6.24A1.15,1.15,0,0,0,19.1,48H29a1.15,1.15,0,0,0,1.23-1.2l.74-6.24a19.11,19.11,0,0,0,4.44-2.4l5.92,2.64a1.2,1.2,0,0,0,1.48-.48l4.94-8.4a1.45,1.45,0,0,0-.25-1.68ZM24,34A10,10,0,1,1,34,24,10,10,0,0,1,24,34Z"
fill="#099DD1"/>
On 2017/11/03 19:04:10, Thomas Greiner wrote:
> Detail: Where is this color coming from? I can't find it in the options page
> style guide color palette.
> 
> I'm also seeing it being used in a bunch of other SVGs.

This is old, color, not all color changes has been addressed, as specification
used to keep being updated, see ->
https://bitbucket.org/adblockplus/spec/src/a3834cf44ff721fa4b3679fcda46b8fe05...


I think we should keep this color and updated accordingly all instances in
future.

https://codereview.adblockplus.org/29596678/diff/29596695/skin/icons/tooltip.svg
File skin/icons/tooltip.svg (right):

https://codereview.adblockplus.org/29596678/diff/29596695/skin/icons/tooltip....
skin/icons/tooltip.svg:1: <svg id="8c46c7bc-3457-406a-9a79-62c8849fc98c"
data-name="artwork" xmlns="http://www.w3.org/2000/svg" viewBox="0 0 48 48"
fill="#099DD1">
On 2017/11/03 19:04:10, Thomas Greiner wrote:
> Detail: You've been cleaning up the SVG but I noticed that there's still some
> meta data attached to the `<svg>` tag so we could clean that up as well.

Yes, missed that, will update.

https://codereview.adblockplus.org/29596678/diff/29596695/skin/icons/tooltip....
skin/icons/tooltip.svg:1: <svg id="8c46c7bc-3457-406a-9a79-62c8849fc98c"
data-name="artwork" xmlns="http://www.w3.org/2000/svg" viewBox="0 0 48 48"
fill="#099DD1">
On 2017/11/03 19:04:10, Thomas Greiner wrote:
> Doesn't `<svg fill="...">` apply the color to the entire SVG rather than to
the
> `<path>` elements? I assume not since you probably tested it but it still
seems
> counter-intuitive.

agree.

https://codereview.adblockplus.org/29596678/diff/29596695/skin/icons/trash.svg
File skin/icons/trash.svg (right):

https://codereview.adblockplus.org/29596678/diff/29596695/skin/icons/trash.sv...
skin/icons/trash.svg:8: <view id="hover" viewBox="0 48 48 48"/>
On 2017/11/03 19:04:10, Thomas Greiner wrote:
> Detail: Let's add a comment above to explain why we need this workaround so
that
> we can get rid of it as soon as we drop support for older Firefox versions
which
> don't support the "mask" property.
> 
> This also applies to other SVG files with this workaround.

This approach looks to be more performant, don't you think that it's better to
keep this approach rather than using masks ?

https://codereview.adblockplus.org/29596678/diff/29596695/skin/icons/trash.sv...
skin/icons/trash.svg:12: <path
d="M44,7H33V5a5,5,0,0,0-5-5H20a5,5,0,0,0-5,5V7H4V9H6.68V39a9,9,0,0,0,9,9H32.23a9,9,0,0,0,9-9V9H44ZM17,5a3,3,0,0,1,3-3h8a3,3,0,0,1,3,3V7H17ZM39.23,39a7,7,0,0,1-7,7H15.68a7,7,0,0,1-7-7V9H39.23Z"
transform="translate(0, 48)" fill="#5CBCE1"/>
On 2017/11/03 19:04:10, Thomas Greiner wrote:
> Suggestion: You can avoid duplicating the "transform" attribute for each
> element, you can group all them inside a `<g>` element and add it to this one
> instead.

Interesting approach, will test and update accordingly.

[saroyanm, 2017-11-03 19:35:09]

I didn't updated colors, as I used here the existing colors, the colors in
general should be updated to reflect the latest color changes in the specs
(there are also more coming there), but I'd rather keep that outside of the
scope of current review.

https://codereview.adblockplus.org/29596678/diff/29596695/skin/icons/checkmar...
File skin/icons/checkmark.svg (right):

https://codereview.adblockplus.org/29596678/diff/29596695/skin/icons/checkmar...
skin/icons/checkmark.svg:5: <view id="approve" viewBox="10 40 30 30"/>
On 2017/11/03 19:18:16, saroyanm wrote:
> On 2017/11/03 19:04:10, Thomas Greiner wrote:
> > Detail: I'd suggest calling it "approved" since "approve" is a verb, not an
> > adjective which would be more fitting for describing a state.
> 
> agree

Done.

https://codereview.adblockplus.org/29596678/diff/29596695/skin/icons/tooltip.svg
File skin/icons/tooltip.svg (right):

https://codereview.adblockplus.org/29596678/diff/29596695/skin/icons/tooltip....
skin/icons/tooltip.svg:1: <svg id="8c46c7bc-3457-406a-9a79-62c8849fc98c"
data-name="artwork" xmlns="http://www.w3.org/2000/svg" viewBox="0 0 48 48"
fill="#099DD1">
On 2017/11/03 19:18:16, saroyanm wrote:
> On 2017/11/03 19:04:10, Thomas Greiner wrote:
> > Doesn't `<svg fill="...">` apply the color to the entire SVG rather than to
> the
> > `<path>` elements? I assume not since you probably tested it but it still
> seems
> > counter-intuitive.
> 
> agree.

Done.

https://codereview.adblockplus.org/29596678/diff/29596695/skin/icons/tooltip....
skin/icons/tooltip.svg:1: <svg id="8c46c7bc-3457-406a-9a79-62c8849fc98c"
data-name="artwork" xmlns="http://www.w3.org/2000/svg" viewBox="0 0 48 48"
fill="#099DD1">
On 2017/11/03 19:18:16, saroyanm wrote:
> On 2017/11/03 19:04:10, Thomas Greiner wrote:
> > Detail: You've been cleaning up the SVG but I noticed that there's still
some
> > meta data attached to the `<svg>` tag so we could clean that up as well.
> 
> Yes, missed that, will update.

Done.

https://codereview.adblockplus.org/29596678/diff/29596695/skin/icons/trash.svg
File skin/icons/trash.svg (right):

https://codereview.adblockplus.org/29596678/diff/29596695/skin/icons/trash.sv...
skin/icons/trash.svg:12: <path
d="M44,7H33V5a5,5,0,0,0-5-5H20a5,5,0,0,0-5,5V7H4V9H6.68V39a9,9,0,0,0,9,9H32.23a9,9,0,0,0,9-9V9H44ZM17,5a3,3,0,0,1,3-3h8a3,3,0,0,1,3,3V7H17ZM39.23,39a7,7,0,0,1-7,7H15.68a7,7,0,0,1-7-7V9H39.23Z"
transform="translate(0, 48)" fill="#5CBCE1"/>
On 2017/11/03 19:18:16, saroyanm wrote:
> On 2017/11/03 19:04:10, Thomas Greiner wrote:
> > Suggestion: You can avoid duplicating the "transform" attribute for each
> > element, you can group all them inside a `<g>` element and add it to this
one
> > instead.
> 
> Interesting approach, will test and update accordingly.

Done.

[Thomas Greiner, 2017-11-03 19:47:36]

LGTM

On 2017/11/03 19:35:09, saroyanm wrote:
> I didn't updated colors, as I used here the existing colors, the colors in
> general should be updated to reflect the latest color changes in the specs
> (there are also more coming there), but I'd rather keep that outside of the
> scope of current review.

That's fine with me.

https://codereview.adblockplus.org/29596678/diff/29596695/skin/icons/trash.svg
File skin/icons/trash.svg (right):

https://codereview.adblockplus.org/29596678/diff/29596695/skin/icons/trash.sv...
skin/icons/trash.svg:8: <view id="hover" viewBox="0 48 48 48"/>
On 2017/11/03 19:18:17, saroyanm wrote:
> On 2017/11/03 19:04:10, Thomas Greiner wrote:
> > Detail: Let's add a comment above to explain why we need this workaround so
> that
> > we can get rid of it as soon as we drop support for older Firefox versions
> which
> > don't support the "mask" property.
> > 
> > This also applies to other SVG files with this workaround.
> 
> This approach looks to be more performant, don't you think that it's better to
> keep this approach rather than using masks ? 

If performance is more important than maintainability. At this point I'm not
sure this is the case but even if it is, we should still be able to find ways to
improve maintainability even without "mask". We'll need to look at that later
on.

[saroyanm, 2017-11-03 20:06:53]

On 2017/11/03 19:47:36, Thomas Greiner wrote:
> LGTM
> 
> On 2017/11/03 19:35:09, saroyanm wrote:
> > I didn't updated colors, as I used here the existing colors, the colors in
> > general should be updated to reflect the latest color changes in the specs
> > (there are also more coming there), but I'd rather keep that outside of the
> > scope of current review.
> 
> That's fine with me.
> 
> https://codereview.adblockplus.org/29596678/diff/29596695/skin/icons/trash.svg
> File skin/icons/trash.svg (right):
> 
>
https://codereview.adblockplus.org/29596678/diff/29596695/skin/icons/trash.sv...
> skin/icons/trash.svg:8: <view id="hover" viewBox="0 48 48 48"/>
> On 2017/11/03 19:18:17, saroyanm wrote:
> > On 2017/11/03 19:04:10, Thomas Greiner wrote:
> > > Detail: Let's add a comment above to explain why we need this workaround
so
> > that
> > > we can get rid of it as soon as we drop support for older Firefox versions
> > which
> > > don't support the "mask" property.
> > > 
> > > This also applies to other SVG files with this workaround.
> > 
> > This approach looks to be more performant, don't you think that it's better
to
> > keep this approach rather than using masks ? 
> 
> If performance is more important than maintainability. At this point I'm not
> sure this is the case but even if it is, we should still be able to find ways
to
> improve maintainability even without "mask". We'll need to look at that later
> on.

agree, I'll followup on that in the separate issue.