817006 - "Remove from list" option in the panel is confusing

Status: VERIFIED FIXED

(Firefox :: Downloads Panel, defect)

Description

[Marco Bonardo [:mak]]

In the panel we have "Remove from list", that basically means "I've handled this notification and don't care to see it anymore".

Unfortunately what the user interprets there is "Remove this download from my browser downloads list", that is not what happens, this is a notification panel, not a manager, so the download STAYS in the Library downloads view.

2 possibilities I see:

1. change the label to "Remove notification"

2. since the panel is reduced to just the last N items, don't provide any "remove" option, if the user wants to get rid of it will do from the manager

Comment 1

[Marco Bonardo [:mak]]

Gentle UX-ping, we are still waiting for an interaction analysis.

Comment 2

[u428464]

Answer in this etherpad I think : https://firefox-ux.etherpad.mozilla.org/remaining-download-panel-ux

"Remove from list" option in the panel is confusing

    https://bugzilla.mozilla.org/show_bug.cgi?id=817006

    Remove from List = Remove from History also"

Comment 3

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

Attachment: Remove from List -> Remove from History

So, here's one solution - we change the label to make it clearer to the user that removing the item removes the download from the downloads history.

Comment 4

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

Another solution is to keep "Remove from List", but to simply not have the download be removed from the history.

I'm investigating this solution - but my instinct is that we might expose bugs if these two lists get out of sync...

Comment 5

[Marco Bonardo [:mak]]

Notice the atm we do NOT remove the download from history...
The other solution would be "Dismiss this notification", but I think we want this to also remove the download from history, so your first solution is fine provided you also fix the functionality.

Comment 6

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

Attachment: Patch v1

Switches the string from "Remove from List" to "Remove from History", and updates DownloadDataItem's remove method to also remove itself from the browsing history.

Comment 7

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

Comment on attachment 691892 [details] [diff] [review]
Patch v1

The relationship between the downloads database and the browsing history is a little unclear to me...but this patch appears to do the job.

Removing a download from the panel now removes it from the Places view.

Comment 8

[Stephen Horlander [:shorlander]]

Talked to Mike and settled on the current approach.

Comment 9

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

Attachment: Patch v2

As discussed in IRC, we're opting to remove the download from the browsing history in downloads.js (as opposed to within DownloadDataItem's remove method).

Comment 10

[Marco Bonardo [:mak]]

Comment on attachment 691902 [details] [diff] [review]
Patch v2

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

::: browser/components/downloads/content/downloads.js
@@ +48,5 @@
>                                    "resource://gre/modules/PrivateBrowsingUtils.jsm");
> +XPCOMUtils.defineLazyModuleGetter(this, "PlacesUtils",
> +                                  "resource://gre/modules/PlacesUtils.jsm");
> +XPCOMUtils.defineLazyModuleGetter(this, "NetUtil",
> +                                  "resource://gre/modules/NetUtil.jsm");

since you are importing this, in _onKeyDown there is a Services.io.newURI(url, null, null); that could be replaced

Comment 11

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

Attachment: Patch v3 (r+'d by mak)

Fixed! Thanks Marco!

Comment 12

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

Landed on mozilla-inbound as https://hg.mozilla.org/integration/mozilla-inbound/rev/0852e5efadac

Comment 13

[Boris Zbarsky [:bzbarsky]]

Unfortunately, I had to back this out when I was backing out the patch for bug 675902, because this one seems to depend on that one (or at least I got merge conflicts from Mercurial).  Backout changeset: https://hg.mozilla.org/integration/mozilla-inbound/rev/f2d9eaf13c19

Comment 14

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

Serves me right for landing something on inbound that relied on something that hadn't been merged into central yet. Thanks for your work.

Comment 15

[Marco Bonardo [:mak]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/048673b6b48b

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/048673b6b48b

Comment 17

[Simona Badau, Desktop QA]

Verified as fixed on the latest Nighlty - "Remove from list" option in the panel is now replaced with "Remove From History" (and after selecting the option to "Remove from History" the downloaded item is removed from the Panel and from Library -> Downloads). 


Verified on Windows 7, Ubuntu 12.10 and Mac OS X 10.7.5:
Mozilla/5.0 (Windows NT 6.1; rv:20.0) Gecko/20121217 Firefox/20.0 Build ID: 20121217030850
Mozilla/5.0 (X11; Linux i686; rv:20.0) Gecko/20121218 Firefox/20.0 Build ID: 20121218030803
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.7; rv:20.0) Gecko/20121218 Firefox/20.0 Build ID: 20121218030803