Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(185)

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

Can't Edit
Can't Publish+Mail
Start Review
Created:
1 year, 11 months ago by saroyanm
Modified:
1 year, 11 months ago
Reviewers:
Thomas Greiner
CC:
Wladimir Palant
Visibility:
Public.

Description

Issue 5973 - broken icons on Firefox v50-53

Patch Set 1 #

Total comments: 4

Patch Set 2 : Use IDs for shared icons #

Total comments: 21

Patch Set 3 : #

Unified diffs Side-by-side diffs Delta from patch set Stats (+105 lines, -59 lines) Patch
M skin/desktop-options.css View 1 2 9 chunks +27 lines, -45 lines 0 comments Download
M skin/icons/attention.svg View 1 chunk +5 lines, -1 line 0 comments Download
M skin/icons/checkmark.svg View 1 2 1 chunk +7 lines, -1 line 0 comments Download
M skin/icons/code.svg View 1 chunk +5 lines, -1 line 0 comments Download
M skin/icons/delete.svg View 1 chunk +13 lines, -1 line 0 comments Download
M skin/icons/gear.svg View 1 chunk +7 lines, -1 line 0 comments Download
M skin/icons/globe.svg View 1 chunk +3 lines, -1 line 0 comments Download
M skin/icons/reload.svg View 1 chunk +3 lines, -1 line 0 comments Download
M skin/icons/tooltip.svg View 1 2 1 chunk +5 lines, -1 line 0 comments Download
M skin/icons/trash.svg View 1 2 1 chunk +15 lines, -1 line 0 comments Download
M skin/mobile-options.css View 1 2 chunks +2 lines, -2 lines 0 comments Download
M skin/social/facebook.svg View 1 chunk +4 lines, -1 line 0 comments Download
M skin/social/googleplus.svg View 1 chunk +5 lines, -1 line 0 comments Download
M skin/social/twitter.svg View 1 chunk +4 lines, -1 line 0 comments Download

Messages

Total messages: 7
saroyanm
@Thomas, @Wladimir: Would be great if somebody from you can have a chance to review ...
1 year, 11 months ago (2017-11-03 18:02:27 UTC) #1
saroyanm
https://codereview.adblockplus.org/29596678/diff/29596679/skin/icons/checkmark.svg File skin/icons/checkmark.svg (right): https://codereview.adblockplus.org/29596678/diff/29596679/skin/icons/checkmark.svg#newcode1 skin/icons/checkmark.svg:1: <svg xmlns="http://www.w3.org/2000/svg"> On 2017/11/03 18:02:26, saroyanm wrote: > This ...
1 year, 11 months ago (2017-11-03 18:28:30 UTC) #2
Thomas Greiner
As a quick fix this should hopefully do the job so most my comments are ...
1 year, 11 months ago (2017-11-03 19:04:11 UTC) #3
saroyanm
https://codereview.adblockplus.org/29596678/diff/29596695/skin/icons/checkmark.svg File skin/icons/checkmark.svg (right): https://codereview.adblockplus.org/29596678/diff/29596695/skin/icons/checkmark.svg#newcode5 skin/icons/checkmark.svg:5: <view id="approve" viewBox="10 40 30 30"/> On 2017/11/03 19:04:10, ...
1 year, 11 months ago (2017-11-03 19:18:17 UTC) #4
saroyanm
I didn't updated colors, as I used here the existing colors, the colors in general ...
1 year, 11 months ago (2017-11-03 19:35:09 UTC) #5
Thomas Greiner
LGTM On 2017/11/03 19:35:09, saroyanm wrote: > I didn't updated colors, as I used here ...
1 year, 11 months ago (2017-11-03 19:47:36 UTC) #6
saroyanm
1 year, 11 months ago (2017-11-03 20:06:53 UTC) #7
Message was sent while issue was closed.
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.
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld 87257f5