Closed Bug 1270012 Opened 8 years ago Closed 7 years ago

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

Categories

(Firefox :: Downloads Panel, defect)

defect
Not set
normal

Tracking

()

VERIFIED FIXED
Firefox 54
Tracking Status
firefox54 --- verified

People

(Reporter: yifan, Assigned: rexboy)

References

Details

(Whiteboard: [CHE-MVP])

Attachments

(2 files, 1 obsolete file)

Add an animation to notify a user whenever a download is completed while a user is downloading multiple files.
Attached file Download Panel and Content Handler.pdf (obsolete) —
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?
Flags: needinfo?(yliao)
Component: General → Downloads Panel
(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.
Flags: needinfo?(yliao)
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
Attachment #8748517 - Attachment is obsolete: true
Assignee: nobody → rexboy
Whiteboard: [CHE-MVP]
Depends on: 1296554
No longer depends on: 1296554
Attached image download_motion.gif
the attachment is the download animation mock up.
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.
Summary: Show download complete animation on downloads button whenever a download is completed → Visual for download animation on download start whenever a download is started
As a reference, this bug will change only 1 UX item - start downloading to trigger jumping twice.
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.)
Is this meant to be in addition to the fading/shrinking animation we already have, or is this a replacement?
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.
Attachment #8820202 - Flags: review?(paolo.mozmail) → feedback+
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 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.
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 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!
Attachment #8820202 - Flags: review?(paolo.mozmail) → review+
(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!
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.
QA Whiteboard: checkin-needed
QA Whiteboard: checkin-needed
Keywords: checkin-needed
Since bug 1270006 has been pushed, Let's checkin this bug too.
Keywords: checkin-needed
You'll likely need to rebase this patch, since bug 1270006 added "browser/themes/shared/downloads/indicator.inc.css" already.
Keywords: checkin-needed
Pushed by paolo.mozmail@amadzone.org:
https://hg.mozilla.org/integration/mozilla-inbound/rev/eefb0717c7cb
Show a bouncing animation for toolbar download icon whenever a download is started. r=paolo
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.
https://hg.mozilla.org/mozilla-central/rev/eefb0717c7cb
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 54
Depends on: 1349096
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
You need to log in before you can comment on or make changes to this bug.