skin/icons/trash.svg - Issue 29596678: Issue 5973 - broken icons on Firefox v50-53

Delta Between Two Patch Sets: skin/icons/trash.svg

Issue 29596678: Issue 5973 - broken icons on Firefox v50-53 (Closed)

Left Patch Set: Use IDs for shared icons Created Nov. 3, 2017, 6:25 p.m.

Right Patch Set: Created Nov. 3, 2017, 7:29 p.m.

Left:
Right:

Use n/p to move between diff chunks; N/P to move between comments.

Jump to:

Left: Side by side diff | Download
Right: Side by side diff | Download

LEFT	RIGHT
1 <svg xmlns="http://www.w3.org/2000/svg">	1 <svg xmlns="http://www.w3.org/2000/svg">

2 <view id="default" viewBox="0 0 48 48"/>	2 <view id="default" viewBox="0 0 48 48"/>

3 <rect x="22.96" y="16" width="2" height="22" fill="#099DD1"/>	3 <rect x="22.96" y="16" width="2" height="22" fill="#099DD1"/>

4 <rect x="29.96" y="16" width="2" height="22" fill="#099DD1"/>	4 <rect x="29.96" y="16" width="2" height="22" fill="#099DD1"/>

5 <rect x="15.96" y="16" width="2" height="22" fill="#099DD1"/>	5 <rect x="15.96" y="16" width="2" height="22" fill="#099DD1"/>

6 <path d="M44,7H33V5a5,5,0,0,0-5-5H20a5,5,0,0,0-5,5V7H4V9H6.68V39a9,9,0,0,0,9,9 H32.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" fill="#099DD1"/>	6 <path d="M44,7H33V5a5,5,0,0,0-5-5H20a5,5,0,0,0-5,5V7H4V9H6.68V39a9,9,0,0,0,9,9 H32.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" fill="#099DD1"/>

7	7

8 <view id="hover" viewBox="0 48 48 48"/>	8 <view id="hover" viewBox="0 48 48 48"/>
Thomas Greiner 2017/11/03 19:04:10 Detail: Let's add a comment above to explain why w 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. saroyanm 2017/11/03 19:18:17 This approach looks to be more performant, don't y Show quoted text 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 ? Thomas Greiner 2017/11/03 19:47:36 If performance is more important than maintainabil Show quoted text 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.
9 <rect x="22.96" y="16" width="2" height="22" transform="translate(0, 48)" fill ="#5CBCE1"/>	9 <g transform="translate(0, 48)">

10 <rect x="29.96" y="16" width="2" height="22" transform="translate(0, 48)" fill ="#5CBCE1"/>	10 <rect x="22.96" y="16" width="2" height="22" fill="#5CBCE1"/>

11 <rect x="15.96" y="16" width="2" height="22" transform="translate(0, 48)" fill ="#5CBCE1"/>	11 <rect x="29.96" y="16" width="2" height="22" fill="#5CBCE1"/>

12 <path d="M44,7H33V5a5,5,0,0,0-5-5H20a5,5,0,0,0-5,5V7H4V9H6.68V39a9,9,0,0,0,9,9 H32.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="#5CB CE1"/>	12 <rect x="15.96" y="16" width="2" height="22" fill="#5CBCE1"/>
Thomas Greiner 2017/11/03 19:04:10 Suggestion: You can avoid duplicating the "transfo 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:16 Interesting approach, will test and update accordi Show quoted text 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 Done. Show quoted text 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.
	13 <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" fill="#5CBCE1"/>

	14 </g>

13 </svg>	15 </svg>

LEFT	RIGHT