Closed Bug 1198181 Opened 4 years ago Closed 4 years ago

Update item structure to display downloads that can be unblocked

Categories

(Firefox :: Downloads Panel, defect, P1)

defect

Tracking

()

VERIFIED FIXED
Firefox 48
Iteration:
48.3 - Apr 25
Tracking Status
firefox43 --- affected
firefox48 --- verified
relnote-firefox --- -

People

(Reporter: Paolo, Assigned: Paolo)

References

(Blocks 4 open bugs)

Details

(Whiteboard: [fxprivacy])

Attachments

(5 files)

This is the actual implementation of the design in bug 1053890.
Flags: qe-verify+
Priority: -- → P3
Paolo, this was nominated for a relnote (in bug 1053890, earlier this year).  I'd like to add a release note for 43 here. Can you suggest wording for it? Is there anything logical to link to from the note? Thanks.
Flags: needinfo?(paolo.mozmail)
Release Note Request (optional, but appreciated)
[Why is this notable]:  New user-facing feature 
[Suggested wording]: Downloads panel allows items to be unblocked
[Links (documentation, blog post, etc)]:

Here's my placeholder -- please do improve it.
We haven't started working on this bug yet, though it's on the team's radar. I think we can wait until we've implemented it, since the details of what we'll do may vary.
Flags: needinfo?(paolo.mozmail)
Oh. right. The other bug said "fixed" and I didn't double check the status. Removing over-hasty release note!
Priority: P3 → P2
Whiteboard: [fxprivacy] → [fxprivacy][blocked]
Depends on: 1241177
Priority: P2 → P3
Whiteboard: [fxprivacy][blocked] → [fxprivacy]
Priority: P3 → P1
QA Contact: paul.silaghi
Assignee: nobody → paolo.mozmail
Status: NEW → ASSIGNED
Iteration: --- → 48.2 - Apr 4
Iteration: 48.2 - Apr 4 → 48.3 - Apr 18
Blocks: 1265335
Blocks: 1265341
Blocks: 1265358
Blocks: 1265359
Blocks: 1265362
Blocks: 1265363
Blocks: 1265384
Blocks: 1265387
Blocks: 1265391
Comment on attachment 8741817 [details]
MozReview Request: Bug 1198181 - Part 1 - Separate functional and styling classes for download item buttons. r=jaws

https://reviewboard.mozilla.org/r/46777/#review43737

This is a tentative r+. I looked through the rest of the patches in this set and I don't see why these changes really need to be made. Can you explain why you had to introduce all these new classes?

::: browser/components/downloads/content/download.xml:63
(Diff revision 1)
>                      tooltiptext="&cmd.showMac.label;"
>  #else
>                      tooltiptext="&cmd.show.label;"
>  #endif
>                      oncommand="DownloadsView.onDownloadCommand(event, 'downloadsCmd_show');"/>
> -        <xul:button class="downloadButton downloadConfirmBlock"
> +        <xul:button class="downloadButton downloadConfirmBlock downloadIconCancel"

This should introduce a downloadIconConfirmBlock class, and then that class can share the same ruleset as downloadIconCancel.
Attachment #8741817 - Flags: review?(jaws) → review+
Comment on attachment 8741818 [details]
MozReview Request: Bug 1198181 - Part 2 - Unify the code that updates the state of download items. r=jaws

https://reviewboard.mozilla.org/r/46779/#review43753
Attachment #8741818 - Flags: review?(jaws) → review+
(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #10)
> This is a tentative r+. I looked through the rest of the patches in this set
> and I don't see why these changes really need to be made. Can you explain
> why you had to introduce all these new classes?

So, currently the classes for the buttons are conflating two different uses.

The first one is determining if the button should be visible, meaning that the associated action can be executed. There is a XUL entry for each button, but only one of them at a time can be visible, and this is currently controlled by functional CSS in the "components/downloads/" folder.

The second one is choosing which icon to use for the button. Some actions share the same icon, for example "cancel" and "remove file", and now we're adding "show" and "confirm unblock".

I was initially going through the dozens of lines I had to update in the theme CSS to reuse the rules for the new action, but then realized that splitting the classes allowed for a cleaner solution. I know that if we had a class linked to the button function, then heavyweight themes may get ahead of us and style the actions differently, but I don't think this is a strong need and I'd prefer to use the solution that gives us the higher maintainability and readability.

I think that ideally we should just have one XUL element for the action button, updated and styled dynamically based on the state of the download, that triggers a command whose effect is determined by the state of the download, similarly to what we do for the return key. In other words the functional classes would eventually be removed, leaving only the styling classes.

Note that the UX team has a redesign of the Downloads Panel in progress, currently represented by the placeholder bug 1265337. If we get that done, we'll probably simplify most of the CSS and we'll be able to use SVG icons, at which point what we choose here will make little difference.
Attachment #8741820 - Flags: review?(past) → review+
Comment on attachment 8741820 [details]
MozReview Request: Bug 1198181 - Part 4 - Differentiate the icons for blocked downloads depending on the verdict. r=past

https://reviewboard.mozilla.org/r/46783/#review44117

::: browser/components/downloads/DownloadsViewUI.jsm:245
(Diff revision 1)
> +          default: // Assume Downloads.Error.BLOCK_VERDICT_MALWARE
> +            stateLabel = s.blockedMalware;

I've treated the default case differently in my downloads-button-badge patch, but I've had a separate attention state that I could use and I don't know if that's the case here. I guess the bottom line is whether we anticipate a future new verdict will concern a more serious or a less serious threat. We should at least be consistent in our approaches, so I'm not raising an issue for this as I'm perfectly happy to change mine after we discuss it.

::: browser/themes/linux/downloads/allDownloadsViewOverlay.css:18
(Diff revision 1)
>  .blockedIcon {
>    list-style-image: url("moz-icon://stock/gtk-dialog-error?size=dialog");
>  }
>  
> +@item@[verdict="PotentiallyUnwanted"] .blockedIcon {
> +  list-style-image: url("moz-icon://stock/gtk-dialog-warning?size=dialog");

Why not size=32 here and below?
(In reply to Panos Astithas [:past] from comment #13)
> I guess the bottom line is whether we
> anticipate a future new verdict will concern a more serious or a less
> serious threat.

Yeah, that's the approach that is followed in all the other code and in the back-end. If a new verdict is added and no action is taken, the default should be to treat it as the most severe threat. I wanted to add a comment to that effect while looking at your patch earlier.

> Why not size=32 here and below?

Good catch! There was still the broken icon in the Library window on Linux, that I forgot to test.

I look forward to the unification of these stylesheets with the panel redesign.
Comment on attachment 8741821 [details]
MozReview Request: Bug 1198181 - Part 5 - Change the unblock dialog actions based on the verdict. r=past

https://reviewboard.mozilla.org/r/46785/#review44127

::: browser/components/downloads/DownloadsCommon.jsm:531
(Diff revision 1)
>     * @return True to unblock the file, false to keep the user safe and
>     *         cancel the operation.

This comment needs updating with the new return values (and it should have also been more clear about the fact that the return value is a promise).
Attachment #8741821 - Flags: review?(past) → review+
Comment on attachment 8741819 [details]
MozReview Request: Bug 1198181 - Part 3 - Showing the unblock dialog should be the visible action for verdicts other than malware. r=past

https://reviewboard.mozilla.org/r/46781/#review44141

Looks good in my testing on OS X, but don't we ned the same CSS changes for Windows?
Attachment #8741819 - Flags: review?(past) → review+
(In reply to Panos Astithas [:past] from comment #16)
> Looks good in my testing on OS X, but don't we ned the same CSS changes for
> Windows?

I've checked again that Windows already has the styles we need.

I'll land this bug now because it is blocking several others, if there are other changes we need to make for the final review we can do them in follow-ups.
This issue is verified fixed for:
Windows 10 x64 using
- 49.0a1 (2016-04-27)
- 48.0a2 (2016-04-28)
Ubuntu 12.04 x32 using
- 49.0a1 (2016-04-27)
- 48.0a2 (2016-04-27)
Mac OS X 10.10.5 using
- 49.0a1 (2016-04-27)
- 48.0a2 (2016-04-27)
Status: RESOLVED → VERIFIED
Too late for 48
You need to log in before you can comment on or make changes to this bug.