Gear on new tab page blurry

VERIFIED FIXED in Firefox 33

Status

()

defect
VERIFIED FIXED
5 years ago
5 years ago

People

(Reporter: manishearth, Assigned: manishearth)

Tracking

Trunk
Firefox 34
x86_64
Linux
Points:
2
Dependency tree / graph
Bug Flags:
firefox-backlog +

Firefox Tracking Flags

(firefox33 verified, firefox34 verified)

Details

Attachments

(8 attachments, 7 obsolete attachments)

1.13 KB, image/svg+xml
Details
11.55 KB, image/svg+xml
Details
1.49 KB, image/svg+xml
phlsa
: ui-review?
athornburgh
Details
1.35 KB, image/svg+xml
Details
18.76 KB, image/png
Details
1.08 KB, image/svg+xml
Details
3.23 KB, patch
Unfocused
: review+
Details | Diff | Splinter Review
27.04 KB, image/png
Details
Posted image Icon_Cog_Static.svg
Moved from bug 1044537

Aaron has already provided assets for this on that bug, copying them over.
Copying over other asset
See Also: → 1044537
Posted patch Update with better assets (obsolete) — Splinter Review
Attachment #8465376 - Flags: ui-review?(philipp)
Attachment #8465376 - Flags: review?(bmcbride)
Posted patch Patch (obsolete) — Splinter Review
Whoops, wrong commit message
Attachment #8465376 - Attachment is obsolete: true
Attachment #8465376 - Flags: ui-review?(philipp)
Attachment #8465376 - Flags: review?(bmcbride)
Attachment #8465378 - Flags: ui-review?(philipp)
Attachment #8465378 - Flags: review?(bmcbride)
Comment on attachment 8465378 [details] [diff] [review]
Patch

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

::: browser/themes/shared/newtab/cog_rollover.svg
@@ +4,5 @@
> +<svg version="1.1" id="Layer_1" xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink" x="0px" y="0px"
> +	 viewBox="0 0 32 32" enable-background="new 0 0 32 32" xml:space="preserve">
> +<g>
> +	
> +		<image overflow="visible" opacity="0.35" width="142" height="142" xlink:href="data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAAJIAAACSCAYAAACue5OOAAAACXBIWXMAAC4jAAAuIwF4pT92AAAA

Dear Adobe Illustrator: vector graphics, you're doing it wrong!

Illustrator has an awful habit of rasterizing shadows in particular :( Which rather defeats the purpose of SVG. 

Could we get these SVG files cleaned up so that:
* Shadow is done via CSS
* All the superfluous attributes/comments are removed
* DOCTYPE is removed (see bug 1023655)

::: browser/themes/shared/newtab/newTab.inc.css
@@ +60,5 @@
>  
>  /* CUSTOMIZE */
>  #newtab-customize-button {
>    background-color: transparent;
> +  background-image: url(chrome://browser/skin/newtab/cog_static.svg);

Nit: Quote URLs.
Attachment #8465378 - Flags: review?(bmcbride) → review-
Aaron, could you fix the shadow as listed above? Thanks.
Flags: needinfo?(athornburgh)
(I can fix the rest of the nits, just need to remove the rasterized shadow)
Here's a blue graphic of the cog without a shadow as suggested in the other bug.
Aaron, what do you think of using this as the hover/active graphic?
Attachment #8466173 - Flags: ui-review?(athornburgh)
New SVG without shadow attached. The only effect now will be a highlight, no more shadow since it adds little value and has been problematic.

Both Static and Rollover versions should render at 22 x 22 pixels.
Flags: needinfo?(athornburgh)
P.S. If it's too subtle, then I'm fine with the blue cog Phillip provided.
Posted patch Blue patch (obsolete) — Splinter Review
This is the patch with the blue cog
Attachment #8465378 - Attachment is obsolete: true
Attachment #8465378 - Flags: ui-review?(philipp)
Attachment #8469236 - Flags: review?(bmcbride)
Posted patch Gray patch (with gradient) (obsolete) — Splinter Review
This is the one with the new gray cog provided by Aaron.

Both look pretty good, thoughts on which one we should pick?
Attachment #8469242 - Flags: review?(bmcbride)
Flags: needinfo?(philipp)
Given that we're moving away from gradients more and more, let's use the blue version.

However, with the patch, the icon grows when hovering which is not intended. It should stay at the same size as in normal state.
Flags: needinfo?(philipp)
Posted patch Patch: blue, same size (obsolete) — Splinter Review
Blue of the same size.

Created with the svg editor commonly known as vim ;)
Attachment #8469236 - Attachment is obsolete: true
Attachment #8469242 - Attachment is obsolete: true
Attachment #8469236 - Flags: review?(bmcbride)
Attachment #8469242 - Flags: review?(bmcbride)
Attachment #8469335 - Flags: review?(bmcbride)
Shouldn't we be able to just take the original svg file:
http://mxr.mozilla.org/mozilla-central/source/browser/themes/shared/newtab/controls.svg?raw=1

Remove the style glyphShape-style-hover-gear-dropshadow

And make the glyphShape-style-hover-gear style be a fill of #4A90E2 or whatever blue is desired?
(In reply to Ed Lee :Mardak from comment #14)
> Shouldn't we be able to just take the original svg file:
> http://mxr.mozilla.org/mozilla-central/source/browser/themes/shared/newtab/
> controls.svg?raw=1
> 
> Remove the style glyphShape-style-hover-gear-dropshadow
> 
> And make the glyphShape-style-hover-gear style be a fill of #4A90E2 or
> whatever blue is desired?

I took the svg file attached above and changed the path fill to the blue from the other file. I'm not sure if we're supposed to apply hover effects to the SVG directly, though I'm open to doing that.
The existing svg contains the hover and active states of other controls. This also doesn't require adding additional svg files for static/rollover as those already exist in controls.svg.

The patch to update the cog icon hover/unhover should only need to touch that one controls.svg file, so no jar.mn nor added files.
Okay, will do.
Blocks: 1036284, 1030832
Points: --- → 2
Incorporated Ed's comments.
Attachment #8469335 - Attachment is obsolete: true
Attachment #8469335 - Flags: review?(bmcbride)
Attachment #8469866 - Flags: review?(bmcbride)
Attachment #8469866 - Flags: feedback?(edilee)
Seems to work for me!
Comment on attachment 8469866 [details] [diff] [review]
Blue on hover, uses same svg file

>+++ b/browser/themes/shared/newtab/controls.svg
>@@ -14,22 +14,17 @@
>       .glyphShape-style {
>         fill: #7a7a7a;
>       }
> 
>       .glyphShape-style-hover-gear {
>+        fill: #0092E5;
nit: the other colors are lowercase, so probably also match with #0092e5.

Although I believe the blue dcrobot provided is #4a90e2
Attachment #8469866 - Flags: feedback?(edilee) → feedback+
dcrobot, just making sure which blue you were expecting for the hovered gear.
Flags: needinfo?(athornburgh)
Flags: firefox-backlog+
(In reply to Ed Lee :Mardak from comment #21)
> dcrobot, just making sure which blue you were expecting for the hovered gear.

I took Phillip's blue (https://bug1046693.bugzilla.mozilla.org/attachment.cgi?id=8466173).
Ed, this is the new rollover cog icon. The blue is #4A90E2
Flags: needinfo?(athornburgh)
This is the "new" blue :)
Attachment #8469866 - Attachment is obsolete: true
Attachment #8469866 - Flags: review?(bmcbride)
Attachment #8470125 - Flags: review?(bmcbride)
Comment on attachment 8470125 [details] [diff] [review]
Blue on hover, uses same svg file (different blue)

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

r+ on the assumption that the blue for the gear should be different from the blue for the pin. But I'd like explicit confirmation on that before landing.
Attachment #8470125 - Flags: review?(bmcbride) → review+
Aaron, could you confirm comment 25? ie, that we want the blue used for the gear to be different from the blue used for the pin? Or should the hover color of the pin be updated to #4A90E2?
Flags: needinfo?(athornburgh)
Blair, I don't know why the pin was different. Please update the pin to #4A90E2 so that all "link blue" instances are consistent. Thanks!
Flags: needinfo?(athornburgh)
Comment on attachment 8470906 [details] [diff] [review]
Blue on hover, uses same svg file, updates pin blue for consistency

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

The system works!
Attachment #8470906 - Flags: review?(bmcbride) → review+
Keywords: checkin-needed
(Does this need a try push? It's just a change to an image.)
https://hg.mozilla.org/mozilla-central/rev/7f1c6919a3ea
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 34
Iteration: --- → 34.2
QA Whiteboard: [qa?]
I can see the blurry gear on the 34.0a1 (2014-08-12) build, and on today's build, 34.0a1 (2014-08-13) it is not blurry and it's the new blue, as is the pin. Huzzah!
Status: RESOLVED → VERIFIED
QA Contact: lhenry
QA Whiteboard: [qa?] → [qa!]
Blocks: 1057602
Uplift has been managed in bug 1057602
Mozilla/5.0 (X11; Linux x86_64; rv:33.0) Gecko/20100101 Firefox/33.0

Verified fixed on latest Aurora, build ID: 20140828004002.
I don't know, it appears blurry to me (on Windows 7). Has that maybe regressed? I expect being an SVG the icon should be crisp at all sizes, so is there a shadow looking like blur?
(In reply to Thomas Stache from comment #38)
> Created attachment 8502409 [details]
> Screenshot of Gear icon on Windows 7
> 
> I don't know, it appears blurry to me (on Windows 7). Has that maybe
> regressed? I expect being an SVG the icon should be crisp at all sizes, so
> is there a shadow looking like blur?

What version are you running? I expect that this hasn't reached the release version of Firefox yet.

(The new icon is blue and has no shadow)
(In reply to Thomas Stache from comment #38)
> Created attachment 8502409 [details]
> Screenshot of Gear icon on Windows 7
> 
> I don't know, it appears blurry to me (on Windows 7). Has that maybe
> regressed? I expect being an SVG the icon should be crisp at all sizes, so
> is there a shadow looking like blur?

Oh, it's the "background-size: 28px auto;" rule that causes this for me!
(In reply to Manish Goregaokar [:manishearth] from comment #39)
> What version are you running? I expect that this hasn't reached the release
> version of Firefox yet.
> 
> (The new icon is blue and has no shadow)

Nightly and Aurora, doesn't matter. The icon is blue on hover.
Filed bug 1081206 for investigation, as this one is already closed.
Depends on: 1081206
(In reply to Thomas Stache from comment #43)
> Filed bug 1081206 for investigation, as this one is already closed.
Thanks. To be clear, the issue this bug addressed was that the gear icon used to have a shadow when "active." This shadow at low resolutions just made the icon look blurry.
You need to log in before you can comment on or make changes to this bug.