The default bug view has changed. See this FAQ.

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

VERIFIED FIXED in Firefox 52

Status

()

Firefox
Downloads Panel
VERIFIED FIXED
11 months ago
2 months ago

People

(Reporter: yifan, Assigned: rexboy)

Tracking

(Blocks: 2 bugs)

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

Firefox Tracking Flags

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

Details

(Whiteboard: [CHE-MVP])

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(2 attachments, 2 obsolete attachments)

(Reporter)

Description

11 months ago
Show an alert icon on the downloads button when a download fails.

Updated

11 months ago
Component: General → Downloads Panel

Comment 1

11 months ago
This can be implemented easily with the badging system from bug 1139472.
Depends on: 1139472
No longer depends on: 1270006
(Reporter)

Comment 2

10 months ago
Created attachment 8752081 [details]
Download alert spec
(Reporter)

Comment 3

10 months 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
Last Resolved: 11 months 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

10 months ago
Panos thank you for clarifying it. Reopened to work on it as suggested.
Status: RESOLVED → REOPENED
Flags: needinfo?(yliao)
Resolution: INVALID → ---
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
(Reporter)

Comment 7

10 months ago
Created attachment 8761116 [details]
Bug 1270014 - Show an alert icon on the downloads button when a download fails

Add ATTENTION_FAILED.

Review commit: https://reviewboard.mozilla.org/r/58452/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/58452/
Attachment #8761116 - Flags: review?(paolo.mozmail)

Comment 8

10 months 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

10 months ago
Attachment #8761116 - Flags: review?(paolo.mozmail)

Comment 10

10 months 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.
Whiteboard: [CHE-MVP]

Comment 11

4 months ago
ni? UX designer Morpheus.

Morpheus, per comment 9 please confirm whether to have different UI treatment for failed downloads.
Flags: needinfo?(mochen)
Created attachment 8813049 [details]
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)

Comment 13

4 months ago
Rex will help implement badge for failed downloads per UX comment 12. Assign owner to him.
Assignee: nobody → rexboy

Comment 14

3 months ago
As a reference, this bug will change 1 UX item - Yellow badge for failed.

Comment 15

3 months 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)
(Assignee)

Comment 16

3 months ago
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 months 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.
Comment hidden (mozreview-request)
(Assignee)

Comment 19

3 months ago
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 months 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 months 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)
(Assignee)

Updated

2 months ago
Blocks: 1329625
(Assignee)

Updated

2 months ago
Keywords: checkin-needed
(Assignee)

Comment 23

2 months ago
Issues in comment 20 and 21 has been addressed.
Thank you Paolo!

Comment 24

2 months 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

2 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/7bf3fa172915
Status: REOPENED → RESOLVED
Last Resolved: 10 months ago2 months ago
status-firefox53: --- → fixed
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!
status-firefox53: fixed → verified

Updated

2 months ago
Attachment #8761116 - Attachment is obsolete: true

Updated

2 months ago
status-firefox52: --- → fix-optional

Comment 27

2 months 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)

Comment 30

2 months ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-aurora/rev/977a08850b05
status-firefox52: fix-optional → fixed
(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
status-firefox52: fixed → 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
relnote-firefox: ? → 52+
You need to log in before you can comment on or make changes to this bug.