Show an alert icon on the downloads button when a download fails

VERIFIED FIXED in Firefox 52

Status

()

defect
VERIFIED FIXED
3 years ago
2 years ago

People

(Reporter: yifan, Assigned: rexboy)

Tracking

(Blocks 2 bugs)

unspecified
Firefox 53
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(relnote-firefox 52+, firefox52 verified, firefox53 verified)

Details

(Whiteboard: [CHE-MVP])

Attachments

(2 attachments, 2 obsolete attachments)

Reporter

Description

3 years ago
Show an alert icon on the downloads button when a download fails.

Updated

3 years ago
Component: General → Downloads Panel

Comment 1

3 years ago
This can be implemented easily with the badging system from bug 1139472.
Depends on: 1139472
No longer depends on: 1270006
Reporter

Comment 2

3 years ago
Posted file Download alert spec (obsolete) —
Reporter

Comment 3

3 years ago
Looks like this feature has been implemented. Guess I had filed an invalid bug. Thank you Paolo for the early notice!
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → INVALID
Are you sure this is actually done? As you can see here, I didn't define any attention state for failed downloads:

https://dxr.mozilla.org/mozilla-central/rev/c3f5e6079284a7b7053c41f05d0fe06ff031db03/browser/components/downloads/DownloadsCommon.jsm#145

I think we still need to add an ATTENTION_FAILED, make sure it is handled properly and update the stylesheets to display the same icon as in the ATTENTION_WARNING case.
Flags: needinfo?(yliao)
Reporter

Comment 5

3 years ago
Panos thank you for clarifying it. Reopened to work on it as suggested.
Status: RESOLVED → REOPENED
Flags: needinfo?(yliao)
Resolution: INVALID → ---

Comment 6

3 years ago
Comment on attachment 8752081 [details]
Download alert spec

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

https://bugzilla.mozilla.org/show_bug.cgi?id=1269956
Attachment #8752081 - Attachment is obsolete: true

Comment 8

3 years ago
Comment on attachment 8761116 [details]
Bug 1270014 - Show an alert icon on the downloads button when a download fails

Panos, take a look at this patch. Since we don't have a separate visual state for "failed" versus "severe" in either the old or the new style of attention badge on the Downloads button, I wonder if we should just rename the current "severe" state to "failed" and use it on failure? Seems overkill to have two states that are visually identical.
Attachment #8761116 - Flags: feedback?(past)
Comment on attachment 8761116 [details]
Bug 1270014 - Show an alert icon on the downloads button when a download fails

I see what you mean. I would say, let's ask UX first to make sure that they don't have plans to use a different UI treatment for the failed state any time soon to make sure we are not throwing away useful code. If that is the case, let's just return the severe case in DownloadsCommon.jsm (with a suitable comment):

this.attention = DownloadsCommon.ATTENTION_SEVERE;

I don't see the point in a mass rewrite from "severe" to "failed", since both are generic enough and will only be accurate sometimes.
Attachment #8761116 - Flags: feedback?(past)

Updated

3 years ago
Attachment #8761116 - Flags: review?(paolo.mozmail)

Comment 10

3 years ago
(In reply to Panos Astithas [:past] from comment #9)
> I don't see the point in a mass rewrite from "severe" to "failed", since
> both are generic enough and will only be accurate sometimes.

Maybe a mass rewrite to "error"... it's the logical counterpart to "warning". But let's hear from UX.

Updated

3 years ago
Whiteboard: [CHE-MVP]
ni? UX designer Morpheus.

Morpheus, per comment 9 please confirm whether to have different UI treatment for failed downloads.
Flags: needinfo?(mochen)
Posted image Badges.png
Please find the attached image for the badging design or find "Download alert as badges" in the released UX spec for more details. Generally speaking, it only shows red badge for malware downloads. Other states display yellow badges, including failed, uncommon, unwanted downloads. Let know we if any questions, thanks.
Flags: needinfo?(mochen)
Rex will help implement badge for failed downloads per UX comment 12. Assign owner to him.
Assignee: nobody → rexboy
As a reference, this bug will change 1 UX item - Yellow badge for failed.

Comment 15

3 years ago
Since we're reusing the yellow badge, this is literally a one or two-line fix. Rex, would you like to do it?
Flags: needinfo?(rexboy)
Sure, however I found an issue when trying to implement that. I’m trying to figure out if there are good solutions with UX. Here's the situation:

1. Download a file that takes awhile
2. Open all downloads overlay
3. Disconnect from the internet
4. Now the downloading file should change to paused state. Right click on it and select “resume” from downloads overlay.
5. The downloading file goes to failed state. Yellow warning badge shown (due to new design).
6. Connect to the internet.
7. The file is resumed automatically but the badge is still there. If the user open the panel now, they will find nothing unusual.

If we care about this scenario and hope the badge disappear when resumed, we need to implement a way more complex badge system which may not worth it. 
We may need a good way for that by either programming or UX's aspect.
Flags: needinfo?(rexboy)

Comment 17

3 years ago
Hm, arguably we shouldn't try to resume failed downloads, because the user took an explicit action on them already. We can add a check for the "cancelled" state in _resumeOfflineDownloads:

https://dxr.mozilla.org/mozilla-central/rev/c91249f41e3766274131a84f9157a4d9d9949520/toolkit/components/jsdownloads/src/DownloadIntegration.jsm#1043

This should be a separate bug, independent from this one.
I think this small change will include both download failed (including server error) and parental control, but not sure if there are other cases that need to be considered.
Paolo may you take a look? Thanks!

Comment 20

3 years ago
mozreview-review
Comment on attachment 8824382 [details]
Bug 1270014 - Show an alert icon on the downloads button when a download fails.

https://reviewboard.mozilla.org/r/102910/#review103366

(In reply to KM Lee [:rexboy] from comment #19)
> I think this small change will include both download failed (including
> server error) and parental control, but not sure if there are other cases
> that need to be considered.

Looks good to me, as far as I can tell these are all the cases we should consider.

::: browser/components/downloads/DownloadsCommon.jsm:1198
(Diff revision 1)
> -    } else if (download.succeeded || download.error) {
> +    } else if (!download.succeeded && download.error) {
> +      this.attention = DownloadsCommon.ATTENTION_WARNING;
> +    } else if (download.succeeded) {

Just put the "error" else case after the "succeeded" else case, so you can simplify the conditions.
Attachment #8824382 - Flags: review?(paolo.mozmail) → review+

Comment 21

3 years ago
mozreview-review
Comment on attachment 8824382 [details]
Bug 1270014 - Show an alert icon on the downloads button when a download fails.

https://reviewboard.mozilla.org/r/102910/#review103368

::: browser/components/downloads/DownloadsCommon.jsm:1199
(Diff revision 1)
>            this.attention = DownloadsCommon.ATTENTION_SEVERE;
>            Cu.reportError("Unknown reputation verdict: " +
>                           download.error.reputationCheckVerdict);
>        }
> -    } else if (download.succeeded || download.error) {
> +    } else if (!download.succeeded && download.error) {
> +      this.attention = DownloadsCommon.ATTENTION_WARNING;

Ah, almost forgot, you need to check "this._attention != DownloadsCommon.ATTENTION_SEVERE" like the code above does.
Comment hidden (mozreview-request)
Issues in comment 20 and 21 has been addressed.
Thank you Paolo!

Comment 24

3 years ago
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/7bf3fa172915
Show an alert icon on the downloads button when a download fails. r=Paolo
Keywords: checkin-needed

Comment 25

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/7bf3fa172915
Status: REOPENED → RESOLVED
Closed: 3 years ago3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 53
I reproduced this issue using 49.0a1, build ID: 20160504030302, on Windows 10 x64.
I can confirm this issue is fixed, I verified using Fx53.0a1, build ID: 20170112030301, on Windows 10 x64 and Ubuntu 14.04 LTS.

Cheers!

Updated

3 years ago
Attachment #8761116 - Attachment is obsolete: true

Comment 27

3 years ago
Comment on attachment 8824382 [details]
Bug 1270014 - Show an alert icon on the downloads button when a download fails.

Approval Request Comment
[Feature/Bug causing the regression]: Downloads Panel redesign
[User impact if declined]: Small, but it would be nice to get this improvement together with the visual redesign in Firefox 52.
[Is this code covered by automated tests?]: No, small front-end visual change.
[Has the fix been verified in Nightly?]: Yes, verified manually by QA.
[Needs manual test from QE? If yes, steps to reproduce]: It would be good to have this tested again on Aurora or Beta, although Nightly testing is likely sufficient.
[List of other uplifts needed for the feature/fix]: None
[Is the change risky?]: Low risk
[Why is the change risky/not risky?]: Self-contained to the failed indication, easy to backout if needed.
[String changes made/needed]: None
Attachment #8824382 - Flags: approval-mozilla-aurora?
Comment on attachment 8824382 [details]
Bug 1270014 - Show an alert icon on the downloads button when a download fails.

visual warning for failed download, aurora52+
Attachment #8824382 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Cristian can you test this again in 52 after the patch is uplifted?  Thanks!
Flags: needinfo?(cristian.comorasu)
(In reply to Julien Cristau [:jcristau] from comment #29)
> Cristian can you test this again in 52 after the patch is uplifted?  Thanks!

Yes, this issue is also fixed on aurora. I verified using Fx 52.0a2, build ID: 20170118004007, on Windows 10 x64, Ubuntu 16.04 LTS x64 and Mac OS X 10.9.5.
Status: RESOLVED → VERIFIED
Flags: needinfo?(cristian.comorasu)
Release Note Request (optional, but appreciated)
[Why is this notable]: Minor improvement to the download panel
[Affects Firefox for Android]: no
[Suggested wording]: If a download fails, the user will be notified with a visual change to the download icon in the toolbar
[Links (documentation, blog post, etc)]:
relnote-firefox: --- → ?
Added to 52 release notes along with bug 1269957:



Download panel improvements:

 - visual notification of failed downloads
 - display 5 most recent downloads rather than 3
You need to log in before you can comment on or make changes to this bug.