Sam Foster [:sfoster] (he/him) Reporter
	Description • 7 years ago

UX mockup shows a fatter/deeper bar while a download is in progress, with a transition to and from the default state and size.

	Sam Foster [:sfoster] (he/him) Reporter
	Comment 1 • 7 years ago

:amylee, when I preview the '9 progress bar' SVG from bug 1353434,  the background/grey bar appears to be oscillating in width. It looks like that background shape is missing from some frames. Can you double check? Also, if I recall correctly, shouldn't the first few frames ease-in the progressbar somehow? I don't see that here either.

	Amy Lee [:amylee] UX
	Comment 2 • 7 years ago

(In reply to Sam Foster [:sfoster] from comment #1)
> :amylee, when I preview the '9 progress bar' SVG from bug 1353434,  the
> background/grey bar appears to be oscillating in width. It looks like that
> background shape is missing from some frames. Can you double check? Also, if
> I recall correctly, shouldn't the first few frames ease-in the progressbar
> somehow? I don't see that here either.

Hi Sam, 

The opening animation with the progress bar is in "1 New Download". I had originally cut up the animation so the start animation is separate from the progress bar which I assume is dependent on the speed of the download. The idea was that the start animation would happen and then switch into the progress bar (9 progress bar). If you have a specific request for how to cut up the animation let me know. I'm not really clear on your comment about the progress bar oscillating in width with missing frames. I ran it through the svg converter tool and it looked okay on my end. Maybe there's something I'm not seeing? 

I'll post up the two files I'm talking about.

	Amy Lee [:amylee] UX
	Comment 3 • 7 years ago

Opening animation with the progress bar animating into place and enlarging

	Amy Lee [:amylee] UX
	Comment 5 • 7 years ago

Isolated the progress bar in the start animation.

	u474838
	Comment 6 • 7 years ago

Sam Foster asked me to critique the mockups over here so that's what
I'll try.

Unless I'm missing something and there are other mockups, I've loaded
the three vector images and one video.

- I like the finishing notification ala NeXT dock.

- In the video the horizontal progress bar/line works because
  it's quickly going from 1 to 100%. This isn't the case we
  need a good progress indicator, so it's not representative.
  Sorry.

  * If you're actually downloading a large file, it won't work
    as well as the 55 vertical blue juice filling glass.

- What is the objection to the old thick arrow that indicated
  progress vertically from bottom to top? It was very easy to
  see the percentage of downloads finished.

	u474838
	Comment 7 • 7 years ago

- The SVGs still regress compared to 55 by making the distinctive
  progress be a minuscule detail that's hard to catch.

	Sam Foster [:sfoster] (he/him) Reporter
	Comment 9 • 7 years ago

Paolo: attachment 8901640 [details] is just the icon/graphic changes for the slightly fatter progressbar spec'd by UX. It does not include changes to the animations to transition to the downloading state. As the clock is ticking and this change is a simple one, I'd like to land this as-is before coming back to those transitions.

	Jared Wein [:jaws] (please needinfo? me)
	Comment 12 • 7 years ago
mozreview-review

Comment on attachment 8901640 [details]
Bug 1387557 - Use a fatter progressbar when download is in progress.

https://reviewboard.mozilla.org/r/173054/#review178786

Is there supposed to be a transition from the thin progress bar to the fat progress bar? When I try it out I just see it immediately jump to the fatter size. I'm holding back r+ due to this, otherwise the rest of it is fine excluding the minor issue below.

::: browser/themes/shared/downloads/indicator.inc.css:37
(Diff revision 2)
> +#downloads-button[progress] > #downloads-indicator-anchor > #downloads-indicator-icon {
> +  background-image: url("chrome://browser/skin/downloads/download-icons.svg#inprogress-arrow");

jwatt has asked us not to use the `use:not(:target)` approach for SVG since it causes us to parse and build up the full SVG document even though we're not using the full one, and each extra use of it causes this issue since the document that gets built doesn't get shared between instances.

Can you please break this out to a separate file? And then file a bug to do the same for the arrow, progress-bar-bg, and progress-bar-fg?

	Jared Wein [:jaws] (please needinfo? me)
	Comment 13 • 7 years ago

Seeing comment #9 now. If UX is OK with this landing now (and potentially us not getting the transition between sizes) then I will grant r+. But I'd like to get their opinion if this can stand on its own.

@amylee, here's a link to a test build on osx, https://queue.taskcluster.net/v1/task/Ee-JUWs9T-OsPAh-pYka1Q/runs/0/artifacts/public/build/target.dmg

	Sam Foster [:sfoster] (he/him) Reporter
	Comment 15 • 7 years ago

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #12)
> jwatt has asked us not to use the `use:not(:target)` approach for SVG since
> it causes us to parse and build up the full SVG document even though we're
> not using the full one, and each extra use of it causes this issue since the
> document that gets built doesn't get shared between instances.
> 
> Can you please break this out to a separate file? And then file a bug to do
> the same for the arrow, progress-bar-bg, and progress-bar-fg?

New patch pushed, interdiff: https://reviewboard.mozilla.org/r/173054/diff/2-3/

Bug 1394609 filed for the other parts of download-icons.svg

	Sam Foster [:sfoster] (he/him) Reporter
	Comment 16 • 7 years ago

Filed bug 1394842 as a follow-up to address the transitions.

	:Paolo Amadini
	Comment 17 • 7 years ago

At the All Hands we discussed how we could transition to a fatter bar by reserving some space without changing the shape of the arrow. This solved the problem of matching the film-strip start animation to the actual shape of the arrow regardless of whether there were already some downloads in progress or not.

If this is the case, why do we need a different arrow shape here? (I haven't had occasion to try the patch locally yet.)

	:Paolo Amadini
	Comment 18 • 7 years ago

(I thought the complexity of this bug resided in the transition rather than just using a different shape for the bar. The transition is now filed as bug 1394842, and it's unclear if that adds additional complexity due to the need of transitioning the arrow shape as well.)

	Sam Foster [:sfoster] (he/him) Reporter
	Comment 19 • 7 years ago

(In reply to :Paolo Amadini from comment #17)
> At the All Hands we discussed how we could transition to a fatter bar by
> reserving some space without changing the shape of the arrow. This solved
> the problem of matching the film-strip start animation to the actual shape
> of the arrow regardless of whether there were already some downloads in
> progress or not.
> 
> If this is the case, why do we need a different arrow shape here? (I haven't
> had occasion to try the patch locally yet.)

The proposed icon changes which would have allowed us to do that - to keep a single arrow shape and just transition the progressbar - were rejected by UX. So this patch provides both the fatter progressbar and the smaller arrow needed to accommodate it. You are right that most of the complexity implied in the mockups is in the transitions. Aligning the state changes with the correct animation frames isn't easy. The mockups have the transition to show the download button, the notification and progress starting all in lock-step, which is not how this works today. I've started working on that, but its definitely a more complex patch and with time running out for 57 nightly it puts the feature at risk.

	:Paolo Amadini
	Comment 20 • 7 years ago

So, I guess this means that when there is a download already in progress and a new one is started, the current start animation will land slightly misaligned. I agree this is a minor issue though, and we shouldn't worry too much about it.

	Amy Lee [:amylee] UX
	Comment 21 • 7 years ago

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #13)
> Seeing comment #9 now. If UX is OK with this landing now (and potentially us
> not getting the transition between sizes) then I will grant r+. But I'd like
> to get their opinion if this can stand on its own.
> 
> @amylee, here's a link to a test build on osx,
> https://queue.taskcluster.net/v1/task/Ee-JUWs9T-OsPAh-pYka1Q/runs/0/
> artifacts/public/build/target.dmg

Hi, 

Without the transition between fat/thin progress bar it feels abrupt and not smooth like all our other animations have been and it feels like a step back. I would rather not land the fatter progress bar without the transition in place.

	Jared Wein [:jaws] (please needinfo? me)
	Comment 22 • 7 years ago
mozreview-review

Comment on attachment 8901640 [details]
Bug 1387557 - Use a fatter progressbar when download is in progress.

https://reviewboard.mozilla.org/r/173054/#review179578

Thank you for splitting out the SVG to a separate file. Based on Amy's feedback I'm marking this as r-. Please implement the transition between sizes in this bug. You can close the separate bug that was filed, as these two parts should land simultaneously.

	Sam Foster [:sfoster] (he/him) Reporter
	Comment 24 • 7 years ago

Comment on attachment 8906242 [details]
Bug 1387557 - Transition to fatter progressbar.

f? jaws as paolo is out. Please forward as necessary. 

There's a couple outstanding issues/questions with this, but its close to something I would like to land next week, so any feedback is appreciated at this stage.  

Apparently DownloadsIndicatorView.percentComplete is sometimes called before showEventNotification, so _refreshAttention has already cleared the attention=success attribute - before _showNotification can check it. Same for progress (which I'm currently papering over by assigning downloadInProgress only when this._percentComplete > 0.)

The mockup for these animations shows specific timing for when the button's arrow and progressbar should visually transition between states. I can do this by hiding the button's indicator element while the notifier animation runs, and having a replacement arrow and progressbar in the frames of the animation which we can fill/stroke as appropriate. That's where download-faux-indicator.svg comes in - a "animation" with each frame containing the same static elements. This is one of a couple backgrounds that are being translateX'd together over the animation duration . There is no doubt a smarter solution to this, but it got me unstuck for now by allowing me to use the same markup & CSS patterns we've used elsewhere for these animations. 

I need to populate stroke/fill style properties when the start notification animation runs, depending on the progress or attention state of the button. But I don't want to muck about with the order these events come in at this stage. So I guess I need to keep track of recent attention and progress values so _showNotification can look back and see what they were in the moments before it was called?

	Jared Wein [:jaws] (please needinfo? me)
	Comment 25 • 7 years ago

Comment on attachment 8906242 [details]
Bug 1387557 - Transition to fatter progressbar.

Redirecting to Mike since I'll be away for the next two days and Mike has a pretty good understanding of animations.

	Mike de Boer [:mikedeboer]
	Comment 26 • 7 years ago
mozreview-review

Comment on attachment 8906242 [details]
Bug 1387557 - Transition to fatter progressbar.

https://reviewboard.mozilla.org/r/177990/#review183312

- I don't see the Download icon turn to black with fat bar when all downloads are finished; instead it stays blue.
- The start animation in the video show that the arrow flies in on top of the toolbar icon that simply remains; right now it poofs the toolbar icon is flies it back into place, scaling from large to small. I think that's preferable, because you don't lose anything out of sight.


If you're missing something to get any of the above done, please let me know - we can look for the right place in indicator.js together.

::: browser/components/downloads/content/indicator.js:389
(Diff revision 1)
>        let widthDiff = anchorRect.width - notifierRect.width;
>        let translateX = (leftDiff + .5 * widthDiff) + "px";
>        let translateY = (topDiff + .5 * heightDiff) + "px";
>        notifier.style.transform = "translate(" + translateX + ", " + translateY + ")";
>        notifier.setAttribute("notification", aType);
> +      if (anchor.hasAttribute("easein")) {

When is this attribute set? When I search through files locally, I never see this 'easein' attribute set on the anchor.

::: browser/components/downloads/content/indicator.js:396
(Diff revision 1)
> +        notifier.setAttribute("easein", "true");
> +      }
> +      if (hasAttention) {
> +        console.log("_showNotification, has attention");
> +        notifier.setAttribute("attention", "true");
> +      }

`} else { notifier.removeAttribute("attention"); }`
In other words: please find a place to clean this attribute up. You can perhaps fold it into `_refreshAttention()`?

::: browser/components/downloads/content/indicator.js:397
(Diff revision 1)
> +      }
> +      if (hasAttention) {
> +        console.log("_showNotification, has attention");
> +        notifier.setAttribute("attention", "true");
> +      }
> +      // is a previous download already in-progress?

nit: s/is/Is/

	Sam Foster [:sfoster] (he/him) Reporter
	Comment 27 • 7 years ago

(In reply to Mike de Boer [:mikedeboer] from comment #26)
> - I don't see the Download icon turn to black with fat bar when all
> downloads are finished; instead it stays blue.

Ah good catch, will look into that. 

> - The start animation in the video show that the arrow flies in on top of
> the toolbar icon that simply remains; right now it poofs the toolbar icon is
> flies it back into place, scaling from large to small. I think that's
> preferable, because you don't lose anything out of sight.

I don't understand if you think the patch is correct/better as-is, or it needs to be fixed to better match the mockup? 

> When is this attribute set? When I search through files locally, I never see
> this 'easein' attribute set on the anchor.

I forgot to call this out in the commit message. There's a transition that isn't possible to fully implement until bug 1397447 lands. I've prototyped this using an old version of that patch and the idea was to set an "easein" attribute or similar when the download button is placed/unhidden the first time. If my patch goes in first, I'll remove the [easein] lines and file a follow-up to add them when possible. 

Thanks for looking at this. Would you be able to review in Paolo's absence?

	Sam Foster [:sfoster] (he/him) Reporter
	Comment 28 • 7 years ago

This is a single download, nothing already downloaded or downloading. 
If I slow down this video, I see a frame or so of white before the notification arrow starts animating in which I would like to improve on. Otherwise I think its a reasonable match for the mockup?

Will try and capture video for the other scenarios too...

	Sam Foster [:sfoster] (he/him) Reporter
	Comment 29 • 7 years ago

(In reply to Sam Foster [:sfoster] from comment #27)
> (In reply to Mike de Boer [:mikedeboer] from comment #26)
> > - I don't see the Download icon turn to black with fat bar when all
> > downloads are finished; instead it stays blue.

Ok, this is by design and is the existing behavior: when a download completes, the button is in an "attention" state (attention="success") until the user interacts with the button - in this or any window.

> > - The start animation in the video show that the arrow flies in on top of
> > the toolbar icon that simply remains; right now it poofs the toolbar icon is
> > flies it back into place, scaling from large to small. I think that's
> > preferable, because you don't lose anything out of sight.

I've attached a video, but I think you are talking about the case where a download is already in progress. The UX decision in that case was to just drop in the notification arrow, but not "dip" or otherwise transition the indicator arrow and progressbar which are already in their "downloading" state.

	Sam Foster [:sfoster] (he/him) Reporter
	Comment 33 • 7 years ago

Mike, I've updated the patch to fix some of the logic and details of how to treat already-downloading vs. download-just-started differently. This logic is particular to the DownloadsIndicatorView. I looked at DownloadsCommon.jsm and possibilities for ensuring this view's showEventNotification was called before its percentComplete setter was called when a new download starts, but I'm not sure it would necessarily improve things. Plus this way is lower risk obviously. 

I also removed the "easein" stuff you saw in the earlier patch. Something like that will need to be re-added when bug 1397447 lands, but it makes more sense to file a follow-up blocked on that rather than try and anticipate it here. 

So, as this bug and the details are probably new to you, here's a summary of what is supposed to happen. There are a full set of mockups attached to bug 1353434. 

* When a download starts and the download button/icon is visible, and there are no downloads currently in progress, a "notifier" arrow drops down onto the indicator(button). It appears to nudge it, the color (attention state if there as one) is reset and the icon and progressbar change to their "downloading" state - which is a slightly smaller arrow, making space for a slightly taller progressbar. 

* If a download is already in progress, the notifier arrow still drops down, but there's no further animation. I used the same treatment for when the download button is in the overflow menu

With that, r? incoming..

	Sam Foster [:sfoster] (he/him) Reporter
	Comment 35 • 7 years ago

Results will be displayed on Treeherder as they come in:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=9786750cf2fedeb8fdeb7614117d2f5f94acee39

Once completed, builds and logs will be available at:
https://archive.mozilla.org/pub/firefox/try-builds/sfoster@mozilla.com-9786750cf2fedeb8fdeb7614117d2f5f94acee39/

	Amy Lee [:amylee] UX
	Comment 37 • 7 years ago

animated svg between thick progress bar to thin

	Amy Lee [:amylee] UX
	Comment 38 • 7 years ago

(In reply to (Away until Sept 13th) Jared Wein [:jaws] (please needinfo? me) from comment #13)
> Seeing comment #9 now. If UX is OK with this landing now (and potentially us
> not getting the transition between sizes) then I will grant r+. But I'd like
> to get their opinion if this can stand on its own.
> 
> @amylee, here's a link to a test build on osx,
> https://queue.taskcluster.net/v1/task/Ee-JUWs9T-OsPAh-pYka1Q/runs/0/
> artifacts/public/build/target.dmg

Here's my feedback:

1. When the download is completed and turns blue, the progress bar also changes back to the original size. In my mock the progress bar only changes back to the default (thin) size after the user has clicked on it and it goes back to grey. I've attached an svg of the transition between the thick/thin progress bar.

2. There are a few strange things happening in the initial arrow drop animation on a first download. 
A. A brief portion of the blue of the progress bar quickly appears and goes away before the download starts. 
B. The arrow flickers blue for a moment during the arrow drop animation. 
C. The progress bar goes from thin to thick and then thin and thick again before the download begins.

I've spoken to Sam already about these items so he's aware.

	Mike de Boer [:mikedeboer]
	Comment 39 • 7 years ago

Comment on attachment 8906242 [details]
Bug 1387557 - Transition to fatter progressbar.

As per conversation on IRC - Sam is working on a new patch that I'll be able to review this afternoon/ evening. Stay tuned!

	Sam Foster [:sfoster] (he/him) Reporter
	Comment 42 • 7 years ago

Cancelling review while I work on this. I've pushed a new revision to attachment 8906242 [details]. I've rebased now that bug 1397447 is landed and am working on getting that initial download notification animation right.

	Mike de Boer [:mikedeboer]
	Comment 43 • 7 years ago
mozreview-review

Comment on attachment 8906242 [details]
Bug 1387557 - Transition to fatter progressbar.

https://reviewboard.mozilla.org/r/177990/#review185046

::: browser/components/downloads/content/indicator.js:439
(Diff revision 7)
> -      let heightDiff = anchorRect.height - notifierRect.height;
> +        let heightDiff = anchorRect.height - notifierRect.height;
> -      let widthDiff = anchorRect.width - notifierRect.width;
> +        let widthDiff = anchorRect.width - notifierRect.width;
> -      let translateX = (leftDiff + .5 * widthDiff) + "px";
> +        let translateX = (leftDiff + .5 * widthDiff) + "px";
> -      let translateY = (topDiff + .5 * heightDiff) + "px";
> +        let translateY = (topDiff + .5 * heightDiff) + "px";
> +
> +        // did the button just get un-hidden?

Please clarify in the comment when you're forwarding the attribute values to the notifier, to be crystal.

::: browser/components/downloads/content/indicator.js:480
(Diff revision 7)
> +  _showNotificationEnd(anchor, notifier) {
> +    if (notifier) {
> +      console.log("_showNotification, cleaning up notifier");
> -        notifier.setAttribute("hidden", "true");
> +      notifier.setAttribute("hidden", "true");
> -        notifier.removeAttribute("notification");
> +      notifier.removeAttribute("notification");
> +      notifier.removeAttribute("overflowed");

...and here it'd be helpful to document that you're clearing attributes that you forwarded earlier...

::: browser/themes/shared/downloads/indicator.inc.css:293
(Diff revision 7)
> +  stroke: #494949;
> +  /* Preload the animation images */
> +  background-image: url('chrome://browser/skin/downloads/notification-start-animation.svg'), /* the arrow-drop animation */
> +                    url('chrome://browser/skin/downloads/progressbar-start-animation.svg'),  /* the ease-in of the progressbar on first use */
> +                    url('chrome://browser/skin/downloads/faux-indicator-animation.svg'); /* a static indicator animation to replace a real one during the animation */
> +  background-repeat: no-repeat, no-repeat, no-repeat;

Doesn't `background-repeat: no-repeat;` work too, or does it fall back to the default value for the other images? Could be _no_, for sure, just curious.

	Mike de Boer [:mikedeboer]
	Comment 44 • 7 years ago

I couldn't apply the patch, because the build fails with 'File "../shared/downloads/progressbar-start-animation.svg" not found'. I think you forgot to `hg add` it!

	Sam Foster [:sfoster] (he/him) Reporter
	Comment 46 • 7 years ago

(In reply to Mike de Boer [:mikedeboer] from comment #44)
> I couldn't apply the patch, because the build fails with 'File
> "../shared/downloads/progressbar-start-animation.svg" not found'. I think
> you forgot to `hg add` it!

Ooops, must have gotten lost in the rebase. attachment 8906242 [details] updated.

	Mike de Boer [:mikedeboer]
	Comment 47 • 7 years ago

Alright, the latest patch is looking quite nice & subtle.

	Mike de Boer [:mikedeboer]
	Comment 48 • 7 years ago

Repeating from Slack: If splitting things up is going to take you an inordinate amount of time (i.e. not worth it) please leave that exercise for another bug. I mean, I’m guessing this one is rather high prio, so let’s add things to the todo pragmatically.

	Sam Foster [:sfoster] (he/him) Reporter
	Comment 50 • 7 years ago
mozreview-review-reply

Comment on attachment 8906242 [details]
Bug 1387557 - Transition to fatter progressbar.

https://reviewboard.mozilla.org/r/177990/#review183312

> When is this attribute set? When I search through files locally, I never see this 'easein' attribute set on the anchor.

Is now spelled "animateunhiding" and is set in DownloadsButton.unhide

	Sam Foster [:sfoster] (he/him) Reporter
	Comment 51 • 7 years ago
mozreview-review-reply

Comment on attachment 8906242 [details]
Bug 1387557 - Transition to fatter progressbar.

https://reviewboard.mozilla.org/r/177990/#review185046

> Doesn't `background-repeat: no-repeat;` work too, or does it fall back to the default value for the other images? Could be _no_, for sure, just curious.

Yeah seems like the single value will work for each of the background-images.

	Sam Foster [:sfoster] (he/him) Reporter
	Comment 53 • 7 years ago

Apologies for the large patch. A good piece of it is new and changed SVG files, honest :)

In indicator.js, I ended up refactoring _showNotification a bit, to pull out the setTimeout/RAF callback into its own method, and more explicitly branching the "start" and "finish" notifications. That was partly necessitated by adding a requestAnimationFrame after un-hiding the notifier, but as :paolo notes in bug 1386456, there is probably more to say and do here. I want to troubleshoot/optimize that further in that bug, but removing the sync flush seems to have helped with smoothness at least. 

re: comment 38, I think all these items are addressed now. 

The goal here is to get this into 57. It should be reasonable candidate for landing in central between now and merge day. Its not a scramble though and what we have today is stable if not optimal, so I don't want to land it half-baked.

	Mike de Boer [:mikedeboer]
	Comment 56 • 7 years ago
mozreview-review

Comment on attachment 8906242 [details]
Bug 1387557 - Transition to fatter progressbar.

https://reviewboard.mozilla.org/r/177990/#review185512

A few comments here until I finish reviewing...

::: browser/components/downloads/content/indicator.js:431
(Diff revision 12)
>      // the notification isn't clipped by overflow properties of the anchor's
>      // container.
>      // Note: no notifier animation for download finished in Photon
>      let notifier = this.notifier;
> +    // This value is determined by the overall duration of animation in CSS.
> +    let animationDuration = aType == "start" ? 760 : 850;

I know this is basically code that was here before, simply moved up, but what was the reason this couldn't be done with an 'animationend' handler?

::: browser/components/downloads/content/indicator.js:460
(Diff revision 12)
> +        // Did the button just get un-hidden?
> +        if (anchor.hasAttribute("animateunhiding")) {
> +          notifier.setAttribute("animateunhiding", "true");
> +        }
> +
> +        // Forward selected attributes onto the notification element which will

nit: please move this comment up to just before the if-statement above.

::: browser/components/downloads/content/indicator.js:472
(Diff revision 12)
> +        // Is a previous download already in-progress?
> +        if (anchor.hasAttribute("progress") &&
> +            !anchor.hasAttribute("startprogress")) {
> +          notifier.setAttribute("progress", "true");
> +        } else if (this._attention !== DownloadsCommon.ATTENTION_NONE) {
> +          // The indicator was in an attention state when download started

nit: missing dot.

::: browser/components/downloads/content/indicator.js:493
(Diff revision 12)
> +    }
> +  },
> +
> +  _showNotificationEnd(anchor, notifier) {
> +    if (notifier) {
> +      // Clear the attributes forwarded from the indicator earlier

nit: missing dot.

::: browser/components/downloads/content/indicator.js:560
(Diff revision 12)
>  
>        if (this._percentComplete >= 0) {
>          this.indicator.setAttribute("progress", "true");
> +        // Setter may be called before showEventNotification
> +        // so mark the element when a new download begins.
> +        // Attribute will be removed when the notification ends

nit: missing dot.

	Sam Foster [:sfoster] (he/him) Reporter
	Comment 57 • 7 years ago

(In reply to Mike de Boer [:mikedeboer] from comment #56)
> > +    let animationDuration = aType == "start" ? 760 : 850;
> 
> I know this is basically code that was here before, simply moved up, but
> what was the reason this couldn't be done with an 'animationend' handler?

I don't know the history of this. I've thought the same thing but elected not to mess with it for now. As it stands we could probably use animationend, but we just have to be careful because we may be waiting for animation to end on 2 different elements. I think it would be best to experiment in a follow-up bug.

	Mike de Boer [:mikedeboer]
	Comment 58 • 7 years ago

(In reply to Sam Foster [:sfoster] from comment #57)
> I don't know the history of this. I've thought the same thing but elected
> not to mess with it for now. As it stands we could probably use
> animationend, but we just have to be careful because we may be waiting for
> animation to end on 2 different elements. I think it would be best to
> experiment in a follow-up bug.

Indeed, please file it!

	Sam Foster [:sfoster] (he/him) Reporter
	Comment 60 • 7 years ago
mozreview-review-reply

Comment on attachment 8906242 [details]
Bug 1387557 - Transition to fatter progressbar.

https://reviewboard.mozilla.org/r/177990/#review185512

> I know this is basically code that was here before, simply moved up, but what was the reason this couldn't be done with an 'animationend' handler?

I filed bug 1400960 for this.

	Mike de Boer [:mikedeboer]
	Comment 62 • 7 years ago
mozreview-review

Comment on attachment 8906242 [details]
Bug 1387557 - Transition to fatter progressbar.

https://reviewboard.mozilla.org/r/177990/#review186156

r=me code wise. Impressive stuff, that will delight many users, I'm sure!

	Sam Foster [:sfoster] (he/him) Reporter
	Comment 63 • 7 years ago

Comment on attachment 8906242 [details]
Bug 1387557 - Transition to fatter progressbar.

OSX build at https://queue.taskcluster.net/v1/task/cCZqrKqfSSqwGDurN9dahQ/runs/0/artifacts/public/build/target.dmg let me know if you need other platforms. This is all shared theme code however.

Let me know with any issues you see if they should block landing this patch (i.e. until an issue is fixed, the experience is better using the existing behavior without the patch) or if we can file a follow-up and try to fix and uplift during the beta.

	Amy Lee [:amylee] UX
	Comment 64 • 7 years ago

Comment on attachment 8906242 [details]
Bug 1387557 - Transition to fatter progressbar.

Hi Sam, 

Here is my feedback. If these can't get fixed up in time then I would rather keep the existing download.

1. Fresh install, animation of arrow drop doesn’t appear.

2. After quitting browser and download icon disappears, when you open the browser and start a new download the arrow goes a bit lower than the icon so you see a jump.

3. Existing download icon, when starts a new download there is a flicker in the transition

4. In progress download, starting new download, arrow drop doesn’t fall exactly into place. Looks like a it’s slightly off.

5. When you cancel a download, the transition is lost between download arrow (fat progress bar) to default (thin progress bar)

	Sam Foster [:sfoster] (he/him) Reporter
	Comment 65 • 7 years ago

Capturing my thoughts on this before putting it on the back-burner: 

> 1. Fresh install, animation of arrow drop doesn’t appear.

It was suggested this might be a race with first time render cache population of the SVG? This whole DOM structure is lazily-loaded in indicatorOverlay.xul, so on first use nothing is ready. While we could preload those images on the placeholder or some other element, we are going to be fighting the intention of lazily-loading this stuff. So there's a strategic balance to be made here, with pre-loading/pre-rendering UI elements that might not be used, vs. waiting for first-use and sacrificing some frames or some detail in this interaction.  

> 2. After quitting browser and download icon disappears, when you open the
> browser and start a new download the arrow goes a bit lower than the icon so
> you see a jump.

If you look at the computed height/width with this patch, the #downloads-button is 28x28. Without the patch we see 28x38. So, something about this patch is changing the sequence or outcome of layout. There's a comment in indicatorOverlay.xul which I think is a good starting point: 

  "The panel's anchor area is smaller than the outer button, but must
  always be visible and must not move or resize when the indicator
  state changes, otherwise the panel could change its position or lose
  its arrow unexpectedly."

> 3. Existing download icon, when starts a new download there is a flicker in
> the transition

This doesnt reproduce much for me, but :amylee was seeing it everytime apparently on her macbook. Interestingly, if you trigger this animation from the console by executing `DownloadsIndicatorView.showEventNotification("start")`, you never(?) see this flicker so we might be getting some unexpected state changing when a real download is driving this. 

> 4. In progress download, starting new download, arrow drop doesn’t fall
> exactly into place. Looks like a it’s slightly off.

I suspect this is a reflow/timing issue, similar to issue 2. above.  

> 5. When you cancel a download, the transition is lost between download arrow
> (fat progress bar) to default (thin progress bar)

The transition is triggered by changing the attention property, and setting an animateindicator attribute on the indicator element when it goes from an attention state to no attention. Possibly cancelling a download takes a path that bypasses this.

Issue 5 here seems like a straightforward fix. The others need some more digging and suggest the current patch is oversimplifying or otherwise missing a trick. Its possible the addition of the #downloads-indicator-animatable-box is wrong-headed, and the states and animations can make use of the existing #downloads-indicator-icon .

	Mike de Boer [:mikedeboer]
	Comment 66 • 7 years ago

Sam, I'm a bit sad that you're not going to be able to work on this anymore. Who would be able to pick this up again? I think the main issue is the initial download (which is, of course, quite crucial/ important) flickering?
I'd be willing to spend a few cycles to investigate this some more, if you like? I think the flicker/ delay between states can be tweaked in follow-ups, so we have a bit more time for those.

	Sam Foster [:sfoster] (he/him) Reporter
	Comment 67 • 7 years ago

(In reply to Mike de Boer [:mikedeboer] from comment #66)
> Sam, I'm a bit sad that you're not going to be able to work on this anymore.
> Who would be able to pick this up again? I think the main issue is the
> initial download (which is, of course, quite crucial/ important) flickering?
> I'd be willing to spend a few cycles to investigate this some more, if you
> like? I think the flicker/ delay between states can be tweaked in
> follow-ups, so we have a bit more time for those.

It is sad, but I'm told we have higher priorities for 57 at this point. So I may be able to pick it back up, but need to focus as a team on burning down some of the other more pressing issues in the short-term.