Last Comment Bug 1299712 - Use a new downloadTypeIcon in Summary Section of Downloads Panel
: Use a new downloadTypeIcon in Summary Section of Downloads Panel
Status: VERIFIED FIXED
[CHE-MVP]
:
Product: Firefox
Classification: Client Software
Component: Downloads Panel (show other bugs)
: unspecified
: All All
-- normal (vote)
: Firefox 51
Assigned To: Sean Lee [:seanlee][:weilonge]
:
: :Paolo Amadini
Mentors:
Depends on:
Blocks: 1269956 1300947
  Show dependency treegraph
 
Reported: 2016-08-31 20:06 PDT by Sean Lee [:seanlee][:weilonge]
Modified: 2016-11-28 04:55 PST (History)
9 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
verified

MozReview Requests
Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:
Show discarded requests

Attachments
download-summary.png (1.91 KB, image/png)
2016-08-31 20:31 PDT, Sean Lee [:seanlee][:weilonge]
no flags Details
Bug 1299712 - Use a new .downloadTypeIcon in SVG format.; (58 bytes, text/x-review-board-request)
2016-09-02 05:20 PDT, Sean Lee [:seanlee][:weilonge]
paolo.mozmail: review+
Details | Review
RTL (315.14 KB, image/png)
2016-09-02 05:38 PDT, Sean Lee [:seanlee][:weilonge]
no flags Details
progress_bar_aligned (251.91 KB, image/png)
2016-09-05 03:11 PDT, Sean Lee [:seanlee][:weilonge]
no flags Details
the original left margin (158.75 KB, image/png)
2016-09-06 00:13 PDT, Sean Lee [:seanlee][:weilonge]
no flags Details
Bug 1299712 - Part 2: Adjust the margin of download content. ; (58 bytes, text/x-review-board-request)
2016-09-07 03:26 PDT, Sean Lee [:seanlee][:weilonge]
no flags Details | Review

Description User image Sean Lee [:seanlee][:weilonge] 2016-08-31 20:06:06 PDT
Since a new downloadTypeIcon is designed [1], this bug is for discussing the icon replacement including the following files[2]:

browser/themes/linux/downloads/download-summary.png
browser/themes/osx/downloads/download-summary.png
browser/themes/windows/downloads/download-summary.png

[1] https://mozilla.invisionapp.com/share/4Y6ZZH1E8#/screens/162255324
[2] https://dxr.mozilla.org/mozilla-central/search?q=path%3A*%2Fdownload-summary.png&redirect=false
Comment 1 User image Sean Lee [:seanlee][:weilonge] 2016-08-31 20:31:55 PDT
Created attachment 8787048 [details]
download-summary.png

Hey Carol,

Based on comment 0, each platform has their own download-summary.png. The original design is an arrow without laying on a plain file icon, so I suppose the different icon is for contrast concern or other reasons based on different background, but the arrow is laid on a plain file icon in the new design. The different background issue could be ignore.

may I know your thoughts that if the new icon can be applied to all platforms (Windows/OSX/Linux)?

Thank you.
Comment 2 User image Carol Huang [:Carol] 2016-08-31 23:04:48 PDT
Hi Sean,
Yes, the new icon could be applied to all platforms (Windows/OSX/Linux)!
But right now I'm thinking about creating SVG files for the "summery icon" 
Would you have any concern?
I will send you the update icon as soon as possible. thanks!!
Comment 3 User image Sean Lee [:seanlee][:weilonge] 2016-09-02 05:20:18 PDT Comment hidden (mozreview-request)
Comment 4 User image Sean Lee [:seanlee][:weilonge] 2016-09-02 05:22:59 PDT
Hey Paolo,

In this patch, download-summary.png for each platform is replaced with download-summary.svg.
The left/right space of .downloadTypeIcon is both 12px based on the spec.

Could you review this patch? Thank you.
Comment 5 User image Sean Lee [:seanlee][:weilonge] 2016-09-02 05:38:16 PDT
Created attachment 8787608 [details]
RTL

Since this patch is with margin-inline-start changes, the screenshot shows the result in RTL mode.
Comment 6 User image :Paolo Amadini 2016-09-05 02:48:52 PDT
Comment on attachment 8787604 [details]
Bug 1299712 - Use a new .downloadTypeIcon in SVG format.;

https://reviewboard.mozilla.org/r/76326/#review74782

::: browser/themes/shared/downloads/download-summary.svg:1
(Diff revision 1)
> +<?xml version="1.0" encoding="utf-8"?>
> +<!-- Generator: Adobe Illustrator 20.1.0, SVG Export Plug-In . SVG Version: 6.00 Build 0)  -->

This file needs to be edited to follow the SVG guidelines:

https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/SVG_Guidelines

I generally find it easier to look at an existing SVG of the same size in the tree as a template, and copy the new paths and polygons with the right "fill" attributes.

::: browser/themes/shared/downloads/downloads.inc.css:157
(Diff revision 1)
>  #downloadsSummary > .downloadTypeIcon {
> -  list-style-image: url("chrome://browser/skin/downloads/download-summary.png");
> +  list-style-image: url("chrome://browser/skin/downloads/download-summary.svg");
>  }
>  
> -%ifdef XP_MACOSX
> -@media (min-resolution: 2dppx) {
> +#downloadsSummaryDescription {
> +  margin-inline-start: 2px;

I'm not sure what these margin changes are doing exactly. I though you wanted to align the progress bar, but from the screenshot it doesn't seem the resulting progress bar is aligned?
Comment 7 User image Sean Lee [:seanlee][:weilonge] 2016-09-05 03:11:10 PDT
Created attachment 8788136 [details]
progress_bar_aligned

(In reply to :Paolo Amadini from comment #6)
> Comment on attachment 8787604 [details]
> Bug 1299712 - Use a new .downloadTypeIcon in SVG format.;
> 
> https://reviewboard.mozilla.org/r/76326/#review74782
> 
> ::: browser/themes/shared/downloads/downloads.inc.css:157
> (Diff revision 1)
> >  #downloadsSummary > .downloadTypeIcon {
> > -  list-style-image: url("chrome://browser/skin/downloads/download-summary.png");
> > +  list-style-image: url("chrome://browser/skin/downloads/download-summary.svg");
> >  }
> >  
> > -%ifdef XP_MACOSX
> > -@media (min-resolution: 2dppx) {
> > +#downloadsSummaryDescription {
> > +  margin-inline-start: 2px;
> 
> I'm not sure what these margin changes are doing exactly. I though you
> wanted to align the progress bar, but from the screenshot it doesn't seem
> the resulting progress bar is aligned?

Yes, this is for progress bar alignment but also for the total left/right margin to 12px of the icon.
Please see this attachment. I add a red rectangle based on the screenshot in comment 5.
You can see the progress bar in Summary Section and Main Menu are aligned to the right and left edge of the red rectangle.
Comment 8 User image :Paolo Amadini 2016-09-05 03:28:24 PDT
Comment on attachment 8787604 [details]
Bug 1299712 - Use a new .downloadTypeIcon in SVG format.;

(In reply to Sean Lee [:seanlee][:weilonge] from comment #7)
> Yes, this is for progress bar alignment but also for the total left/right
> margin to 12px of the icon.

We already declare 12px: 

https://dxr.mozilla.org/mozilla-central/rev/dbe4b47941c7b3d6298a0ead5e40dd828096c808/browser/themes/shared/downloads/downloads.inc.css#195

It seems strange that we need to add 2px sometimes, although we may have to do the opposite and reset the default description element margin to get the exact result. I see you have black borders around the individual icons to illustrate the space, are these border added to the screenshot later, or is this CSS interfering?

We should move the margin adjustment to a different changeset for clarity. Since they're simple we can still keep both changesets this bug, but I'm also fine using a different one.
Comment 9 User image Sean Lee [:seanlee][:weilonge] 2016-09-06 00:13:19 PDT
Created attachment 8788344 [details]
the original left margin

Sorry that I should describe this more clearly.

2px adjustment is for .downloadTarget and .downloadDetails in richlistitem > vbox.downloadContainer ,
and also #downloadsSummaryDescription #downloadsSummaryDetails in Summary Section.

In this attachment (original design), you can see there is a margin at the left side of .downloadTarget (A), .downloadProgress (B), and .downloadDetails (C) .

Exactly, the left margin of .downloadTarget, .downloadProgress, and .downloadDetails are 6px, 4px, and 6px.

That's why the icon looks like a little bit left.

For (A) and (C), please see the definition here:
http://searchfox.org/mozilla-central/source/toolkit/themes/osx/global/global.css#190

So my solution is to adjust (B) to 0, and Both (A) and (C) minuses 4px. That's why it's given 2px.

The same login can be applied to Summary Section as well.
Comment 10 User image Sean Lee [:seanlee][:weilonge] 2016-09-06 00:14:39 PDT
Comment on attachment 8787604 [details]
Bug 1299712 - Use a new .downloadTypeIcon in SVG format.;

Hi Paolo, could you see comment 9 and review the patch again? Thank you.
Comment 11 User image :Paolo Amadini 2016-09-06 00:35:24 PDT
(In reply to Sean Lee [:seanlee][:weilonge] from comment #9)
> Exactly, the left margin of .downloadTarget, .downloadProgress, and
> .downloadDetails are 6px, 4px, and 6px.

Thanks for the screenshot and for clarifying your reasoning! I see that you wanted to preserve the relative alignment of the three elements, but actually the current margins are not deliberate, when this was implemented we just left the default margins in place. You don't need to preserve the relative alignment. This means that we can reset all those horizontal margins to zero. Also, you should override both horizontal margins so we don't find extra space on the other end.

Please split in two changesets, and I'll review the next version with this fixed and the SVG optimized.
Comment 12 User image Sean Lee [:seanlee][:weilonge] 2016-09-06 00:40:59 PDT Comment hidden (mozreview-request)
Comment 13 User image Sean Lee [:seanlee][:weilonge] 2016-09-06 00:45:18 PDT
Comment on attachment 8787604 [details]
Bug 1299712 - Use a new .downloadTypeIcon in SVG format.;

ooh, race condition here :P
I push a new patch with margin adjustment and SVG simplification before seeing comment 11.

Please ignore this review request.
Comment 14 User image Sean Lee [:seanlee][:weilonge] 2016-09-06 00:49:19 PDT
Comment on attachment 8787604 [details]
Bug 1299712 - Use a new .downloadTypeIcon in SVG format.;

https://reviewboard.mozilla.org/r/76326/#review74982

::: browser/themes/shared/downloads/downloads.inc.css:187
(Diff revision 2)
>    border-bottom: 1px solid transparent;
>  }
>  
>  .downloadTypeIcon {
>    margin-top: 8px;
> -  margin-inline-end: 12px;
> +  margin-inline-end: 8px;

The icon lefty issue can be solved with the adjustment to  margin-inline-end: 8px;
I think this is easier to understand.
How do you think if keeping the changeset just into one?
Comment 15 User image Sean Lee [:seanlee][:weilonge] 2016-09-06 00:58:40 PDT
Comment on attachment 8787604 [details]
Bug 1299712 - Use a new .downloadTypeIcon in SVG format.;

Hey Paolo, could you review the latest patch with SVG optimization and a simplified icon left solution?

I suppose the solution is easier to understand and don't have to put into another changeset.

Thank you.
Comment 16 User image :Paolo Amadini 2016-09-06 01:08:15 PDT
(In reply to Sean Lee [:seanlee][:weilonge] from comment #15)
> simplified icon left solution?

I think your original approach was better because it removed any assumption on default margins, and the value we declare is the value we actually get. It also fixes the alignment of the progress bar.

The changes are logically distinct enough that it's better to have a separate changeset anyways, so when looking at history one wouldn't wonder why the icon change needed a margin change as well, when the icon is the same size. Also, this would ideally be done in a separate bug, but we're also moving to a model of review that is more changeset-centric than bug-centric, so we may have both changes on the same bug if the commit messages are correct.
Comment 17 User image :Paolo Amadini 2016-09-06 01:13:34 PDT
Comment on attachment 8787604 [details]
Bug 1299712 - Use a new .downloadTypeIcon in SVG format.;

https://reviewboard.mozilla.org/r/76326/#review74986

::: browser/themes/shared/downloads/download-summary.svg:5
(Diff revision 2)
> +<svg xmlns="http://www.w3.org/2000/svg"
> +  width="32" height="32" viewBox="0 0 32 32">

Align "width" under "xmlns".

::: browser/themes/shared/downloads/download-summary.svg:7
(Diff revision 2)
> +<!-- 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/. -->
> +<svg xmlns="http://www.w3.org/2000/svg"
> +  width="32" height="32" viewBox="0 0 32 32">
> +  <g>

Remove the unneeded grouping.

::: browser/themes/shared/downloads/download-summary.svg:12
(Diff revision 2)
> +  <g>
> +    <path fill="#fff" d="M26.2,31.4H6.8c-1.8,0-3.4-1.5-3.4-3.3V3.8c0-1.8,1.6-3.3,3.4-3.3H23l5.6,6.4V28C28.7,29.9,28,31.4,26.2,31.4z"/>
> +    <path fill="#939393" d="M26.2,31.9H6.8c-2,0-3.8-1.7-3.8-3.8V3.8C3,1.7,4.8,0,6.8,0h16.4L29,6.7v21.4C29,30.2,28.2,31.9,26.2,31.9z M6.8,1C5.2,1,4,2.3,4,3.8V28c0,1.6,1.4,3,2.9,3h19.2c1.6,0,2-1.5,2-3V7.1L22.7,1C22.7,1,6.8,1,6.8,1z"/>
> +  </g>
> +  <g>
> +    <path fill="#4c4c4d" d="M22.5,18.2L17,23.6c-0.3,0.3-0.6,0.4-1.1,0.4c-0.3,0-0.8-0.1-1.1-0.4l-5.5-5.4c-0.5-0.5-0.4-1.1,0.4-1.1H13

I suspect this would originally be 4c4c4c and the final d is a rounding issue when exporting.

::: browser/themes/shared/downloads/download-summary.svg:15
(Diff revision 2)
> +  </g>
> +  <g>
> +    <path fill="#4c4c4d" d="M22.5,18.2L17,23.6c-0.3,0.3-0.6,0.4-1.1,0.4c-0.3,0-0.8-0.1-1.1-0.4l-5.5-5.4c-0.5-0.5-0.4-1.1,0.4-1.1H13
> +      v-5.9c0-0.5,0.4-1,1-1h3.9c0.5,0,1,0.4,1,1v5.9h3.1C22.8,17.2,23,17.6,22.5,18.2z"/>
> +  </g>
> +  <polygon fill="#939393" points="27,9 21,9 21,1.9 22,1.9 22,8 27,8 "/>

Remove extra space at the end of the string. Also the most common style is to have a space before "/>" at the end of all tags without children.
Comment 18 User image :Paolo Amadini 2016-09-06 01:17:59 PDT
Comment on attachment 8787604 [details]
Bug 1299712 - Use a new .downloadTypeIcon in SVG format.;

https://reviewboard.mozilla.org/r/76326/#review74990

::: browser/themes/shared/downloads/download-summary.svg:13
(Diff revision 2)
> +    <path fill="#fff" d="M26.2,31.4H6.8c-1.8,0-3.4-1.5-3.4-3.3V3.8c0-1.8,1.6-3.3,3.4-3.3H23l5.6,6.4V28C28.7,29.9,28,31.4,26.2,31.4z"/>
> +    <path fill="#939393" d="M26.2,31.9H6.8c-2,0-3.8-1.7-3.8-3.8V3.8C3,1.7,4.8,0,6.8,0h16.4L29,6.7v21.4C29,30.2,28.2,31.9,26.2,31.9z M6.8,1C5.2,1,4,2.3,4,3.8V28c0,1.6,1.4,3,2.9,3h19.2c1.6,0,2-1.5,2-3V7.1L22.7,1C22.7,1,6.8,1,6.8,1z"/>
> +  </g>
> +  <g>
> +    <path fill="#4c4c4d" d="M22.5,18.2L17,23.6c-0.3,0.3-0.6,0.4-1.1,0.4c-0.3,0-0.8-0.1-1.1-0.4l-5.5-5.4c-0.5-0.5-0.4-1.1,0.4-1.1H13
> +      v-5.9c0-0.5,0.4-1,1-1h3.9c0.5,0,1,0.4,1,1v5.9h3.1C22.8,17.2,23,17.6,22.5,18.2z"/>

Ah, also the attribute should be on a single line. I didn't notice it wrapped.
Comment 19 User image Sean Lee [:seanlee][:weilonge] 2016-09-06 02:54:21 PDT
(In reply to :Paolo Amadini from comment #11)
> Thanks for the screenshot and for clarifying your reasoning! I see that you
> wanted to preserve the relative alignment of the three elements, but
> actually the current margins are not deliberate, when this was implemented
> we just left the default margins in place. You don't need to preserve the
> relative alignment. This means that we can reset all those horizontal
> margins to zero. Also, you should override both horizontal margins so we
> don't find extra space on the other end.

Is it enough to adjust margin-inline-start for the following elements? I don't get how many elements should be "reset to zero"
#downloadsSummaryDescription,
.downloadTarget {
  margin-inline-start: 2px;

#downloadsSummaryProgress,
.downloadProgress {
  margin-inline-start: 0;

#downloadsSummaryDetails,
.downloadDetails {
  margin-inline-start: 2px;
Comment 20 User image Sean Lee [:seanlee][:weilonge] 2016-09-07 03:26:19 PDT Comment hidden (mozreview-request)
Comment 21 User image Sean Lee [:seanlee][:weilonge] 2016-09-07 03:26:19 PDT Comment hidden (mozreview-request)
Comment 22 User image Sean Lee [:seanlee][:weilonge] 2016-09-07 03:27:28 PDT
Hello Paolo, the part 2 patch is for adjusting the icon margin. Could you review these two patches? Thank you.
Comment 23 User image :Paolo Amadini 2016-09-07 05:26:29 PDT
Comment on attachment 8787604 [details]
Bug 1299712 - Use a new .downloadTypeIcon in SVG format.;

https://reviewboard.mozilla.org/r/76326/#review75420

r+ with the change below. Thanks!

::: browser/themes/osx/jar.mn
(Diff revision 3)
>    skin/classic/browser/downloads/download-notification-finish.png  (downloads/download-notification-finish.png)
>    skin/classic/browser/downloads/download-notification-finish@2x.png  (downloads/download-notification-finish@2x.png)
>    skin/classic/browser/downloads/download-notification-start.png  (downloads/download-notification-start.png)
>    skin/classic/browser/downloads/download-notification-start@2x.png  (downloads/download-notification-start@2x.png)
> -  skin/classic/browser/downloads/download-summary.png       (downloads/download-summary.png)
> -  skin/classic/browser/downloads/download-summary@2x.png    (downloads/download-summary@2x.png)

You have to remove this from the file system too.
Comment 24 User image :Paolo Amadini 2016-09-07 05:32:36 PDT
Comment on attachment 8788834 [details]
Bug 1299712 - Part 2: Adjust the margin of download content. ;

https://reviewboard.mozilla.org/r/77176/#review75422

::: browser/themes/shared/downloads/downloads.inc.css:232
(Diff revision 1)
>    font-size: calc(100% * var(--downloads-item-font-size-factor));
>  }
>  
>  #downloadsSummaryDescription,
>  .downloadTarget {
> +  margin-inline-start: 2px;

I think both the left and right margins need to be reset to zero, otherwise the result is a 14px margin. You can use a shorthand rule like this:

margin: (current top margin) 0 var(--downloads-item-target-margin-bottom);

And you can do a similar change for the other label.

::: browser/themes/shared/downloads/downloads.inc.css:243
(Diff revision 1)
>    font-size: calc(100% / var(--downloads-item-font-size-factor));
>  }
>  
> +#downloadsSummaryProgress,
> +.downloadProgress {
> +  margin-inline-start: 0;

Just "margin: 0;" should work fine here, if the top and bottom margins are already taken into account in the labels.
Comment 25 User image Sean Lee [:seanlee][:weilonge] 2016-09-07 06:34:43 PDT
Comment on attachment 8788834 [details]
Bug 1299712 - Part 2: Adjust the margin of download content. ;

https://reviewboard.mozilla.org/r/77176/#review75422

> I think both the left and right margins need to be reset to zero, otherwise the result is a 14px margin. You can use a shorthand rule like this:
> 
> margin: (current top margin) 0 var(--downloads-item-target-margin-bottom);
> 
> And you can do a similar change for the other label.

2px remained here is based on that the original desgin of #downloadsSummaryDescription and #downloadsSummaryProgress are both 6px.

I would like to reset the progress bar only is for the round left side of progress bar.
For visula concern, ever there are 2px difference, that looks like more align that reseting to 0.
Comment 26 User image Sean Lee [:seanlee][:weilonge] 2016-09-07 06:40:47 PDT
Comment on attachment 8788834 [details]
Bug 1299712 - Part 2: Adjust the margin of download content. ;

https://reviewboard.mozilla.org/r/77176/#review75422

> Just "margin: 0;" should work fine here, if the top and bottom margins are already taken into account in the labels.

I found there are 2px for top/bottom margin and 4px for right margin. Should we reset them to 0 too?
Comment 27 User image :Paolo Amadini 2016-09-07 07:11:07 PDT
(In reply to Sean Lee [:seanlee][:weilonge] from comment #25)
> 2px remained here is based on that the original desgin of
> #downloadsSummaryDescription and #downloadsSummaryProgress are both 6px.

As I said, these were not intentional to begin with.

> I would like to reset the progress bar only is for the round left side of
> progress bar.
> For visula concern, ever there are 2px difference, that looks like more
> align that reseting to 0.

Did you try this on different platforms? The effect might be different. A comparison screenshot for different platforms might help.

(In reply to Sean Lee [:seanlee][:weilonge] from comment #26)
> I found there are 2px for top/bottom margin and 4px for right margin. Should
> we reset them to 0 too?

Yes, but if we want to keep the current vertical margin we might want to absorb that value in the CSS variable for the label margin.

I'm thinking that maybe we can move this changeset to the bug that will implement the unified progress bar across platforms, and only land the first changeset here? It seems these margin changes are more related to the other work we'll do for the progress bar.
Comment 28 User image Sean Lee [:seanlee][:weilonge] 2016-09-07 07:41:53 PDT Comment hidden (mozreview-request)
Comment 29 User image Sean Lee [:seanlee][:weilonge] 2016-09-07 07:49:22 PDT
(In reply to :Paolo Amadini from comment #27)
> (In reply to Sean Lee [:seanlee][:weilonge] from comment #26)
> > I found there are 2px for top/bottom margin and 4px for right margin. Should
> > we reset them to 0 too?
> 
> Yes, but if we want to keep the current vertical margin we might want to
> absorb that value in the CSS variable for the label margin.
> 
> I'm thinking that maybe we can move this changeset to the bug that will
> implement the unified progress bar across platforms, and only land the first
> changeset here? It seems these margin changes are more related to the other
> work we'll do for the progress bar.

I am fine to land the first patch first. :)
Do we have a bug to implement unified progress bar?
Comment 30 User image Pulsebot 2016-09-07 14:52:52 PDT
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/fx-team/rev/b9d4367322d4
Use a new .downloadTypeIcon in SVG format. r=paolo
Comment 31 User image Carsten Book [:Tomcat] 2016-09-08 03:08:46 PDT
https://hg.mozilla.org/mozilla-central/rev/b9d4367322d4
Comment 32 User image Maruf Rahman[:mMARUF] 2016-11-24 23:24:31 PST
I have reproduced this bug with Nightly 51.0a1 (2016-08-31) on Windows 7 , 64 Bit ! 

This bug's fix is verified with latest Beta 

Build   ID    20161124073320
User Agent    Mozilla/5.0 (Windows NT 6.1; WOW64; rv:51.0) Gecko/20100101 Firefox/51.0
[testday-20161125]
Comment 33 User image Md.Majedul isalm 2016-11-25 01:16:02 PST
I have reproduced this bug with Nightly 51.0a1 (2016-08-31) on Ubuntu 16.10, 64 Bit !

This bug's fix is verified with latest Beta .


Build ID  	20161124073320 
User Agent      Mozilla/5.0 (X11; Linux x86_64; rv:51.0) Gecko/20100101 Firefox/51.0

[testday-20161125]

Note You need to log in before you can comment on or make changes to this bug.