1019045 - Unify magnifying glass/search icon

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Theme and Visual Design, defect)

Description

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

Attachment: mob_searchicons_unify.jpg

Currently we have a couple magnifying glass/search icons that are presented to the user during different stages of their Firefox experience. I'd like to propose that we unify these. Especially in light of toolbar refinements.

I've made a new search icon that has a little bit more detail and I think provides a bit more character. 

Thoughts?

Comment 1

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

Attachment: mob_toolbar_new.png

Preview of proposed search icon (in action :)

Comment 2

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

Attachment: icon_search.zip

Search icon assets (if we need them) from MDPI > XXHDPI

Comment 3

[:Margaret Leibovic]

These are the current icons in the tree:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/resources/drawable-mdpi/ab_search.png
http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/resources/drawable-mdpi/ic_url_bar_search.png

In mdpi, one is 32x32 and the other is 24x24.

We also have this icon in the action bar for adding a new search engine:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/resources/drawable-mdpi/ab_add_search_engine.png

Not sure if we want to change that as well.

This seems like an easy win, but I think we would want to keep the same icon sizes as before, so I think we need some new assets (the ones included look too small).

Comment 4

[Ian Barlow (:ibarlow)]

(In reply to Anthony Lam (:antlam) from comment #1)
> Created attachment 8432595 [details]
> mob_toolbar_new.png
> 
> Preview of proposed search icon (in action :)

<3

Comment 5

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

(In reply to :Margaret Leibovic from comment #3)
> These are the current icons in the tree:
> http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/resources/
> drawable-mdpi/ab_search.png
> http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/resources/
> drawable-mdpi/ic_url_bar_search.png
> 
> In mdpi, one is 32x32 and the other is 24x24.
> 
> We also have this icon in the action bar for adding a new search engine:
> http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/resources/
> drawable-mdpi/ab_add_search_engine.png
> 
> Not sure if we want to change that as well.
> 
> This seems like an easy win, but I think we would want to keep the same icon
> sizes as before, so I think we need some new assets (the ones included look
> too small).

The images aren't loading for me... so I'm having trouble understanding which part of the UI these are surfacing in. Could you elaborate a bit more so I can prep the new icons in the correct dimensions for us? :)

Thanks Margaret!

Comment 6

[:Margaret Leibovic]

(In reply to Anthony Lam (:antlam) from comment #5)
> (In reply to :Margaret Leibovic from comment #3)
> > These are the current icons in the tree:
> > http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/resources/
> > drawable-mdpi/ab_search.png
> > http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/resources/
> > drawable-mdpi/ic_url_bar_search.png
> > 
> > In mdpi, one is 32x32 and the other is 24x24.
> > 
> > We also have this icon in the action bar for adding a new search engine:
> > http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/resources/
> > drawable-mdpi/ab_add_search_engine.png
> > 
> > Not sure if we want to change that as well.
> > 
> > This seems like an easy win, but I think we would want to keep the same icon
> > sizes as before, so I think we need some new assets (the ones included look
> > too small).
> 
> The images aren't loading for me... so I'm having trouble understanding
> which part of the UI these are surfacing in. Could you elaborate a bit more
> so I can prep the new icons in the correct dimensions for us? :)
> 
> Thanks Margaret!

Sorry for the slow response. I put the mdpi icons in a folder here:
http://people.mozilla.org/~mleibovic/search-icons/

The ab_* ones are the ones used in the action bar, and ic_url_bar_search.png is what's used in the url bar.

Comment 7

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

Attachment: icon_ab_search.zip

Uploading new ab_search icons but still yet to do the ab_addsearchengine icon cause I want to nail down the metaphor first.

The toolbar ones are already uploaded

Comment 8

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

NI'ing Margaret (possibly incorrectly :P) for next steps

Comment 9

[:Margaret Leibovic]

(In reply to Anthony Lam (:antlam) from comment #8)
> NI'ing Margaret (possibly incorrectly :P) for next steps

It looks like we just need a patch for this. I'm not going to be able to get to this before I take off for PTO, but I'm making it a mentor bug and hopefully lucasr will be up for mentoring it :)

Comment 10

[amoghbl1]

Is it fine to take up work on this :lucasr ? Also, which one of the two icons are we keeping? Or are we replacing it with icon_ab_search ?

Comment 11

[:Margaret Leibovic]

I can mentor this again, now that I'm back from PTO.

(In reply to amoghbl1 from comment #10)
> Is it fine to take up work on this :lucasr ? Also, which one of the two
> icons are we keeping? Or are we replacing it with icon_ab_search ?

We're going to just update ab_search.png and ic_url_bar_search.png.

So, the file names will stay the same, but we'll replace the current icons in the tree with the ones antlam attached to this bug.

Comment 12

[amoghbl1]

Attachment: fix_1019045.patch

Replaced the mdpi, hdpi and xhdpi files for ab_search.png and ic_url_bar_search.png and added the same in the xxhdpi folder.

Comment 13

[:Margaret Leibovic]

Attachment: screenshot (Nexus 4)

Hey amoghbl1, it looks like the ab_search.png icon is too large. Can you check to make sure the new icons are the same dimensions as the old icons?

Looking at the zip filed antlam attached, it looks like the new icons he provided are the correct size, so you should just double check to make sure that the right images ended up in the right directories.

Comment 14

[:Margaret Leibovic]

Comment on attachment 8461097 [details] [diff] [review]
fix_1019045.patch

This looks like it's on the right track, I just want you to double check to make sure all the icons are the correct dimensions.

Comment 15

[amoghbl1]

Attachment: Screenshot.png

Hey :margaret , as per this screenshot, they all seem to be in the right directories!

Comment 16

[amoghbl1]

You could also apply the patch, navigate to src/mobile/android/base/resources and try |printf '\nMDPI\n'; file drawable-mdpi/ab_search.png drawable-mdpi/ic_url_bar_search.png ;printf '\nHDPI\n'; file drawable-hdpi/ab_search.png drawable-hdpi/ic_url_bar_search.png ;printf '\nXHDPI\n';file drawable-xhdpi/ab_search.png drawable-xhdpi/ic_url_bar_search.png ;printf '\nXXHDPI\n' ;file drawable-xxhdpi/ab_search.png drawable-xxhdpi/ic_url_bar_search.png ;| to check it out for yourself!

Comment 17

[:Margaret Leibovic]

(In reply to amoghbl1 from comment #16)
> You could also apply the patch, navigate to
> src/mobile/android/base/resources and try |printf '\nMDPI\n'; file
> drawable-mdpi/ab_search.png drawable-mdpi/ic_url_bar_search.png ;printf
> '\nHDPI\n'; file drawable-hdpi/ab_search.png
> drawable-hdpi/ic_url_bar_search.png ;printf '\nXHDPI\n';file
> drawable-xhdpi/ab_search.png drawable-xhdpi/ic_url_bar_search.png ;printf
> '\nXXHDPI\n' ;file drawable-xxhdpi/ab_search.png
> drawable-xxhdpi/ic_url_bar_search.png ;| to check it out for yourself!

The file names don't actually verify that the file contents are correct, though :)

I verified that the image sizes stay the same when your patch is applied, but I think that there must be a different amount of empty space around the actual magnifying glass in the new ab_search icon (that's the one where I'm seeing a problem).

antlam, can you take a look at the old icons and compare them to the ones you posted? See the screenshot I posted to see my concern.

Comment 18

[amoghbl1]

Oh, when you were talking about sizes, I thought you meant the dimensions of the pictures were wrong! The screenshot just checks if the image sizes are right, don't know about the empty space part!

Comment 19

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

Just talked to Margaret and Lucas about this and it seems as though the padding is around these icons whereas mine do not have the padding.

This might be more finicky:

1) Adding the padding around the new icons mean we can't re-use them elsewhere in the UI as easily (places that do not have built in padding)

2) If we don't just add the padding to this new icon, we have to audit the others in the menu and remove the padding from them.

I can add the padding (if I find out how much to add) but I'm not sure we want to keep adding to the assets and I'd like to reuse this icon in the input bar/toolbar for tablets/mobile if we could to save some space.

I know this might not be as high P but wanted to get some thoughts on this since it seems like an easy fix in anycase :)

Comment 20

[Lucas Rocha (:lucasr)]

Just so that you know where the padded icons come from, it's the iconography guideline described here:
http://developer.android.com/design/style/iconography.html#action-bar

With that said, we don't need to follow the guidelines in this case because we implement our own action bar i.e. we can decide how we want to handle iconography here. For instance, we can simply add the padding in code instead.

Comment 21

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

(In reply to Lucas Rocha (:lucasr) from comment #20)
> For instance, we can simply add the padding in code instead.

That sounds like the ideal solution so we can save space and reuse as many images as possible. Can we do that with this search icon?

Comment 22

[:Margaret Leibovic]

(In reply to Anthony Lam (:antlam) from comment #21)
> (In reply to Lucas Rocha (:lucasr) from comment #20)
> > For instance, we can simply add the padding in code instead.
> 
> That sounds like the ideal solution so we can save space and reuse as many
> images as possible. Can we do that with this search icon?

If we do that with this search icon we would probably need to update all of the action bar icons to also not have padding.

Adding wesj to this bug since he implemented this action bar.

Comment 23

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

How can I help move this along?

Comment 24

[:Margaret Leibovic]

Hey amoghbl1, it looks like this bug is turning out to be more complicated than we initially expected. Are you still interested in trying to drive it to completion?

If we're going to solve this image padding problem by adding the padding to the views ourselves, we'll need to audit all the action bar icons and update them to remove their built-in padding, then add logic to the action bar to add that padding itself.

wesj, what do you think of this? Where would we add that padding? I started looking through the action bar code, and it's difficult to tell how it all works.

This seems like it might be a lot of effort just for being able to share an icon... I'm tempted to suggest we should just keep the padding in ab_search and create a separate icon for the about:home favicon that was added in bug 1054268.

Comment 25

[amoghbl1]

I'd love to continue working on this margeret, if we could decide on what to do next and come up with a solution, I'd be happy to finish this!

Comment 26

[:Margaret Leibovic]

After thinking about this a bit more, I think the easiest way forward just have separate search icons for places where they need to be different sizes/have different padding. However, this would mean we would have 3 search icons in the tree :/

ab_search.png -> action bar, with padding
ic_url_bar_search.png -> small icon in search suggestions
favicon_search.png -> new search icon that we would use as favicon for about:home

I also think we should rename ic_url_bar_search to be less confusing - it's not even used in the url bar!

Lucas, what are your thoughts?

Comment 27

[Lucas Rocha (:lucasr)]

(In reply to :Margaret Leibovic from comment #26)
> After thinking about this a bit more, I think the easiest way forward just
> have separate search icons for places where they need to be different
> sizes/have different padding. However, this would mean we would have 3
> search icons in the tree :/
> 
> ab_search.png -> action bar, with padding
> ic_url_bar_search.png -> small icon in search suggestions
> favicon_search.png -> new search icon that we would use as favicon for
> about:home
> 
> I also think we should rename ic_url_bar_search to be less confusing - it's
> not even used in the url bar!
> 
> Lucas, what are your thoughts?

I agree. Let's simplify this effort.

Comment 28

[:Margaret Leibovic]

To the icon machine!

antlam, see comment 26 to explain what kind of 3 icons we need. I think we already have a correct one for ic_url_bar_search (the search suggestions one), but we probably need updated ones for the action bar and for the favicon.

Comment 29

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

Attachment: iconpack_magnifyingglass.zip

Here goes the icon machine!

I think I got all these right... give it a whirl! :) I found that the smallest ones (ic_URL_bar_search) were not really coming out that well. But I wouldn't worry too much about it for the time being. If we could get a build out to test first, we might find that we could leave those ones as is, but let's try it first :)

Comment 30

[:Margaret Leibovic]

I should also note, I was looking into where ic_url_bar_search was used, and although it's 24x24dp, I found that it's used in a view that's 16x16dp:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/resources/layout/suggestion_item.xml#16

So I told antlam that it should really be 16dp. Just FYI, in case you think the icons are the wrong size :)

Comment 31

[Lucas Rocha (:lucasr)]

Hey amoghbl1, are you still planning to work on this?

Comment 32

[amoghbl1]

hey lucasr, sorry for the delay! I've actually got a partial patch ready. I actually need more info on when to apply the favicon normal and favicon active images, I should have commented that here but I was waiting for monday so that I could discuss it on irc, will get this done tonight.

Comment 33

[:Margaret Leibovic]

(In reply to amoghbl1 from comment #32)
> hey lucasr, sorry for the delay! I've actually got a partial patch ready. I
> actually need more info on when to apply the favicon normal and favicon
> active images, I should have commented that here but I was waiting for
> monday so that I could discuss it on irc, will get this done tonight.

I left you a message on IRC, but I'll also comment here for posterity.

We don't actually have active/inactive favicon states (there is no favicon displayed when the urlbar is active), so you can just use the inactive favicon where we currently set the about:home favicon in Favicons.java.

If we want to tweak the design to have an active favicon, we can do that in a follow-up bug. Let's just focus on swapping the existing images here.

Comment 34

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

(In reply to :Margaret Leibovic from comment #33)
> Let's just focus on swapping the existing images here.

+1

Comment 35

[amoghbl1]

Attachment: fix_1019045_v4.patch

Replaced all files, renamed ab_search -> favicon_search in the favicon class.
Also renamed ic_url_bar_search -> suggestion_item_search as it's used in the suggestion_item xml.

Comment 36

[:Margaret Leibovic]

Attachment: ab_search

It looks like the ab_search icon is too light. Is this the intended color?

Comment 37

[:Margaret Leibovic]

Here's an APK with the patch applied if you want to test: http://people.mozilla.org/~mleibovic/search_icons.apk

The patch looks good to me, I just want to wait to hear from antlam if there's anything we need to tweak before we land this.

Comment 38

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

Comment on attachment 8483098 [details]
ab_search

Not intended to be a different grey there... not sure what went wrong. I thought I sampled the right grey. 

Could you give it to me here so I don't get it wrong again?

Comment 39

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

(In reply to :Margaret Leibovic from comment #37)
> Here's an APK with the patch applied if you want to test:
> http://people.mozilla.org/~mleibovic/search_icons.apk

This feels great! Minus the different weird grey in the ab_search icon..

Comment 40

[:Margaret Leibovic]

(In reply to Anthony Lam (:antlam) from comment #38)
> Comment on attachment 8483098 [details]
> ab_search
> 
> Not intended to be a different grey there... not sure what went wrong. I
> thought I sampled the right grey. 
> 
> Could you give it to me here so I don't get it wrong again?

Here is the icon in the tree:
http://hg.mozilla.org/mozilla-central/raw-file/5e9826980be5/mobile/android/base/resources/drawable-mdpi/ab_search.png

Comment 41

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

Attachment: icon_ab_search2.zip

try these! I think I had an opacity set on the last set.. :\

Comment 42

[amoghbl1]

Attachment: fix_1019045_v5.patch

Replaced ab_search images with the new ones.

Comment 43

[:Margaret Leibovic]

Comment on attachment 8483698 [details] [diff] [review]
fix_1019045_v5.patch

Excellent! I'll land this for you.

Comment 44

[:Margaret Leibovic]

https://hg.mozilla.org/integration/fx-team/rev/ec6952a71088

We can request uplift to Aurora once we make sure there are no problems on Nightly.

Comment 45

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/ec6952a71088

Comment 46

[:Margaret Leibovic]

Lucas, I see that bug 1054372 blocks the toolbar-v2 meta bug, but do we want to uplift this patch to 34 for v1 toolbar refresh?

Comment 47

[Lucas Rocha (:lucasr)]

(In reply to :Margaret Leibovic from comment #46)
> Lucas, I see that bug 1054372 blocks the toolbar-v2 meta bug, but do we want
> to uplift this patch to 34 for v1 toolbar refresh?

Given how simple this change is, I'm fine with uplifting.

Comment 48

[:Margaret Leibovic]

Comment on attachment 8483698 [details] [diff] [review]
fix_1019045_v5.patch

Approval Request Comment
[Feature/regressing bug #]: this improvement is part of the new toolbar design (bug 1052004)
[User impact if declined]: search icons don't look as nice
[Describe test coverage new/current, TBPL]: no tests, landed in m-c 9/3
[Risks and why]: low risk, just swaps some images
[String/UUID change made/needed]: none

Comment 49

[Lawrence Mandel [:lmandel] (use needinfo)]

Comment on attachment 8483698 [details] [diff] [review]
fix_1019045_v5.patch

Nice little polish bug (which is quite different from a nice little Polish bug). Aurora+

Comment 50

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/ad669f120744