1905274 - Replace fx_mobile_private_text_color_primary with fx_mobile_private_icon_color_primary for tab counter menu icons

Status: RESOLVED FIXED

(Firefox for Android :: Toolbar, task, P3)

Description

[Petru-Mugurel Lingurar [:petru]]

As a followup to https://phabricator.services.mozilla.com/D214361#inline-1192788 we should ensure that in all scenarios we provide the icon colour resource for the colours of the icons in the tab counter menu.

At this time we have the following improper usages of text_color where icon_color should be:

• for the tab counter used in the home fragment
• for the tab counter used in the browser fragment
• for the inactive tabs CFR button
• for the info button of the search suggestions in private tabs popup

This allows to start getting familiar with the application features, code and resources.
Can offer more context / help for who would want to pick this up, just add a needinfo for me in this ticket.

** We have also some improper usages in readerview_private_* but these might require more work and consideration and can be left for later.

Comment 1

[Gabriel Luong [:gl] (ΦωΦ)]

@petru, We have an effort as part of #mobile-community-outreach (slack) to improve our good-first-bugs. The following is the guidelines that we have come up with https://docs.google.com/document/d/14-MeY0ymC5OgZAOYJ7Vy14h2ztnlconoMtVWZwqoO6c/edit?usp=sharing
I am looking to see the following: (1) a mentor listed and (2) links to the the file and/or line of code that needs to be changed.

It's currently not totally clear from the comment what needs to be changed since it appears that we already updated fx_mobile_private_text_color_primary to fx_mobile_private_icon_color_primary in your patch.

Comment 2

[Petru-Mugurel Lingurar [:petru]]

Thanks Gabriel!
Updated now the first comment for these details to have better visibility.

Comment 3

[jkurape]

Hello @Petru. First time contributor here.
Does (https://searchfox.org/mozilla-central/source/mobile/android/fenix/app/src/main/res/layout/search_suggestions_hint.xml#35) need to be changed? On a cursory glance the context the "fx_mobile_icon_color_primary" is already defined under the Icon color.

I'd also like to ask if I could be assigned this.

Comment 4

[Petru-Mugurel Lingurar [:petru]]

Welcome @jkurape!
Regarding search_suggestions_hint.xml#35 I see that it's using fx_mobile_private_text_color_primary for an image tint so I think fx_mobile_private_icon_color_primary would be more appropriate
But looking more closely I see that that tools:tint attribute actually breaks the design view for me while no tools:tint shows the icon as expected.
If it's the same for you maybe we can just delete this declaration - preferable in a separate commit as it's a different change than the others & not initially covered in this ticket or leave it for another ticket if you prefer to only focus on the text_color_primary to icon_color_primary update.

Comment 5

[jkurape]

All right @petru.
I'll go ahead create the commit for the text_color_primary to icon_color_primary update. I'll delete the tools:tint declaration in a separate commit.

Comment 6

[jkurape]

Attachment: Bug 1905274 - Provide the correct icon color resource for the colors of the icons in the tab counter menu r?#android-reviewers,petru,gl

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/566ff9e7266f
Provide the correct icon color resource for the colors of the icons in the tab counter menu r=petru,android-reviewers

Comment 8

[tszentpeteri]

https://hg.mozilla.org/mozilla-central/rev/566ff9e7266f

Comment 9

[Petru-Mugurel Lingurar [:petru]]

And the patch is merged.
Thank you @jkurape!

If you're interested in discovering more of Firefox and help to improve it even more there are other tickets recommended for first time contributors here - https://mzl.la/3VYlFdQ

Comment 10

[jkurape]

My pleasure, @petru. Thanks for the help.