1269957 - Unify the spacing and change the number of maximum download items displayed

Status: VERIFIED FIXED

(Firefox :: Downloads Panel, defect)

Description

[Sean Lee [:seanlee][:weilonge]]

In the current design, there are only three items will be displayed in the downloads panel. We have to change to five ones based on the UX design.

Comment 1

[Wesly Huang (EPM)]

As offline discussed, we use https://bugzilla.mozilla.org/show_bug.cgi?id=1269956 instead, to track download panel specific works.

Comment 2

[Mike Conley (:mconley) (:⚙️)]

Note for yliao - the property to tweak the number of listed entries is here: https://dxr.mozilla.org/mozilla-central/source/browser/components/downloads/content/downloads.js#673

Comment 3

[yifan [:yifan][:yliao]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=2bb6dad94c52

Comment 4

[yifan [:yifan][:yliao]]

Attachment: MozReview Request: Bug 1269957 - Change the number of maximum download items displayed r?mconley

Change the maximum number of downloads displayed in the downloads panel to 5.

Review commit: https://reviewboard.mozilla.org/r/51889/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/51889/

Comment 5

[yifan [:yifan][:yliao]]

Thank you Mike for pointing out the source of the number!

Comment 6

[:Paolo Amadini]

Comment on attachment 8751153 [details]
MozReview Request: Bug 1269957 - Change the number of maximum download items displayed r?mconley

I think we definitely want to increase the number of items as part of the new Downloads Panel design, but this should be the last change to be done, not the first.

The reason is that we should first decrease the height of the download items. This decrease in height is not just a change of CSS values, it is the result of rearranging the contents of the download items, as you can see in Stephen Horlander's design mockup here:

http://people.mozilla.org/~shorlander/mockups-interactive/australis-interactive-mockups/windows8.html

There are two reasons why the limit is currently calculated to be three.

One is the supported screen resolution, as we must handle a height of 768 pixels. One source for screen resolution statistics you can check out is <http://www.w3schools.com/browsers/browsers_display.asp>. Since the panel is designed to be an easily accessible overview of your most recent downloads, and not a complex management interface, a scrollbar should never appear in the Downloads Panel. Unfortunately, a scrollbar can appear on these resolutions when you have five items and the panel is anchored near the center of the screen.

The other reason is that we have statistics on how many concurrent downloads are normally made, and only 3.5% of sessions where a download is executing have a second one running at the same time, and less than 1% have four or more (<http://mzl.la/1WoLBvS>). This is why the panel is optimized to display a small number of items. While three is enough, five items is still a good limit, and also looks good visually when we have a smaller item height.

Although the use case of keeping an eye on multiple downloads only applies to 4% or less of sessions, it is still worth making it easier for power users or for those times where it is needed. If there is one change we can make to that effect, it is moving the Downloads View from the Library to a browser tab.

Comment 7

[:Paolo Amadini]

Meta bug 1265337 is about the redesign of the Downloads Panel to use similar styles across platforms.

See also bug 780837 (investigate ways to show more recent downloads in the panel) and the original bug 747903 (limit the number of items in the Downloads Panel) for more context. I don't think the dynamic approach in bug 780837 is worth the effort, we can just increase the limit once we have decreased the height.

Comment 8

[yifan [:yifan][:yliao]]

Thank you Paolo for the feedback! Morpheus could you share some idea on this?

Comment 9

[Bryant Mao [:bryantmao]]

(In reply to :Paolo Amadini from comment #6)
> Comment on attachment 8751153 [details]
> MozReview Request: Bug 1269957 - Change the number of maximum download items
> displayed r?mconley
> 
> I think we definitely want to increase the number of items as part of the
> new Downloads Panel design, but this should be the last change to be done,
> not the first.

Agree, I think that's something we take as a second priority.

> The reason is that we should first decrease the height of the download
> items. This decrease in height is not just a change of CSS values, it is the
> result of rearranging the contents of the download items, as you can see in
> Stephen Horlander's design mockup here:
> 
> http://people.mozilla.org/~shorlander/mockups-interactive/australis-
> interactive-mockups/windows8.html

The reason we keep the linear progress bar is that we think it's much clearer to represent the download progress, especially when there are multiple downloading items. We've gone through the idea with Stephen, and it seems he is quite positive with that. Under this arrangement, shrinking the height of download item might  have limited effect as we imagine. But anyway, it's a good suggestion and something we can try. We will discuss that with our visual designer Carol to assess how to reduce the height without sacrificing the legibility :)  

> There are two reasons why the limit is currently calculated to be three.
> 
> One is the supported screen resolution, as we must handle a height of 768
> pixels. One source for screen resolution statistics you can check out is
> <http://www.w3schools.com/browsers/browsers_display.asp>. Since the panel is
> designed to be an easily accessible overview of your most recent downloads,
> and not a complex management interface, a scrollbar should never appear in
> the Downloads Panel. Unfortunately, a scrollbar can appear on these
> resolutions when you have five items and the panel is anchored near the
> center of the screen.
> 
> The other reason is that we have statistics on how many concurrent downloads
> are normally made, and only 3.5% of sessions where a download is executing
> have a second one running at the same time, and less than 1% have four or
> more (<http://mzl.la/1WoLBvS>). This is why the panel is optimized to
> display a small number of items. While three is enough, five items is still
> a good limit, and also looks good visually when we have a smaller item
> height.
> 
> Although the use case of keeping an eye on multiple downloads only applies
> to 4% or less of sessions, it is still worth making it easier for power
> users or for those times where it is needed. If there is one change we can
> make to that effect, it is moving the Downloads View from the Library to a
> browser tab.

Thanks, the information is really helpful! We've talked through this internally and here are reasons why we still think 5 might be a better number to show:

1. Chances that people need to scroll on download panel is quite low (6.3% x percentage of having 5 download items in a session)
2. By shrinking the height of each download item, we might also further reduce the chances that people need to scroll.
3. Most major browsers have more than 5 items in their download panel (ex. Chrome, Safari, Opera, Edge...)
4. Even if user moves the browser window in the middle of the screen, we can render it on the side of the window (like Opera) 

like you say, download panel is aimed for easy access to the most recent downloads. However, if space is large enough, we do want to present more items on the panel since download manager is not somewhere people will often access to, and we think it will do more good than harm for other 93.7% of the visitors. 

For now, we do have the plan to blend the download manager into a tab of the Activity Stream. I believe it's something we all expecting to happen (current library do have lots of UX issues), but that would be another project to talk to. 

Finally, I do hold an optimistic view of the dynamic approach. Although it seems an overkill for the current problem, in a long run it might be worthwhile as I assume desktop screen resolution will only become higher and more diverse in the future. Maybe it's a bit naive, but I think it's something worth to explore.

Comment 10

[Wesly Huang (EPM)]

(In reply to Bryant Mao [:bryantmao] from comment #9)
> (In reply to :Paolo Amadini from comment #6)
> > The reason is that we should first decrease the height of the download
> > items. This decrease in height is not just a change of CSS values, it is the
> > result of rearranging the contents of the download items, as you can see in
> > Stephen Horlander's design mockup here:
> > 
> > http://people.mozilla.org/~shorlander/mockups-interactive/australis-
> > interactive-mockups/windows8.html
> 
> The reason we keep the linear progress bar is that we think it's much
> clearer to represent the download progress, especially when there are
> multiple downloading items. We've gone through the idea with Stephen, and it
> seems he is quite positive with that. Under this arrangement, shrinking the
> height of download item might  have limited effect as we imagine. But
> anyway, it's a good suggestion and something we can try. We will discuss
> that with our visual designer Carol to assess how to reduce the height
> without sacrificing the legibility :)  
> 

Hi Carol:

As discussed offline would you help share the latest plan of this? My read is this should be the only open item in this bug for now. Thanks.

Comment 11

[Carol Huang [:Carol]]

As [:bryantmao] metioned,

> 1. Chances that people need to scroll on download panel is quite low (6.3% x percentage of having 5 download items in a session)

I explored some layout with different height of the list items. I will reduce the original list height a little. It might be the maximum adjustment for the list height because we don't want to loose the legibility for most of the users. Visually the information might look too crowded or complicated if we shrink too much of the space. 
Thanks!

Comment 12

[Sean Lee [:seanlee][:weilonge]]

Attachment: Bug 1269957 - Change the maximum number of displayed download items in Downloads Panel from 3 to 5.

Review commit: https://reviewboard.mozilla.org/r/60992/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/60992/

Comment 13

[Sean Lee [:seanlee][:weilonge]]

Comment on attachment 8765807 [details]
Bug 1269957 - Change the maximum number of displayed download items in Downloads Panel from 3 to 5.

Hello Paolo,

Based on UX's comment, I suppose that we can resume the design of changing to 5 download items in Downloads Panel.
Could you help to review the patch? Thank you.

Comment 14

[:Paolo Amadini]

Comment on attachment 8765807 [details]
Bug 1269957 - Change the maximum number of displayed download items in Downloads Panel from 3 to 5.

(In reply to Sean Lee [:seanlee][:weilonge] from comment #13)
> Based on UX's comment, I suppose that we can resume the design of changing
> to 5 download items in Downloads Panel.

I still think this should be done together with the specified visual design improvements for the download items, that include reducing the height of the items. Clustering this change with other significant improvements in visuals and usability makes sense to me.

Comment 15

[Sean Lee [:seanlee][:weilonge]]

(In reply to :Paolo Amadini from comment #14)
> Comment on attachment 8765807 [details]
> Bug 1269957 - Change the maximum number of displayed download items in
> Downloads Panel from 3 to 5.
> 
> (In reply to Sean Lee [:seanlee][:weilonge] from comment #13)
> > Based on UX's comment, I suppose that we can resume the design of changing
> > to 5 download items in Downloads Panel.
> 
> I still think this should be done together with the specified visual design
> improvements for the download items, that include reducing the height of the
> items. Clustering this change with other significant improvements in visuals
> and usability makes sense to me.

Does this bug 1265337 cover all works for "the specified visual design improvements for the download items" including reducing download items?

Comment 16

[Carol Huang [:Carol]]

(In reply to :Paolo Amadini from comment #14)
> Comment on attachment 8765807 [details]
> Bug 1269957 - Change the maximum number of displayed download items in
> Downloads Panel from 3 to 5.
> 
> (In reply to Sean Lee [:seanlee][:weilonge] from comment #13)
> > Based on UX's comment, I suppose that we can resume the design of changing
> > to 5 download items in Downloads Panel.




> I still think this should be done together with the specified visual design
> improvements for the download items, that include reducing the height of the
> items. Clustering this change with other significant improvements in visuals
> and usability makes sense to me.

Hello Paolo,
In the visual spec, I have reduced the original list height. It might be the maximum adjustment for the list height because we don't want to loose the legibility for most of the users. Visually the information might look too clustered and complicated if we shrink too much of the space.
I don’t know if this answer solved your concerns.
thanks!!

Comment 17

[:Paolo Amadini]

(In reply to Sean Lee [:seanlee][:weilonge] from comment #15)
> Does this bug 1265337 cover all works for "the specified visual design
> improvements for the download items" including reducing download items?

Yes, and I suggest using it as a meta-bug whose "depends on" field links to various parts of the redesign process. For example one could be about updating the colors and the button icons keeping the current item structure, then another one could be about expanding the click target of the buttons, and so on.

Comment 18

[Wesly Huang (EPM)]

(In reply to :Paolo Amadini from comment #17)
> (In reply to Sean Lee [:seanlee][:weilonge] from comment #15)
> > Does this bug 1265337 cover all works for "the specified visual design
> > improvements for the download items" including reducing download items?
> 
> Yes, and I suggest using it as a meta-bug whose "depends on" field links to
> various parts of the redesign process. For example one could be about
> updating the colors and the button icons keeping the current item structure,
> then another one could be about expanding the click target of the buttons,
> and so on.

Thanks Paolo, just to make sure my read is correct, the suggestion is to 

1. include 1265337 into our project scope
2. currently its "depends on" field is blank, and we should break 1265337 into small pieces and collect them here. (all become the project scope)
3. 1265337 "blocks" 3 bugs for malware/blocked downloads, those 3 are *NOT* part of our project scope
4. and to complete 1269957, the proposed clustering is 
   a. 1269957 (this one)
   b. height reduction (Carol has covered it in comment#11 & comment#16) 
   c. 1265337

I'm confirming this because recently we are defining the project MVP and it's mostly based on the UX spec. Today 1269957 is already included, but for 1265337 we anticipate there will be further discussion needed so not marked as MVP. Haven't checked with UX but do you think it's feasible to cover "4a + 4b" in here and MVP, and put 4c to backlog (we are collecting them as well) for next step, while starting needed UX discussion and design there? Not sure if there is a technical hard dependency between 4c and "4a + 4b", what I know is the engineering complexity/effort is one of the team's work planning consideration[1], as they are still new to Firefox.

[1] https://docs.google.com/spreadsheets/d/1Urq6bvpYmhFafrlf_g4CADIhlYbQc6FyccDYvrKeFGU/edit#gid=0

Comment 19

[:Paolo Amadini]

(In reply to Wesly Huang (Firefox EPM) from comment #18)
> 1. include 1265337 into our project scope
> 2. currently its "depends on" field is blank, and we should break 1265337
> into small pieces and collect them here. (all become the project scope)
> 3. 1265337 "blocks" 3 bugs for malware/blocked downloads, those 3 are *NOT*
> part of our project scope

This is exactly what I suggested, correct.

> 4. and to complete 1269957, the proposed clustering is 
>    a. 1269957 (this one)
>    b. height reduction (Carol has covered it in comment#11 & comment#16) 
>    c. 1265337

I guess you're referring to comment 14, which I think still makes sense.

> I'm confirming this because recently we are defining the project MVP and
> it's mostly based on the UX spec. Today 1269957 is already included, but for
> 1265337 we anticipate there will be further discussion needed so not marked
> as MVP.

So, what I currently see in the spreadsheet from comment 18 confirms that the visual redesign is not part of the currently defined MVP. But I'm surprised by this choice considering that:

* The visual spec is already very well detailed in bug 1269956.
* The required code changes are straightforward compared to other parts. It allows the team to get familiar with the code before tackling more difficult design problems.
* Most of the CSS changes will involve removing lines of code, especially because we can use SVG for all the assets. This will make other functional changes easier.
* Since there aren't functional changes in the visual redesign, we're less likely to find regressions or roadblocks down the way compared to, say, the toolbar button functional redesign (it's a smaller area but much more information in there!).
* The changes are highly visible and useful to the user, and representative of progress on the overall project.

In other words, it seems one of the clusters with the lowest cost and highest impact. It can still be challenging, but everything is when working on a product with the quality and user base of Firefox!

> what I know is the engineering complexity/effort is one of the team's work
> planning consideration[1], as they are still new to Firefox.

For someone new to the code, I would definitely recommend tackling the existing visuals first. This still involves practicing skills like front-end debugging that are a subset of those required by more thorough redesigns. It also requires less reviews compared for example to string changes or web interaction changes.

Comment 20

[Wesly Huang (EPM)]

Thanks Paolo, you've raised good points thus looks to me it could be worth considering, even that means the project has to change its MVP scope and implementation plan (assuming mostly FE's work). In order to make a better evaluation, would you help us see more about 1265337[1], so we get a clearer picture like the expected scope, UX's design concept (saw some UX material there), and relevant offline discussion/history we should bear in mind?

Maybe we should move the following discussion to 1265337[1], later I'll add a comment there to follow this topic.

[1] https://bugzilla.mozilla.org/show_bug.cgi?id=1265337

(In reply to :Paolo Amadini from comment #19)
> (In reply to Wesly Huang (Firefox EPM) from comment #18)
> 
> > 4. and to complete 1269957, the proposed clustering is 
> >    a. 1269957 (this one)
> >    b. height reduction (Carol has covered it in comment#11 & comment#16) 
> >    c. 1265337
> 
> I guess you're referring to comment 14, which I think still makes sense.
> 
> > I'm confirming this because recently we are defining the project MVP and
> > it's mostly based on the UX spec. Today 1269957 is already included, but for
> > 1265337 we anticipate there will be further discussion needed so not marked
> > as MVP.
> 
> So, what I currently see in the spreadsheet from comment 18 confirms that
> the visual redesign is not part of the currently defined MVP. But I'm
> surprised by this choice considering that:
> 
> * The visual spec is already very well detailed in bug 1269956.
> * The required code changes are straightforward compared to other parts. It
> allows the team to get familiar with the code before tackling more difficult
> design problems.
> * Most of the CSS changes will involve removing lines of code, especially
> because we can use SVG for all the assets. This will make other functional
> changes easier.
> * Since there aren't functional changes in the visual redesign, we're less
> likely to find regressions or roadblocks down the way compared to, say, the
> toolbar button functional redesign (it's a smaller area but much more
> information in there!).
> * The changes are highly visible and useful to the user, and representative
> of progress on the overall project.
> 
> In other words, it seems one of the clusters with the lowest cost and
> highest impact. It can still be challenging, but everything is when working
> on a product with the quality and user base of Firefox!
> 
> > what I know is the engineering complexity/effort is one of the team's work
> > planning consideration[1], as they are still new to Firefox.
> 
> For someone new to the code, I would definitely recommend tackling the
> existing visuals first. This still involves practicing skills like front-end
> debugging that are a subset of those required by more thorough redesigns. It
> also requires less reviews compared for example to string changes or web
> interaction changes.

Comment 21

[KM Lee [:rexboy] (inactivate after Sep. 22)]

Attachment: 1280x720 under osx

So until now most of the important part of bug 1265337 has been done.
will it be a good time to consider checking-in this patch?
If we're targeting the lowest resolution height down to 768 or 720px, I can help on making sure if the resolution works or not across different platforms.
a 1280x720 resolution osx seems to be ok for me. See attachment.

Comment 22

[KM Lee [:rexboy] (inactivate after Sep. 22)]

Attachment: 1280x720 under windows

Comment 23

[KM Lee [:rexboy] (inactivate after Sep. 22)]

Attachment: 1280x720 under linux

Comment 24

[:Paolo Amadini]

We need to reduce and unify the height of the items according to the visual design. We can do this after the progress bar styling is unified in bug 1301384. I suggest using this bug to do the height reductions at the same time as increasing the limit.

When evaluating the total height we can assume higher resolutions, but we have to account for a possibly different anchor position on the screen when the windows is not maximized.

[Tracking Requested - why for this release]:
This is the last bug in the visual redesign of the Downloads Panel, that we want to deliver in Firefox 52.

Comment 25

[KM Lee [:rexboy] (inactivate after Sep. 22)]

Alright, I'll finish bug 1301384 first before trying to land this patch.

I'm not quite sure about the different anchor position; do you mean for example if we put the window at the bottom of the screen, the popup goes upside rather than downside?

If we need to make sure the popup doesn't exceed the screen anywhere the window is placed, then the height can only be in maximum half of the resolution height (since when the button goes lower half of the screen, it pops out upside). This would be a relatively strict criteria.
Since user can resolve this by moving the window, a looser criteria may be plenty; but we can try it in more detail after landing 1301384.

Comment 26

[KM Lee [:rexboy] (inactivate after Sep. 22)]

set ni? for comment 25

Comment 27

[Carol Huang [:Carol]]

(In reply to :Paolo Amadini from comment #24)
> We need to reduce and unify the height of the items according to the visual
> design. We can do this after the progress bar styling is unified in bug
> 1301384. I suggest using this bug to do the height reductions at the same
> time as increasing the limit.

Hi Paolo,
I have reduced the current panel list height for the new design. If we shrink too much space, the information might look too clustered or complicated. We don't want to loose the legibility and viewing comfort for most of the users. 

thanks!!

> When evaluating the total height we can assume higher resolutions, but we
> have to account for a possibly different anchor position on the screen when
> the windows is not maximized.
> 
> [Tracking Requested - why for this release]:
> This is the last bug in the visual redesign of the Downloads Panel, that we
> want to deliver in Firefox 52.

Comment 28

[Ben Tian (inactive)]

Assign to Rex to follow this bug.

Comment 29

[:Paolo Amadini]

Sounds good, let's try what we can do after bug 1301384 lands, by setting the same height of the items on all platforms. We don't have to be strict about using less than half the screen height of supported resolutions, but we have to consider that the Firefox window may not be used in a maximized state by some users.

Comment 30

[KM Lee [:rexboy] (inactivate after Sep. 22)]

Attachment: item height 70px in windows

Seems the new progressbar keeps item height under 70px well in windows.

Comment 31

[KM Lee [:rexboy] (inactivate after Sep. 22)]

Attachment: item height 70px in mac

Comment 32

[KM Lee [:rexboy] (inactivate after Sep. 22)]

Attachment: item 70px in linux

But it is 72px which is slightly higher than the visual spec with progressbar in my ubuntu. In Linux the height was 6em and translated into 80px in my environment.
Seems the em is larger here due to font selection?

hmm... not sure if we should keep em as the unit.

Comment 33

[KM Lee [:rexboy] (inactivate after Sep. 22)]

Attachment: Bug 1269957 - Unify height of download items and allow maximum 5 items to show in downloads panel.

Review commit: https://reviewboard.mozilla.org/r/97178/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/97178/

Comment 34

[KM Lee [:rexboy] (inactivate after Sep. 22)]

Comment on attachment 8816422 [details]
Bug 1269957 - Unify height of download items and allow maximum 5 items to show in downloads panel.

Well, at last I found that using 70px height given by visual spec under linux seems to be a little bit too narrow for some theme since the font used tend to be larger than windows and mac. 
After talked about the issue with visual, we think preserving the unique height on most of the default theme would be good unless the font size is set too large to fit in the item. so I decide to keep the unit as em, give it a value correspond to 70px under default theme for mac and windows; while giving an em value that won't cause the context exceed the height under certain theme. Specifically the value I picked corresponds to 71px under my kde and 78px under default ubuntu theme. I tested these values with progressbar included and they are safe from exceed the setting height.

Not sure if there's a better idea. Paolo would you take a look and give some feedback for me? Thanks a lot!

Comment 35

[KM Lee [:rexboy] (inactivate after Sep. 22)]

Just found the code to determine the initial size:
https://dxr.mozilla.org/mozilla-central/source/browser/base/content/browser.js#1031

So it launches maximized when screen size * 0.9 is less then 1280 x 1040,
which applies to 1366 x 768 size (1229 x 691 when multiplied by 0.9).

Comment 36

[KM Lee [:rexboy] (inactivate after Sep. 22)]

By this condition 1440 x 900 (which is a common size for 15" notebook) doesn't start maximized but 1280 x 800 do.
By this rule the height should be ok even down to 1024 x 640.

Comment 37

[KM Lee [:rexboy] (inactivate after Sep. 22)]

Comment on attachment 8816422 [details]
Bug 1269957 - Unify height of download items and allow maximum 5 items to show in downloads panel.

Based on our face-to-face discussion change f? to r?.

Comment 38

[:Paolo Amadini]

Comment on attachment 8816422 [details]
Bug 1269957 - Unify height of download items and allow maximum 5 items to show in downloads panel.

https://reviewboard.mozilla.org/r/97178/#review98578

::: browser/themes/linux/downloads/downloads.css:18
(Diff revision 1)
>  }
>  
>  /*** List items and similar elements in the summary ***/
>  
>  :root {
> -  --downloads-item-height: 6em;
> +  --downloads-item-height: 5.325em;

It's good to have these in em, so they adapt to the chosen font size.

We probably need to round these values so they make sense as em values independently from the final pixel height, for example 5.75em, 6.25em, or 5.25em. Even on Windows or Mac, the default themes may be different based on which version you use, so there's little point in having those values calculated precisely. If we could find a value that works well on most platforms, that would be best.

::: browser/themes/linux/downloads/downloads.css:20
(Diff revision 1)
>  
>  :root {
> -  --downloads-item-height: 6em;
> +  --downloads-item-height: 5.325em;
>    --downloads-item-font-size-factor: 0.9;
> -  --downloads-item-target-margin-bottom: 7px;
>    --downloads-item-details-margin-top: 1px;

Since this is the last bug for the visual redesign, we should adjust both the horizontal and vertical margins to be consistent and provide visual symmetry like we have in the specification.

In fact, most margins are leftovers from previous work that we postponed to this last bug to get right.

The margin between the labels and the progress bar should be the same. It seems that a 2px margin does the job. Define these as a top margin on the progress bar and the bottom label, so that when these elements are hidden what remains keeps the right margins.

Since the bottom label has a smaller font size than the top one, we should calc() a bottom margin or a fixed height for that label so that its height is equal to the top label. By doing that, we ensure that the progress bar is always vertically centered in the item, and aligned with the button.

Ensure that all other left, right, and vertical margins on the labels and progress bar are set to zero. If we need more space, we should adjust the margins on the container.

According to the visual design, the space between the border of the panel and the start of the text should be the same width as the action button. Let's see what we can do. Regardless of the actual width we choose here, the file type icon should be horizontally centered in that space. I think Sean noticed a bug with regard to the icon spacing in the footer of the panel as well.

::: browser/themes/osx/downloads/downloads.css:23
(Diff revision 1)
>  
>  /*** List items and similar elements in the summary ***/
>  
>  :root {
> -  --downloads-item-height: 7em;
> +  --downloads-item-height: 6.363em;
>    --downloads-item-font-size-factor: 0.95;

Do we still need a different font size factor for Mac?

Comment 39

[Ben Tian (inactive)]

As a reference, this bug will change following UX items:
- Change height of download items to 70px
- Change the number of list items to 5

Comment 40

[KM Lee [:rexboy] (inactivate after Sep. 22)]

(In reply to :Paolo Amadini from comment #38)

> The margin between the labels and the progress bar should be the same. It
> seems that a 2px margin does the job. Define these as a top margin on the
> progress bar and the bottom label, so that when these elements are hidden
> what remains keeps the right margins.
> 
I don't get it. Do you mean set their margin-top all to 2px and get rid of
margin-bottom? That would make a very squashed looking to me which doesn't
look like the visual spec.

> 
> ::: browser/themes/osx/downloads/downloads.css:23
> (Diff revision 1)
> >  
> >  /*** List items and similar elements in the summary ***/
> >  
> >  :root {
> > -  --downloads-item-height: 7em;
> > +  --downloads-item-height: 6.363em;
> >    --downloads-item-font-size-factor: 0.95;
> 
> Do we still need a different font size factor for Mac?
By spec the file title is 13px and description is 11px and seems
the font factor makes them right. So I don't get the reason for
removing it. Could you describe it more?

Comment 41

[KM Lee [:rexboy] (inactivate after Sep. 22)]

And for the equal width, The border-to-text width is 44+8+6=58 which is the same as the action button under my mac. The visual spec says 56px so we may decrease the margin by 2 though. Did you see uneven width on other platforms?

Comment 42

[:Paolo Amadini]

(In reply to KM Lee [:rexboy] from comment #40)
> I don't get it. Do you mean set their margin-top all to 2px and get rid of
> margin-bottom? That would make a very squashed looking to me which doesn't
> look like the visual spec.

The actual value can be more than 2px if you think it matches the visuals better, what I mean is that we have these element combinations displayed at any given time:
 1) The file name
 2) The file name and the status line
 3) The file name, the progress bar, and the status line
...and all of these combinations should appear vertically centered. The easiest way is to specify the same margin-top on the progress bar and the status line, and no margins on the file name, because it is the only one that can appear on its own, and it is always visible.

> > Do we still need a different font size factor for Mac?
> By spec the file title is 13px and description is 11px and seems
> the font factor makes them right. So I don't get the reason for
> removing it. Could you describe it more?

It's fine if we have different ratios because they are needed to do the right thing visually on the different platforms. I just wondered if you had considered this possible opportunity for unification, and it seems you did and determined that the different ratios are still needed.

Comment 43

[Stephan Sokolow]

Just a data point for you:

If the number of downloads were increased to 5 (four active, plus one for momentaries like saving images) and the behaviour were changed so I didn't need "middle-click to remove from panel" to fake "always sort in-progress downloads above completed downloads", then I'd have no need for the Download Panel Tweaker extension.

(I tend to download four things in parallel because I buy DRM-free game/book bundles and four simultaneous downloads is a number I settled on as a good balance point between having to babysit the download panel and running up against connection limits set by sites.)

Comment 44

[KM Lee [:rexboy] (inactivate after Sep. 22)]

Comment on attachment 8816422 [details]
Bug 1269957 - Unify height of download items and allow maximum 5 items to show in downloads panel.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/97178/diff/1-2/

Comment 45

[KM Lee [:rexboy] (inactivate after Sep. 22)]

I set both height of downloadTarget and downloadDetails to 1.45em (the latter is multiplied by font factor of course). Not sure if there're better way to keep the heights the same.

1.45em comes from the minimum height that can contain the height of default font across different platforms.

Comment 46

[:Paolo Amadini]

(In reply to KM Lee [:rexboy] from comment #41)
> And for the equal width, The border-to-text width is 44+8+6=58 which is the
> same as the action button under my mac. The visual spec says 56px so we may
> decrease the margin by 2 though. Did you see uneven width on other platforms?

Sorry, I was interrupted yesterday and I ended up not sending this comment. A 2px difference on the horizontal spacing is definitely fine, I don't worry about it, but what I see on Mac at least is that the icon is not centered in the space.

The 6px margin on the labels and progress bar should not exist, we should add to the container's margin instead if needed. This makes the CSS code easier to follow, and can help with centering the icon without a lot of pre-calculated magical numbers.

Comment 47

[KM Lee [:rexboy] (inactivate after Sep. 22)]

ok, I may have the patch revised by tomorrow.

Comment 48

[KM Lee [:rexboy] (inactivate after Sep. 22)]

Comment on attachment 8816422 [details]
Bug 1269957 - Unify height of download items and allow maximum 5 items to show in downloads panel.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/97178/diff/2-3/

Comment 49

[KM Lee [:rexboy] (inactivate after Sep. 22)]

I ended up adjusting the border in download overlay because they share the same progress bar. Basically I just try to keep it looks like the download panel but keep the modification minimized.

Comment 50

[:Paolo Amadini]

Comment on attachment 8816422 [details]
Bug 1269957 - Unify height of download items and allow maximum 5 items to show in downloads panel.

https://reviewboard.mozilla.org/r/97178/#review102024

::: browser/themes/linux/downloads/downloads.css:18
(Diff revision 3)
>  }
>  
>  /*** List items and similar elements in the summary ***/
>  
>  :root {
> -  --downloads-item-height: 6em;
> +  --downloads-item-height: 5.35em;

I've found that using fractional values other than halves may cause pixel rounding issues for which a white horizontal line appears in the panel.

To me, it also makes sense for these em values to be assigned like now rather than calculated precisely, as I've noted in my previous comment.

I've tested new rounded values that work fine for me on various platforms. For Linux, use 5.5em here.

::: browser/themes/osx/downloads/downloads.css:22
(Diff revision 3)
>  }
>  
>  /*** List items and similar elements in the summary ***/
>  
>  :root {
> -  --downloads-item-height: 7em;
> +  --downloads-item-height: 6.35em;

Use 6em here for Mac OS X.

::: browser/themes/shared/downloads/allDownloadsViewOverlay.inc.css:24
(Diff revision 3)
>  %ifdef XP_WIN
>  @media (-moz-os-version: windows-xp) {
>  %endif
>  #downloadsRichListBox > richlistitem.download {
> -  padding: 5px 8px;
> +  padding: 5px 0;
>  }
>  %ifdef XP_WIN
>  }
>  %endif

This rule and the special case for Windows XP can now be removed.

::: browser/themes/shared/downloads/allDownloadsViewOverlay.inc.css:35
(Diff revision 3)
>  %ifdef XP_WIN
>  }
>  %endif
>  
>  .downloadTypeIcon {
> -  margin-top: 8px;
> +  margin: 8px 12px;

"margin: 8px 13px;", so the value is the same as the Downloads Panel and there is no doubt if the CSS files are merged in the future.

::: browser/themes/shared/downloads/allDownloadsViewOverlay.inc.css:41
(Diff revision 3)
> -  }
> -}
> -%endif
> -
>  .downloadBlockedBadge {
>    margin: 0 4px;

"margin: 0 5px;" for the same reason.

::: browser/themes/shared/downloads/allDownloadsViewOverlay.inc.css:62
(Diff revision 3)
>  @item@ > toolbarseparator {
>    display: none;
>  }
>  
>  .downloadTarget {
> -  margin-bottom: 3px;
> +  height: 1.45em;

Remove the height property, as I don't think we need to increase the height of this label.

A height of 1em corresponds to the font size already, we just need to slightly increase the height of .downloadDetails so that it's the same as this label. To do this, I've found a solution using "margin-bottom" that survives rounding better than using "height".

::: browser/themes/shared/downloads/allDownloadsViewOverlay.inc.css:63
(Diff revision 3)
> +  margin-top: 0;
> +  margin-bottom: 0;
> +  margin-inline-start: 0;

"margin: 0;"

::: browser/themes/shared/downloads/allDownloadsViewOverlay.inc.css:71
(Diff revision 3)
> +  /* Keep height consist with .downladTarget,
> +     so that progressbar can be vertically centered */
> +  height: calc(1.45em / .95);
> +  margin-top: 4px;
> +  margin-bottom: 0;
> +  margin-inline-start: 0;

Here is my solution using the margin. I've also edited the comment and fixed the typo.

  /* Use calc() to keep the height consistent with .downloadTarget, so that the
     progress bar can be vertically centered. */
  margin: 4px 0 calc(1em / 0.95 - 1em);

::: browser/themes/shared/downloads/allDownloadsViewOverlay.inc.css:85
(Diff revision 3)
> -  margin: 3px;
> +  margin: 0 18px;
>    border: none;
> -  padding: 5px;

This change turns the active area of the button into a vertical rectangle, which is strange when hovering with the mouse. The active area should be roughly square around the visible part of the button. Can you find better values?

::: browser/themes/shared/downloads/downloads.inc.css:26
(Diff revision 3)
>  #downloadsListBox {
>    background: transparent;
>    padding: 0;
>    color: inherit;
>    -moz-appearance: none;
>    margin: 0;
>    border: none;
>  }

While testing I found out that the padding and border properties in this existing rule are unnecessary. I suggest removing them in this patch since we're touching other margins too.

::: browser/themes/shared/downloads/downloads.inc.css:153
(Diff revision 3)
>    width: 16px;
>    height: 16px;
>  }
>  
>  #downloadsSummary {
> -  padding: 0 12px;
> +  padding: 0;

You can just remove this property.

::: browser/themes/shared/downloads/downloads.inc.css:170
(Diff revision 3)
>    padding-inline-end: 0;
>    color: inherit;

Similarly, "padding-inline-end" can be removed here.

"color: inherit;" can be moved below to the rule for richlistitem[type="download"], because it's not needed for the summary.

::: browser/themes/shared/downloads/downloads.inc.css:175
(Diff revision 3)
>    margin: 0;
>    border-bottom: 1px solid var(--panel-separator-color);
>    background: transparent;
>    padding: 0;

Looks like the margin and padding properties can be removed from this rule too.

::: browser/themes/shared/downloads/downloads.inc.css:185
(Diff revision 3)
>  richlistitem[type="download"] > .downloadMainArea {
> -  padding: 8px;
> +  padding: 8px 0;
>  }

Remove this rule, because we can use the margin on ".downloadContainer".

::: browser/themes/shared/downloads/downloads.inc.css:196
(Diff revision 3)
>    width: 32px;
>    height: 32px;
>  }
>  
>  .downloadBlockedBadge {
> -  margin: 0 4px;
> +  margin: 0 8px;

"margin: 0 5px;" so that the badge is centered over the corner of the reserved 32x32 icon space.

::: browser/themes/shared/downloads/downloads.inc.css:226
(Diff revision 3)
>  
>     Finally, since we want .downloadTarget's font-size to be at 100% of the
>     font-size of .downloadContainer's parent, we use calc to go from the
>     smaller font-size back to the original font-size.
>   */
>  #downloadsSummaryDetails,

Remove the #dowloadsSummaryDetails selector. I found out that we've been applying font sizes incorrectly in the summary, this should fix the situation.

::: browser/themes/shared/downloads/downloads.inc.css:229
(Diff revision 3)
>     smaller font-size back to the original font-size.
>   */
>  #downloadsSummaryDetails,
>  .downloadContainer {
>    font-size: calc(100% * var(--downloads-item-font-size-factor));
> +  margin-inline-end: 8px;

"margin-inline-end: 13px;" to match the 13px margin around the icon.

::: browser/themes/shared/downloads/downloads.inc.css:234
(Diff revision 3)
> -  margin-bottom: var(--downloads-item-target-margin-bottom);
> +  margin-top: 0;
> +  margin-bottom: 0;
> +  margin-inline-start: 0;
> +  height: 1.45em;

"margin: 0;" and remove the height.

::: browser/themes/shared/downloads/downloads.inc.css:240
(Diff revision 3)
>  .downloadTarget {
>    font-size: calc(100% / var(--downloads-item-font-size-factor));
>  }

Remove this rule and move the font-size property to the rule above, because it should apply to both the summary and the items.

::: browser/themes/shared/downloads/downloads.inc.css:244
(Diff revision 3)
>  #downloadsSummaryDetails,
>  .downloadDetails {
> -  margin-top: var(--downloads-item-details-margin-top);
> +  margin-top: 4px;
> +  margin-bottom: 0;
> +  margin-inline-start: 0;
>    opacity: var(--downloads-item-details-opacity);
> +  /* use calc() to keep height consist with .downladTarget,
> +     so that progressbar can be vertically centered */
> +  height: calc(1.45em / var(--downloads-item-font-size-factor));
>  }

Remove the "height" and edit the margins:

  /* Use calc() to keep the height consistent with .downloadTarget, so that the
     progress bar can be vertically centered. */
  margin: 4px 0 calc(1em / var(--downloads-item-font-size-factor) - 1em);

::: browser/themes/shared/downloads/downloads.inc.css
(Diff revision 3)
>    -moz-appearance: none;
>    min-width: 58px;
>    margin: 0;
>    border: none;
>    background: transparent;
> -  padding: 8px;

This changes the way the focus ring is displayed. It's not directly related to this bug, but the result looks better on Windows and Mac OS X, so we could take the occasion and update it.

You probably need to set "padding: 0;" instead of removing the rule, to override platform values.

On Linux this change makes the focus ring disappear, to fix this you likely need to add an outline offset of minus one pixel in the platform-specific CSS file.

::: browser/themes/shared/downloads/progressmeter.inc.css:5
(Diff revision 3)
> -  margin-top: 2px;
> -  margin-bottom: 2px;
> -  margin-inline-start: 6px;
> +  margin-top: 4px;
> +  margin-bottom: 0;
> +  margin-inline-start: 0;
>    margin-inline-end: 12px;

"margin: 4px 0 0;"

::: browser/themes/windows/downloads/downloads.css:22
(Diff revision 3)
>  }
>  
>  /*** List items and similar elements in the summary ***/
>  
>  :root {
> -  --downloads-item-height: 7em;
> +  --downloads-item-height: 5.85em;

Use 5.5em for Windows.

Comment 51

[:Paolo Amadini]

(In reply to :Paolo Amadini from comment #50)
> On Linux this change makes the focus ring disappear, to fix this you likely
> need to add an outline offset of minus one pixel in the platform-specific
> CSS file.

Actually, this should match whatever we do for the footer, which looks like an outline offset of -5px on Linux.

Comment 52

[KM Lee [:rexboy] (inactivate after Sep. 22)]

Comment on attachment 8816422 [details]
Bug 1269957 - Unify height of download items and allow maximum 5 items to show in downloads panel.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/97178/diff/3-4/

Comment 53

[KM Lee [:rexboy] (inactivate after Sep. 22)]

Comment on attachment 8816422 [details]
Bug 1269957 - Unify height of download items and allow maximum 5 items to show in downloads panel.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/97178/diff/4-5/

Comment 54

[KM Lee [:rexboy] (inactivate after Sep. 22)]

Thanks for those useful comments to make the patch more simplified.

I still don't know very well how the em was handled because the height start to grow after 1.3em for me, so I tend to think the actual font size is slightly bigger than 1em for some reason (and that was the reason I picked a relatively big value)... but seems the calc() you gave works very well with 1em.

And hope I addressed all the changes we needed. I tested them on my environment and they looks good.

Comment 55

[:Paolo Amadini]

(In reply to KM Lee [:rexboy] from comment #54)
> Thanks for those useful comments to make the patch more simplified.

Thanks to you for working on the bug!

> I still don't know very well how the em was handled because the height start
> to grow after 1.3em for me, so I tend to think the actual font size is
> slightly bigger than 1em for some reason (and that was the reason I picked a
> relatively big value)... but seems the calc() you gave works very well with
> 1em.

I don't know why that happens, maybe it's related to pixel rounding or some other internal CSS calculation I don't know about. I'm glad the margin calculation works though!

Comment 56

[:Paolo Amadini]

Comment on attachment 8816422 [details]
Bug 1269957 - Unify height of download items and allow maximum 5 items to show in downloads panel.

https://reviewboard.mozilla.org/r/97178/#review102880

::: browser/themes/shared/downloads/downloads.inc.css:229
(Diff revision 5)
> -  margin-top: var(--downloads-item-details-margin-top);
> +  margin-top: 4px;
> +  margin-bottom: 0;
> +  margin-inline-start: 0;

You only forgot to remove these margin properties, they're overridden by the shorthand property below.

For the rest, looks good!

Comment 57

[KM Lee [:rexboy] (inactivate after Sep. 22)]

Comment on attachment 8816422 [details]
Bug 1269957 - Unify height of download items and allow maximum 5 items to show in downloads panel.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/97178/diff/5-6/

Comment 58

[KM Lee [:rexboy] (inactivate after Sep. 22)]

Thank you Paolo!

Those removal has been updated to the patch.

Comment 59

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/55d5ffe4d13b
Unify height of download items and allow maximum 5 items to show in downloads panel. r=Paolo

Comment 60

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/55d5ffe4d13b

Comment 61

[KM Lee [:rexboy] (inactivate after Sep. 22)]

Comment on attachment 8816422 [details]
Bug 1269957 - Unify height of download items and allow maximum 5 items to show in downloads panel.

Approval Request Comment
[Feature/Bug causing the regression]: Bug 1269956
[User impact if declined]: The download items shown keeps at 3 and UI design remains the old one.
[Is this code covered by automated tests?]: No 
[Has the fix been verified in Nightly?]: No
[Needs manual test from QE? If yes, steps to reproduce]: No
[List of other uplifts needed for the feature/fix]: None
[Is the change risky?]: No
[Why is the change risky/not risky?]: Contains mainly minor css changes for visual polish
[String changes made/needed]: No

Comment 62

[Julien Cristau [:jcristau]]

Comment on attachment 8816422 [details]
Bug 1269957 - Unify height of download items and allow maximum 5 items to show in downloads panel.

download panel style unification, aurora52+

Comment 63

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/1d0d4ed301e0

Comment 64

[Cristian Comorasu, QA [:ccomorasu], Releae Desktop QA - Please contact [e-mail]]

I reproduced this issue using 53.0a1, build ID:20161218030213, on Windows 10 x64.
I can confirm this issue is fixed, I verified using Fx52.0a2, build ID: 20170113004016 and Fx53.0a1, build ID: 20170113030227, on Windows 10 x64, Mac OS X 10.12.3 and Ubuntu 14.04 LTS.

Cheers!

Comment 65

[Pascal Chevrel:pascalc]

Release Note Request (optional, but appreciated)
[Why is this notable]: The download panel got a minor visual and functional refresh
[Affects Firefox for Android]: no
[Suggested wording]: The download panel now displays the 5 most recent downloads instead of 3 
[Links (documentation, blog post, etc)]:

Comment 66

[Julien Cristau [:jcristau]]

Added to 52beta release notes along with bug 1270014

Comment 67

[Cristian Comorasu, QA [:ccomorasu], Releae Desktop QA - Please contact [e-mail]]

This issue is fixed on Fx 54.0b3 and the latest nightly (28-04-2017). I am going to close this issue.