1265341 - The icons for blocked downloads should be a document with an exclamation or block badge

Status: VERIFIED FIXED

(Firefox :: Downloads Panel, defect, P3)

Description

[:Paolo Amadini]

The icons for blocked downloads should display a standard document icon with a small badge showing an exclamation or a block symbol.

Comment 1

[Jonathan Kingston [:jkt] he/him]

Attachment: MozReview Request: Bug 1265341: Add document icon for blocked downloads with an overlay information and warning symbol

Review commit: https://reviewboard.mozilla.org/r/49017/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/49017/

Comment 2

[Jonathan Kingston [:jkt] he/him]

Comment on attachment 8745370 [details]
MozReview Request: Bug 1265341: Add document icon for blocked downloads with an overlay information and warning symbol

I showed the icons as they actually displayed the correct icon here. So I used stack to overlay the icons.

Comment 3

[:Paolo Amadini]

So we are able to keep the right in-progress icon after all! I'll test this later today or tomorrow.

The patch looks good so far! I think we eventually want to use the "warning icon" and "block badge" linked from the document in the "URL" field of this bug, so we'll have two types of badges instead of three, as the information icon will map to the exclamation mark. You can work on integrating them in the meantime.

Comment 4

[Jonathan Kingston [:jkt] he/him]

Attachment: MozReview Request: Bug 1265341: Adding new svg icons and change overlay position of blocked downloads

Review commit: https://reviewboard.mozilla.org/r/49223/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/49223/

Comment 5

[:Paolo Amadini]

Testing this on various platforms is taking some time, my first impression is that it changes the size of the download items or the icons, which I don't think is something we actually want as part of this bug. I have to rebuild to be sure as I may have applied the patch incorrectly.

Unfortunately this review may be delayed a bit, but we're not making other changes to this area of the code so it should be able to land without conflicts once the review is done.

Comment 6

[Jonathan Kingston [:jkt] he/him]

Hey thanks!
I'm not really sure we can decrease the size of the items themselves but I can reduce the margin perhaps to compensate.
I'm getting a Mac to test with by next week so I can fully ensure there isn't any issues on Mac and Windows.

Comment 7

[:Paolo Amadini]

This will take some time to review as there is higher priority work, but I'd still like this improvement to land eventually. Can you fold the two changesets into one so it's easier for me to apply locally and test?

If you now have Mac OS X to test with, please check that the item sizes and file type icon sizes are still the same as without patch applied.

Comment 8

[:Paolo Amadini]

Comment on attachment 8745370 [details]
MozReview Request: Bug 1265341: Add document icon for blocked downloads with an overlay information and warning symbol

Clearing request until the new version is posted.

Comment 9

[Jonathan Kingston [:jkt] he/him]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=9df50ef64600

Comment 10

[Jonathan Kingston [:jkt] he/him]

Attachment: Bug 1265341 - changing blocked download icons to be smaller overlays

Review commit: https://reviewboard.mozilla.org/r/53902/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/53902/

Comment 11

[Jonathan Kingston [:jkt] he/him]

Comment on attachment 8754313 [details]
Bug 1265341 - changing blocked download icons to be smaller overlays

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/53902/diff/1-2/

Comment 12

[Jonathan Kingston [:jkt] he/him]

Attachment: Demonstrating mac icon looks the same pre and post patch

The patch show there is no size difference on osx.

Comment 13

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

Comment on attachment 8754313 [details]
Bug 1265341 - changing blocked download icons to be smaller overlays

https://reviewboard.mozilla.org/r/53902/#review53322

Sorry for not reviewing this sooner. Holding back r+ due to the variable issues described below.

::: browser/components/downloads/content/download.xml:23
(Diff revision 2)
>                   validate="always"
>                   xbl:inherits="src=image"/>

Please line up the indent for these two lines too.

::: browser/components/downloads/content/downloads.css
(Diff revision 2)
> -
>  .download-state:not(:-moz-any([state="6"], /* Blocked (parental) */
>                                [state="8"], /* Blocked (dirty)    */
>                                [state="9"]) /* Blocked (policy)   */)
>                                             .downloadTypeIcon.blockedIcon,
> -

Can you put this blank line back?

::: browser/themes/shared/download-blocked.svg:10
(Diff revision 2)
> +<svg xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink" width="16" height="16" viewBox="0 0 16 16">
> +  <style>
> +    circle {
> +      fill: #D92215;
> +    }
> +    

nit, trailing whitespace

::: browser/themes/shared/downloads/allDownloadsViewOverlay.inc.css:42
(Diff revision 2)
> -  width: 32px;
> -  height: 32px;
> +  width: calc(--icon-size + --inline-offset);
> +  height: calc(--icon-size + --block-offset);

How does this work? I just tried to test this and I needed to wrap the variable references inside of the calc() with their own var() function call, like so:
calc(var(--icon-size) + var(--inline-offset));

::: browser/themes/shared/downloads/allDownloadsViewOverlay.inc.css:56
(Diff revision 2)
> -  list-style-image: url("chrome://global/skin/icons/Error.png");
> +  --overlay-image-dimentions: bottom right / 16px no-repeat;
> +  background: url("chrome://global/skin/icons/Error.png") var(--overlay-image-dimentions);
> +  --overlay-image-dimentions: top right / 16px no-repeat;

I don't think we should be redefining a varaible twice within the same ruleset. This gets confusing and can break easily if someone rearranges or removes what they think are duplicate rules.

Comment 14

[Jonathan Kingston [:jkt] he/him]

Comment on attachment 8754313 [details]
Bug 1265341 - changing blocked download icons to be smaller overlays

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/53902/diff/2-3/

Comment 15

[Jonathan Kingston [:jkt] he/him]

Comment on attachment 8754313 [details]
Bug 1265341 - changing blocked download icons to be smaller overlays

Hey Jared, This should address your previous comments. Let me know if you need more info.

Comment 16

[Jonathan Kingston [:jkt] he/him]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=78d08523abdf

Comment 17

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

Comment on attachment 8754313 [details]
Bug 1265341 - changing blocked download icons to be smaller overlays

https://reviewboard.mozilla.org/r/53902/#review53824

::: browser/themes/shared/downloads/downloads.inc.css:104
(Diff revision 3)
> -  height: 32px;
> -  width: 32px;
> +  width: calc(--icon-size + --inline-offset);
> +  height: calc(--icon-size + --block-offset);

These duplicated lines still need to be fixed in this file.

::: browser/themes/shared/downloads/downloads.inc.css:110
(Diff revision 3)
> -  list-style-image: url("chrome://global/skin/icons/Error.png");
> +  --overlay-image-dimentions: top right / 16px no-repeat;
> +  background: url("chrome://global/skin/icons/Error.png") var(--overlay-image-dimentions);
> +  padding: 0;
> +  background: url("chrome://browser/skin/download-blocked.svg") var(--overlay-image-dimentions);

Ditto.

Comment 18

[Jonathan Kingston [:jkt] he/him]

Comment on attachment 8754313 [details]
Bug 1265341 - changing blocked download icons to be smaller overlays

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/53902/diff/3-4/

Comment 19

[Jonathan Kingston [:jkt] he/him]

Comment on attachment 8754313 [details]
Bug 1265341 - changing blocked download icons to be smaller overlays

Sorry for the back and forwards with this, I should check code submissions when half asleep. The use of the vars is pretty subtle and some editor transform removed them I think.

Again sorry, should be good now.

Comment 20

[:Paolo Amadini]

https://reviewboard.mozilla.org/r/53902/#review54050

::: browser/themes/shared/downloads/allDownloadsViewOverlay.inc.css:58
(Diff revision 4)
>  %endif
>  
>  .blockedIcon {
> -  list-style-image: url("chrome://global/skin/icons/Error.png");
> +  --overlay-image-dimentions: top right / 16px no-repeat;
> +  padding: 0;
> +  background: url("chrome://browser/skin/download-blocked.svg") var(--overlay-image-dimentions);

Sorry for not noting this earlier, please use the spelling "dimensions" because it's what we commonly use in the source tree and a different one might create confusion when typing instead of copy and pasting.

Comment 21

[Jonathan Kingston [:jkt] he/him]

Comment on attachment 8754313 [details]
Bug 1265341 - changing blocked download icons to be smaller overlays

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/53902/diff/4-5/

Comment 22

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

Comment on attachment 8754313 [details]
Bug 1265341 - changing blocked download icons to be smaller overlays

https://reviewboard.mozilla.org/r/53902/#review54842

Comment 23

[Jonathan Kingston [:jkt] he/him]

Thanks :jaws!

Comment 24

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/fx-team/rev/adaef7e67064
changing blocked download icons to be smaller overlays. r=jaws

Comment 25

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/adaef7e67064

Comment 26

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/adaef7e67064

Comment 27

[:Paolo Amadini]

Comment on attachment 8754313 [details]
Bug 1265341 - changing blocked download icons to be smaller overlays

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #13)
> ::: browser/themes/shared/downloads/allDownloadsViewOverlay.inc.css:42
> (Diff revision 2)
> > -  width: 32px;
> > -  height: 32px;
> > +  width: calc(--icon-size + --inline-offset);
> > +  height: calc(--icon-size + --block-offset);

That's probably why the item size changed when I tested the patch.

Thanks Jonathan for the fix and Jared for the review!

Clearing my review flag.

Comment 28

[Sean Lee [:seanlee][:weilonge]]

Attachment: Downlaods Icon Size Changed

Hello Jaw, Jonathan,

The attachment is an screenshot for showing a bigger .downloadTypeIcon .
After reverting the codes mentioned by comment 27, the smaller icon is shown.
Is this a regression or new design? Thanks.

Comment 29

[Sean Lee [:seanlee][:weilonge]]

It can be reproduced in MacOSX but Ubuntu.

Comment 30

[Jonathan Kingston [:jkt] he/him]

Seems like a different issue, reverting doesn't seem necessary. I raised Bug 1287384 to cover this.

Thanks!

Comment 31

[Ciprian Georgiu, Desktop QA]

I've managed to reproduce this issue on Firefox 48.0.2 (2016-08-23).

This is verified fixed on 50.0b6 (2016-10-10) on the following OSes:
- Windwos 10 x64
- Ubuntu 16.04 x64 LTS
- Mac OS X 10.11 (El Capitan)