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

Status: VERIFIED FIXED

(Firefox :: Downloads Panel, defect)

Description

[yifan [:yifan][:yliao]]

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

Comment 1

[:Paolo Amadini]

This can be implemented easily with the badging system from bug 1139472.

Comment 2

[yifan [:yifan][:yliao]]

Attachment: Download alert spec

Comment 3

[yifan [:yifan][:yliao]]

Looks like this feature has been implemented. Guess I had filed an invalid bug. Thank you Paolo for the early notice!

Comment 4

[Panos Astithas (he/him) [:past] (please ni?)]

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.

Comment 5

[yifan [:yifan][:yliao]]

Panos thank you for clarifying it. Reopened to work on it as suggested.

Comment 6

[Wesly Huang (EPM)]

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

Comment 7

[yifan [:yifan][:yliao]]

Attachment: 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/

Comment 8

[:Paolo Amadini]

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.

Comment 9

[Panos Astithas (he/him) [:past] (please ni?)]

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.

Comment 10

[:Paolo Amadini]

(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.

Comment 11

[Ben Tian (inactive)]

ni? UX designer Morpheus.

Morpheus, per comment 9 please confirm whether to have different UI treatment for failed downloads.

Comment 12

[Morpheus Chen [:morpheus] UX]

Attachment: 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.

Comment 13

[Ben Tian (inactive)]

Rex will help implement badge for failed downloads per UX comment 12. Assign owner to him.

Comment 14

[Ben Tian (inactive)]

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

Comment 15

[:Paolo Amadini]

Since we're reusing the yellow badge, this is literally a one or two-line fix. Rex, would you like to do it?

Comment 16

[KM Lee [:rexboy] (inactivate after Sep. 22)]

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.

Comment 17

[:Paolo Amadini]

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 18

[KM Lee [:rexboy] (inactivate after Sep. 22)]

Attachment: Bug 1270014 - Show an alert icon on the downloads button when a download fails.

Review commit: https://reviewboard.mozilla.org/r/102910/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/102910/

Comment 19

[KM Lee [:rexboy] (inactivate after Sep. 22)]

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

[:Paolo Amadini]

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.

Comment 21

[:Paolo Amadini]

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 22

[KM Lee [:rexboy] (inactivate after Sep. 22)]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/102910/diff/1-2/

Comment 23

[KM Lee [:rexboy] (inactivate after Sep. 22)]

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

Comment 24

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/7bf3fa172915
Show an alert icon on the downloads button when a download fails. r=Paolo

Comment 25

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/7bf3fa172915

Comment 26

[Cristian Comorasu, QA [:ccomorasu], Releae Desktop QA - Please contact [e-mail]]

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!

Comment 27

[:Paolo Amadini]

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

Comment 28

[Julien Cristau [:jcristau]]

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+

Comment 29

[Julien Cristau [:jcristau]]

Cristian can you test this again in 52 after the patch is uplifted?  Thanks!

Comment 30

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/977a08850b05

Comment 31

[Cristian Comorasu, QA [:ccomorasu], Releae Desktop QA - Please contact [e-mail]]

(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.

Comment 32

[Pascal Chevrel:pascalc]

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)]:

Comment 33

[Julien Cristau [:jcristau]]

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