1137909 - Implement minimum requirements to allow unblocking of downloads through the downloads UI

Status: RESOLVED FIXED

(Firefox :: Downloads Panel, defect)

Description

[Jared Wein [:jaws] (please needinfo? me)]

Bug 1053890 outlines the desired UX for blocked downloads and unblocking of them. Some of the requests will require much more work than others. This bug outlines the minimum required to satisfy this functionality.

1. Update the richlistitem status lines and icon to show that the download has been blocked and why.
2. Add a context menu item to unblock, leaving the richlistitem showing the now unblocked download as a regular download.
3. Re-use the X button (which normally is used to cancel download) to also be used for "Remove file"

Walk-through of the user experience:
1. User clicks a link to download a file
2. When the download completes, the remote-lookup tags this as a potentially unsafe download.
3. The user notices that the download has finished by seeing the normal "download finished" animation
4. The user clicks on the Downloads button in the toolbar to see the downloads panel.
5. The user sees that this is a blocked download.
6a. To remove the download, the user clicks on the X at the end of the download item. The download is removed from the panel. [END]
6b. To unblock the download, the user right-clicks on the download item and chooses "Unblock download"
7. A dialog appears asking the user to confirm unblocking the dialog.
8a. The user confirms the unblock and the downloads panel now shows the download as a regular, safe download. The user can now open the downloaded file by clicking on the download item or using the right-click context menu.
8b. The user cancels the unblock and the download stays "blocked". [END]

Notes for assignee:
- DownloadsCommon::confirmUnblockDownload is already in the tree and implements the confirmation dialog.
- Strings for the status line may already be checked-in and available through DownloadsCommon.strings
- Check the download.hasBlockedData property to see if the download has been blocked.
- Much of the UI code lives in DownloadsViewUI.jsm
- Only "blockedMalware" can be used at this time. The implementations for "blockedPotentiallyUnwanted" and "blockedUncommon" are not complete enough at this time.

Comment 1

[:Paolo Amadini]

Attachment: Mockup

Comment 2

[[:mmc] Monica Chew (no longer reading bugmail)]

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #0)
> - Only "blockedMalware" can be used at this time. The implementations for
> "blockedPotentiallyUnwanted" and "blockedUncommon" are not complete enough
> at this time.

The platform changes for these are trivial but do require passing that state to the downloads manager rather than a simple boolean blocked vs. not-blocked.

Comment 3

[[:mmc] Monica Chew (no longer reading bugmail)]

Gavin, are you willing to accept this alternative UX as a replacement for https://bugzilla.mozilla.org/show_bug.cgi?id=1068656?

Thanks,
Monica

Comment 4

[Jared Wein [:jaws] (please needinfo? me)]

Yes, I will be working on implementing it this week.

Comment 5

[(Currently slow to respond) Philipp Sackl [:phlsa] (Firefox UX) please use needinfo]

As discussed via email, I'm generally fine with that approach.

The one addition we'd need though is the red download indicator when a download is blocked. Otherwise, it is hard to find out that something went wrong with the download.

Also, Jared, would it be possible to use a trash can icon instead of the x if we supplied the asset?

Comment 6

[:Paolo Amadini]

(In reply to Philipp Sackl [:phlsa] please use needinfo to make me respond from comment #5)
> The one addition we'd need though is the red download indicator when a
> download is blocked. Otherwise, it is hard to find out that something went
> wrong with the download.

We do show a prominent red "X" icon near the download item though, so it's pretty obvious the download hasn't completed. The green indicator is actually more of a problem with downloads that failed but not because they were blocked, because we don't have the icon anyways.

The technical work to detect transitions to failure states is definitely orthogonal and not trivial, so it should be done in a separate bug anyways, but because of the above I don't think the other bug should block this one.

> Also, Jared, would it be possible to use a trash can icon instead of the x
> if we supplied the asset?

This may not be too difficult to implement, you just need to evaluate the cost of providing the asset for all platforms and testing it, versus just waiting for the final design that we're implementing anyways.

Comment 7

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Part 1 - Blocked downloads should keep data on Windows and Unix

Comment 8

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Part 2 - Context menuitem to unblock downloads in the downloads panel

Comment 9

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Part 3 - Add an X button to confirm the block in the downloads panel

Comment 10

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Part 4 - Context menuitem to unblock downloads in the library and about:downloads

Comment 11

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Part 5 - Add an X button to confirm the block in the library and about:downloads

Comment 12

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Part 5 - Add an X button to confirm the block in the library and about:downloads

Comment 13

[:Paolo Amadini]

Comment on attachment 8572227 [details] [diff] [review]
Part 1 - Blocked downloads should keep data on Windows and Unix

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

::: toolkit/components/jsdownloads/src/DownloadIntegration.jsm
@@ +197,5 @@
>      }
>  
> +#if defined(XP_WIN) || defined(XP_UNIX)
> +    return true;
> +#endif

Unfortunately, I'm not an expert with our preprocessing macros, but basically we need this to return true only for Firefox Desktop, but not for example Android, B2G, or Thunderbird. I believe this would return true on Android too. Here's some code using various preprocessor macros for inspiration:

http://mxr.mozilla.org/mozilla-central/source/toolkit/components/jsdownloads/src/DownloadIntegration.jsm#362

I don't exclude we could also do this with an nsIXULAppInfo check if it's simpler.

As a side note, this would be simpler once bug 899013 is done.

mini-nit: I'd use #else

Comment 14

[:Paolo Amadini]

Comment on attachment 8572229 [details] [diff] [review]
Part 2 - Context menuitem to unblock downloads in the downloads panel

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

I assume the CSS to hide the menu item in the panel for non-blocked downloads is in another patch?

::: browser/components/downloads/content/downloads.js
@@ +1166,5 @@
>        case "downloadsCmd_copyLocation":
>        case "downloadsCmd_doDefault":
>          return true;
> +      case "downloadsCmd_unblock":
> +        return this.download.hasBlockedData;

I assumed what we wanted to do was to show/hide the menu item purely based on hasBlockedData.

For downloads blocked permanently (legacy states 6, 8, and 9) the option to unblock should not be available. I thought having the option hidden (keeping the current interface) would be clearer, but now I'm not sure if having the option visible but grayed out may be better. One thing to take into account is we don't explain why the download can't be unblocked anywhere.

@@ +1203,5 @@
> +                                             window).then((result) => {
> +        if (result) {
> +          this.download.unblock().catch(Cu.reportError);
> +        }
> +      });

Move .catch to the outer promise and return this.download.unblock(). It's fine to not return anything when result if false.

nit: rename result -> confirmed ?

May also express this as ".then(confirmed => confirmed && this.download.unblock())" but may be less clear.

Comment 15

[:Paolo Amadini]

Comment on attachment 8572230 [details] [diff] [review]
Part 3 - Add an X button to confirm the block in the downloads panel

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

::: browser/components/downloads/content/downloads.css
@@ +71,5 @@
>                                [state="4"]) /* Paused             */)
>                                             .downloadCancel,
>  
> +.download-state:not(          [state="8"]  /* Blocked (dirty)    */)
> +                                           .downloadConfirmBlock,

That's not enough, as the button should only be visible when hasBlockedData is true, but not for permanently blocked downloads.

Maybe we need a new attribute on the download item, or we have to control visibility from code.

::: browser/components/downloads/content/downloads.js
@@ +1211,5 @@
> +    downloadsCmd_confirmBlock() {
> +      this.download.confirmBlock().catch(Cu.reportError);
> +      DownloadsCommon.removeAndFinalizeDownload(this.download);
> +      PlacesUtils.bhistory.removePage(
> +                             NetUtil.newURI(this.download.source.url));

The effect of this button should be to turn the item into a permanently blocked download, rather than remove it completely. Not providing a main UI to remove unsuccessful items from the panel is an explicit design decision.

This will be handy for testing the permanently blocked state as well.

Comment 16

[:Paolo Amadini]

Comment on attachment 8572234 [details] [diff] [review]
Part 5 - Add an X button to confirm the block in the library and about:downloads

By the way, thanks for all this laborious CSS work.

Comment 17

[:Paolo Amadini]

Hm, so what happens when we remove a download with blocked data from history, or clear all downloads?

We should ensure we leave no untracked ".part" files behind.

Comment 18

[(Currently slow to respond) Philipp Sackl [:phlsa] (Firefox UX) please use needinfo]

(In reply to :Paolo Amadini from comment #6)
> (In reply to Philipp Sackl [:phlsa] please use needinfo to make me respond
> from comment #5)
> > The one addition we'd need though is the red download indicator when a
> > download is blocked. Otherwise, it is hard to find out that something went
> > wrong with the download.
> 
> We do show a prominent red "X" icon near the download item though, so it's
> pretty obvious the download hasn't completed. The green indicator is
> actually more of a problem with downloads that failed but not because they
> were blocked, because we don't have the icon anyways.
Yes, but that assumes that the user accesses downloads through that menu. We provide no indication that anything went wrong until the user manually checks.

> The technical work to detect transitions to failure states is definitely
> orthogonal and not trivial, so it should be done in a separate bug anyways,
> but because of the above I don't think the other bug should block this one.
Do we have a bug for this?

> > Also, Jared, would it be possible to use a trash can icon instead of the x
> > if we supplied the asset?
> 
> This may not be too difficult to implement, you just need to evaluate the
> cost of providing the asset for all platforms and testing it, versus just
> waiting for the final design that we're implementing anyways.
I just asked Sevaan about the trashcan icon. We have all the templates, so it shouldn't be too hard to create the assets.

Comment 19

[:Paolo Amadini]

(In reply to Philipp Sackl [:phlsa] please use needinfo to make me respond from comment #18)
> > The technical work to detect transitions to failure states is definitely
> > orthogonal and not trivial, so it should be done in a separate bug anyways,
> > but because of the above I don't think the other bug should block this one.
> Do we have a bug for this?

Filed bug 1139472.

Comment 20

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Part 1 - Blocked downloads should keep data on Windows and Unix (v2)

Comment 21

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Part 2 - Context menuitem to unblock downloads in the downloads panel (v2)

Comment 22

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Part 3 - Add an X button to confirm the block in the downloads panel (v2)

Comment 23

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Part 4 - Context menuitem to unblock downloads in the library and about:downloads (v2)

Comment 24

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Part 5 - Add an X button to confirm the block in the library and about:downloads (v2)

Comment 25

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Part 6 - Fix browser_confirm_unblock_download.js to use correct enum values

Comment 26

[Jared Wein [:jaws] (please needinfo? me)]

I confirmed that "remove" removes the .part file.

I did notice a weird issue where the library context menu alternates between a disabled unblock and no unblock upon restart of the browser with a blocked download in the list. Debugging this issue doesn't explain why download.hasBlockedData alternates between true/false in the function call to classList.toggle. Inspecting the object in the debugger shows that this property doesn't exist for historyDownloads.

Comment 28

[:Paolo Amadini]

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #26)
> I did notice a weird issue where the library context menu alternates between
> a disabled unblock and no unblock upon restart of the browser with a blocked
> download in the list. Debugging this issue doesn't explain why
> download.hasBlockedData alternates between true/false in the function call
> to classList.toggle. Inspecting the object in the debugger shows that this
> property doesn't exist for historyDownloads.

I was actually surprised by the definition of the second argument to "toggle". Why not define a separate method on classList that always takes the second argument?

Comment 29

[:Paolo Amadini]

Comment on attachment 8572887 [details] [diff] [review]
Part 2 - Context menuitem to unblock downloads in the downloads panel (v2)

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

::: browser/components/downloads/content/downloads.js
@@ +1024,5 @@
>      this.element.setAttribute("image", this.image);
>      this.element.setAttribute("state",
>                                DownloadsCommon.stateOfDownload(this.download));
> +    this.element.classList.toggle("temporary-block",
> +                                  this.download.hasBlockedData);

Since this is the method we have, it's better to write "!!this.download.hasBlockedData" explicitly, I believe.

Comment 30

[:Paolo Amadini]

Comment on attachment 8572887 [details] [diff] [review]
Part 2 - Context menuitem to unblock downloads in the downloads panel (v2)

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

::: browser/components/downloads/content/downloads.js
@@ +1203,5 @@
>      },
>  
> +    downloadsCmd_unblock() {
> +      DownloadsCommon.confirmUnblockDownload(DownloadsCommon.BLOCK_VERDICT_MALWARE,
> +                                             window).then((confirmed) => {

I believe we should call "DownloadsPanel.hidePanel();" just before confirmUnblockDownload.

At least on Mac OS X, it gives a better result.

Comment 31

[:Paolo Amadini]

Comment on attachment 8572889 [details] [diff] [review]
Part 4 - Context menuitem to unblock downloads in the library and about:downloads (v2)

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

::: browser/components/downloads/content/allDownloadsViewOverlay.js
@@ +292,5 @@
>      this.element.setAttribute("image", this.image);
>      this.element.setAttribute("state",
>                                DownloadsCommon.stateOfDownload(this.download));
> +    this.element.classList.toggle("temporary-block",
> +                                  this.download.hasBlockedData);

Same here.

@@ +1287,5 @@
>      let download = element._shell.download;
>      contextMenu.setAttribute("state",
>                               DownloadsCommon.stateOfDownload(download));
> +    contextMenu.classList.toggle("temporary-block",
> +                                 download.hasBlockedData);

And here.

Comment 32

[:Paolo Amadini]

(In reply to :Paolo Amadini from comment #6)
> (In reply to Philipp Sackl [:phlsa] please use needinfo to make me respond
> from comment #5)
> > Also, Jared, would it be possible to use a trash can icon instead of the x
> > if we supplied the asset?
> 
> This may not be too difficult to implement, you just need to evaluate the
> cost of providing the asset for all platforms and testing it, versus just
> waiting for the final design that we're implementing anyways.

Speaking of cost, a quick look at attachment 8572890 [details] [diff] [review] can be enlightening.

We have 32 versions of the X icon. Thirty-two.

Comment 33

[:Paolo Amadini]

Jared, did you observe bug 1139913 and bug 1139914 in your tests as well?

These are likely issues in the back-end.

Comment 34

[Jared Wein [:jaws] (please needinfo? me)]

(In reply to :Paolo Amadini from comment #33)
> Jared, did you observe bug 1139913 and bug 1139914 in your tests as well?
> 
> These are likely issues in the back-end.

Yes, I noticed both of these issues. Bug 1139914 explains the second half of my comment #26.

Comment 35

[Jared Wein [:jaws] (please needinfo? me)]

(In reply to :Paolo Amadini from comment #28)
> I was actually surprised by the definition of the second argument to
> "toggle". Why not define a separate method on classList that always takes
> the second argument?

Good question for those who work in standards :)

Comment 36

[Jared Wein [:jaws] (please needinfo? me)]

(In reply to :Paolo Amadini from comment #29)
> Since this is the method we have, it's better to write
> "!!this.download.hasBlockedData" explicitly, I believe.

This fixed the second half of the issue in comment #26.

Comment 37

[Jared Wein [:jaws] (please needinfo? me)]

(In reply to :Paolo Amadini from comment #29)
> Since this is the method we have, it's better to write
> "!!this.download.hasBlockedData" explicitly, I believe.

This fixed the second issue in comment #26.

Comment 38

[Jared Wein [:jaws] (please needinfo? me)]

https://hg.mozilla.org/integration/fx-team/rev/87a0cdb9e19b
https://hg.mozilla.org/integration/fx-team/rev/8a5db88a76d2
https://hg.mozilla.org/integration/fx-team/rev/8d8f5adb27d5

Comment 39

[Jared Wein [:jaws] (please needinfo? me)]

STR for QA to test this:
http://www.eicar.org/85-0-Download.html and http://testsafebrowsing.appspot.com/ should work intermittently (on chrome as well as firefox) for triggering a blocked download.

Comment 40

[Jared Wein [:jaws] (please needinfo? me)]

(In reply to :Paolo Amadini from comment #32)
> We have 32 versions of the X icon. Thirty-two.

I beg to differ. I counted the X variations within the three download theme folders and reached 36. Thirty-six.

Comment 41

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/87a0cdb9e19b
https://hg.mozilla.org/mozilla-central/rev/8a5db88a76d2
https://hg.mozilla.org/mozilla-central/rev/8d8f5adb27d5

Comment 42

[Jared Wein [:jaws] (please needinfo? me)]

Paolo, will you be able to uplift the download refactorings so this can get uplifted?

Comment 43

[:Paolo Amadini]

Comment on attachment 8572886 [details] [diff] [review]
Part 1 - Blocked downloads should keep data on Windows and Unix (v2)

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #42)
> Paolo, will you be able to uplift the download refactorings so this can get
> uplifted?

The refactorings are already on Aurora so this should be easy to uplift to 38.

Comment 44

[:Paolo Amadini]

Comment on attachment 8572887 [details] [diff] [review]
Part 2 - Context menuitem to unblock downloads in the downloads panel (v2)

Approval Request Comment
[Feature/regressing bug #]: Application Reputation
[User impact if declined]: Remote Application Reputation checks cannot be enabled
[Describe test coverage new/current, TreeHerder]: Requires QA
[Risks and why]: This is a minimal implementation of unblocking downloads, less discoverbale than the final design
[String/UUID change made/needed]: None

Comment 45

[Ryan VanderMeulen [:RyanVM]]

There's six parts attached to this bug (and approved for uplift), but it looks like only 3 ever landed on m-c?

Comment 46

[Jared Wein [:jaws] (please needinfo? me)]

(In reply to Ryan VanderMeulen [:RyanVM UTC-5] from comment #45)
> There's six parts attached to this bug (and approved for uplift), but it
> looks like only 3 ever landed on m-c?

Part 1 remained "Part 1". Parts 2-5 were folded together to become "Part 2". Part 6 remained separate as "Part 3". Sorry for the confusion. The 6 separate parts were to make reviews easier, but they weren't as useful in pieces so it didn't make sense for bi-section purposes to land them separately.

We don't need to uplift the Part 6 (what landed as "part 3") because it is a test-only fix that doesn't affect the outcome of the test.

Comment 47

[Jared Wein [:jaws] (please needinfo? me)]

The patches on this bug are not what should get uplifted though. There were some review comments addressed before landing, and the patches need rebasing for Aurora. I will push these myself tomorrow.

need-info flag set just to give you a heads-up.

Comment 48

[Jared Wein [:jaws] (please needinfo? me)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/3b9fb49515b4
https://hg.mozilla.org/releases/mozilla-aurora/rev/3cd97926186e
https://hg.mozilla.org/releases/mozilla-aurora/rev/f30f71665f72

Comment 49

[Catalin Varga [QA][:VarCat]]

Verified this fix on the following environment:

FF 38
Build id: 20150323004010
OS: Win 7 x64, Ubuntu 14.04 x86, Mac Os X 10.7.5

I encountered the following issues:

- dismissing the unblock related dialog window unblocks the download bug 1146907
- malicious files are successfully downloaded through a download area using the secure, SSL enabled protocol bug 1146911
- also several of the tests from  http://testsafebrowsing.appspot.com/ are failing (but this issues are not exactly related to this fix)