Closed Bug 1139472 Opened 5 years ago Closed 4 years ago

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

Categories

(Firefox :: Downloads Panel, defect, P1)

defect

Tracking

()

VERIFIED FIXED
Firefox 48
Iteration:
48.3 - Apr 25
Tracking Status
firefox48 --- verified

People

(Reporter: Paolo, Assigned: past)

References

(Depends on 1 open bug, Blocks 1 open bug)

Details

(Whiteboard: [fxprivacy])

Attachments

(2 files)

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.
Duplicate of this bug: 1186736
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
Flags: qe-verify?
Whiteboard: [fxprivacy]
Flags: qe-verify? → qe-verify+
Priority: -- → P3
Priority: P3 → P2
Priority: P2 → P1
Reduced in priority to accommodate higher priority work for this release.
Priority: P1 → P2
Priority: P2 → P3
Priority: P3 → P1
QA Contact: paul.silaghi
Assignee: nobody → past
Status: NEW → ASSIGNED
Iteration: --- → 48.1 - Mar 21
Iteration: 48.1 - Mar 21 → 48.2 - Apr 4
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 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/
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.
Iteration: 48.2 - Apr 4 → 48.3 - Apr 18
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/
Attachment #8736779 - Attachment description: MozReview Request: Bug 1139472 - Extend the attention state of the Downloads Indicator to indicate success or failure → MozReview Request: Bug 1139472 - Extend the attention state of the Downloads Indicator to indicate success or failure.
Attachment #8736779 - Flags: review?(paolo.mozmail)
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.
Attachment #8736779 - Flags: review?(paolo.mozmail)
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/
Attachment #8736779 - Flags: review?(paolo.mozmail)
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.
Attachment #8736779 - Flags: review?(paolo.mozmail)
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/
Attachment #8736779 - Attachment description: MozReview Request: Bug 1139472 - Extend the attention state of the Downloads Indicator to indicate success or failure. → MozReview Request: Bug 1139472 - Extend the attention state of the Downloads Indicator to indicate success or failure. r?paolo
Attachment #8736779 - Flags: review?(paolo.mozmail)
Fixed the styling on all platforms and got encouraging results on try:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=c455cf901d32
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.
Attachment #8736779 - Flags: review?(paolo.mozmail)
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/
Attachment #8736779 - Flags: review?(paolo.mozmail)
I believe I addressed all your oral review comments.
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.
Attachment #8736779 - Flags: review?(paolo.mozmail)
Somehow the style is also broken for in-progress downloads.
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/
Attachment #8736779 - Flags: review?(paolo.mozmail)
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.
(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.
The rule refers specifically to the small arrow shown over the progress bar.
Attached image 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!
Windows and Linux probably require a similar adjustment.
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 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.
Attachment #8736779 - Flags: review?(paolo.mozmail) → review+
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 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/
Attachment #8736779 - Flags: review+ → review?(paolo.mozmail)
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).
Attachment #8736779 - Flags: review+
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/
Attachment #8736779 - Attachment description: MozReview Request: Bug 1139472 - Extend the attention state of the Downloads Indicator to indicate success or failure. r?paolo → MozReview Request: Bug 1139472 - Extend the attention state of the Downloads Indicator to indicate success or failure. r=paolo
https://hg.mozilla.org/mozilla-central/rev/ab85f19b67b1
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 48
Attachment #8736779 - Flags: review?(paolo.mozmail) → review+
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.
Status: RESOLVED → VERIFIED
Flags: qe-verify+
Depends on: 1270052
See Also: → 1269954
Blocks: 1270014
You need to log in before you can comment on or make changes to this bug.