Closed Bug 1277277 Opened 8 years ago Closed 3 years ago

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

Categories

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

All
Android
defect

Tracking

(firefox49 affected)

RESOLVED INCOMPLETE
Tracking Status
firefox49 --- affected

People

(Reporter: JanH, Unassigned)

References

Details

Attachments

(3 files, 1 obsolete file)

(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.
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?
Flags: needinfo?(alam)
Summary: Make icon_most_recent_empty more easily tintable → Consider using the new "Recent" icon for search history suggestions, too
Attached image old icon.png
This is the old icon we're currently using (icon_most_recent_empty).
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.
(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..
Flags: needinfo?(alam) → needinfo?(jh+bugzilla)
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...
Flags: needinfo?(jh+bugzilla)
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.
Flags: needinfo?(alam)
Blocks: fatfennec
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'
Attachment #8851265 - Flags: review?(liuche) → review+
Oh, my bad, this is a png deletion so nevermind on crushing.
I can check icon_recent.png anyway just to be absolutely sure.
(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.
Flags: needinfo?(alam)
Attachment #8851265 - Attachment is obsolete: true
Not looking at it at the moment and needs reassessment in how far this is still valid with the Photon UI changes.
Assignee: jh+bugzilla → nobody
Hi Carol
Do you think this icon fits Photon design?
I think it's nice:)
Flags: needinfo?(chuang)
Priority: -- → P3
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!
Flags: needinfo?(chuang) → needinfo?(cnevinchen)
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.
Flags: needinfo?(cnevinchen) → needinfo?(chuang)
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!
Flags: needinfo?(chuang) → needinfo?(cnevinchen)
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.
Hi Carol
Can you make these icons SVG and we can update them in this bug.

Hi Jan
How do you think?
Flags: needinfo?(jh+bugzilla)
Flags: needinfo?(cnevinchen)
Flags: needinfo?(chuang)
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!!
Flags: needinfo?(chuang) → needinfo?(cnevinchen)
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.
Flags: needinfo?(jh+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.
Flags: needinfo?(cnevinchen)
Re-triaging per https://bugzilla.mozilla.org/show_bug.cgi?id=1473195

Needinfo :susheel if you think this bug should be re-triaged.
Priority: P3 → P5
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.
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → INCOMPLETE
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: