Closed Bug 1700238 Opened 4 years ago Closed 4 years ago

Refresh the download icons and animation

Categories

(Firefox :: Theme, enhancement, P1)

enhancement

Tracking

()

VERIFIED FIXED
90 Branch
Tracking Status
firefox89 --- verified
firefox90 --- verified

People

(Reporter: sfoster, Assigned: sfoster)

References

(Blocks 1 open bug, Regressed 1 open bug)

Details

(Whiteboard: [proton-icons] [proton-uplift])

Attachments

(6 files, 1 obsolete file)

We have newly drawn svgs for the download toolbar button (browser/themes/shared/downloads/download-icons.svg)
Note that this is currently a composite SVG with a few <use href="#"/> elements to draw the various states consistently from one single image.

One of those uses is in the animation when a download starts/ends, so updating this image needs to be tightly coordinated with updating those animations.

(In reply to Sam Foster [:sfoster] (he/him) from comment #0)

Note that this is currently a composite SVG with a few <use href="#"/> elements to draw the various states consistently from one single image.

That's a hack that's bad for perf afaik, that should stop being used ideally.

As the animation and icon are interlinked, lets update both on this bug.

(In reply to Tim Nguyen :ntim from comment #1)

(In reply to Sam Foster [:sfoster] (he/him) from comment #0)

Note that this is currently a composite SVG with a few <use href="#"/> elements to draw the various states consistently from one single image.

That's a hack that's bad for perf afaik, that should stop being used ideally.

Noted. That might make sense as the designs I've seen for the new animation and progress indicator look like they don't need these parts broken out and reusable in the same way.

Summary: Refresh the download icons → Refresh the download icons and animation
Attached file download assets - svg.zip (deleted) —
Attached file download assets - json.zip (deleted) —

Assets for animation

I'm going to attempt to take this.

Assignee: nobody → mconley

I'll note that this is very much not a drop-in replacement. It looks like the old download icons SVG contained multiple images:

  • chrome://browser/skin/downloads/download-icons.svg#arrow
  • chrome://browser/skin/downloads/download-icons.svg#arrow-with-bar
  • chrome://browser/skin/downloads/download-icons.svg#default-bar
  • chrome://browser/skin/downloads/download-icons.svg#progress-bar-bg
  • chrome://browser/skin/downloads/download-icons.svg#progress-bar-fg

Attempting to detangle this now.

It looks like these sub images are all components of the various pre-existing downloads button animations. The vast majority of the uses of download-icons.svg is the "arrow-with-bar" variant, which is what the downloads.svg looks like. I'm going to update all of the uses of "arrow-with-bar" to use downloads.svg directly.

For the animations... there's no way I can just drop this in and have it work. Replacing downloads.svg means inserting the downloads animations simultaneously, so it's all going to be one big change. I guess that's why we split this out into its own bug.

Attached file WIP: [WIP] Bug 1700238 - Replace downloads icon. (obsolete) (deleted) —

This mainly replaces all usages of the historical "arrow-with-bar" variant of the icon. This
does not update any of the download animations.

sfoster said he'd take over, since he's more familiar with the downloads button.

Assignee: mconley → nobody
Assignee: nobody → sfoster
  • New start and finish animations using svg filmstrip images
  • Toolbarbutton progress "bar" is a pie-chart, using the same filmstrip animation technique

Status:

  • Basic start/progress/finish animations hooked.
  • First pass at SVG optimization done, more todo.
  • SVG colors are currently hard-coded, not using context-fill/stroke
  • Not looked yet at indeterminate downloads
  • Not yet verified download button customization
Attachment #9217933 - Attachment description: WIP: Bug 1700238 - (WIP) Update downloads animations → Bug 1700238 - Update download icon and animations. r?Gijs
Attached video Download - CLOSED PANEL.mp4 (deleted) —

Reference video for the download animation (download panel is closed as download completes)

Attached video Download - OPENED PANEL.mp4 (deleted) —

Reference video for the download animation (download panel is open as download completes)

how does it work on indetermined size downloads, where there's no known progress?

Oh sorry, I just saw that in the TODO section of comment 10

Pushed by sfoster@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/025fbea4eca1 Update download icon and animations. r=Gijs
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → 90 Branch

Comment on attachment 9217933 [details]
Bug 1700238 - Update download icon and animations. r?Gijs

Beta/Release Uplift Approval Request

  • User impact if declined: Inconsistent icon weight in toolbar, old/non-photon UX for downloading animations.
  • Is this code covered by automated tests?: No
  • Has the fix been verified in Nightly?: No
  • Needs manual test from QE?: Yes
  • If yes, steps to reproduce: The behavior of the downloading button should not changed by this patch, but the visual effects in the download button for the start of a download, download progress and download completion have been updated. Verification should include these steps in supported themes.
  • List of other uplifts needed: None
  • Risk to taking this patch: Low
  • Why is the change risky/not risky? (and alternatives if risky): Icon and animation assets updated.
    Code changes are limited to the download component and are visual changes only with no behavior change or expected side-effects.
  • String changes made/needed: None
Attachment #9217933 - Flags: approval-mozilla-beta?
Attachment #9213657 - Flags: approval-mozilla-beta?
Flags: qe-verify+
Whiteboard: [proton-icons] → [proton-icons] [proton-uplift]

Sam, you asked to uplift 025fbea4eca1 which landed on m-c 2 days ago but also D110864 which is marked as WIP and has no review, did you really mean to have this revision imported into beta? Thanks

Flags: needinfo?(sfoster)
QA Whiteboard: [qa-triaged]

Comment on attachment 9213657 [details]
WIP: [WIP] Bug 1700238 - Replace downloads icon.

Clearing uplift request on this. This patch was folded into the other.

Flags: needinfo?(sfoster)
Attachment #9213657 - Flags: approval-mozilla-beta?
Attachment #9213657 - Attachment is obsolete: true

:pascalc, thanks for catching that. Yeah 025fbea4eca1 already contains the relevant pieces of that WIP patch. I've removed the uplift flag and marked that attachment as obsolete.

Comment on attachment 9217933 [details]
Bug 1700238 - Update download icon and animations. r?Gijs

Planned proton work, approved for 89 beta 9, thanks.

Attachment #9217933 - Flags: approval-mozilla-beta? → approval-mozilla-beta+

Confirming here that we (UX and PM) want this uplifted for MR1 to Beta.

Attached image download tool.png (deleted) —

Verified that the refreshed download icon and animations are implemented on 89.0b10 and 90.0a1 (2021-05-10) on Windows 10, Ubuntu 20.04 and macOS 10.15.7.

However we noticed that the panel and progress bar in Comment 12 is different from what we are seeing (see attachment).

Sam, is this intended or the changes in Comment 12 are not yet implemented?

Flags: needinfo?(sfoster)

(In reply to Catalin Sasca, QA [:csasca] from comment #23)

However we noticed that the panel and progress bar in Comment 12 is different from what we are seeing (see attachment).
Sam, is this intended or the changes in Comment 12 are not yet implemented?

Yeah the panel progress bar has not yet been implemented. Thanks for the reminder, I've filed bug 1710918 to get that done.

Flags: needinfo?(sfoster)

(In reply to Sam Foster [:sfoster] (he/him) from comment #24)

(In reply to Catalin Sasca, QA [:csasca] from comment #23)

However we noticed that the panel and progress bar in Comment 12 is different from what we are seeing (see attachment).
Sam, is this intended or the changes in Comment 12 are not yet implemented?

Yeah the panel progress bar has not yet been implemented. Thanks for the reminder, I've filed bug 1710918 to get that done.

Thank you, Sam! Closing this as verified per comment 23 and comment 24.

Status: RESOLVED → VERIFIED
Flags: qe-verify+
Regressions: 1727315
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: