Closed Bug 1270014 Opened 4 years ago Closed 4 years ago

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

Categories

(Firefox :: Downloads Panel, defect)

defect
Not set
normal

Tracking

()

VERIFIED FIXED
Firefox 53
Tracking Status
relnote-firefox --- 52+
firefox52 --- verified
firefox53 --- verified

People

(Reporter: yifan, Assigned: rexboy)

References

(Blocks 1 open bug)

Details

(Whiteboard: [CHE-MVP])

Attachments

(2 files, 2 obsolete files)

Show an alert icon on the downloads button when a download fails.
Component: General → Downloads Panel
This can be implemented easily with the badging system from bug 1139472.
Depends on: 1139472
No longer depends on: 1270006
Attached file Download alert spec (obsolete) —
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: 4 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)
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
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)
Attachment #8761116 - Flags: review?(paolo.mozmail)
(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]
ni? UX designer Morpheus.

Morpheus, per comment 9 please confirm whether to have different UI treatment for failed downloads.
Flags: needinfo?(mochen)
Attached 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.
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)
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 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 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.
Issues in comment 20 and 21 has been addressed.
Thank you Paolo!
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
https://hg.mozilla.org/mozilla-central/rev/7bf3fa172915
Status: REOPENED → RESOLVED
Closed: 4 years ago4 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!
Attachment #8761116 - Attachment is obsolete: true
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.