Closed Bug 1088712 Opened 5 years ago Closed 5 years ago

Download glow icon looks small on dark themes

Categories

(Firefox :: Theme, defect)

defect
Not set

Tracking

()

VERIFIED FIXED
Firefox 36
Tracking Status
firefox35 --- verified
firefox36 --- verified

People

(Reporter: bgrins, Assigned: jsantell)

References

Details

Attachments

(4 files, 2 obsolete files)

Attached image download-glow.png
See screenshot.  There seems to be a dark shadow along the outside of the active download arrow which causes it to appear smaller than the other icons.
There are 5 download icons, the one I've noticed this being a problem with is download-glow.png (and the 2x version): https://mxr.mozilla.org/mozilla-central/source/browser/themes/osx/downloads/.

It may make sense to refactor this to reside in the toolbarbutton icons (where we already have a normal and inverted state).  I think it's being applied differently than most of those icons though - it's using a background-image instead of list-style-image: http://dxr.mozilla.org/mozilla-central/search?q=download-glow.png&case=true&redirect=true.

Alternatively we could just swap the current icon with one that has a more neutral shadow rather than adding new icons.  Stephen, what do you think?
Flags: needinfo?(shorlander)
I think we should go ahead and update the Toolbar.png sprite so this doesn't get overlooked anymore. I will provide new assets.
Flags: needinfo?(shorlander)
Assignee: nobody → ntim007
Status: NEW → ASSIGNED
Assignee: ntim007 → nobody
Status: ASSIGNED → NEW
Assignee: nobody → jsantell
Status: NEW → ASSIGNED
Patch for the changes -- going to test them out later on different machines..

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=e2eae5e9fd50
Attachment #8512246 - Flags: review?(bgrinstead)
Comment on attachment 8512246 [details] [diff] [review]
1088712-make-download-glow-in-toolbar.patch

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

Gijs (or someone else that he chooses) would be a better reviewer than me for this.
Attachment #8512246 - Flags: review?(bgrinstead) → review?(gijskruitbosch+bugs)
Comment on attachment 8512246 [details] [diff] [review]
1088712-make-download-glow-in-toolbar.patch

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

This is close, but the linux Toolbar.png image isn't fully optimized (chuck it through the most recent version of ImageOptim.app), and there's a small mistake in the Windows stylesheet.

::: browser/themes/windows/downloads/indicator.css
@@ +48,5 @@
>  @media (-moz-os-version: windows-vista),
>         (-moz-os-version: windows-win7) {
>  %endif
>    #downloads-button[attention] > #downloads-indicator-anchor > #downloads-indicator-icon {
> +    background-image: -moz-image-rect(url("chrome://browser/skin/Toolbar-aero.png"), 18, 198, 36, 180);

I *think* this is incorrect, because on XP, we'll be applying this file without -aero, and so the media query is not there, and this is just applied - but Toolbar-aero.png doesn't exist in classic (ie the non-aero bit of the jar) and so there'll be no background.

I suspect this rule will need its own media query which is always there (not ifdef'd) - make sure to keep the existing ifdef+media query unholy thingymajig so we don't break the menu panel thing going on below here. :-(

@@ +73,5 @@
>  }
>  
>  #downloads-button:not([counter])[attention] > #downloads-indicator-anchor > #downloads-indicator-progress-area > #downloads-indicator-counter {
> +  background-image: -moz-image-rect(url("chrome://browser/skin/Toolbar.png"), 18, 198, 36, 180);
> +}

Have you or shorlander checked what this looks like? It gets downscaled to 12px...
Attachment #8512246 - Flags: review?(gijskruitbosch+bugs)
Actually, seeing as I did this already...
* Added Gijs' optimized Linux toolbar
* Removed the Toolbar-aero within the ifdef'd media query for reasons in comment #6 -- looks like the Toolbar[-inverted] versions of this are right above it, so just added a media query for windows-vista/windows-win7.

Resending on try to run the builds in a VM for a visual test -- got nothing set up for this kind of thing :)

Thanks Gijs and Brian!
Note the downscaling mentioned in comment #6 -- that's what was there before, so will check it out to see if it still looks good with the toolbar
Looks good on Windows XP -- still waiting on Win7 build. Linux VM having trouble even running, checking dependencies.
Looks good on Win7 and Linux 32 bit builds.

Scenarios tested -- downloaded small file, download arrow lit up. download large file, make sure the progress bar looked good and lit up after completed.
Comment on attachment 8512803 [details] [diff] [review]
1088712-make-download-glow-in-toolbar.patch

Passing this back to you, Gijs!
Attachment #8512803 - Flags: review?(gijskruitbosch+bugs)
Comment on attachment 8512803 [details] [diff] [review]
1088712-make-download-glow-in-toolbar.patch

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

r=me, but upon closer inspection, I believe you can remove the Toolbar-aero thing (that I made you move) and its media query entirely. The overrides at the bottom of windows/jar.mn take care of this. Please doublecheck that visually. :-)
Attachment #8512803 - Flags: review?(gijskruitbosch+bugs) → review+
Removed extra query, and rechecked -- looks good in W7 -- thanks!
Attachment #8512803 - Attachment is obsolete: true
Attachment #8513109 - Flags: review+
This needs to be uplifted, right?
Flags: needinfo?(jsantell)
Not sure of the deployment plan for this, but it needs to be a part of the dev edition dark theme, pinging bgrins
Flags: needinfo?(jsantell) → needinfo?(bgrinstead)
(In reply to Jordan Santell [:jsantell] [@jsantell] from comment #19)
> Not sure of the deployment plan for this, but it needs to be a part of the
> dev edition dark theme, pinging bgrins

It would be great to get this uplifted ASAP, but forwarding the question onto Gijs about if/when it would make sense to do.
Flags: needinfo?(bgrinstead) → needinfo?(gijskruitbosch+bugs)
https://hg.mozilla.org/mozilla-central/rev/95270b360927
https://hg.mozilla.org/mozilla-central/rev/e3194c623ac6
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 36
(In reply to Brian Grinstead [:bgrins] from comment #20)
> (In reply to Jordan Santell [:jsantell] [@jsantell] from comment #19)
> > Not sure of the deployment plan for this, but it needs to be a part of the
> > dev edition dark theme, pinging bgrins
> 
> It would be great to get this uplifted ASAP, but forwarding the question
> onto Gijs about if/when it would make sense to do.

I think for aurora it makes sense to do it soon. Request uplift on Friday and it should have had 2+ days for people to notice issues if there are any?
Flags: needinfo?(gijskruitbosch+bugs)
(In reply to :Gijs Kruitbosch from comment #22)
> (In reply to Brian Grinstead [:bgrins] from comment #20)
> > (In reply to Jordan Santell [:jsantell] [@jsantell] from comment #19)
> > > Not sure of the deployment plan for this, but it needs to be a part of the
> > > dev edition dark theme, pinging bgrins
> > 
> > It would be great to get this uplifted ASAP, but forwarding the question
> > onto Gijs about if/when it would make sense to do.
> 
> I think for aurora it makes sense to do it soon. Request uplift on Friday
> and it should have had 2+ days for people to notice issues if there are any?

Given the timescales involved, I think we should ask for uplift straight away. We'd like to collect things together for a concerted QA push and keeping things away from Aurora hurts that.
Don't forget to add linux/Toolbar-inverted.png in the uplift.
Blocks: 1056923
Comment on attachment 8513109 [details] [diff] [review]
1088712-make-download-glow-in-toolbar.patch

Approval Request Comment
[Feature/regressing bug #]: n/a
[User impact if declined]: downloads icon looks off on aurora; webide button has no icon
[Describe test coverage new/current, TBPL]: icons only, so no
[Risks and why]: pretty low
[String/UUID change made/needed]: no
Attachment #8513109 - Flags: approval-mozilla-aurora?
Whiteboard: please transplant both landed csets (see comment #21, comment 24) when uplifting
Attachment #8513109 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
https://hg.mozilla.org/releases/mozilla-aurora/rev/b10e61a46771

I folded the follow-up into the original patch since that seemed more logical.
Whiteboard: please transplant both landed csets (see comment #21, comment 24) when uplifting
Verified fixed on Mac OS X 10.9.5, Ubuntu 14.04 64-bit and Windows 7 64-bit using:
- latest Nightly, build ID: 20141102030204
- latest Aurora, build ID: 20141102004001.
Status: RESOLVED → VERIFIED
This may be obvious, but I don't see it right away .. what's the last icon in Toolbar.png for (the wrench inside a little popup shape)?  TIA.
You need to log in before you can comment on or make changes to this bug.