Use a new downloadTypeIcon in Summary Section of Downloads Panel

VERIFIED FIXED in Firefox 51

Status

()

Firefox
Downloads Panel
VERIFIED FIXED
9 months ago
6 months ago

People

(Reporter: seanlee, Assigned: seanlee)

Tracking

(Blocks: 1 bug)

unspecified
Firefox 51
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox51 verified)

Details

(Whiteboard: [CHE-MVP])

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(5 attachments, 1 obsolete attachment)

(Assignee)

Description

9 months ago
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
(Assignee)

Comment 1

9 months ago
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.
Flags: needinfo?(chuang)

Comment 2

9 months ago
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!!
Flags: needinfo?(chuang)
Comment hidden (mozreview-request)
(Assignee)

Comment 4

9 months ago
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.
(Assignee)

Comment 5

9 months ago
Created attachment 8787608 [details]
RTL

Since this patch is with margin-inline-start changes, the screenshot shows the result in RTL mode.

Comment 6

9 months ago
mozreview-review
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?
(Assignee)

Comment 7

9 months ago
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

9 months ago
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.
Attachment #8787604 - Flags: review?(paolo.mozmail)
(Assignee)

Comment 9

9 months ago
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.
(Assignee)

Comment 10

9 months ago
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.
Attachment #8787604 - Flags: review?(paolo.mozmail)
(Assignee)

Updated

9 months ago
Attachment #8787604 - Flags: review?(paolo.mozmail)

Comment 11

9 months ago
(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 hidden (mozreview-request)
(Assignee)

Comment 13

9 months ago
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.
Attachment #8787604 - Flags: review?(paolo.mozmail)
(Assignee)

Comment 14

9 months ago
mozreview-review
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?
(Assignee)

Comment 15

9 months ago
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.
Attachment #8787604 - Flags: review?(paolo.mozmail)

Comment 16

9 months ago
(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

9 months ago
mozreview-review
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.
Attachment #8787604 - Flags: review?(paolo.mozmail)

Comment 18

9 months ago
mozreview-review
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.
(Assignee)

Comment 19

9 months ago
(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;
Flags: needinfo?(paolo.mozmail)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 22

9 months ago
Hello Paolo, the part 2 patch is for adjusting the icon margin. Could you review these two patches? Thank you.

Comment 23

9 months ago
mozreview-review
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.
Attachment #8787604 - Flags: review?(paolo.mozmail) → review+

Comment 24

9 months ago
mozreview-review
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.
Attachment #8788834 - Flags: review?(paolo.mozmail)
(Assignee)

Comment 25

9 months ago
mozreview-review-reply
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.
(Assignee)

Comment 26

9 months ago
mozreview-review-reply
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

9 months ago
(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.
Flags: needinfo?(paolo.mozmail)
Comment hidden (mozreview-request)
(Assignee)

Updated

9 months ago
Attachment #8788834 - Attachment is obsolete: true
(Assignee)

Updated

9 months ago
Keywords: checkin-needed
(Assignee)

Comment 29

9 months ago
(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

9 months ago
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/fx-team/rev/b9d4367322d4
Use a new .downloadTypeIcon in SVG format. r=paolo
Keywords: checkin-needed

Comment 31

9 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/b9d4367322d4
Status: ASSIGNED → RESOLVED
Last Resolved: 9 months ago
status-firefox51: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 51
(Assignee)

Updated

9 months ago
Blocks: 1300947
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

6 months ago
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]
Status: RESOLVED → VERIFIED
status-firefox51: fixed → verified
You need to log in before you can comment on or make changes to this bug.