1139472 - Extend the "attention" state of the Downloads Indicator to indicate success or failure

Status: VERIFIED FIXED

(Firefox :: Downloads Panel, defect, P1)

Description

[:Paolo Amadini]

Currently, the Download Indicator has an "attention" state. This makes the indicator bright and colored, and the color used to be platform-specific. In recent versions, the color used is often green, which may be easily mistaken for a success state.

One solution is to use different colors to indicate different types of "attention". If a download succeeds and another one fails before the panel is opened, the failure color should take precedence.

A different symbol for failure instead of just changing the color may not be needed, as the details for the attention reason are available inside the Downloads Panel already.

Comment 2

[Djfe]

I support this idea and hope it will be implemented some day, because it is quite useful to see whether downloads failed or not :)

yellow/orange could indicate issue with some of the downloads

red could indicate issues with all of the downloads

Comment 3

[Marco Mucci [:MarcoM]]

Reduced in priority to accommodate higher priority work for this release.

Comment 4

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

Attachment: MozReview Request: Bug 1139472 - Extend the attention state of the Downloads Indicator to indicate success or failure. r=paolo

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

Comment 5

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

Just parking my WIP. I still need to add a badge to the downloads button when it is on the main toolbar and not in the menu panel.

Comment 6

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

Comment on attachment 8736779 [details]
MozReview Request: Bug 1139472 - Extend the attention state of the Downloads Indicator to indicate success or failure. r=paolo

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

Comment 7

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

This should work AFAICT, but for some reason the XBL binding for the badge is not applied on the downloads button. There must be some bad interaction with the indicatorOverlay going on, because I get the expected xul:stack in other buttons in the toolbar.

Comment 8

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

Comment on attachment 8736779 [details]
MozReview Request: Bug 1139472 - Extend the attention state of the Downloads Indicator to indicate success or failure. r=paolo

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

Comment 9

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

Comment on attachment 8736779 [details]
MozReview Request: Bug 1139472 - Extend the attention state of the Downloads Indicator to indicate success or failure. r=paolo

Oops, this is not ready for review yet.

Comment 10

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

Comment on attachment 8736779 [details]
MozReview Request: Bug 1139472 - Extend the attention state of the Downloads Indicator to indicate success or failure. r=paolo

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/43549/diff/3-4/

Comment 11

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

Comment on attachment 8736779 [details]
MozReview Request: Bug 1139472 - Extend the attention state of the Downloads Indicator to indicate success or failure. r=paolo

Grrr, I can't make MozReview forget about the review. The good news is that this version fixes the style on Linux. Windows is next.

Comment 12

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

Comment on attachment 8736779 [details]
MozReview Request: Bug 1139472 - Extend the attention state of the Downloads Indicator to indicate success or failure. r=paolo

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/43549/diff/4-5/

Comment 13

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

Fixed the styling on all platforms and got encouraging results on try:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=c455cf901d32

Comment 14

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

Comment on attachment 8736779 [details]
MozReview Request: Bug 1139472 - Extend the attention state of the Downloads Indicator to indicate success or failure. r=paolo

Discussed it with Paolo over vidyo, so clearing review until I address his comments.

Comment 15

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

Comment on attachment 8736779 [details]
MozReview Request: Bug 1139472 - Extend the attention state of the Downloads Indicator to indicate success or failure. r=paolo

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/43549/diff/5-6/

Comment 16

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

I believe I addressed all your oral review comments.

Comment 17

[:Paolo Amadini]

Comment on attachment 8736779 [details]
MozReview Request: Bug 1139472 - Extend the attention state of the Downloads Indicator to indicate success or failure. r=paolo

https://reviewboard.mozilla.org/r/43549/#review44843

95% there!

The attention indicator for completed downloads doesn't appear anymore on OS X (HiDPI) during my testing with this patch applied.

I haven't figured out why by looking at the code.

::: browser/components/downloads/DownloadsCommon.jsm:1129
(Diff revision 6)
> +    if (download.error && download.error.reputationCheckVerdict) {
> +      switch (download.error.reputationCheckVerdict) {

A bit confusingly, "success" and "error" are true at the same time for unblocked downloads. We should add a guard against "!success" or the badge will re-appear after unblocking.

For downloads blocked by application reputation we'll also update "attention" two times, maybe we can rewrite this code block so we only do this once.

::: browser/themes/linux/downloads/indicator.css:29
(Diff revision 6)
>  toolbar[brighttext] #downloads-button[cui-areatype="toolbar"]:not([attention]) > #downloads-indicator-anchor > #downloads-indicator-icon {
>    background: -moz-image-rect(url("chrome://browser/skin/Toolbar-inverted.png"),
>                                0, 198, 18, 180) center no-repeat;
>  }

This [brighttext] rule might need to be updated as well to read :not([attention="success"]). It should be tested with a dark lightweight theme applied.

Worth re-testing the theme with dark lightweight themes on other platforms too, but only if it doesn't take too much time.

Comment 18

[:Paolo Amadini]

Somehow the style is also broken for in-progress downloads.

Comment 19

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

Comment on attachment 8736779 [details]
MozReview Request: Bug 1139472 - Extend the attention state of the Downloads Indicator to indicate success or failure. r=paolo

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/43549/diff/6-7/

Comment 20

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

Thanks for the thorough testing. I hadn't updated some style rules properly to account for the extra badge stack and that resulted in 2 elements with the arrow image being displayed simultaneously. I also tested with a dark theme on OS X and everything looks fine.

I didn't update the :not([attention]) rules to :not([attention="success"]), as I believe they are supposed to apply to the ATTENTION_NONE case. Let me know if that assumption is incorrect.

Comment 21

[:Paolo Amadini]

(In reply to Panos Astithas [:past] from comment #20)
> I didn't update the :not([attention]) rules to :not([attention="success"]),
> as I believe they are supposed to apply to the ATTENTION_NONE case. Let me
> know if that assumption is incorrect.

I think this would result in the download arrow becoming dark when one of the badges is present on a dark lightweight theme, but I'll have to test it to be sure.

Comment 22

[:Paolo Amadini]

The rule refers specifically to the small arrow shown over the progress bar.

Comment 23

[:Paolo Amadini]

Attachment: Glitch

If it looks wrong, it's wrong!

This is a _really_ rare edge case requiring a dark lightweight theme on Mac OS X in non-HiDPI mode with a file being blocked by malware checks when an existing paused download is present.

However, it helps to be consistent not only for the bug but because people might wonder later if the CSS rule was actually meant to be special for some reason...

We're also looking into using this badging for normal error states and the glitch would be more frequent in that case.

We should simplify toolbar button CSS sooner or later!

Comment 24

[:Paolo Amadini]

Windows and Linux probably require a similar adjustment.

Comment 25

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

Comment on attachment 8736779 [details]
MozReview Request: Bug 1139472 - Extend the attention state of the Downloads Indicator to indicate success or failure. r=paolo

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/43549/diff/7-8/

Comment 26

[:Paolo Amadini]

Comment on attachment 8736779 [details]
MozReview Request: Bug 1139472 - Extend the attention state of the Downloads Indicator to indicate success or failure. r=paolo

98% there, but we must resolve the problem for which the progress bra for paused downloads appears when the button is in the main menu on Windows.

Comment 27

[:Paolo Amadini]

That is also an issue on Linux. I've not checked if it's a regression, but it's likely something we want to fix anyways. It may also be related to changing the specificity of some rule.

Comment 28

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

Comment on attachment 8736779 [details]
MozReview Request: Bug 1139472 - Extend the attention state of the Downloads Indicator to indicate success or failure. r=paolo

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/43549/diff/8-9/

Comment 29

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

Comment on attachment 8736779 [details]
MozReview Request: Bug 1139472 - Extend the attention state of the Downloads Indicator to indicate success or failure. r=paolo

https://reviewboard.mozilla.org/r/43549/#review45263

I fixed the last remaining issue plus another minor issue on OS X that I spotted while testing (the indicator opacity wasn't decreased when the window was in the background).

Comment 30

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

Comment on attachment 8736779 [details]
MozReview Request: Bug 1139472 - Extend the attention state of the Downloads Indicator to indicate success or failure. r=paolo

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/43549/diff/9-10/

Comment 31

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/ab85f19b67b1

Comment 32

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/ab85f19b67b1

Comment 33

[Bogdan Maris, Desktop QA [Holiday+PTO 1-6 May]]

Verified all the states of the download icon using malicious (red icon) and uncommon (yellow icon) sample files from http://testsafebrowsing.appspot.com/ using latest Nightly 49.0a1 and latest Developer Edition 48.0a2. 

I found an issue here though, logged bug 1268483.