1277277 - Consider using the new "Recent" icon for search history suggestions, too

Status: RESOLVED INCOMPLETE

(Firefox for Android Graveyard :: General, defect, P5)

Description

[Jan Henning [:JanH]]

(In reply to Chenxia Liu [:liuche] from comment #124)
> Comment on attachment 8757648 [details]
> MozReview Request: Bug 1251362 - Part 4 - Tint Recent Tabs icon to match the
> colour of the Synced Devices cloud icon. r=liuche
> 
> https://reviewboard.mozilla.org/r/56044/#review53430
> 
> :::
> mobile/android/base/java/org/mozilla/gecko/home/CombinedHistoryAdapter.java:
> 59
> (Diff revision 2)
> >  
> > -    public CombinedHistoryAdapter(Resources resources) {
> > +    public CombinedHistoryAdapter(Context context, Resources resources) {
> >          super();
> >          sectionHeaders = new SparseArray<>();
> >          HistorySectionsHelper.updateRecentSectionOffset(resources, sectionDateRangeArray);
> > +        // Colors chosen so that the end result after tinting is disabled_grey (#BFBFBF).
> 
> We can do this, but I think the real solution is to either have another
> resource (not great) or to get a new resource in white/black that we tint in
> both places.

Comment 1

[Jan Henning [:JanH]]

Antlam has posted some icons which don't need tinting to use them within the History panel, maybe we can use those for the search term history, too and replace the current icon version.
Although in that case the colour we're tinting them with for the search term history (https://dxr.mozilla.org/mozilla-central/rev/e27fe24a746fa839f1cabe198faf1bad42c7dc4b/mobile/android/base/java/org/mozilla/gecko/home/SearchEngineRow.java#148) might need adjusting to maintain the visual end result?

Comment 2

[Jan Henning [:JanH]]

Attachment: old icon.png

This is the old icon we're currently using (icon_most_recent_empty).

Comment 3

[Jan Henning [:JanH]]

Attachment: new icon (tint unchanged).png

And this is how things look if we simply swap it out for the new "icon_recent" that's currently used for the "Recently closed" history smartfolder.

Comment 4

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

(In reply to Jan Henning [:JanH] (away 15 - 27 June) from comment #3)
> Created attachment 8761657 [details]
> new icon (tint unchanged).png
> 
> And this is how things look if we simply swap it out for the new
> "icon_recent" that's currently used for the "Recently closed" history
> smartfolder.

Nice! two quick nits...

1) This looks darker than the previous color... Can I see a screenshot of this tinted correctly?
2) can we maintain the same padding as spec'd out with the old icon? the padding here looks off..

Comment 5

[Jan Henning [:JanH]]

The old icon had a lighter base colour and was semi-transparent on top I think. I can of course try tinting the new icon with a lighter colour to approximately maintain the visual end result.
The same goes for the padding - the original drawable includes some slight padding within the image file itself.
Updated screenshots and a test build will follow...

Comment 6

[Jan Henning [:JanH]]

Attachment: new icon (tint and image dimensions adjusted).png

This is tinting with "disabled_grey" [1], which is only slightly darker than the original colour (according to that screenshot above, the end result of tinting the old icon comes out as #C1C2C6, while disabled_grey is #BFBFBF).
I've also reduced the icon size from 18 dip to 16 dip and accordingly increased the left and right margins by 1 dip each to compensate for the reduced padding on the image file itself.

[1] Which means that we could in fact get rid of the dynamic tinting here, since icon_recent's base colour is already disabled_grey.

Comment 7

[Jan Henning [:JanH]]

APK for testing:
https://archive.mozilla.org/pub/mobile/try-builds/[e-mail]-117ee7bfe73685b062aa307a64d145db627f564d/try-android-api-15/fennec-53.0a1.en-US.android-arm.apk

Comment 8

[Jan Henning [:JanH]]

Attachment: Bug 1277277 - Update the clock icon used for search history suggestions.

The old icon was tinted so that the visual end result approximated disabled_grey. Since the new icon's colour already is disabled_grey, there's no need for tinting anymore.

Review commit: https://reviewboard.mozilla.org/r/123616/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/123616/

Comment 9

[Chenxia Liu [:liuche]]

Comment on attachment 8851265 [details]
Bug 1277277 - Update the clock icon used for search history suggestions.

https://reviewboard.mozilla.org/r/123616/#review126988

Looks great! One possible nit: I doubt the image is big, but if you haven't done so you should pngcrush or run ImageOptim on the new image file.
I see that sebastian has used this in the past: 'pngcrush -brute infile outfile'

Comment 10

[Chenxia Liu [:liuche]]

Oh, my bad, this is a png deletion so nevermind on crushing.

Comment 11

[Jan Henning [:JanH]]

I can check icon_recent.png anyway just to be absolutely sure.

Comment 12

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

(In reply to Jan Henning [:JanH] from comment #6)
> Created attachment 8767482 [details]
> new icon (tint and image dimensions adjusted).png
> 
> This is tinting with "disabled_grey" [1], which is only slightly darker than
> the original colour (according to that screenshot above, the end result of
> tinting the old icon comes out as #C1C2C6, while disabled_grey is #BFBFBF).
> I've also reduced the icon size from 18 dip to 16 dip and accordingly
> increased the left and right margins by 1 dip each to compensate for the
> reduced padding on the image file itself.
> 
> [1] Which means that we could in fact get rid of the dynamic tinting here,
> since icon_recent's base colour is already disabled_grey.

Sorry! but this might change now with some of the Photon UI changes we're looking at. So I think we should hold off on this for now.

Comment 13

[Jan Henning [:JanH]]

Not looking at it at the moment and needs reassessment in how far this is still valid with the Photon UI changes.

Comment 14

[Nevin Chen(Not active on Bugzilla)]

Hi Carol
Do you think this icon fits Photon design?
I think it's nice:)

Comment 15

[Carol Huang [:Carol]]

The icon on Nightly now is Photon style but it's the wrong one. the clock supposed be 3 o'clock. We need to update it.

Hi Nevin, 
Could you let me know the icon on the screen is which icon from below?

search_history.png

icon_most_recent_empty.png

icon_recent.png

Thanks!

Comment 16

[Nevin Chen(Not active on Bugzilla)]

in search suggestion, it's icon_most_recent_empty. 

btw,the image in combined history panel ( Home Panel -> Hisotry -> Recent Closed Tab) is also seven o'clock(icon_recent.png). Not sure if we should also update this one.

Comment 17

[Carol Huang [:Carol]]

Hi Nevin, 
Yes! we need to update all the icon (it supposed to be three o'clock)
for 
icon_most_recent_empty.png
icon_recent.png
search_history.png

Do we need a new bug for replacing these icons?
Here's the asset: https://drive.google.com/drive/folders/19eXPl2FlXPT0yliDRDcZukqHIK8PjSQp?usp=sharing

Let's review it together again once the icon is updated!
thanks!

Comment 18

[Jan Henning [:JanH]]

icon_recent is the newer one and is used for the recently closed tabs smart folder. Originally we only had a single nodpi version that would be resized dynamically, but apparently the photon update changed this into separate copies for each density again.
icon_most_recent_empty is in a lighter grey because it was originally used for the empty layout of the Awesome screen Recently Closed folder, but it is no longer used there. At the moment it is only used for search history suggestions and tinted such that the effective colour is almost the same as that of icon_recent.

The original idea was to save some APK size and also eliminate the dynamic tinting by using icon_recent in both places - since we've also created separate files for each density level again, maybe we should now also replace it with a SVG version to save some more size and end up with a single file again.

Comment 19

[Nevin Chen(Not active on Bugzilla)]

Hi Carol
Can you make these icons SVG and we can update them in this bug.

Hi Jan
How do you think?

Comment 20

[Carol Huang [:Carol]]

Hi Nevin, 
Here's the SVG for the 'clock' icon.
https://drive.google.com/open?id=1xuWKNGNqMXCopWmykpXMjlWncJsI2TlZ

and the icon size/color spec:
https://drive.google.com/open?id=12kcThh01w63frv4Jp0OQb3ExNU79xYo-

Let me know if you need anything else! thank you!!

Comment 21

[Jan Henning [:JanH]]

I've noticed that the search history now actually uses a lighter grey than recently closed tabs, so we won't be able to avoid the tinting, but having a common SVG file for both sounds great, thanks.

Comment 22

[Nevin Chen(Not active on Bugzilla)]

I think that's enoguh. Sorry I'm full now so I'll leave this bug as is till someone help pick it up.

Comment 23

[Firefox Bug Husbandry Bot]

Re-triaging per https://bugzilla.mozilla.org/show_bug.cgi?id=1473195

Needinfo :susheel if you think this bug should be re-triaged.

Comment 24

[BMO Automation]

We have completed our launch of our new Firefox on Android. The development of the new versions use GitHub for issue tracking. If the bug report still reproduces in a current version of [Firefox on Android nightly](https://play.google.com/store/apps/details?id=org.mozilla.fenix) an issue can be reported at the [Fenix GitHub project](https://github.com/mozilla-mobile/fenix/). If you want to discuss your report please use [Mozilla's chat](https://wiki.mozilla.org/Matrix#Connect_to_Matrix) server https://chat.mozilla.org and join the [#fenix](https://chat.mozilla.org/#/room/#fenix:mozilla.org) channel.