Closed Bug 1265341 Opened 5 years ago Closed 4 years ago

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

Categories

(Firefox :: Downloads Panel, defect, P3)

defect

Tracking

()

VERIFIED FIXED
Firefox 50
Tracking Status
firefox48 --- affected
firefox50 --- verified

People

(Reporter: Paolo, Assigned: jkt)

References

(Blocks 1 open bug, )

Details

(Whiteboard: [fxprivacy])

Attachments

(3 files, 2 obsolete files)

The icons for blocked downloads should display a standard document icon with a small badge showing an exclamation or a block symbol.
Priority: -- → P3
Whiteboard: [fxprivacy][triage] → [fxprivacy]
Assignee: nobody → jkingston
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.
Attachment #8745370 - Flags: review?(paolo.mozmail)
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.
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.
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.
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 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.
Attachment #8745370 - Flags: review?(paolo.mozmail)
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/
The patch show there is no size difference on osx.
Attachment #8754313 - Flags: review?(paolo.mozmail) → review?(jaws)
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.
Attachment #8754313 - Flags: review?(jaws)
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 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.
Attachment #8754313 - Flags: review?(jaws)
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 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 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.
Attachment #8754313 - Flags: review?(jaws)
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 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/
Attachment #8754313 - Attachment description: MozReview Request: Bug 1265341 - changing blocked download icons to be smaller overlays → Bug 1265341 - changing blocked download icons to be smaller overlays
Attachment #8754313 - Flags: review?(paolo.mozmail)
Attachment #8754313 - Flags: review?(jaws) → review+
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/fx-team/rev/adaef7e67064
changing blocked download icons to be smaller overlays. r=jaws
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/adaef7e67064
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 50
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.
Attachment #8754313 - Flags: review?(paolo.mozmail)
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.
Flags: needinfo?(jkt)
Flags: needinfo?(jaws)
It can be reproduced in MacOSX but Ubuntu.
Seems like a different issue, reverting doesn't seem necessary. I raised Bug 1287384 to cover this.

Thanks!
Flags: needinfo?(jkt)
Flags: needinfo?(jaws)
Flags: qe-verify+
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)
Status: RESOLVED → VERIFIED
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.