When a download is started, the download button should expand in to view on the navigation toolbar (previously 0-width), and the "down" arrow should fill up from bottom to top. When the download is complete, there should be an accompanying animation of the "library" button that will show another "book" being added to library. See accompanying MP4 for demonstration.

Assorted initial questions: 1. Is there a plan for how we'll make this available for 57 (and not until then?) A pref? 2. I'm looking at bug 1329109 which seems like its safe to land in preparation for this work - and that bit could just ride the trains? 3. I think the current plan is to use SVG for the "filling" animation. Do we want to use that svg to replace the multiple dppi pngs with a single png. What about the reversed image(s)? Is the ambition to convert all of Toolbar.png and friends to SVG?

(In reply to Sam Foster [:sfoster] from comment #1) Sorry for not replying sooner. > Assorted initial questions: > 1. Is there a plan for how we'll make this available for 57 (and not until > then?) A pref? Yes, we'll need a pref and a build-time define in our CSS. I see you've started this work in bug 1358215. > 2. I'm looking at bug 1329109 which seems like its safe to land in > preparation for this work - and that bit could just ride the trains? Yeah, it looks like this can ride the trains. > 3. I think the current plan is to use SVG for the "filling" animation. Do we > want to use that svg to replace the multiple dppi pngs with a single png. > What about the reversed image(s)? Is the ambition to convert all of > Toolbar.png and friends to SVG? Bug 1347543 is converting the toolbar to use SVG. You should check with Nihanth about these extra download images to make sure that they are being converted to SVG as well.

Updated Download animation. No longer dependent on library being beside download icon - factoring in customization of toolbar.

Just a snapshot of work-in-progress. This version just replaces the zoomy-arrow animation with the Photon dropping-arrow animation when you start a download. I'm seeing this message: "Animation cannot be run on the compositor because the frame size (2520, 128) is too large relative to the viewport (larger than (1440, 507)) or larger than the maximum allowed value (2048, 2048) [hbox with id 'downloads-indicator-notification']" - this is a known issue after talking w. Marcus, and I'll probably re-implement this particular animation using CSS instead of the svg filmstrip technique.

Markus, you had wanted to see this patch (attachment 8866590 [details] [diff] [review])

I'm requesting review on attachment 8870336 [details]. I'll list a couple of known issues but I think its pretty close: * The download arrow seems larger and thinner than the icon in the mockup (attachment 8863487 [details]) so I may need to update that. * A large/slow download shows no visible progress bar until it hits 1%. And this that looks like a dot until it gets nearer to 4 or 5%. The animation is a linear 0-100% of the total width, with 0% being an empty frame. That could be tweaked. Which reminds me I should attach the script I used to generate this spritesheet. * With a slow connection (I throttled using Charles) it can take a while (5+ seconds) for the prompt to open/save to show up, and only then do we see any visible state change indicating the click on the link to download was successful. I don't think that has changed in this patch, but it would be nice to get notification earlier if possible. * I'm currently building to test the MOZ_PHOTON_ANIMATIONS= case, to verify nothing has regressed. * The progress animation is 100 frames of 16x16. The action is all in the lower 4 vertical px of each frame though, so this could likely be optimized to reduce the overall height of the spritesheet (at the expense of an extra XUL element I think) Clearing ni for :mstange, that patch is now obsolete.

node.js script to generate the 0-100% progress bar spritesheet.

Patch updated - I found and fixed a non-MOZ_PHOTON_ANIMATIONS bug that had broken the indicatorArrowProgress animation (setting bottom="0" rendered that element with 0 height with the non-photon styles)

Comment on attachment 8870336 [details] Bug 1352065 - Implement Photon download animations behind a MOZ_PHOTON_ANIMATIONS flag. This code can be made much more maintainable by not using a spritesheet. There's no particular reason to pre-render the frames, and this button isn't performance sensitive. The two alternative approaches are to use a progressbar element, just like the previous version of the indicator, or to use an inline SVG whose DOM is edited by JavaScript based on the current progress. Is there a particular reason to use a build time setting? I think most other Photon work is switched by a preference, and this is much easier to work with. If there are parts of the patch that can be separated to their own changesets, that would make reviewing them faster.

(In reply to :Paolo Amadini from comment #13) > Is there a particular reason to use a build time setting? I think most other > Photon work is switched by a preference, and this is much easier to work > with. There's some discussion about this on bug 1358215. It boils down to a change in CSS specificity in our selectors if we use the pref to add a class or attribute at runtime for these rules. > If there are parts of the patch that can be separated to their own > changesets, that would make reviewing them faster. Yeah I think there are some bits I can break out into their own changeset. I appreciate it did end up being rather a large patch. The new arrow and progress bar could land ahead of the start/finish animations potentially - they would just need to use the new assets temporarily. And the AppConstants change could be its own bug. Would that work for you Paolo?

Yes, I think that's a good way to separate the changes. You can then land them separately or at the same time.

Comment on attachment 8870336 [details] Bug 1352065 - Implement Photon download animations behind a MOZ_PHOTON_ANIMATIONS flag. https://reviewboard.mozilla.org/r/141790/#review146014 This is what it looks like with the patch applied for me: https://www.screencast.com/t/NaH65o0mukX A couple frames get dropped due to the screen recording but the second half of the start animation is missing, and the finish animation is on top of the star icon (though maybe that is expected since this will move to the library icon once that is completed). But we would have to hide the library icon as part of this animation so I guess this type of logic would be necessary anyways. I'm not sure why only the first couple frames of the start animation are showing for me. ::: browser/base/content/browser.xul:505 (Diff revision 2) > #include ../../components/controlcenter/content/panel.inc.xul > > +#ifdef MOZ_PHOTON_ANIMATIONS > + <vbox id="downloads-animation-container" mousethrough="always"> > + <deck> > + <hbox top="0"> Is `top` useful outside of <stack>?

I've got the broken-out patches underway. In the meantime, I'd like to address the concerns with the approach from comment #13. Its true we probably don't _need_ to prerender the progressbar "animation", but I think there's also value to adopting a consistent approach to animation. So for me the question is, is there a particular reason *not* to use the SVG spritesheet here? I'm pretty new to this codebase so I'll defer to your experience and insight into where the maintenance pain points are. Are there other concerns - and if there are, I wonder if those are unique to downloads and this animation in particular, or if they apply to use of the spritesheet technique elsewhere? I think we could process this quickly on a Vidyo call, are you both available tomorrow (early west-coast time)?

(In reply to Sam Foster [:sfoster] from comment #17) > is there a particular reason *not* to use the SVG spritesheet here? The manual build step with the script is an unnecessary burden, same goes for the large amount of SVG code that would show up in searches. > I wonder if those are unique to downloads and this animation in particular This is specific to this case. In other cases the performance benefit may justify the downsides, but there is no performance need here as download progress is throttled to at maximum a few times per second anyways. The fact you've already implemented the spritesheet approach does not make it a better choice either. Sure, if there were dozens of spritesheets in the code base already and this was a somewhat common approach, then one more wouldn't make a difference, but I understand this isn't the case right now or in the immediate future. > I think we could process this quickly on a Vidyo call, are you both > available tomorrow (early west-coast time)? Feel free to ping me later today if you want.

(In reply to :Paolo Amadini from comment #18) > (In reply to Sam Foster [:sfoster] from comment #17) > > is there a particular reason *not* to use the SVG spritesheet here? > > The manual build step with the script is an unnecessary burden, same goes > for the large amount of SVG code that would show up in searches. > > > I wonder if those are unique to downloads and this animation in particular SVG code showing up in searches doesn't sound like something we should be concerned with. > This is specific to this case. In other cases the performance benefit may > justify the downsides, but there is no performance need here as download > progress is throttled to at maximum a few times per second anyways. The fact > you've already implemented the spritesheet approach does not make it a > better choice either. > > Sure, if there were dozens of spritesheets in the code base already and this > was a somewhat common approach, then one more wouldn't make a difference, > but I understand this isn't the case right now or in the immediate future. This approach will be used for other animations that we are in the process of implementing now for Photon (see bug 1355925 for one that already has a patch in progress). We also plan to implement similar animations for bookmarking, adding items to pocket, and items moving to the overflow menu. The approach that Sam has taken here will also allow us much greater freedom with how the progress bar and other animations look without having to change much business logic compared to using a <progressbar> element. > > I think we could process this quickly on a Vidyo call, are you both > > available tomorrow (early west-coast time)? > > Feel free to ping me later today if you want. I am more free today than tomorrow, but I don't think a Vidyo call should be necessary. The consistency win of having the animation approach shared among various other front-end implementations as well as ability to separate appearance from the DOM are strong enough wins on their own to continue pushing forward here.

I'm working with UX to have them produce the progressbar animation using the same toolchain and process as we are using for the others - so the node.js script would go away and this asset would be owned by UX. I think we can dial in the right duration with linear easing to output the 0-100% frames we need.

(In reply to Jared Wein [:jaws] (behind on reviews and bugmail) (please needinfo? me) from comment #19) > The approach that Sam has taken here will also allow us much greater freedom > with how the progress bar and other animations look without having to change > much business logic compared to using a <progressbar> element. Note that an alternative approach I mentioned is to edit the path in the SVG without using a separate progressbar element, basically doing what the generation script does, but in-tree. Using the spritesheet still requires a manual build step to re-generate the SVG file in case something needs to change, and we'd have to do this for every asset if we need more than one variation. Having UX people do this step instead of engineering doesn't take away its cost. This was already an issue with the old PNG assets, and was lessened with the conversion to SVG. However, as I said, if the plan is to use this same system for more animations then we're already paying the process cost, so I agree it makes sense to continue in this direction.

Comment on attachment 8870337 [details] make-progressbar-spritesheet.js The make-progressbar-spritesheet.js scroipt is no longer in use. I've been using SVGO to process the animations that come out of AfterEffects+BodyMovin. We turn the animation into a spritesheet (filmstrip) via https://sfoster.github.io/svgspritesheet/ and run the result through SVGO. I've made some additional plugins to tidy some of the particular quirks we encounter, those are in the "photon-plugins" branch of my fork: https://github.com/sfoster/svgo/tree/photon-plugins. A sample config is included there (photon.config)

:paolo, I broke out the patch into 2 parts. Attachment 8873251 [details] has the first part which only addresses the photon-style download icon, and the progressbar "animation". I have a 2nd patch I'm finishing up to add the photon download notification animations (download-start and download-finish.) I'm leaning towards landing that independently in its own bug (dependant on this one)

This is part 2, it adds the Photon notification download animations. I moved a couple rules that were duplicated across each platform's theme into the shared indicator.inc.css. The download-start animation is pretty straightforward - it replaces the scale/zoom-to-toolbar animation with an arrow dropping onto the download toolbar button. The download-finish animation shows an item being added to the library. The library button hasn't landed yet (bug 1354155) so I've temporarily targeted the bookmarks button. The logic should be the same, just the id changes. As bug 1354155 is actively being worked on, I think it makes sense to stall on landing this patch until then, but while you have your head in this space, any feedback/early review comments would be appreciated. One more thought: As we get to the other animations - and once the library button lands - it may turn out the new download-finish animation should belong to the library component.

Comment on attachment 8873251 [details] Bug 1352065 - Add progressbar to download button using SVG progressbar spritesheet animation. Thanks for splitting the patches into two parts, this makes the reviews easier. We have some UX issues to sort out though. From our recent IRC conversation, it turns out that the mockup on this bug is possibly obsolete, that there are some new mockups in bug 1353434, and that what's been implemented is something different discussed offline with Eric, not reflected in any of these mockups. Also, a key element of the new mockups seems to be that the button is invisible until the first download. Apparently this is still part of the design, but has been overlooked in the current implementation. This change brings its own set of challenges though, and at this stage it does seems premature to implement it without pushing the UX treatment a little bit further first. Even assuming for a moment that we only want to land a visual change in the progress animation, with no interaction change with the Downloads Panel and the Library, the visuals that today are in bug 1353434 have the same contrast issue as bug 1338984 and bug 1359062, exacerbated by the fact that the progress area itself is now really thin. It also seems to me that the issues we have today with the "attention" and "progress" states sharing the same area do not apply to the new design. This is in fact explored by the mockups in bug 1353434, but glossed over by this implementation. Instead of keeping the same code we have today and put a new animation on top of it, we can go directly to an implementation that reflects the design we want. The implementation in this patch, including the SVG file, would need to change completely to reflect the mockups and cannot be reused for the final implementation, so we have to go back to the drawing board, clarify what is the final result we want, and define which intermediate steps can make sense on their own, if any.

Comment on attachment 8873329 [details] [diff] [review] download-notifications.patch As far as this other part of the patch goes, one open question that is unclear from the mockups is whether the Downloads Indicator will still be a button that opens a panel. If it is a button and clicking it shows your most recent downloads, then the completion animation is probably misplaced, as the user would normally have no reason to go to the Library to handle a recently completed download at all. Also, if we still have an option to clear recent downloads in it, it is unclear if using it would cause the button to disappear again. One of the motivations that Stephen had for the current design, where the button is always visible, was in fact to avoid annoying users who would like to keep the interface in a pristine state with an extra step after the download completed. If it is not a button, then the "attention" state with the highlighted icon is probably misplaced, and would need to apply to the Library instead. This leaves open the question of whether this new Downloads Indicator would disappear after some time, or will remain visible for the rest of the session. Also, it seems this would bury downloads to an area of the interface that is more difficult to reach and identify, and after clicking on the Library icon the user would have to scan the menu to find the Downloads entry. Even if we opened the Downloads subview of the Library menu automatically for them if a download recently completed, they might be surprised if the Library menu resets to the default state when it is opened again. Also in the case where this is not a button, we should get an understanding of the behavior for failed downloads and blocked downloads. Failed downloads currently get a round yellow badge in the Downloads Indicator, this would likely have to be moved. Blocked downloads get a badge, a special highlighting of the item, and a dedicated sliding subview to unblock. This functionality would have to be implemented somewhere else. Also, both from the general Photon mockups and from bug 1354532 it is unclear what is the behavior of the Recent Downloads view. It can be very different both from an interaction and a performance perspective, based on whether it should only show downloads from this session or always show all downloads from previous sessions. About the Recent Downloads view contents, there were recent changes to remove all information like the file size and time from the items of the Downloads Panel, so it should be specified if such information would still need to be included. I'm mentioning this here, but this last aspect won't directly influence this bug. The others have a direct influence on what we implement here though.

Based on comment 26 and comment 27, the next steps are to define the exact interactions we want. Eric, if you want we can set up a meeting on Friday with Sam and myself.

Comment on attachment 8863487 [details] Downloading File.mp4 See bug 1353434 for the latest spec for download animations

Comment on attachment 8865590 [details] download_library.svg See bug 1353434 for the latest animation assets

Comment on attachment 8865591 [details] Library_Download.json See bug 1353434 for the latest animation assets

(In reply to :Paolo Amadini from comment #27) > Comment on attachment 8873329 [details] [diff] [review] > download-notifications.patch Hi Paolo, I've tried my best to answer your questions below and happy to clarify if you have more questions. Also if you would like to see the latest animation specs, this is where they will be updated https://bugzilla.mozilla.org/show_bug.cgi?id=1353434. > > As far as this other part of the patch goes, one open question that is > unclear from the mockups is whether the Downloads Indicator will still be a > button that opens a panel. > Yes, the download indicator will still be a button that opens up the download panel. > If it is a button and clicking it shows your most recent downloads, then the > completion animation is probably misplaced, as the user would normally have > no reason to go to the Library to handle a recently completed download at > all. The original intent of the library animation was to introduce the library as a "this is where all your stuff is" area since this is a new feature of Photon. After talking to Bryan we decided to remove the library animation and instead have an animation on the download's icon once the download is complete (this was just discussed today so the animation haven't been updated yet). Also, if we still have an option to clear recent downloads in it, it is > unclear if using it would cause the button to disappear again. Yes we are keeping the option to clear downloads. Once the user hits the clear downloads button then the download icon will disappear from the toolbar. One of the > motivations that Stephen had for the current design, where the button is > always visible, was in fact to avoid annoying users who would like to keep > the interface in a pristine state with an extra step after the download > completed. > > If it is not a button, then the "attention" state with the highlighted icon > is probably misplaced, and would need to apply to the Library instead. This > leaves open the question of whether this new Downloads Indicator would > disappear after some time, or will remain visible for the rest of the > session. The only time the download icon will disappear from the toolbar is when the user selects clear recent download history from the download door hanger and when the user ends their browsing session (closing the browser). > > Also, it seems this would bury downloads to an area of the interface that is > more difficult to reach and identify, and after clicking on the Library icon > the user would have to scan the menu to find the Downloads entry. Even if we > opened the Downloads subview of the Library menu automatically for them if a > download recently completed, they might be surprised if the Library menu > resets to the default state when it is opened again. We will be keeping the downloads door hanger like we have in our current browser. The user can access their downloads at a later time in the library if they decide to close their browser or clear their recent downloads history. > > Also in the case where this is not a button, we should get an understanding > of the behavior for failed downloads and blocked downloads. Failed downloads > currently get a round yellow badge in the Downloads Indicator, this would > likely have to be moved. Blocked downloads get a badge, a special > highlighting of the item, and a dedicated sliding subview to unblock. This > functionality would have to be implemented somewhere else. See above. > > Also, both from the general Photon mockups and from bug 1354532 it is > unclear what is the behavior of the Recent Downloads view. It can be very > different both from an interaction and a performance perspective, based on > whether it should only show downloads from this session or always show all > downloads from previous sessions. We will be keeping the recent downloads door hanger it's current behavior. > > About the Recent Downloads view contents, there were recent changes to > remove all information like the file size and time from the items of the > Downloads Panel, so it should be specified if such information would still > need to be included. I'm mentioning this here, but this last aspect won't > directly influence this bug. The others have a direct influence on what we > implement here though. If there was a decision to remove the information then I would keep it removed.

I pushed a version of the wip patch to try and noticed that I was getting unreferenced file test failures: https://treeherder.mozilla.org/#/jobs?repo=try&revision=f4db82f575fdd24c33796827c922e8a397c52590 As we're using the MOZ_PHOTO_ANIMATIONS build flag, which defaults to true for nightly, I guess it thinks those files needed for when MOZ_PHOTO_ANIMATIONS is true are orphaned. Is there a way to whitelist them or something?

You shouldn't need to whitelist them since that flag should be enabled on these builds, right? Are you sure that these files aren't actually unreferenced? browser_all_files_referenced.js does have a whitelist inside of it that you can add to so that the test won't fail.

Download animations in all scenarios

clearing NI on myself

Comment on attachment 8886723 [details] Bug 1352065 Photon download notifications. Oops, moving this patch/review request to bug 1376519