Closed
Bug 832672
Opened 12 years ago
Closed 12 years ago
Downloads Panel gives no indication or feedback on missing files
Categories
(Firefox :: Downloads Panel, defect)
Tracking
()
RESOLVED
FIXED
Firefox 23
People
(Reporter: caspy77, Assigned: Paolo)
References
(Blocks 1 open bug)
Details
Attachments
(2 files, 2 obsolete files)
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
image/png
|
shorlander
:
ui-review+
|
Details |
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.
Assignee | ||
Comment 1•12 years ago
|
||
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?
Attachment #722204 -
Flags: review?(mconley)
Attachment #722204 -
Flags: review?(mak77)
Assignee | ||
Comment 2•12 years ago
|
||
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!).
Attachment #722204 -
Flags: ui-review?(shorlander)
Comment 3•12 years ago
|
||
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.
Attachment #722204 -
Flags: review?(mconley)
Assignee | ||
Comment 4•12 years ago
|
||
(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).
Attachment #722204 -
Attachment is obsolete: true
Attachment #722204 -
Flags: ui-review?(shorlander)
Attachment #722204 -
Flags: review?(mak77)
Attachment #722284 -
Flags: review?(mconley)
Assignee | ||
Updated•12 years ago
|
Attachment #722284 -
Flags: ui-review?(shorlander)
Attachment #722284 -
Flags: review?(mak77)
Comment 5•12 years ago
|
||
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
Attachment #722284 -
Flags: review?(mak77) → review+
Assignee | ||
Comment 6•12 years ago
|
||
(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! :-)
Assignee: nobody → paolo.mozmail
Attachment #722284 -
Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #722284 -
Flags: ui-review?(shorlander)
Attachment #722284 -
Flags: review?(mconley)
Attachment #724859 -
Flags: ui-review?(shorlander)
Assignee | ||
Comment 7•12 years ago
|
||
The file associated to the first item has been removed from disk.
Attachment #738977 -
Flags: ui-review?(shorlander)
Assignee | ||
Updated•12 years ago
|
Attachment #724859 -
Flags: ui-review?(shorlander)
Comment 8•12 years ago
|
||
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•12 years ago
|
||
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
Attachment #738977 -
Flags: ui-review?(shorlander) → ui-review+
Assignee | ||
Comment 10•12 years ago
|
||
Comment 11•12 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 23
You need to log in
before you can comment on or make changes to this bug.
Description
•