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

RESOLVED FIXED in Firefox 38

Status

()

RESOLVED FIXED
4 years ago
3 years ago

People

(Reporter: jaws, Assigned: jaws)

Tracking

(Depends on: 1 bug, Blocks: 1 bug)

Trunk
Firefox 39
Points:
5
Dependency tree / graph
Bug Flags:
firefox-backlog +

Firefox Tracking Flags

(firefox38+ fixed, firefox39 fixed)

Details

Attachments

(7 attachments, 6 obsolete attachments)

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.

Comment 1

4 years ago
Created attachment 8570716 [details]
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)

Comment 6

4 years ago
(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.
Created attachment 8572227 [details] [diff] [review]
Part 1 - Blocked downloads should keep data on Windows and Unix
Attachment #8572227 - Flags: review?(paolo.mozmail)
Created attachment 8572229 [details] [diff] [review]
Part 2 - Context menuitem to unblock downloads in the downloads panel
Attachment #8572229 - Flags: review?(paolo.mozmail)
Created attachment 8572230 [details] [diff] [review]
Part 3 - Add an X button to confirm the block in the downloads panel
Attachment #8572230 - Flags: review?(paolo.mozmail)
Created attachment 8572231 [details] [diff] [review]
Part 4 - Context menuitem to unblock downloads in the library and about:downloads
Attachment #8572231 - Flags: review?(paolo.mozmail)
Created attachment 8572232 [details] [diff] [review]
Part 5 - Add an X button to confirm the block in the library and about:downloads
Attachment #8572232 - Flags: review?(paolo.mozmail)
Created attachment 8572234 [details] [diff] [review]
Part 5 - Add an X button to confirm the block in the library and about:downloads
Attachment #8572232 - Attachment is obsolete: true
Attachment #8572232 - Flags: review?(paolo.mozmail)

Comment 13

4 years ago
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 14

4 years ago
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 15

4 years ago
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)

Updated

4 years ago
Attachment #8572231 - Flags: review?(paolo.mozmail)

Comment 16

4 years ago
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)

Comment 17

4 years ago
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.

Comment 19

4 years ago
(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.
Created attachment 8572886 [details] [diff] [review]
Part 1 - Blocked downloads should keep data on Windows and Unix (v2)
Attachment #8572227 - Attachment is obsolete: true
Attachment #8572886 - Flags: review?(paolo.mozmail)
Created attachment 8572887 [details] [diff] [review]
Part 2 - Context menuitem to unblock downloads in the downloads panel (v2)
Attachment #8572229 - Attachment is obsolete: true
Attachment #8572887 - Flags: review?(paolo.mozmail)
Created attachment 8572888 [details] [diff] [review]
Part 3 - Add an X button to confirm the block in the downloads panel (v2)
Attachment #8572230 - Attachment is obsolete: true
Attachment #8572888 - Flags: review?(paolo.mozmail)
Created attachment 8572889 [details] [diff] [review]
Part 4 - Context menuitem to unblock downloads in the library and about:downloads (v2)
Attachment #8572231 - Attachment is obsolete: true
Attachment #8572889 - Flags: review?(paolo.mozmail)
Created attachment 8572890 [details] [diff] [review]
Part 5 - Add an X button to confirm the block in the library and about:downloads (v2)
Attachment #8572234 - Attachment is obsolete: true
Attachment #8572890 - Flags: review?(paolo.mozmail)
Created attachment 8572891 [details] [diff] [review]
Part 6 - Fix browser_confirm_unblock_download.js to use correct enum values
Attachment #8572891 - Flags: review?(paolo.mozmail)
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.

Updated

4 years ago
Duplicate of this bug: 1139564

Updated

4 years ago
Attachment #8572886 - Flags: review?(paolo.mozmail) → review+

Comment 28

4 years ago
(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

4 years ago
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 30

4 years ago
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.

Updated

4 years ago
Attachment #8572888 - Flags: review?(paolo.mozmail) → review+

Comment 31

4 years ago
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+

Updated

4 years ago
Attachment #8572890 - Flags: review?(paolo.mozmail) → review+

Updated

4 years ago
Attachment #8572891 - Flags: review?(paolo.mozmail) → review+

Comment 32

4 years ago
(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.

Updated

4 years ago
Blocks: 1139913

Updated

4 years ago
Blocks: 1139914

Comment 33

4 years ago
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 :)
Comment hidden (obsolete)
(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.
https://hg.mozilla.org/mozilla-central/rev/87a0cdb9e19b
https://hg.mozilla.org/mozilla-central/rev/8a5db88a76d2
https://hg.mozilla.org/mozilla-central/rev/8d8f5adb27d5
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
status-firefox39: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 39
QA Contact: catalin.varga
Paolo, will you be able to uplift the download refactorings so this can get uplifted?
Flags: needinfo?(paolo.mozmail)

Comment 43

4 years ago
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 44

4 years ago
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?

Updated

4 years ago
Attachment #8572888 - Flags: approval-mozilla-aurora?

Updated

4 years ago
Attachment #8572889 - Flags: approval-mozilla-aurora?

Updated

4 years ago
Attachment #8572890 - Flags: approval-mozilla-aurora?

Updated

4 years ago
Attachment #8572891 - Flags: approval-mozilla-aurora?
status-firefox38: --- → affected
tracking-firefox38: --- → +
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)

Updated

4 years ago
Depends on: 1147845

Updated

4 years ago
Duplicate of this bug: 1147285
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.