Closed Bug 848150 Opened 8 years ago Closed 8 years ago

Update about:home favicons

Categories

(Firefox :: Theme, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 22

People

(Reporter: fryn, Assigned: fryn)

References

Details

Attachments

(2 files)

Attached patch patchSplinter Review
Currently, we're using specialized 16x16 images as about:home favicons, but UX actually prefers the appearance of the 32x32 images even when downscaled to 16x16.

Also, we're using an outdated Google search engine icon in the search bar. The updated 32x32 image that Google is now using looks great even when downscaled to 16x16 on non-HiDPI displays, so let's use that.

Not all the other search engine icons downscale as well, so I'm leaving them as 16x16, but enabling `image-rendering: -moz-crisp-edges;` on OS X HiDPI mode for upscaling like we do for favicons.

We've tested all of these images on both HiDPI and non-HiDPI hardware.
With these changes, all of our primary UI will look crisp on both HiDPI and non-HiDPI hardware!
Attachment #721481 - Flags: ui-review?(limi)
Attachment #721481 - Flags: review?(dolske)
Didn't you start this in another bug? Or was that in one of the mega hidpi fixes, and we just dropped it?

Got screenshots of the FF and Google icons on lowdpi with this patch?
(In reply to Justin Dolske [:Dolske] from comment #1)
> Didn't you start this in another bug? Or was that in one of the mega hidpi
> fixes, and we just dropped it?

I wrote a different patch that tried to provide HiDPI versions of all the search engine icons in bug 795495, but that didn't work out, so I've put that aside for now. This bug is just for the about:home favicons and the Google icon only, which downscale well.

> Got screenshots of the FF and Google icons on lowdpi with this patch?

Limi will post them shortly.
Comment on attachment 721481 [details] [diff] [review]
patch

Yes, please!

(In reply to Justin Dolske [:Dolske] from comment #1)
> Didn't you start this in another bug? Or was that in one of the mega hidpi
> fixes, and we just dropped it?
> 
> Got screenshots of the FF and Google icons on lowdpi with this patch?

Firefox 32px icon scaled to 16px by the browser:
http://cl.ly/image/2B0V1E1l1e1S

Google 32px icon scaled to 16px by the browser:
http://cl.ly/image/2q0M2v341C2M

Looks great!
Attachment #721481 - Flags: ui-review?(limi) → ui-review+
Attached image lodpi screenshot
(In reply to Alex Limi (:limi) — Firefox UX Team from comment #3)

> > Got screenshots of the FF and Google icons on lowdpi with this patch?
> 
> Firefox 32px icon scaled to 16px by the browser:
> http://cl.ly/image/2B0V1E1l1e1S
> 
> Google 32px icon scaled to 16px by the browser:
> http://cl.ly/image/2q0M2v341C2M
> 
> Looks great!

That's not what I see at all. Attached is a screenshot showing the 2 icons for this patch on a non-Retina display; with (from left to right) 16x16, 32x32 (scaled with -moz-crisp-edges), and 32x32 (with regular scaling).

Both of the scaled 32x32 icons looks noticeably worse.

I don't think a shortcut is going to work well here, we're actually going to need to show the right icon for the right screen. :(
Oh, wait, what am I thinking. The crisp-edge scaling isn't used on non-Retina pages.
(In reply to Justin Dolske [:Dolske] from comment #5)
> Oh, wait, what am I thinking. The crisp-edge scaling isn't used on
> non-Retina pages.

That is correct if you replace the word "pages" with "displays".

For the Firefox logo, the downscaled version is preferred because it looks more like the full-size Firefox logo. The specially made 16×16 icon attempted to make the fox more defined at the heavy cost of being less visually balanced and arguably closer to our old logo than the new one and less recognizable.

For the Google logo, the downscaled version is either exactly or almost exactly what Google is using on www.google.com, not our outdated version.
Comment on attachment 721481 [details] [diff] [review]
patch

The Google icon (with regular scaling) isn't great, but it's not a very big delta. I'm more concerned about the Firefox icon, since the 16x16 version is heavily tweaked. But I'll defer to Steven since he has final say on visual stuff.

[Bugzilla doesn't do multiple ui-review requests, so the clearing of limi's ui-r+ here isn't meant as a some kind of passive-aggressive thing. :-)]
Attachment #721481 - Flags: ui-review?
Attachment #721481 - Flags: ui-review+
Attachment #721481 - Flags: review?(dolske)
Attachment #721481 - Flags: review+
Comment on attachment 721481 [details] [diff] [review]
patch

[Let me try that again.]
Attachment #721481 - Flags: ui-review? → ui-review?(shorlander)
Comment on attachment 721481 [details] [diff] [review]
patch

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

This looks better to me, or at least worth the trade-off.
Attachment #721481 - Flags: ui-review?(shorlander) → ui-review+
Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/6c3ea4720ab9 - you wouldn't think we'd do such a thing, but we actually have a test, browser_google.js, to be sure you haven't changed the Google search icon.
Target Milestone: Firefox 22 → ---
Wow, the test literally compares the data url in the product to the data url in the test, string to string. :|

Pushed a one-line test fix with the patch.

Patch: https://hg.mozilla.org/integration/mozilla-inbound/rev/f4beba4909b0
Test fix: https://hg.mozilla.org/integration/mozilla-inbound/rev/1666ca9b946b
Target Milestone: --- → Firefox 22
Looks like Ms2ger backed this out again due to 'bc' test failures:
https://hg.mozilla.org/integration/mozilla-inbound/rev/37b4a5f15295
Target Milestone: Firefox 22 → ---
(In reply to Mats Palmgren [:mats] from comment #13)
> Looks like Ms2ger backed this out again due to 'bc' test failures:
> https://hg.mozilla.org/integration/mozilla-inbound/rev/37b4a5f15295

Ugh, it got backed out when it shouldn't have been.
These two changesets weren't the ones causing any of the failures. I'll push again when inbound reopens.
Hmm, it does seem to fail reliably on linux64 (Fedora64) opt mochitest-bc on Mozilla-Inbound, but it's failing immediately right at the beginning of the first mochitest-bc test.

Here is the log:
https://tbpl.mozilla.org/php/getParsedLog.php?id=20726056&tree=Mozilla-Inbound
Full log if needed:
https://tbpl.mozilla.org/php/getParsedLog.php?id=20726056&tree=Mozilla-Inbound&full=1

Since it only fails on linux64, and I don't have a linux64 box, I pushed a few changes to Tryserver to see what's up.
(In reply to Frank Yan (:fryn) from comment #14)
> (In reply to Mats Palmgren [:mats] from comment #13)
> > Looks like Ms2ger backed this out again due to 'bc' test failures:
> > https://hg.mozilla.org/integration/mozilla-inbound/rev/37b4a5f15295
> 
> Ugh, it got backed out when it shouldn't have been.
> These two changesets weren't the ones causing any of the failures. I'll push
> again when inbound reopens.

They may not have been, but all Fedora64 opt and pgo builds failed the exact same test, starting from your push, and ending with my backout.
(In reply to :Ms2ger from comment #16)
> They may not have been, but all Fedora64 opt and pgo builds failed the exact
> same test, starting from your push, and ending with my backout.

After reading through TBPL and the logs, I see that your backout was the right decision. I'm investigating it. See comment 15. I won't push again until I figure it out.
Findings:

Making the Google search engine icon not 16×16 is what causes mochitest-bc to fail almost every time, and it fails near the beginning of the first mochitest-bc test:
chrome://mochitests/content/browser/browser/base/content/test/browser_CTPScriptPlugin.js
logging the following TEST-UNEXPECTED-FAIL:
should have a click-to-play notification (about:blank)
instead of the following TEST-PASS:
should have a click-to-play notification (plugin_test_noScriptNoPopup.html)

I have no idea why this is.
In fact, the test actually passed once, despite the image size change, which is even more baffling:
https://tbpl.mozilla.org/?tree=Try&rev=4f5b0beb1528

Perhaps, I'll split off the Google search engine icon change into a separate bug, so I can land the rest of the patch and close this bug.
Pushed everything except the Google search engine icon change to inbound:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e69bc37414dd

Renamed bug to reflect this.
Filed bug 851953 for the remaining Google search engine icon part.
Summary: Update about:home favicons and Google search engine icon → Update about:home favicons
Target Milestone: --- → Firefox 22
Why did this patch add a scrollbar to the Downloads button/Manager with only 1 (one) download in the list?

Screenshot:
http://i.imgur.com/bAo0Qat.jpg
(In reply to Jim Jeffery not reading bug-mail 1/2/11 from comment #20)

Why do you think this patch had anything to do with that?
(In reply to Frank Yan (:fryn) from comment #21)
> (In reply to Jim Jeffery not reading bug-mail 1/2/11 from comment #20)
> 
> Why do you think this patch had anything to do with that?

From regression testing...scrollbar does not appear until I load the build from cset:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e69bc37414dd
Interesting, may be somehow related to the position of the search bar, may be a rounding px problem?
(In reply to Marco Bonardo [:mak] from comment #23)
> Interesting, may be somehow related to the position of the search bar, may
> be a rounding px problem?

I have nothing modified or moved,resized.  I believe in testing builds with the barest setup possible and everything pretty muck 'stock'.  Although my old eyes can't detect 1px shifts as a general rule. 

For the record all testing was done either with m-c or m-i hourly builds.
win7 x64 and I run the browser maximized.
No longer depends on: 851976
https://hg.mozilla.org/mozilla-central/rev/e69bc37414dd
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.