832672 - Downloads Panel gives no indication or feedback on missing files

Status: RESOLVED FIXED

(Firefox :: Downloads Panel, defect)

Description

[Caspy7]

When a file has moved or been deleted the downloads panel gives no indication of this, visually, on mouseover, or on click.

On the web turning the pointer/cursor into a hand communicates that a click will give an action (generally open a new page).  As it stands every square inch of the downloads panel turns the pointer into a hand.  But if a file is missing the UI should not communicate that there is an actionable click by changing the pointer.
A visual change should also be made for the item - perhaps desaturating the icon and graying/italicizing the filename text.

Also, as was mentioned on the wiki, the 'Open Containing Folder' button remains unchanged and clickable, but with no feedback.  If the button and the rest of the entry seemed fully disabled and perhaps communicated the lack of file, there would be no need for feedback on clicking it.

Comment 1

[:Paolo Amadini]

Attachment: Handle the file moved case in the Downloads Panel only

From the bug reports we received during the beta cycle, it seems that the case
described here is likely to occur in the Downloads Panel, and is noticeable
exactly because the item looks clickable but clicking has no effect.

What I'm trying now is to find a solution that can be uplifted to beta, in
other words not to scope creep into other dependencies of bug 726451. The idea
is to check existence and set an attribute that can be used by the theme.

The state of this patch:
 * Only touches the Downloads Panel (no Library window changes)
 * Checks existence in the front-end (no changes to the back-end data items)
 * Tested with the Linux theme, not tested on OS X and Windows
 * When the file does not exist, hides the "open in folder" button completely

Mike, do you have time to check whether the changes work on OS X and Windows?

> A visual change should also be made for the item - perhaps desaturating the
> icon and graying/italicizing the filename text.

Also, I'm not sure about how to implement these in all themes without
regressing color visibility or other things I'm not aware of, maybe Mike
you can try some ideas while verifying my other changes?

Comment 2

[:Paolo Amadini]

Comment on attachment 722204 [details] [diff] [review]
Handle the file moved case in the Downloads Panel only

We're hiding the button to "open containing folder" when the target file does
not exist. Previously, we displayed the button but clicking it had no effect.

Is this fine from a UX / visual design point of view?

The only alternative I can think of is showing a grayed out version, but it
might be difficult to distinguish from the normal one (not to mention we'd
need a new icon!).

Comment 3

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 722204 [details] [diff] [review]
Handle the file moved case in the Downloads Panel only

Review of attachment 722204 [details] [diff] [review]:
-----------------------------------------------------------------

A few things:

1) This rule also needs to be modified for Aero: https://mxr.mozilla.org/mozilla-central/source/browser/themes/windows/downloads/downloads-aero.css#15
2) The downloadShow icon is not being hidden in the Library when the file has been removed... did we want to extend this patch's change to the Library as well?

Beyond that, this looks good on OSX.

Comment 4

[:Paolo Amadini]

Attachment: Updated Aero rule

(In reply to Mike Conley (:mconley) from comment #3)
> 2) The downloadShow icon is not being hidden in the Library when the file
> has been removed... did we want to extend this patch's change to the Library
> as well?

No, not in this patch. That's a different problem because the list item in the
Library may be visible while the file disappears, so we would have to check
continuously in the background or something like that. In addition, we would
have to limit the number of checks we do (it is automatically limited to the
three visible items in the panel).

Comment 5

[Marco Bonardo [:mak]]

Comment on attachment 722284 [details] [diff] [review]
Updated Aero rule

Review of attachment 722284 [details] [diff] [review]:
-----------------------------------------------------------------

please, file a bug for the Library/incontent view, blocking bug 726451

Fwiw, we already call exists() each time we update commands when we check isCommandEnabled of downloadCmd_show, this patch is somehow adding duplicate stat calls, in bug 827010 we should fix usage of OS.file in the panel code, and coalesce these calls, maybe we could just updateCommands when we open the panel and just do all the files checks async there.
Or use a similar strategy as we do in the Library, when an element is made visible (in this case added to the list) start the async fetch process and use fallbacks in isCommandEnabled until it is done.

That said, if this is a temporary fix to bring to Aurora until we properly fix async file stat on trunk, I'm fine with its simplicity.

::: browser/components/downloads/content/downloads.css
@@ +95,1 @@
>  #downloadsSummary:not([inprogress]) > vbox > #downloadsSummaryProgress,

the relation between these rules is nonexistent, I'd split a separate display: none instead of reusing this one

::: browser/components/downloads/content/downloads.js
@@ +741,5 @@
> +  /**
> +   * Starts checking whether the target files of visible items are still
> +   * available on disk, and updates the allowed items interactions accordingly.
> +   */
> +  verifyVisibleTargetsExist: function DV_verifyVisibleTargetsExist()

I think this additional method is not really needed, I'd just hardcode the loop in the single callpoint

@@ +743,5 @@
> +   * available on disk, and updates the allowed items interactions accordingly.
> +   */
> +  verifyVisibleTargetsExist: function DV_verifyVisibleTargetsExist()
> +  {
> +    for (let [, viewItem] in Iterator(this._viewItems)) {

nit: you know, I still prefer for each...in :)

@@ +1101,5 @@
>      // supported by the nsIMozIconURI interface.
>      if (aOldState != Ci.nsIDownloadManager.DOWNLOAD_FINISHED &&
>          aOldState != this.dataItem.state) {
>        this._element.setAttribute("image", this.image + "&state=normal");
> +      

trailing spaces

Comment 6

[:Paolo Amadini]

Attachment: Updated patch

(In reply to Marco Bonardo [:mak] from comment #5)
> ::: browser/components/downloads/content/downloads.js
> > +  verifyVisibleTargetsExist: function DV_verifyVisibleTargetsExist()
> 
> I think this additional method is not really needed, I'd just hardcode the
> loop in the single callpoint

Yeah, _viewItems is supposedly private but I agree to go for simplicity.

> nit: you know, I still prefer for each...in :)

And I keep forgetting it exists! :-)

Comment 7

[:Paolo Amadini]

Attachment: Appearance of item pointing to a removed file

The file associated to the first item has been removed from disk.

Comment 8

[Marco Bonardo [:mak]]

fwiw, we have "Unknown size" string and the Library is using such when the file is missing. We may also improve the Library text in nightly clearly.

Comment 9

[Stephen Horlander [:shorlander]]

Comment on attachment 738977 [details]
Appearance of item pointing to a removed file

As discussed in the Firefox Desktop Meeting:

- This is better than what we have now and could be uplifted as is
- For Nightly we can use this approach and put in another line with a message describing why the item is unavailable

Comment 10

[:Paolo Amadini]

https://hg.mozilla.org/integration/mozilla-inbound/rev/12b482031469

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/12b482031469