Closed Bug 758515 Opened 8 years ago Closed 7 years ago

New download's button graphic upon completion is buggy

Categories

(Firefox :: Theme, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 19

People

(Reporter: divjot94, Assigned: mconley)

References

(Blocks 1 open bug)

Details

(Keywords: icon)

Attachments

(8 files, 8 obsolete files)

52.34 KB, image/png
Details
5.00 KB, image/png
Details
4.91 KB, image/png
Details
1.98 KB, image/png
Details
4.20 KB, image/png
Details
6.21 KB, image/png
Details
14.46 KB, image/png
Details
30.24 KB, patch
Details | Diff | Splinter Review
http://s13.postimage.org/4tl33qh03/Untitled.png

This should be fixed before shipping it in Beta / Aurora
Blocks: 726447
No longer blocks: 757569
no doubt, it is quite visible so we are clearly aware of that from a long time, though we are still waiting for the final icons.
the mockups are not the icons, we need the UX team to give us the final artwork.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Attached image Mac screenshot
Mac Download button needs a new icon, too.
OS: Windows 8 → All
Hardware: x86 → All
Version: 14 Branch → Trunk
Attached image windows 7 screenshot
Shorlander, can you help? This is even awfuller on Windows.
How long would it take? :( Sorry for being impatient but it seems such a small cosmetic bug , but really annoying... Thanks
Duplicate of this bug: 784629
Duplicate of this bug: 785787
Attached patch Patch (obsolete) — Splinter Review
Untested on Linux or Mac, but the icon size is the same for all three platforms.

In order to get the proper glow around the icon, the icon must be larger than 16x16, hence the 48x48 size of the icon.

Here's a screenshot of how it looks on Windows: http://screencast.com/t/U8FTDHLOZZz
Assignee: nobody → jaws
Status: NEW → ASSIGNED
Attachment #666276 - Flags: review?(dao)
Here is another screenshot of it on Windows, showing the button with the :hover state, http://screencast.com/t/zWhPlfWhe
Comment on attachment 666276 [details] [diff] [review]
Patch

Review of attachment 666276 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for looking into this and writing this patch, Jared.
Although it may not ideal to have the glow expand that far outside of the toolbar button, it's definitely better than the current situation.
Just a few things to tweak before approving:

::: browser/themes/gnomestripe/downloads/downloads.css
@@ +179,5 @@
>                                0, 16, 16, 0) center no-repeat;
>  }
>  
>  #downloads-indicator[attention] > #downloads-indicator-anchor > #downloads-indicator-icon {
> +  background: url("chrome://browser/skin/downloads/download-glow.png") center no-repeat;

We should investigate how to apply this fix to #downloads-indicator:not([counter])[attention]
#downloads-indicator-counter in a followup bug. The flow of states is difficult to follow, and I have not seen #downloads-indicator:not([counter])[attention] actually occur.

::: browser/themes/pinstripe/downloads/downloads.css
@@ +189,5 @@
>  
>  #downloads-indicator[attention]
>  #downloads-indicator-icon {
> +  background: url("chrome://browser/skin/downloads/download-glow.png") center no-repeat;
> +  margin: -16px;

This should probably be -14px, given the -moz-image-rect offsets that were here.

::: browser/themes/winstripe/downloads/downloads.css
@@ +200,5 @@
>  }
>  
>  #downloads-indicator[attention] > #downloads-indicator-anchor > #downloads-indicator-icon {
> +  background: url("chrome://browser/skin/downloads/download-glow.png") center no-repeat;
> +  margin: -16px;

This should probably be -15px.

::: toolkit/themes/gnomestripe/global/alerts/alert.css
@@ +66,5 @@
>  }
> +
> +.alertBox[hide] {
> +  visibility
> +}
\ No newline at end of file

Please remove this from the diff.
Attachment #666276 - Flags: review?(fryn)
Attachment #666276 - Flags: review?(dao)
Attachment #666276 - Flags: review-
Attached patch Patch v2 (obsolete) — Splinter Review
Thanks for the speedy review Frank. I filed bug 795806 for the :not([counter])[attention] rule.

I updated the patch to have the more correct margin values. Thanks for catching that!
Attachment #666276 - Attachment is obsolete: true
Attachment #666439 - Flags: review?(fryn)
Attachment #666439 - Flags: review?(fryn) → review+
Attached patch Patch for checkin (obsolete) — Splinter Review
Attachment #666439 - Attachment is obsolete: true
Component: Downloads Panel → Theme
Attached image Ubuntu screenshot (after patch) (obsolete) —
Patch looks good on Ubuntu.
what happens if the button is on the lowest or highest toolbar (eg bookmarks toolbar), I suppose the glow is still cut but the border of the toolbox... if so likely we should ensure the glow fits into the button size, not go over it.
Thanks for asking about that Marco. I loaded about:blank and compared and saw that the green was affecting the color of the page. I was able to lower the value on Windows to |margin: -8px -15px| and still achieve the desired effect without bleeding into content. I'll fix the patch.
Keywords: checkin-needed
Please check small icons mode too.
Thanks, we'll need new graphics from Stephen that don't have the glow around them. Small icon mode in particular makes it hard to have any glow that expands in the y-axis.
Flags: needinfo?(shorlander)
As well the download button looks kind of ugly in Linix
when compared to the one in windows
No matter what theme (Firefox uses internal icons for the button)

Also on small buttons the download icons looks odd
Attached patch Patch v2.1 (obsolete) — Splinter Review
The overflow on to content is fixed by setting overflow:hidden on the #navigator-toolbox. I set it on #navigator-toolbox instead of #nav-bar so that it wouldn't clip at the boundaries of other toolbars.
Attachment #666443 - Attachment is obsolete: true
Attachment #669875 - Flags: review?(dao)
Flags: needinfo?(shorlander)
Comment on attachment 669875 [details] [diff] [review]
Patch v2.1

This adds a scroll frame for the toolbox, which is overhead we don't want.
Attachment #669875 - Flags: review?(dao) → review-
Unassigning as I think the only reasonable approach is to get a new icon that doesn't have the glow (unless we are OK with overlapping content, which I doubt).
Assignee: jaws → nobody
Status: ASSIGNED → NEW
Flags: needinfo?(shorlander)
An icon with a glow that stays inside the usual size would be fine too, I suppose that glow should be like 1 or 2 px (don't remember how much space there's available in the current icon)
Blocks: 794332
Blocks: 795806
No longer blocks: 795806
Duplicate of this bug: 802267
What is wrong about overlapping content or other toolbars with the glow?
(In reply to Jared Wein [:jaws] from comment #27)
> What is wrong about overlapping content or other toolbars with the glow?

I think it looks weird, so we would be exchanging a weird look for another one. Plus the current glow is far too large and hurts on background themes, something thinner would be nicer globally.
The content area is for Web content. The glow just doesn't belong there. I guess overlaying content like this could also hurt performance.
We overlay content for things like doorhangers, and bug 541656, so this wouldn't be the first time.
Neither doorhanger notifications nor the status panel are comparable to this. They don't spill over to the content area, they intentionally overlay it.

Their implementation is also different such that performance characteristics may differ. Popups get their own widget, the status panel has position:fixed.
Basically this needs a new icon.
Keywords: icon
I'm starting thinking we should just remove the glow completely.

It's non-native, since no other button does any glowing stuff.Plus whatever color we take it will look bad on some Persona.
It's definitely creating more issues than benefits, since the icon is already changing color to green, that looks like enough to show a status change without being distracting.
Thus, imo, we should just change download-glow.png (And the aero version) to have no external glow, the internal one is fine. At least this is the interim solution we should take before next Aurora uplift.
Stephen agreed.
Assignee: nobody → mconley
Flags: needinfo?(shorlander)
Attachment #666553 - Attachment is obsolete: true
Attached patch WIP Patch 1 (obsolete) — Splinter Review
I've got new graphics from shorlander, and I've got them up on Windows 7 and XP. Screenshots coming.
Attachment #669875 - Attachment is obsolete: true
Attached image Patch on Windows XP
Attached image Patch on Windows 7
Attached patch WIP Patch 2 (obsolete) — Splinter Review
Fixes OSX. I've made the required changes for Retina displays, but I haven't tested that yet. I'll see if I can scrounge one up here once I'm done with gnomestripe.
Attachment #682030 - Attachment is obsolete: true
Attached image Patch on OSX
Attached patch Patch v1 (obsolete) — Splinter Review
Ok, I think this covers it.
Attachment #682042 - Attachment is obsolete: true
Attached image Patch on Ubuntu
yes, we definitely need a follow-up bug for gnomestripe.
Attachment #682047 - Flags: review?(mak77)
Blocks: 812221
Comment on attachment 682047 [details] [diff] [review]
Patch v1

Review of attachment 682047 [details] [diff] [review]:
-----------------------------------------------------------------

You forgot to remove download-glow-aero.png from the winstripe/downloads folder, apart this looks fine.
Attachment #682047 - Flags: review?(mak77) → review+
Duplicate of this bug: 794332
(In reply to Marco Bonardo [:mak] from comment #43)
> Comment on attachment 682047 [details] [diff] [review]
> Patch v1
> 
> Review of attachment 682047 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> You forgot to remove download-glow-aero.png from the winstripe/downloads
> folder, apart this looks fine.

Thanks! Fixed.
Attachment #682047 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/61232c5c748d
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 19
You need to log in before you can comment on or make changes to this bug.