1270012 - Visual for download animation on download start whenever a download is started

Status: VERIFIED FIXED

(Firefox :: Downloads Panel, defect)

Description

[yifan [:yifan][:yliao]]

Add an animation to notify a user whenever a download is completed while a user is downloading multiple files.

Comment 1

[yifan [:yifan][:yliao]]

Attachment: Download Panel and Content Handler.pdf

Comment 2

[:Gijs (out for now; he/him)]

There is already an animation, and the design pdf does not show a design for a different animation. What is the goal of this bug?

Comment 3

[Wesly Huang (EPM)]

(In reply to :Gijs Kruitbosch from comment #2)
> There is already an animation, and the design pdf does not show a design for
> a different animation. What is the goal of this bug?

There will be a redefined animation per plan and Visual team is working on it. We should put that Visual spec (different from current UX one) here then, or at least clearly indicate where it is, for reference.

Thanks for point it out, Gijs! And let me know if there's any question.

Comment 4

[Wesly Huang (EPM)]

Comment on attachment 8748517 [details]
Download Panel and Content Handler.pdf

should refer to its parent bug 1269956[1] for the latest UX spec. 

https://bugzilla.mozilla.org/show_bug.cgi?id=1269956

Comment 5

[Carol Huang [:Carol]]

Attachment: download_motion.gif

the attachment is the download animation mock up.

Comment 6

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

Just figured out the required work remaining should be a "jumping" animation to the download button when a download starts. There's no changed needed for the complete animation. Change the title accordingly.

Comment 7

[Ben Tian (inactive)]

As a reference, this bug will change only 1 UX item - start downloading to trigger jumping twice.

Comment 8

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

Attachment: Bug 1270012 - Show a bouncing animation for toolbar download icon whenever a download is started.

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

Comment 9

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

This is a simple patch that adds a bouncing animation to the arrow in toolbar when download starts.

(It should be land after bug 1270006. Now it bounces both the progressbar and the arrow. This behavior won't be the case anymore after bug 1270006 lands, which leaves just the arrow.)

Comment 10

[:Paolo Amadini]

Is this meant to be in addition to the fading/shrinking animation we already have, or is this a replacement?

Comment 11

[:Paolo Amadini]

Comment on attachment 8820202 [details]
Bug 1270012 - Show a bouncing animation for toolbar download icon whenever a download is started.

Looks good to me! Just waiting for design feedback on the starting animation.

Comment 12

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

Carol confirmed the animation for the patch (on Mac and Windows) so I think that should be OK for them. But you can double confirm if you have any concern.

Comment 13

[:Paolo Amadini]

Comment on attachment 8820202 [details]
Bug 1270012 - Show a bouncing animation for toolbar download icon whenever a download is started.

https://reviewboard.mozilla.org/r/99734/#review102190

The code looks good, just a small change needed. Thanks for creating the "indicator.inc.css" file!

Please show the animation to Carol and confirm the exact timing, it seems slow to me but if this is the intended effect then it's all good.

::: browser/components/downloads/content/indicator.js:366
(Diff revision 1)
>      notifier.setAttribute("notification", aType);
>      this._notificationTimeout = setTimeout(() => {
>        notifier.removeAttribute("notification");
>        notifier.style.transform = '';
>      }, 1000);
> +
> +    anchor.setAttribute("notification", aType);
> +    this._anchorNotificationTimeout = setTimeout(() => {
> +      anchor.removeAttribute("notification");
> +    }, 3000);

In practice the timeout determines the minimum interval between animations, so if a new event occurs within the timeout, an animation for it may not be shown. It's required because our CSS animations are triggered by the attribute changes.

I think you can simplify the code by keeping only one timeout and making it longer to include both animations.

By reading the CSS it seems to me that the longest animation is 2 seconds from when the attribute is set, so 2000 could be a good value to use here, or higher if you don't want two consecutive animations to occur.

It may be a good idea to add a comment here saying this value is based on the length of the animation in the CSS, and a comment in the CSS that reminds to update this value if the animation length changes.

Comment 14

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

Comment on attachment 8820202 [details]
Bug 1270012 - Show a bouncing animation for toolbar download icon whenever a download is started.

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

Comment 15

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

Double confirmed the animation with visual. It seems the speed is good.

If we land this bug alone it ends up animating the progressbar, so we may need to land it together with bug 1270006 if we think that matters.

Comment 16

[:Paolo Amadini]

Comment on attachment 8820202 [details]
Bug 1270012 - Show a bouncing animation for toolbar download icon whenever a download is started.

https://reviewboard.mozilla.org/r/99734/#review103804

::: browser/components/downloads/content/indicator.js:287
(Diff revision 2)
> +  _anchorNotificationTimeout: null,
> +

Remove this...

::: browser/components/downloads/content/indicator.js:345
(Diff revision 2)
> +    if (this._anchorNotificationTimeout) {
> +      clearTimeout(this._anchorNotificationTimeout);
> +    }
> +

...and this. After doing that check that you don't leave any unnecessary whitespace changes.

Looks good for the rest!

Comment 17

[:Paolo Amadini]

(In reply to KM Lee [:rexboy] from comment #15)
> If we land this bug alone it ends up animating the progressbar, so we may
> need to land it together with bug 1270006 if we think that matters.

I'm fine either way, feel free to land when you think is best. Thanks!

Comment 18

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

Just discussed with Carol.
If we land this alone, it turns out that the old-styled progressbar is animated to jump twice, which doesn't look ok per visual's point of view. So let's land it when 1270006 is ready.

Comment 19

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

Comment on attachment 8820202 [details]
Bug 1270012 - Show a bouncing animation for toolbar download icon whenever a download is started.

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

Comment 20

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

Since bug 1270006 has been pushed, Let's checkin this bug too.

Comment 21

[:Paolo Amadini]

You'll likely need to rebase this patch, since bug 1270006 added "browser/themes/shared/downloads/indicator.inc.css" already.

Comment 22

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/eefb0717c7cb
Show a bouncing animation for toolbar download icon whenever a download is started. r=paolo

Comment 23

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

Oh I'm sorry for the late reply. I was absent on Friday.
Thanks for pushing it for me!

I'll take care if there are any actions needed.

Comment 24

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/eefb0717c7cb

Comment 25

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

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!