Closed Bug 1270006 Opened 8 years ago Closed 7 years ago

Replace the downloads remaining time on the downloads button with a downloading icon

Categories

(Firefox :: Downloads Panel, defect)

Product:
Firefox
Component:
Downloads Panel
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
VERIFIED FIXED
Milestone:
Firefox 54
Tracking Flags:
Tracking Status
firefox54 --- verified

People

(Reporter: yifan, Assigned: rexboy)

References

(Depends on 1 open bug, Blocks 1 open bug)

Details

(Whiteboard: [CHE-MVP])

Attachments

(9 files, 1 obsolete file)

Download Panel and Content Handler.pdf
6.98 MB, application/pdf
Details
preview for "unknown progress" in the download button
24.37 KB, image/gif
Details
WIP
12.25 KB, text/x-pascal
Details
Bug 1270006 - Fill portion of the icon on download button as indicator to download progress
58 bytes, text/x-review-board-request
Paolo
: review+
Details
Indicator when a download just started
6.19 KB, image/png
Details
Indicator when a download is midway
6.21 KB, image/png
Details
Indicator when a download is almost completed
6.24 KB, image/png
Details
Comparison of different OSs for a download in midway
93.18 KB, image/png
Details
Screen Shot 2016-09-02 at 4.28.03 PM.png
211.59 KB, image/png
Details 
Replace the downloads remaining time on the downloads button when downloading files with a smaller downloads indicator icon.
Blocks: 1270012
Blocks: 1270014
Component: General → Downloads Panel
Summary: Replace the downloads remaining time on the downloads button with an downloading icon → Replace the downloads remaining time on the downloads button with a downloading icon
(In reply to yifan [:yifan][:yliao] from comment #0)
> Replace the downloads remaining time on the downloads button when
> downloading files with a smaller downloads indicator icon.

I'm not quite sure while the compact indication of the remaining time would have to be removed from the Downloads Indicator. This is a feature that is especially effective and applies to the most common download cases, where there is only one download running and the server provides the file size. The time indication is designed to be coarse in the early stages of the download, in order to have less visual noise, with updates becoming more frequent as the download end time approaches.
No longer blocks: 1270014
Flags: needinfo?(mochen)
Flags: needinfo?(mochen)
Flags: needinfo?(mochen)
Flags: needinfo?(bmao)
(In reply to :Paolo Amadini from comment #1)
> (In reply to yifan [:yifan][:yliao] from comment #0)
> > Replace the downloads remaining time on the downloads button when
> > downloading files with a smaller downloads indicator icon.
> 
> I'm not quite sure while the compact indication of the remaining time would
> have to be removed from the Downloads Indicator. This is a feature that is
> especially effective and applies to the most common download cases, where
> there is only one download running and the server provides the file size.
> The time indication is designed to be coarse in the early stages of the
> download, in order to have less visual noise, with updates becoming more
> frequent as the download end time approaches.

Here are a few reasons why we removed the remaining time on download indicator:

1. It's too small to see
2. With the arrow missing from the indicator, it's hard to find the it in a glance
3. When there are multiple downloading, the estimated time becomes more inaccurate 
4. Progress bar provides similar sense to the remaining time

We've went through this idea with Firefox UX, and they has the same feeling as we do. By removing the remaining time, we could focus the indicator more on providing information of status change (downloading, completed, errors) which user can easily check the indicator in a glance. 

I agree remaining time is important information to user, but as we've introduced the download notification, user will get the estimated time at the beginning of the download, then progress bar will take over to give user a general sense of how the download goes. During the process, if user really want to know the exact remaining time, they just click the indicator for more information.
Flags: needinfo?(mochen)
Flags: needinfo?(bmao)
Flags: needinfo?(paolo.mozmail)
Is there any question for me here?
Flags: needinfo?(paolo.mozmail)
Hi Paolo,

I believe Bryant was trying to share UX's idea behind the design change here, and see if you have any further thought/comment about that (after all we are newcomer for download panel). You are right there is no question for you, but glad to see if you have any for us. Once ready we'll then move on the implementation :)

(In reply to :Paolo Amadini from comment #3)
> Is there any question for me here?
Flags: needinfo?(paolo.mozmail)
I use this feature, but this doesn't necessarily mean it doesn't make sense to remove it as part of a larger design. We have indicated some elements to think about in the previous comments, how to proceed is defined in the realm of user experience design.
Flags: needinfo?(paolo.mozmail)
Thanks Paolo for all the sharing! I'll make sure our UX team have take those elements and discuss with Firefox UX team before making the final design.

(In reply to :Paolo Amadini from comment #5)
> I use this feature, but this doesn't necessarily mean it doesn't make sense
> to remove it as part of a larger design. We have indicated some elements to
> think about in the previous comments, how to proceed is defined in the realm
> of user experience design.
Assignee: nobody → rexboy
Hello Paolo:
Based on the proposal, we are thinking about how to implement the "arrow-shaped progress icon". How do you think of:
(1)making a serial of icons from empty to full, and we use them as sprites. Just change to corresponding sprites when progress changes.
(2)overlap a "full icon" over an "empty icon" and show partial of full icon to represent the progress, but this may have some problem in case the icon have some space around its broder.

Do you have a preferred way for it?
(Currently We're preferring (1) for possible problems that may caused by different themes if we use(2))
Flags: needinfo?(paolo.mozmail)
(In reply to KM Lee [:rexboy] from comment #8)
> (2)overlap a "full icon" over an "empty icon" and show partial of full icon
> to represent the progress

This is the simplest, because you need only one new image per theme, and you may be able to reuse the current "attention" image for the purpose.

> (Currently We're preferring (1) for possible problems that may caused by
> different themes if we use(2))

What are these possible problems exactly?

Regardless of the above, you probably need to implement the new "attention" badging first, otherwise you would conflate progress with attention.

In fact, the mockup only shows the case of one download. What is the treatment in the case of multiple downloads running concurrently, but started at different times?

How do you handle the case of one download just completed while another is in progress?

Note that currently, shortly after a download reaches 100% progress, it goes to the "completed" state and the progress bar disappears. However, the "attention" state, which is completely independent, is kept until the user has opened the panel.
Flags: needinfo?(paolo.mozmail)
(In reply to :Paolo Amadini from comment #9)
> What are these possible problems exactly?
Oh, I mean we need to confirm the sprite leaves no blank space at bottom and top; or the progress shown wouldn't reflect 0% to 100% correctly.
Just checked all assets again and seems it's not the case. 
I'll try to use (1) first then.


> In fact, the mockup only shows the case of one download. What is the
> treatment in the case of multiple downloads running concurrently, but
> started at different times?
> 
Is it ok if we let the progress be (overall downloaded size/overall file size)?
And exclude unknown sized files.
> How do you handle the case of one download just completed while another is
> in progress?
> 
This should perform a download complete animation (scaling the arrow),
whereas the icon progress is still kept there to indicate the overall progress at that time.

Thanks for the reply!
Let me first take a look for the attention badge/state then.
(In reply to KM Lee [:rexboy] from comment #10)
> (In reply to :Paolo Amadini from comment #9)
> > What are these possible problems exactly?
> Oh, I mean we need to confirm the sprite leaves no blank space at bottom and
> top; or the progress shown wouldn't reflect 0% to 100% correctly.

Not necessarily - you can choose a mapping between the value of the progress and how many pixels of the images are shown that doesn't map linearly from 0px to 18px. For example you can map 0% to 0px, jump to 4px for 1%, move linearly to 14px for 99%, and jump to 18px for 100%.

> I'll try to use (1) first then.

How would you plan to integrate the images in the source tree for the various themes exactly?

Do you plan to generate and test different images for all the variations we currently have for the Downloads button, including various combinations of dark lightweight themes, Developer Edition, HiDPI, all the supported editions of OS X, and all the supported themes of Windows? How many images will you need for each case?

Do you plan to create the images yourself, or ask someone from the UX team to do it? What happens if later the UX team decides that the style of the toolbar icons for one particular theme combination should change? Who updates the images, and how?

> > In fact, the mockup only shows the case of one download. What is the
> > treatment in the case of multiple downloads running concurrently, but
> > started at different times?
> > 
> Is it ok if we let the progress be (overall downloaded size/overall file
> size)?
> And exclude unknown sized files.

What if I have 10 downloads with a known file size already completed, and I start a new one? Do the previous downloads count in the total? This would result for the progress bar for a new download to proceed start at 80% and go to 100% instead of 0% to 100%.

What would we show if there is only one download with an unknown size running? We currently show nothing, and this is an existing bug, that I'd like to see finally fixed. We originally planned to do this by showing a progress bar, but with no indication of progress in it. How would this be handled in this new design?

> > How do you handle the case of one download just completed while another is
> > in progress?
> > 
> This should perform a download complete animation (scaling the arrow),
> whereas the icon progress is still kept there to indicate the overall
> progress at that time.

Doesn't this leave no visible indication that a download has completed inside the panel, if I'm looking away from the screen when the animation happens? Or is this replaced by a different type of "download complete" attention badging?

Currently, even if the progress bar fro a different download is shown, the small arrow is highlighted if there is a completed download, so I can know to look for it in the panel, even if another download is in progress.

> Let me first take a look for the attention badge/state then.

That might definitely be an easier bug to move forward while the design of this one is discussed.
(In reply to :Paolo Amadini from comment #11)
> > I'll try to use (1) first then.
> 
> How would you plan to integrate the images in the source tree for the
> various themes exactly?
Ouch. sorry for my mistyping. It’s my bad. I mean I can try to use (2) first, the overlapping way to implement first. I agree with you that making these assets just need much more additional works and the way of implementation you mentioned sounds quite good, so I'd try that first and see if it's good.

> > Is it ok if we let the progress be (overall downloaded size/overall file
> > size)?
> > And exclude unknown sized files.
> 
> What if I have 10 downloads with a known file size already completed, and I
> start a new one? Do the previous downloads count in the total? This would
> result for the progress bar for a new download to proceed start at 80% and
> go to 100% instead of 0% to 100%.
> 
Completed files and unknown-size files should be excluded from the calculation. So in this case it should start from 0%.
By this rule, the percentage may jump to a new value when any of the current downloading files completed; and UX team think it’s acceptable for the jump. (The jump also happen on other browsers' download progress like Opera and Chrome). How do you think?

> What would we show if there is only one download with an unknown size
> running? We currently show nothing, and this is an existing bug, that I'd
> like to see finally fixed. We originally planned to do this by showing a
> progress bar, but with no indication of progress in it. How would this be
> handled in this new design?
I was told there would be a new animation of running reflective on the arrow. I'm not quite sure what kind of technique should we use for it now, maybe from sprite, or PNG/GIF animation. And we will need to make varied versions for this. I’d ask visual’s opinion.

> > This should perform a download complete animation (scaling the arrow),
> > whereas the icon progress is still kept there to indicate the overall
> > progress at that time.
> 
> Doesn't this leave no visible indication that a download has completed
> inside the panel, if I'm looking away from the screen when the animation
> happens? Or is this replaced by a different type of "download complete"
> attention badging?
> 
> Currently, even if the progress bar fro a different download is shown, the
> small arrow is highlighted if there is a completed download, so I can know
> to look for it in the panel, even if another download is in progress.
I'm not quite sure where to find the highlighted small arrow, isn't there only remaining time shown when there are still other files downloading? Or should I download an unknown-sized file to see that?
Flags: needinfo?(paolo.mozmail)
Whiteboard: [CHE-MVP]
(In reply to KM Lee [:rexboy] from comment #12)
> Completed files and unknown-size files should be excluded from the
> calculation. So in this case it should start from 0%.
> By this rule, the percentage may jump to a new value when any of the current
> downloading files completed; and UX team think it’s acceptable for the jump.

This sounds good to me, but by this rule also when the last download completes apparently the progress should return to zero.

> I'm not quite sure where to find the highlighted small arrow, isn't there
> only remaining time shown when there are still other files downloading? Or
> should I download an unknown-sized file to see that?

Hm, apparently it's only shown if the other download time is undetermined. So the missing notification for completed downloads while others are running is an existing issue.
Flags: needinfo?(paolo.mozmail)
Oh and another one thing. What’s the preferred format for the animation of unknown-progress arrow? I've ever used animated PNG in prior project but not sure if it’s acceptable here.
animated GIF doesn't support alpha so it wouldn't be ok. PNG sprite need additional code for animation.
Flags: needinfo?(paolo.mozmail)
(In reply to KM Lee [:rexboy] from comment #14)
> Oh and another one thing. What’s the preferred format for the animation of
> unknown-progress arrow?

It depends on which type of animation we want. In general I'd say we shouldn't use animated graphics files but CSS animations as far as possible, for example changing opacity or the position of masks.
Flags: needinfo?(paolo.mozmail)
The animation would be something like this
it's a little bit hard to recognize due to color quantization by GIF; basically there's a white glow dropping down repeatedly in the arrow.

Since Firefox doesn't support bitmap-based image mask, I can't use the icon itself as mask for the glow.. Not sure if there's a better way than making a same version SVG file as mask.
Attachment #8776548 - Attachment description: preview for unknown progress → preview for "unknown progress" in the download button
Currently 3 ways can be considered,

1 - Make the icon SVG version and do CSS animation and mask correspondingly. But we need to evaluate if the SVG rendering satisfies the visual requirement.

2 - Keep PNG version and do animation by sprite and javascript to avoid using GIF and aPNGs. 

3 - Change the design to an easier one that can be achieved by like CSS filter.
(In reply to KM Lee [:rexboy] from comment #17)
> 1 - Make the icon SVG version and do CSS animation and mask correspondingly.
> But we need to evaluate if the SVG rendering satisfies the visual
> requirement.

Currently, we have performance issues that prevent us from using SVG or filters in the buttons that are shown by default when the browser starts (bug 1054016). However, we can still use SVG for state that is different from the default, for example when there are highlights and badges. The difficulty, as you said, is that the visual style of SVG may not match the visual style of the default button or other buttons in the toolbar.
Attached file WIP — 
Paste a WIP here.
This only contains mainly CSS change.

Things done are:
- Show progress by filling arrow
- Cross-platform tested under linux, mac, windows on normal/inverted theme
- Keep showing progress when there are downloads running; the "attention" state is shown by visual when all downloads are done.

TODOs are:
- Calculation of remaining percentage
- Unsure if we need to change style when pause
- Remove unused codes and styles
- "Unknown progress" animation
the "Calculation of remaining percentage" means adjusting current rule to the one we discussed above (if required)
Just confirmed with visual & UX that we're not going to change the color when pausing for now.
See Also: → 1290706
Since UXs are reviewing about unifying style of download panel under different OS, which may (or may not) include changes about "Unknown progress" animation; I'd prefer to open another bug for that animation.
And this bug should resolve the remaining things.
(In reply to KM Lee [:rexboy] from comment #22)
> Since UXs are reviewing about unifying style of download panel under
> different OS, which may (or may not) include changes about "Unknown
> progress" animation; I'd prefer to open another bug for that animation.
> And this bug should resolve the remaining things.

Sounds good to me, because at present we don't show any indication for unknown download progress anyways. Still, this is a bug that is important to fix in the new design.
Comment on attachment 8782857 [details]
Bug 1270006 - Fill portion of the icon on download button as indicator to download progress

Seems we are not going to use the text counter, I removed the related code.
The current percentage calculation seems ok and don't need to be modified.
So this patch basically just set the icon as our new progressbar and remove some codes that are not used anymore.

Paolo would you mind giving some feedback on this version?
Thank you!
Attachment #8782857 - Flags: feedback?(paolo.mozmail)
Comment on attachment 8782857 [details]
Bug 1270006 - Fill portion of the icon on download button as indicator to download progress

The approach looks good to me, I will test and provide more details on the XUL and CSS changes later, during the code review stage.

We must remove all the code that becomes unused, including the "paused" state and the overall time estimate calculation itself. If we need it later, we can just resurrect it from version control.

Since there is a lot of code to remove as a result, I recommend using multiple changesets. This will make the review easier.

You can have a "Part 1" that implements the new XUL structure and removes all the CSS that was added for the old structure, but leaves the "set counter(aValue)" and "set paused(aValue)" as methods that do nothing.

Then you can have a "Part 2" that removes all the unneeded code. If there are helper functions like DownloadsCommon.formatTimeLeft that become unused as a result, they should be removed too.
Attachment #8782857 - Flags: feedback?(paolo.mozmail) → feedback+
Comment on attachment 8785254 [details]
Bug 1270006 - Fill portion of the icon on download button as indicator to download progress (Part 2)

ok, I tried to splitted them into 2 parts. Basically,
Part1: Features, and removal of css
Part2: removal of javascript and test

Hope I covered all necessary things to be removed. Paolo may you take a look for it again?
Attachment #8785254 - Flags: review?(paolo.mozmail)
Attachment #8782857 - Flags: review?(paolo.mozmail)
Thanks for the update! I've tried part 1 locally and it works well, but I have a few design questions first.

This screenshots shows the indicator when a download just started. If the download is long, the icon remains in this state for a long time. Should we start by showing some more of the highlight to show that something is actually happening?

If we have a doorhanger notification anchored to the indicator, shown when the download starts, maybe this would be less of a concern. Do you think we should only land this patch after we have implemented the doorhanger notification, so the design is done as a whole?
This screenshot shows the indicator when a download is roughly halfway. I've only tested on OS X, and it might be different on other platforms, but the tonal contrast is really low so it's difficult to see the progress changing at all.

We should probably update the PNG icons to be lighter, or use a CSS brightness filter on the icons we have.

Another minor issue related to the progress state is that in the current patch the attention state is suppressed when a download is in progress. This doesn't seem right because if a long download is in progress or paused there is no permanent indication that another download may have completed. We should probably just remove the ":not([progress])" selector for the attention state.
Similarly to the just started state, the almost completed state is identical to the attention state. For long downloads that are near completion it may be difficult to know whether they have finished without checking the panel repeatedly, if I have looked away from the screen and missed the fading animation.
Comment on attachment 8785254 [details]
Bug 1270006 - Fill portion of the icon on download button as indicator to download progress (Part 2)

https://reviewboard.mozilla.org/r/74528/#review72386

I'll do the code review for part 1 in the next few days, in parallel, while we get the answers to the design questions. Part 2 looks fine but I'll review it last, after the changes for part 1 are final.
Attachment #8785254 - Flags: review?(paolo.mozmail)
(In reply to :Paolo Amadini from comment #30)

> This screenshots shows the indicator when a download just started. If the
> download is long, the icon remains in this state for a long time. Should we
> start by showing some more of the highlight to show that something is
> actually happening?
> 
> If we have a doorhanger notification anchored to the indicator, shown when
> the download starts, maybe this would be less of a concern. Do you think we
> should only land this patch after we have implemented the doorhanger
> notification, so the design is done as a whole?
Oh, thanks for bringing this up. 
There should be an "download start" animation but I thought it should be covered in bug 1270012 (although the title seems a little bit different now) so didn't implement it here. For the animation spec you can see:
https://mozilla.invisionapp.com/share/4Y6ZZH1E8#/screens/162255197

But as you said they would be better to land together to complete the design; So one possible way may be changing the title of 1270012 to let it take care of start and end animation; and we land 1270012 with this bug together after they're both reviewed.
Does it make sense?
(In reply to :Paolo Amadini from comment #31)
> Created attachment 8785278 [details]
> Indicator when a download is midway
> 
> This screenshot shows the indicator when a download is roughly halfway. I've
> only tested on OS X, and it might be different on other platforms, but the
> tonal contrast is really low so it's difficult to see the progress changing
> at all.
> 
I have no preference for this issue (since it looks ok for me personally);
But maybe Visual team does. Let's confirm with Carol.

(In reply to :Paolo Amadini from comment #32)
> Similarly to the just started state, the almost completed state is identical
> to the attention state. For long downloads that are near completion it may
> be difficult to know whether they have finished without checking the panel
> repeatedly, if I have looked away from the screen and missed the fading
> animation.
The easiest way that come to me is just leave more unfinished space for an almost-done files; But let's also ask Carol to see if there are any good ideas.

Hi Carol, would you mind take a look on the issue of comment 31 and 32?
If You need to see the animation in detail you can just come to my seat. Thanks!
Flags: needinfo?(chuang)
(In reply to KM Lee [:rexboy] from comment #34)
> There should be an "download start" animation but I thought it should be
> covered in bug 1270012 (although the title seems a little bit different now)
> so didn't implement it here. For the animation spec you can see:
> https://mozilla.invisionapp.com/share/4Y6ZZH1E8#/screens/162255197

We already have a download starting animation that looks like the one in the document you've pointed to. This is the same Gijs said in bug 1270012 comment 2, and I haven't seen any new design since then, so I'm confused about what that bug is about and if there is any work at all to do there.

> But as you said they would be better to land together to complete the
> design; So one possible way may be changing the title of 1270012 to let it
> take care of start and end animation; and we land 1270012 with this bug
> together after they're both reviewed.

The download starting animation is not what I was referring to, and it already exists, so bug 1270012 doesn't seem relevant to this discussion.

I'm asking specifically about the doorhanger notification, the one the document says to "pop out immediately, hide after 5 seconds". According to the user experience team, is this notification something we need to land together with this bug, or can we land this bug without having implemented the doorhanger notification first?

I've asked this in comment 30 because this bug makes it less noticeable that a download is in progress, at the beginning, if the download takes a long time. The indicator looks the same as when no download is running.
(In reply to KM Lee [:rexboy] from comment #35)
> (In reply to :Paolo Amadini from comment #31)
> > Created attachment 8785278 [details]
> > Indicator when a download is midway
> > 
> > This screenshot shows the indicator when a download is roughly halfway. I've
> > only tested on OS X, and it might be different on other platforms, but the
> > tonal contrast is really low so it's difficult to see the progress changing
> > at all.
> > 
> I have no preference for this issue (since it looks ok for me personally);
> But maybe Visual team does. Let's confirm with Carol.
> 
> (In reply to :Paolo Amadini from comment #32)
> > Similarly to the just started state, the almost completed state is identical
> > to the attention state. For long downloads that are near completion it may
> > be difficult to know whether they have finished without checking the panel
> > repeatedly, if I have looked away from the screen and missed the fading
> > animation.
> The easiest way that come to me is just leave more unfinished space for an
> almost-done files; But let's also ask Carol to see if there are any good
> ideas.
> 
> Hi Carol, would you mind take a look on the issue of comment 31 and 32?
> If You need to see the animation in detail you can just come to my seat.
> Thanks!

Hi Rex and Paolo,
Thank you for the feedback! I agree that the indicator of downloading state doesn't have enough contrast. I will update a new asset for the blue indicator.
Flags: needinfo?(chuang)
Quick update:(In reply to KM Lee [:rexboy] from comment #35)
> (In reply to :Paolo Amadini from comment #31)
> > Created attachment 8785278 [details]
> > Indicator when a download is midway
> > 
> > This screenshot shows the indicator when a download is roughly halfway. I've
> > only tested on OS X, and it might be different on other platforms, but the
> > tonal contrast is really low so it's difficult to see the progress changing
> > at all.
> > 
> I have no preference for this issue (since it looks ok for me personally);
> But maybe Visual team does. Let's confirm with Carol.
> 
A quick update about this issue:
Seems the assets below OSX 10.7 produces this less satisfied result. I can now reproduce it by modifying jar.mn so tracing on it.
(In reply to :Paolo Amadini from comment #36)
> 
> We already have a download starting animation that looks like the one in the
> document you've pointed to. This is the same Gijs said in bug 1270012
> comment 2, and I haven't seen any new design since then, so I'm confused
> about what that bug is about and if there is any work at all to do there.
The animation seems not identical on my Nightly so let me double-check it (and move further discussion to bug 1270012).
If there are nothing need to do I'll just close the bug. Otherwise working items for start/end animation should go to that bug;

> 
> I'm asking specifically about the doorhanger notification, the one the
> document says to "pop out immediately, hide after 5 seconds". According to
> the user experience team, is this notification something we need to land
> together with this bug, or can we land this bug without having implemented
> the doorhanger notification first?
Oh I got it. Thanks for the explanation.
UX team also think it would be better to land the doorhanger notification together with the progress icon. Should that work belongs to bug 1269954?
It's ok for me to implement that, but from discussion seems it takes time to implement that, is it? My only concern would be if we leave less time in version 52 for testing and need to land them very late as version 53.
Here's a comparison picture.
As you can see, for MacOS below 10.10@2x (the second one), the buttons itself appears lighter while the highlighted blue keeps almost the same brightness, causing the border less noticeable.

But there's another thing I care about. around all assets, the button for 10.10 and 10.10@2x appears quite different despite they are the in same OS version. It just looks like somebody wrongly exchanged the normal-state and click-state for one of them. I'm not sure which one is wrong, but if the 10.10@2x is the wrong one, we simply need to exchange the asset back to get plenty contrast.
If the 10.10 is the wrong one, we may need additional version-specific css or asset to overcome that.
Oops. Correct for the typo. I mean, in the comment above, the buttons for *below 10.10* and *below 10.10@2x* appears quite different.
Hi Bram:

The assets Toolbar.png and Toolbar@2x.png (for MacOS below 10.10 Yosemite) appears in different order for the first and second row (You may refer to the screenshot in Comment 40). May you confirm that if it's intended or by accident? If it's by accident, which order should be the correct one?
Thanks!
Flags: needinfo?(bram)
(In reply to KM Lee [:rexboy] from comment #39)
> UX team also think it would be better to land the doorhanger notification
> together with the progress icon. Should that work belongs to bug 1269954?

Yes.

> It's ok for me to implement that, but from discussion seems it takes time to
> implement that, is it? My only concern would be if we leave less time in
> version 52 for testing and need to land them very late as version 53.

Yes, development takes time. We can easily put the work in bug 1269954 behind a preference, but it's more difficult to do that for this bug. So, a simple option is to work on the other bug before this lands, and land this one when the other bug is completed and we enable the preference.
(In reply to KM Lee [:rexboy] from comment #42)
> Hi Bram:
> 
> The assets Toolbar.png and Toolbar@2x.png (for MacOS below 10.10 Yosemite)
> appears in different order for the first and second row (You may refer to
> the screenshot in Comment 40). May you confirm that if it's intended or by
> accident? If it's by accident, which order should be the correct one?
> Thanks!

I don’t work on this project, but it looks as if it’s by accident. For MacOS below Yosemite, the retina version – lighter shade of grey, inner shadow – is the correct one to use.

The pre-Yosemite toolbar icons should look something like this:
https://d17oy1vhnax1f7.cloudfront.net/items/3h2E3c3Z0H371E3F151d/Toolbar.2R3F3U3s3H2c.png
Flags: needinfo?(bram)
Thanks Bram. I'll exchange the assets back then.

After some discussion with Carol we would try to use the pushed-state icon as background when there's progress running. The patch would be updated accordingly.
Hi Stephen, 
Based on comment 44, is it correct that the "pre-Yosemite toolbar icons" are supposed to look lighter on the pressed state than normal state? Because the toolbar icons on other os versions(please see the attachment), their pressed states are darker than the normal state.
thanks!
Flags: needinfo?(shorlander)
Comment on attachment 8782857 [details]
Bug 1270006 - Fill portion of the icon on download button as indicator to download progress

https://reviewboard.mozilla.org/r/72890/#review74994

Here is a first pass review on this patch. If you want you can work on a new version now, or you can wait until the prerequisite bug is implemented first. The final review will be after the prerequisite lands anyways.

::: browser/themes/linux/downloads/indicator.css:75
(Diff revision 2)
>  #downloads-button[cui-areatype="toolbar"][attention="severe"] > .toolbarbutton-badge-stack > .toolbarbutton-badge:-moz-window-inactive,
>  #downloads-button[cui-areatype="toolbar"][attention="warning"] > .toolbarbutton-badge-stack > .toolbarbutton-badge:-moz-window-inactive {
>    filter: none;
>  }
>  
> -#downloads-button[cui-areatype="toolbar"][attention="success"] > #downloads-indicator-anchor > #downloads-indicator-icon {
> +#downloads-button[cui-areatype="toolbar"][attention="success"]:not([progress]) > #downloads-indicator-anchor > #downloads-indicator-icon {

We need to respect the attention state even when a download is in progress.

::: browser/themes/linux/downloads/indicator.css:134
(Diff revision 2)
>    animation-name: downloadsIndicatorNotificationFinish;
>    animation-duration: 1s;
>  }
>  
>  /*** Progress bar and text ***/
> -
> +#downloads-button[cui-areatype="toolbar"] #downloads-indicator-progress-area {

I'm not sure that we need to limit this rule only to #downloads-button[cui-areatype="toolbar"], especially because it has the "position" specification.

Have you tested what happens on Linux when there is a download in progress and the indicator is customized into the main menu?

::: browser/themes/linux/downloads/indicator.css:149
(Diff revision 2)
> +  height: 0;
>  }
>  
> -#downloads-button[paused] > #downloads-indicator-anchor > #downloads-indicator-progress-area > #downloads-indicator-progress > .progress-remainder {
> -  background-image: linear-gradient(#4b5000, #515700);
> +toolbar[brighttext] #downloads-button[cui-areatype="toolbar"] #downloads-indicator-progress-icon {
> +  background-image: -moz-image-rect(url("chrome://browser/skin/Toolbar-inverted.png"),
> +                              18, 198, 36, 180);

nit: Lines that wrap should be aligned at a sensible place, under "url" in this case.

::: browser/themes/osx/downloads/indicator.css:84
(Diff revision 2)
>    toolbar[brighttext] #downloads-indicator-icon {
>      background-image: -moz-image-rect(url("chrome://browser/skin/Toolbar-inverted@2x.png"), 0, 396, 36, 360);
>    }
>  
> -  #downloads-button:not([counter]) > #downloads-indicator-anchor > #downloads-indicator-progress-area > #downloads-indicator-counter {
> -    background-image: -moz-image-rect(url("chrome://browser/skin/Toolbar@2x.png"), 0, 396, 36, 360);
> +  #downloads-indicator-progress-icon {
> +    background-image: -moz-image-rect(url("chrome://browser/skin/Toolbar@2x.png"), 72, 396, 108, 360);

The HiDPI rule is not applied because it comes first in the file. It should be moved after the standard DPI rule.

::: browser/themes/osx/downloads/indicator.css:175
(Diff revision 2)
> +                              36, 198, 54, 180) bottom no-repeat;
> +  position: absolute;
> +  bottom: 0;
> +  width: 100%;
> +  height: 0;
> +  transition: height 0.5s;

This approach works well and is fine for this patch. However, I wonder if we can implement the same indication using multiple backgrounds on the same element, updating the CSS from code, instead of having multiple elements.

If we can do that, we can get rid of a lot of code that implements the custom XBL binding for the button, as well as the custom lazy loaded overlay. We would just apply multiple backgrounds to the "toolbarbutton-icon" of the standard button.

I'm saying this now just because in this simpler solution we would not be able to implement this "transition" property, but I don't think we really need it, because it applies to a few cases where a new download is started while another one is in progress.
Attachment #8782857 - Flags: review?(paolo.mozmail)
Comment on attachment 8782857 [details]
Bug 1270006 - Fill portion of the icon on download button as indicator to download progress

https://reviewboard.mozilla.org/r/72890/#review74994

> We need to respect the attention state even when a download is in progress.

This case is excluded because if there are multiple downloads and one of them has finished, the attention state would switch the toolbar icon to attention state; as a result the progress would not be visible even though there are other downloads in progress.
In the previous implementation this is not a problem since we got progressbar separately; but if we want to preserve that then we need to do some change to our design.

> I'm not sure that we need to limit this rule only to #downloads-button[cui-areatype="toolbar"], especially because it has the "position" specification.
> 
> Have you tested what happens on Linux when there is a download in progress and the indicator is customized into the main menu?

There should be no progress shown when it's moved to main menu just as previous design; but yes we should hide it somewhere explicitly. I'll double confirm about this.

> The HiDPI rule is not applied because it comes first in the file. It should be moved after the standard DPI rule.

My bad :-/ Thanks!

> This approach works well and is fine for this patch. However, I wonder if we can implement the same indication using multiple backgrounds on the same element, updating the CSS from code, instead of having multiple elements.
> 
> If we can do that, we can get rid of a lot of code that implements the custom XBL binding for the button, as well as the custom lazy loaded overlay. We would just apply multiple backgrounds to the "toolbarbutton-icon" of the standard button.
> 
> I'm saying this now just because in this simpler solution we would not be able to implement this "transition" property, but I don't think we really need it, because it applies to a few cases where a new download is started while another one is in progress.

Hmm, I don't care the transition so much either.
The reason I didn't take multiple background is that I'm not sure if that works well for those platform-specific rules and inside main menu. It's something can be tried anyway. But if I get trouble like need to hard-coded css inside javascript for different platforms I may suggest to keep the current way.
(In reply to KM Lee [:rexboy] from comment #48)
> This case is excluded because if there are multiple downloads and one of
> them has finished, the attention state would switch the toolbar icon to
> attention state; as a result the progress would not be visible even though
> there are other downloads in progress.

The progress would be visible again as soon as the panel is opened, though.

> In the previous implementation this is not a problem since we got
> progressbar separately; but if we want to preserve that then we need to do
> some change to our design.

Can you get feedback on this design issue?

> The reason I didn't take multiple background is that I'm not sure if that
> works well for those platform-specific rules and inside main menu. It's
> something can be tried anyway. But if I get trouble like need to hard-coded
> css inside javascript for different platforms I may suggest to keep the
> current way.

I can definitely accept a few lines of CSS set in JavaScript if this allows us to remove the XUL overlay loading code and custom XBL bindings we have now, so it's worth trying.
I'd prefer such a relatively big change to be prompted at earlier stage anyway.
(In reply to :Paolo Amadini from comment #49)
> (In reply to KM Lee [:rexboy] from comment #48)
> 
> > In the previous implementation this is not a problem since we got
> > progressbar separately; but if we want to preserve that then we need to do
> > some change to our design.
> 
> Can you get feedback on this design issue?
> 
Due to UX's comment, the animation for download complete is kept to notice user there's a completed download, and the progress should be shown if there are still downloads remaining.
How do you think?
(In reply to KM Lee [:rexboy] from comment #50)
> I'd prefer such a relatively big change to be prompted at earlier stage
> anyway.

Yeah, I'm fine if you want to try this now, but I'm also fine with doing this optimization and XBL removal in a follow-up bug if you prefer.

We should also understand what state of things helps more with the button jump animation you mentioned in bug 1270012. If we can do it by animating a "transform" property on the toolbar button icon, with the appropriate "z-order", that would be great, as we could then remove the custom XBL binding. I'm not sure if it is feasible.

(In reply to KM Lee [:rexboy] from comment #51)
> Due to UX's comment, the animation for download complete is kept to notice
> user there's a completed download, and the progress should be shown if there
> are still downloads remaining.
> How do you think?

Okay, thanks for asking the UX team. Sounds like we're mostly moving away from giving status information in the button, with a very simplified experience.
Clearing the needinfo for Stephen, it makes sense to set it again when the work on this bug resumes after the dependency of bug 1269954.
Depends on: 1269954
Flags: needinfo?(shorlander)
As a reference, this bug will change following UX items:
- Remove “time display” inside the arrow
- Remove line progress bar
- Use the Arrow to only show “overall download progress” (no notification state)
There's a noticeable change with the last version: I merged the similar CSS codes across different OSs into a shared file called indicator.inc.css in part 1.
The part 2 basically keeps the same.
Comment on attachment 8785254 [details]
Bug 1270006 - Fill portion of the icon on download button as indicator to download progress (Part 2)

As Bryant suggested, we can use Nightly for testing this new experience.

To me this means that we should keep a killswitch for a few releases, and only then remove the old code. You should file a separate bug for removing the old code, and you can attach Part 2 there as a starting point, but there is no need to request review for now.
Attachment #8785254 - Flags: review?(paolo.mozmail)
Comment on attachment 8782857 [details]
Bug 1270006 - Fill portion of the icon on download button as indicator to download progress

Bug 1269957 should be uplifted to Firefox 52 to finish the visual redesign of the Panel contents, and we should do that as soon as possible, please ensure you have the other one in review before continuing on this one.

Here, we need to keep the code for the old experience in place for a few releases, although not directly accessible to users. You can add a "DownloadsCommon.newStyleIndicator" property whose value is true, but can be overridden from user studies or add-ons if needed to bring back the old experience.

We explicitly don't want an about:config preference, because we'll remove the old code anyways once the new experience is confirmed, and keeping the old experience will not be an option that we have to maintain indefinitely.
Attachment #8782857 - Flags: review?(paolo.mozmail)
If we are going to keep both old and new codes for a while, the new feature then need to be bound on a new XUL element until the old one is removed (and of course the XUL elements for the old version are kept to switching between).
I may need some time to survey how to do it easier.
(In reply to KM Lee [:rexboy] from comment #60)
> If we are going to keep both old and new codes for a while, the new feature
> then need to be bound on a new XUL element until the old one is removed (and
> of course the XUL elements for the old version are kept to switching
> between).
> I may need some time to survey how to do it easier.

A possible alternative is to keep just one XUL binding and handle the visibility of elements in a single stack based on an attribute. On the other hand, separate XUL elements might make it easier to separate the two versions. I'm fine with whatever solution ends up being easier.
Attachment #8785254 - Attachment is obsolete: true
Comment on attachment 8782857 [details]
Bug 1270006 - Fill portion of the icon on download button as indicator to download progress

What I do here is to attach a class to DownloadButton based on the progressbar type we choose; then keep both old and new css rules. Seems the newly added rules are OK to be kept as is; but those rules that are modified then need to be co-existed with the class added to distinguish them.

This way we don't need to re-organize the XUL and just search for the class name when we need to remove one of them... But it looks ugly :(

Haven't test around different OS yet because I'm not sure if is a desirable way to achieve it. Paolo may you take a quick look and give me some feedback?
Attachment #8782857 - Flags: review?(paolo.mozmail) → feedback?(paolo.mozmail)
Update: Just tested the patch on windows/linux with both old and new style and seems they work well too.
Comment on attachment 8782857 [details]
Bug 1270006 - Fill portion of the icon on download button as indicator to download progress

Sorry for the delay on this request, I think the general approach is good.

You can look for opportunities for simplification, for example for some CSS rules it might be easier to change the way we set the attributes in JavaScript. Maybe don't set some attributes when newStyleIndicator is true, or change the attribute name based on newStyleIndicator, so we don't always need two slightly different versions of the same rules.

Is it possible to move "#downloads-indicator-progress-icon" out of "#downloads-indicator-progress-area", and have "position: relative;" on "#downloads-button" instead, or does it cause issues? This might help with simplifying the rules by making "#downloads-indicator-progress-area" only visible when using the progress bar.

nit: I think ".withProgressBar", ".withProgressArrow" would be clearer.
Attachment #8782857 - Flags: feedback?(paolo.mozmail) → feedback+
Still working.
Use javascript to remove attention="success" rather than keep 2-versions of css does work, I can delete most of the rules with 2 versions with just a few javascript changes.
Yet when the button is sent to the panel, we need to put the attention="success" back again if there are downloads in progress, otherwise neither the success attention nor progress indication presents. I think we have these ways:
1 If we don't care that very much, just ignore this issue (or solve it in another bug)
2 Use javascript to handle it too (which I'm not sure if there is an easy way to do it)
3 Go back to use css to handle it

I want to make a try on 2 but not quite sure if it's easy to do.
Thanks for the status update! If you're also asking for an opinion on the possible options, unfortunately I didn't understand the issue clearly, and I might have to look at the code. Feel free to ask for review on the solution that turns out to be easier, or ask if you have any specific question.
So here’s the patch. It’s much simpler than the previous version, although it fails in one case:
If I move the icon into panel, and having something downloading in progress while "success" attention presents, the “success” attention for prior completed downloads would not be displayed because it’s suppressed in Javascript.
This may be the easiest way to me though. If we think the case above is not so important then the patch should do things well. Otherwise we need to improve for that case.
Comment on attachment 8782857 [details]
Bug 1270006 - Fill portion of the icon on download button as indicator to download progress

https://reviewboard.mozilla.org/r/72890/#review109244

::: browser/components/downloads/content/indicator.js:229
(Diff revision 5)
>  
> +  _setIndicatorType() {
> +    // We keep a killerswitch for old-styled progressbar for now. Corresponding
> +    // css class is added here to reflect the type chosen for showing progress.
> +    let node = CustomizableUI.getWidget("downloads-button")
> +                               .forWindow(window).node;

nit: remove two spaces to align the dots

::: browser/components/downloads/content/indicator.js:440
(Diff revision 5)
> +    const PROGRESS_ICON_EMPTY_POSITION = 20;
> +    const ICON_MEASURE_FULL_POSITION = 87;

PROGRESS_ICON_EMPTY_HEIGHT_PERCENT
PROGRESS_ICON_FULL_HEIGHT_PERCENT

This makes the naming consistent, and clarifies the CSS unit we are using below.

::: browser/components/downloads/content/indicator.js:450
(Diff revision 5)
> +        if (DownloadsCommon.arrowStyledIndicator &&
> +            this._attention == DownloadsCommon.ATTENTION_SUCCESS) {
> +          this.attention = DownloadsCommon.ATTENTION_NONE;
> +        }

Yes, the setters shouldn't change the value of other properties, or ignore the value they have been provided with. This not only causes the issue you observed, but it makes it difficult to follow the code flow.

You can refactor the "attention" setter to call a _refreshAttention helper that is also called here. The helper can then set the attributes using both the _attention and _percentComplete properties.

::: browser/components/downloads/content/indicator.js:456
(Diff revision 5)
> -      else
> +        this._progressIcon.style.height = (this._percentComplete *
> +          (ICON_MEASURE_FULL_POSITION - PROGRESS_ICON_EMPTY_POSITION) / 100 + PROGRESS_ICON_EMPTY_POSITION) + "%";

nit: try to wrap this at 80 characters.

::: browser/themes/linux/downloads/indicator.css:222
(Diff revision 5)
> +#downloads-button[cui-areatype="toolbar"] #downloads-indicator-progress-area {
> +  position: relative;
> +}

Looks like this rule and similar ones are a leftover from previous versions?

::: browser/themes/linux/downloads/indicator.css:227
(Diff revision 5)
> +  background: -moz-image-rect(url("chrome://browser/skin/Toolbar.png"),
> +                              18, 198, 36, 180) bottom no-repeat;

I think it's worth refactoring this, moving the images to CSS variables for all platforms. This way, you can also share the actual rules in "indicator.inc.css".

::: browser/themes/shared/downloads/indicator.inc.css:5
(Diff revision 5)
> +@keyframes downloadsIndicatorStartJump {
> +  0% {
> +    transform: translateY(0);
> +    animation-timing-function: ease-out;
> +  }
> +  50% {
> +    transform: translateY(-3px);
> +    animation-timing-function: ease-in;
> +  }
> +  100% {
> +    transform: translateY(0);
> +  }
> +}

This could be an issue with MozReview, but we shouldn't mix different changesets, just rebase either one of them on top of the other. The first changeset will create the ".inc.css" file with only the rules that it needs.
Attachment #8782857 - Flags: review?(paolo.mozmail)
The previous version separates the commits because one is dedicated for removing unused features. Since we're not removing features now, maybe one commit is enough.
Seems the refactoring corrected the problem I met. Thanks!
Please take a look at this revision.
(In reply to KM Lee [:rexboy] (Away 1/27-2/1 for Lunar new year) from comment #72)
> The previous version separates the commits because one is dedicated for
> removing unused features. Since we're not removing features now, maybe one
> commit is enough.

Sorry for being unclear, I was referring to bug 1270012. If you want to land it after this bug, you should simply remove downloadsIndicatorStartJump and #downloads-button[notification="start"] from this patch. Then you should rebase bug 1270012 on top of this one.
Comment on attachment 8782857 [details]
Bug 1270006 - Fill portion of the icon on download button as indicator to download progress

https://reviewboard.mozilla.org/r/72890/#review112082

::: browser/components/downloads/content/indicator.js:436
(Diff revision 6)
> +     // For arrow type only:
> +     // We show portion of the success icon in propotional with the download progress
> +     // as an indicator. the PROGRESS_ICON_EMPTY_HEIGHT_PERCENT and PROGRESS_ICON_FULL_HEIGHT_PERCENT
> +     // correspond to how much portion of the icon should be displayed in 0% and 100%.

nit: spacing is incorrect, also wrap the comment at 80 characters.

::: browser/components/downloads/content/indicator.js:440
(Diff revision 6)
> +    const PROGRESS_ICON_EMPTY_HEIGHT_PERCENT = 20;
> +    const PROGRESS_ICON_FULL_HEIGHT_PERCENT = 87;

PROGRESS_ICON_EMPTY_HEIGHT_PERCENT should be something like 40, because we need to fill about one third of the visible icon when the download starts. We do this to show that a download is running, even if it takes a long time to make progress. You can explain this in the comment.

::: browser/themes/linux/downloads/indicator.css:221
(Diff revision 6)
> +
> +#downloads-button[cui-areatype="toolbar"] #downloads-indicator-progress-area {
> +  position: relative;
> +}

This seems unnecessary given that there is a global rule doing the same for "#downloads-indicator-progress-area".

::: browser/themes/osx/downloads/indicator.css:174
(Diff revision 6)
> +  background-image: -moz-image-rect(url("chrome://browser/skin/Toolbar.png"),
> +                              36, 198, 54, 180);

So, maybe I've been unclear in my previous review comment as well, but I've actually asked for the opposite change here...

The Linux and Windows themes use CSS variables such as "--downloads-indicator-icon". You can change the OS X theme to use the same technique as the Windows and Linux theme.

If you use the same name for variables in all the themes, you can use them from the shared file. This way, you don't have to worry about overriding rules in the theme-specific files.

While doing this, rename "--downloads-indicator-icon" and friends to "--downloads-indicator-image", to avoid confusion with the element of the same name.

::: browser/themes/osx/downloads/indicator.css:189
(Diff revision 6)
>    #downloads-notification-anchor[notification="start"] > #downloads-indicator-notification {
>      background-image: url("chrome://browser/skin/downloads/download-notification-start@2x.png");
>    }
> +
> +  #downloads-indicator-progress-icon {
> +    background-image: -moz-image-rect(url("chrome://browser/skin/Toolbar@2x.png"), 72, 396, 108, 360);

We previously determined that the contrast of the icon on OS X 10.9 is insufficient. We either need to ask someone from UX to update the toolbar icon in Toolbar.png and Toolbar@2x.png, or use an SVG filter to brighten it.

::: browser/themes/shared/downloads/indicator.inc.css:23
(Diff revision 6)
> +#downloads-indicator-progress-area {
> +  position: relative;
> +}
> +

This is probably a leftover as well, can be removed.

Can you check if there are any other unnecessary rules? I'll review this again after you've cross-checked.
Attachment #8782857 - Flags: review?(paolo.mozmail)
(In reply to :Paolo Amadini from comment #74)
> ::: browser/themes/osx/downloads/indicator.css:189
> (Diff revision 6)
> >    #downloads-notification-anchor[notification="start"] > #downloads-indicator-notification {
> >      background-image: url("chrome://browser/skin/downloads/download-notification-start@2x.png");
> >    }
> > +
> > +  #downloads-indicator-progress-icon {
> > +    background-image: -moz-image-rect(url("chrome://browser/skin/Toolbar@2x.png"), 72, 396, 108, 360);
> 
> We previously determined that the contrast of the icon on OS X 10.9 is
> insufficient. We either need to ask someone from UX to update the toolbar
> icon in Toolbar.png and Toolbar@2x.png, or use an SVG filter to brighten it.
> 
Let's do this on another bug.

> ::: browser/themes/shared/downloads/indicator.inc.css:23
> (Diff revision 6)
> > +#downloads-indicator-progress-area {
> > +  position: relative;
> > +}
> > +
> 
> This is probably a leftover as well, can be removed.
> 
> Can you check if there are any other unnecessary rules? I'll review this
> again after you've cross-checked.
This one is necessary for the #downloads-indicator-progress-icon, or it can't locate at the correct position.


Just revised. Please take a look, thanks!
(In reply to KM Lee [:rexboy] from comment #76)
> Let's do this on another bug.

Good idea, feel free to file a separate bug about the new toolbar PNG, blocking this one.

I'm also fine with an SVG filter workaround if the updated PNG is not ready and we want to land this.

> > ::: browser/themes/shared/downloads/indicator.inc.css:23
> > > +#downloads-indicator-progress-area {
> > > +  position: relative;
> > > +}
> > 
> This one is necessary for the #downloads-indicator-progress-icon, or it
> can't locate at the correct position.

The downloads-indicator-progress-icon is not a child of downloads-indicator-progress-area, so I don't see how that could be the case, but I'll test this as part of the review on this iteration.
Oh, I'm sorry, I replied on the wrong rule.

You're right and the #downloads-indicator-progress-area is removed. But we need this rule for #downloads-indicator-icon and it's kept in the patch.
No longer blocks: 1338984
Depends on: 1338984
No longer depends on: 1269954
Comment on attachment 8782857 [details]
Bug 1270006 - Fill portion of the icon on download button as indicator to download progress

https://reviewboard.mozilla.org/r/72890/#review113296

::: browser/components/downloads/DownloadsCommon.jsm:148
(Diff revision 7)
>  this.DownloadsCommon = {
>    ATTENTION_NONE: "",
>    ATTENTION_SUCCESS: "success",
>    ATTENTION_WARNING: "warning",
>    ATTENTION_SEVERE: "severe",
> +  arrowStyledIndicator: true,

Add this JSDoc: This can be used by add-on experiments as a killswitch for the new style progress indication. This will be removed in bug 1329109 after the new indicator is released.

nit: remember to add an empty line before the JSDoc

::: browser/components/downloads/content/indicator.js:225
(Diff revision 7)
> +  _setIndicatorType() {
> +    // We keep a killerswitch for old-styled progressbar for now. Corresponding
> +    // css class is added here to reflect the type chosen for showing progress.
> +    let node = CustomizableUI.getWidget("downloads-button")
> +                             .forWindow(window).node;
> +
> +    if (DownloadsCommon.arrowStyledIndicator) {
> +      node.classList.add('withProgressArrow');
> +    } else {
> +      node.classList.add('withProgressBar');
> +    }
> +  },

I just tested what happens if we change the indicator type from code.

Unfortunately, ensureInitialized is called early during window initialization, and not after the delayed initialization of the downloads that happens after 10 seconds. This means that changing the variable alone after the window is shown doesn't have any effect.

We should ensure that the following sequence from the Browser Console changes the indicator type:

DownloadsCommon.arrowStyledIndicator = false;
DownloadsIndicatorView._setIndicatorType();
DownloadsIndicatorView._refreshAttention();

The only missing piece is that you need to ensure that _setIndicatorType also resets the classes if their value changes. You can use the "classList.toggle" method with the boolean argument.

::: browser/components/downloads/content/indicator.js:231
(Diff revision 7)
> +    if (DownloadsCommon.arrowStyledIndicator) {
> +      node.classList.add('withProgressArrow');
> +    } else {
> +      node.classList.add('withProgressBar');
> +    }

nit: double quotes for strings (this will also be an ESLint rule at some point)

::: browser/components/downloads/content/indicator.js:443
(Diff revision 7)
>      if (!this._operational) {
>        return this._percentComplete;
>      }

nit: one extra empty line after the constants

::: browser/components/downloads/content/indicator.js:451
(Diff revision 7)
>  
>      if (this._percentComplete !== aValue) {
>        this._percentComplete = aValue;
> -      if (this._percentComplete >= 0)
> +      if (this._percentComplete >= 0) {
> +
> +        this._refreshAttention(this._attention);

It's surely confusing, and likely buggy, that you're passing the attention value to _refreshAttention and special-casing there before re-assigning the argument to the member variable.

The _refreshAttention method should take no arguments and make no changes to the member variables. You should call it unconditionally when _percentComplete changes or when _attention changes.

Here, you need to move the call outside of the inner "if" block, otherwise the method wouldn't be called if the percentage returns to be undefined.

::: browser/components/downloads/content/indicator.js:460
(Diff revision 7)
> +          (PROGRESS_ICON_FULL_HEIGHT_PERCENT -
> +           PROGRESS_ICON_EMPTY_HEIGHT_PERCENT) / 100 +
> +           PROGRESS_ICON_EMPTY_HEIGHT_PERCENT) + "%";
> +      } else {
>          this.indicator.removeAttribute("progress");
> +        this._progressIcon.style.height = "";

nit: I suppose this should be height = "0" to match what is in the CSS by default.

::: browser/components/downloads/content/indicator.js:465
(Diff revision 7)
> +        this._progressIcon.style.height = "";
> +      }
>        // We have to set the attribute instead of using the property because the
>        // XBL binding isn't applied if the element is invisible for any reason.
>        this._indicatorProgress.setAttribute("value", Math.max(aValue, 0));
> +      this._progressIcon.setAttribute("value", Math.max(aValue, 0));

I don't think we're using the "value" attribute on the progress icon, is that correct?

::: browser/components/downloads/content/indicator.js:501
(Diff revision 7)
>    set attention(aValue) {
>      if (!this._operational) {
>        return this._attention;
>      }
>  
> -    if (this._attention != aValue) {
> +    this._refreshAttention(aValue);

So, if the value of this._attention changed, you should assign it here and call _refreshAttention.

::: browser/components/downloads/content/indicator.js:507
(Diff revision 7)
> +    if (aValue != this._attention ||
> +        // There's a chance that success attention need to be suppressed by
> +        // download progress.
> +        aValue == DownloadsCommon.ATTENTION_SUCCESS) {
>        this._attention = aValue;

Meaning that this code wouldn't be here.

::: browser/components/downloads/content/indicator.js:518
(Diff revision 7)
> +      // Suppress success attention if we have progress in toolbar
> +      if (DownloadsCommon.arrowStyledIndicator && !inMenu &&
> +          aValue == DownloadsCommon.ATTENTION_SUCCESS &&
> +          this._percentComplete >= 0) {
> +        aValue = DownloadsCommon.ATTENTION_NONE;
> +      }
> +
>        if (aValue == DownloadsCommon.ATTENTION_NONE) {

Here you'll have something like "let suppressAttention = DownloadsCommon.arrowStyledIndicator && ...", then "if (suppressAttention || this._attention == DownloadsCommon.ATTENTION_NONE)".

::: browser/themes/shared/downloads/indicator.inc.css:5
(Diff revision 7)
> +@keyframes downloadsIndicatorStartJump {
> +  0% {
> +    transform: translateY(0);
> +    animation-timing-function: ease-out;
> +  }
> +  50% {
> +    transform: translateY(-3px);
> +    animation-timing-function: ease-in;
> +  }
> +  100% {
> +    transform: translateY(0);
> +  }
> +}
> +

Remove the downloadsIndicatorStartJump rule.

::: browser/themes/shared/downloads/indicator.inc.css:37
(Diff revision 7)
> +#downloads-button[notification="start"] > #downloads-indicator-anchor {
> +  animation-name: downloadsIndicatorStartJump;
> +  /* Upon changing the overall duration below, please keep the delay time of
> +     setTimeout() identical in indicator.js for this animation. */
> +  animation-duration: 0.5s;
> +  animation-delay: 1s;
> +  animation-iteration-count: 2;
> +}

And this related rule as well.
Attachment #8782857 - Flags: review?(paolo.mozmail)
Comment on attachment 8782857 [details]
Bug 1270006 - Fill portion of the icon on download button as indicator to download progress

https://reviewboard.mozilla.org/r/72890/#review113296

> I just tested what happens if we change the indicator type from code.
> 
> Unfortunately, ensureInitialized is called early during window initialization, and not after the delayed initialization of the downloads that happens after 10 seconds. This means that changing the variable alone after the window is shown doesn't have any effect.
> 
> We should ensure that the following sequence from the Browser Console changes the indicator type:
> 
> DownloadsCommon.arrowStyledIndicator = false;
> DownloadsIndicatorView._setIndicatorType();
> DownloadsIndicatorView._refreshAttention();
> 
> The only missing piece is that you need to ensure that _setIndicatorType also resets the classes if their value changes. You can use the "classList.toggle" method with the boolean argument.

I considered only the case from startup but this do make testing and maintaining easier. Thanks for pointing that out.

> It's surely confusing, and likely buggy, that you're passing the attention value to _refreshAttention and special-casing there before re-assigning the argument to the member variable.
> 
> The _refreshAttention method should take no arguments and make no changes to the member variables. You should call it unconditionally when _percentComplete changes or when _attention changes.
> 
> Here, you need to move the call outside of the inner "if" block, otherwise the method wouldn't be called if the percentage returns to be undefined.

I think I got what you mean but not pretty sure. Please take a look on next revision.

> I don't think we're using the "value" attribute on the progress icon, is that correct?

I just thought we need to keep a same attribute there but yes it's not used by anyone. I'll remove it.

> And this related rule as well.

My bad for didn't notice that :-/
Revised and hope it's going to be ok.
I tried on switching the progressbar style by console and it works without problem on 3 platforms.

Paolo may you take a look on it?
Comment on attachment 8782857 [details]
Bug 1270006 - Fill portion of the icon on download button as indicator to download progress

https://reviewboard.mozilla.org/r/72890/#review114500

Looks good, there is only a small improvement I just noticed.

We should resolve the contrast issue in bug 1338984 before this can get the final r+.

::: browser/base/content/browser.css:966
(Diff revision 9)
> -#downloads-button:-moz-any([progress], [counter], [paused]) #downloads-indicator-icon,
> -#downloads-button:not(:-moz-any([progress], [counter], [paused]))
> -                                                   #downloads-indicator-progress-area
> -{
> +#downloads-button.withProgressBar:-moz-any([progress], [counter], [paused]) #downloads-indicator-icon,
> +#downloads-button.withProgressBar:not(:-moz-any([progress], [counter], [paused])) #downloads-indicator-progress-area,
> +#downloads-button.withProgressArrow:not([progress]) #downloads-indicator-progress-area {
> +  visibility: hidden;
> +}
> +
> +/* Hide elements for another type of progressmeter if it's not in use. */
> +#downloads-button.withProgressBar #downloads-indicator-progress-icon,
> +#downloads-button.withProgressArrow #downloads-indicator-progress,
> +#downloads-button.withProgressArrow #downloads-indicator-counter {
>    visibility: hidden;
>  }

Sorry if this comes late in the review cycle, I should have seen this earlier.

Some rules here can be rewritten to be simpler. Also, we can use ":not(.withProgressBar)", so we don't need to set the "withProgressArrow" attribute:

#downloads-button.withProgressBar:-moz-any([progress], [counter], [paused]) #downloads-indicator-icon,
#downloads-button:not(:-moz-any([progress], [counter], [paused])) #downloads-indicator-progress-area {
  visibility: hidden;
}

/* Hide elements for another type of progressmeter if it's not in use. */
#downloads-button.withProgressBar #downloads-indicator-progress-icon,
#downloads-button:not(.withProgressBar) #downloads-indicator-progress-area {
  visibility: hidden;
}

::: browser/components/downloads/content/indicator.js:231
(Diff revision 9)
> +    node.classList.toggle(
> +                     "withProgressArrow", DownloadsCommon.arrowStyledIndicator);
> +    node.classList.toggle(
> +                      "withProgressBar", !DownloadsCommon.arrowStyledIndicator);

For the remaining class, use the parameter alignment indentation style:

    node.classList.toggle("withProgressBar",
                          !DownloadsCommon.arrowStyledIndicator);

::: browser/components/downloads/content/indicator.js:498
(Diff revision 9)
>     */
>    set attention(aValue) {
>      if (!this._operational) {
>        return this._attention;
>      }
> -
> +    if (aValue != this._attention) {

nit: don't swap

::: browser/components/downloads/content/indicator.js:517
(Diff revision 9)
> +    if (suppressAttention || this._attention == DownloadsCommon.ATTENTION_NONE)
> +    {

nit: brace on the same line, even if it exceeds 80 characters (this is an ESLint rule, but this file is still in the ignore list at present). You can optionally wrap after "||" if you want.

::: browser/themes/shared/downloads/indicator.inc.css:4
(Diff revision 9)
> +/* 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/. */
> +#downloads-indicator-icon {

nit: empty line before first rule
Attachment #8782857 - Flags: review?(paolo.mozmail)
Changes are addressed. Sorry for the late reply.

Although the patch just addressed the suggestions from last pass (and tested under differnt OSes), I changed some implementation of animating progressbar in bug 1338984 based on our discussion there. I didn't address them here together to decrease the complexity of reviewing and writing and you may want to take a look there too.
Comment on attachment 8782857 [details]
Bug 1270006 - Fill portion of the icon on download button as indicator to download progress

https://reviewboard.mozilla.org/r/72890/#review116702

Yes, splitting the patches definitely makes reviewing easier, thanks!
Attachment #8782857 - Flags: review?(paolo.mozmail) → review+
Thank you Paolo!

Let's check-in this one first so that bug 1338984 can land after it.
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/74f078c39e3c
Fill portion of the icon on download button as indicator to download progress r=Paolo
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/74f078c39e3c
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox54: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 54
Blocks: 1322819
Depends on: 1347257
I can confirm this issue is fixed, I verified Fx 54.0a2, build ID:20170323004002 on Windows 10 x64, Mac OS X 10.11.6 and Ubuntu 14.04.

Cheers!
Status: RESOLVED → VERIFIED
status-firefox54: fixed → verified
No longer depends on: 1359062
Depends on: 1354278
While I've read this entire thread, I don't understand who benefits from the removal of estimated download time from the download button? 

- "Progress bar provides similar sense to the remaining time" The progress arrow doesn't offer useful information for long download times. What does a half-filled arrow mean if it's just sitting there and seemingly not progressing for awhile? Is my download stalled or downloading from a slow server? Is it a huge file that just takes awhile before it can hit the next progress animation? I can no longer tell without having to open the download window itself. At least in the previous version I could look at the download button and see an actual amount of time left. 
- I understand that the time indicator was deemed "too small to see"(completely disagree), but I find this blue progress color much harder to see than the time + green progress color. There's not enough contrast between the blue and the black of the arrow to see the blue until it's about a quarter of the way through. Before then, I don't have an easy visual indication that I have anything downloading.
- I keep seeing mention of a notification being used to inform the user roughly how long a download is going to take, but I'm definitely not seeing any notification or any other indication of download time outside of opening the download window.
- "With the arrow missing from the indicator, it's hard to find the it in a glance" I don't understand this point. Aside from user customization, the download button never moves. Are people forgetting where it is? 

I'm using Firefox 54(compact light theme), Windows 10 creators update if that matters.
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: