Closed Bug 1137909 Opened 10 years ago Closed 10 years ago

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

Categories

(Firefox :: Downloads Panel, defect)

defect
Not set
normal
Points:
5

Tracking

()

RESOLVED FIXED
Firefox 39
Iteration:
39.1 - 9 Mar
Tracking Status
firefox38 + fixed
firefox39 --- fixed

People

(Reporter: jaws, Assigned: jaws)

References

(Blocks 1 open bug)

Details

Attachments

(7 files, 6 obsolete files)

114.16 KB, image/png
Details
1.18 KB, patch
Paolo
: review+
Details | Diff | Splinter Review
7.98 KB, patch
Paolo
: review+
Details | Diff | Splinter Review
15.84 KB, patch
Paolo
: review+
Details | Diff | Splinter Review
7.88 KB, patch
Paolo
: review+
Details | Diff | Splinter Review
16.09 KB, patch
Paolo
: review+
Details | Diff | Splinter Review
1.50 KB, patch
Paolo
: review+
Details | Diff | Splinter Review
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.
Attached image Mockup
(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.
Gavin, are you willing to accept this alternative UX as a replacement for https://bugzilla.mozilla.org/show_bug.cgi?id=1068656?

Thanks,
Monica
Flags: needinfo?(gavin.sharp)
Yes, I will be working on implementing it this week.
Assignee: nobody → jaws
Status: NEW → ASSIGNED
Flags: needinfo?(gavin.sharp)
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?
Flags: needinfo?(jaws)
(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.
Flags: needinfo?(jaws)
Attachment #8572232 - Attachment is obsolete: true
Attachment #8572232 - Flags: review?(paolo.mozmail)
Attachment #8572234 - Flags: review?(paolo.mozmail)
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
Attachment #8572227 - Flags: review?(paolo.mozmail)
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.
Attachment #8572229 - Flags: review?(paolo.mozmail) → feedback+
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.
Attachment #8572230 - Flags: review?(paolo.mozmail)
Attachment #8572231 - Flags: review?(paolo.mozmail)
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.
Attachment #8572234 - Flags: review?(paolo.mozmail)
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.
(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.
(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.
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.
Attachment #8572886 - Flags: review?(paolo.mozmail) → review+
(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 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.
Attachment #8572887 - Flags: review?(paolo.mozmail) → review+
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.
Attachment #8572888 - Flags: review?(paolo.mozmail) → review+
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.
Attachment #8572889 - Flags: review?(paolo.mozmail) → review+
Attachment #8572890 - Flags: review?(paolo.mozmail) → review+
Attachment #8572891 - Flags: review?(paolo.mozmail) → review+
(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.
Blocks: 1139913
Blocks: 1139914
Jared, did you observe bug 1139913 and bug 1139914 in your tests as well?

These are likely issues in the back-end.
(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.
(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 :)
(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.
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.
(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.
QA Contact: catalin.varga
Paolo, will you be able to uplift the download refactorings so this can get uplifted?
Flags: needinfo?(paolo.mozmail)
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.
Flags: needinfo?(paolo.mozmail)
Attachment #8572886 - Flags: approval-mozilla-aurora?
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
Attachment #8572887 - Flags: approval-mozilla-aurora?
Attachment #8572888 - Flags: approval-mozilla-aurora?
Attachment #8572889 - Flags: approval-mozilla-aurora?
Attachment #8572890 - Flags: approval-mozilla-aurora?
Attachment #8572891 - Flags: approval-mozilla-aurora?
Attachment #8572886 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #8572887 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #8572888 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #8572889 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #8572890 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #8572891 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
There's six parts attached to this bug (and approved for uplift), but it looks like only 3 ever landed on m-c?
Flags: needinfo?(jaws)
(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.
Flags: needinfo?(jaws)
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.
Flags: needinfo?(ryanvm)
Whiteboard: [jaws will do the uplift to aurora]
Flags: needinfo?(ryanvm)
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)
Depends on: 1147845
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: