1212596 - Highlight Remove account preference in red color text

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Vivek Balakrishnan[:vivek]]

This is a follow up to 1205817. 
We would like to highlight the disconnect button in "rejection_red" color.

There are three ways update the text color for Preference in Android.

1) Use custom layout like we do for here message [1]
2) Extend Preference class, we can customize this to change color for 
   both text and summary
3) Use spannables as mentioned here [2] (feels like hack)

[1] http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/resources/xml/fxaccount_status_prefscreen.xml#44
[2] http://goo.gl/jvvVBc

Comment 1

[Vivek Balakrishnan[:vivek]]

Kalpesh,

You did a great job with bug 1205817. 
Are you interested in working on this follow up bug.
I will comment with more details on the approach to solve this by tomorrow.

Comment 2

[Kalpesh Krishna [:martianwars]]

Yeah I would be interested :)

Comment 3

[Vivek Balakrishnan[:vivek]]

Kalpesh,

Thanks for your interest.
To solve this bug lets use method 1 from https://bugzilla.mozilla.org/show_bug.cgi?id=1212596#c0.
I feel the third options is very hacky and I don't see any need for customizable Preference.

I have given reference to custom layouts in the above comment.
Please let me know if something is not clear.

Comment 4

[Kalpesh Krishna [:martianwars]]

Attachment: fxremoveaccountcolor.patch

Do I need to make a new custom layout? The custom layout you have hyperlinked seems to be highlighting in "rejection_red"

Comment 5

[Vivek Balakrishnan[:vivek]]

Kalpesh,

Sorry for the delay in response. Yes, you need to create a new custom layout.
The referenced layout was meant as an example.

For our case we would need a custom layout that has said red color text for "Disconnect..." . 
Also, note that the styles should match otherwise with existing preference.

Please let me know if anything is not clear.

Comment 6

[Vivek Balakrishnan[:vivek]]

Comment on attachment 8672199 [details] [diff] [review]
fxremoveaccountcolor.patch

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

Kalpesh,

I'm closing this review request.
To solve this bug, we need to define a new custom layout that has text color in red.
Also, we don't need icons for this Disconnect preference.

Please NI me if you need more info.

Comment 7

[Kalpesh Krishna [:martianwars]]

Attachment: colorfix.patch

Comment 8

[Vivek Balakrishnan[:vivek]]

Comment on attachment 8673152 [details] [diff] [review]
colorfix.patch

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

Kalpesh,

This patch is missing fxaccount_remove_account.xml
When you add a new file you need to do "hg add file" for mercurial to track it.

Can you please re-submit a new patch.

::: mobile/android/base/resources/xml/fxaccount_status_prefscreen.xml
@@ +105,5 @@
>          <Preference
>              android:editable="false"
>              android:key="remove_account"
>              android:persistent="false"
> +            android:layout="@layout/fxaccount_remove_account"

Also, lets maintain parity with other layout preferences.
can you please rename this to something like fxaccount_remove_account_preference

Comment 9

[Kalpesh Krishna [:martianwars]]

Attachment: colorfix.patch

I hope it's better now.

Comment 10

[Vivek Balakrishnan[:vivek]]

Comment on attachment 8673363 [details] [diff] [review]
colorfix.patch

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

Kalpesh,

This is a really good attempt but we need to improve upon the styling.
Styling this text item will require lots of iteration to get it right.
Instead, we could simply copy the preference layout from android source and then set the text color for title item [1].
This is lame but it is required for this bug.

So the solution for this patch :
1) copy the layout from [1]
2) fix all styling issues that IDE complains
3) set textcolor and text for the title view 
4) set the icon, summary and widget_frame to visibility gone.

Also, note that when you create a new file you always add the Mozilla Public license comment.
For xml files this must be immediately after the xml tag. 
For java/js files this must be very first line even before package declaration. 


Thanks for your effort :)

[1] https://android.googlesource.com/platform/frameworks/base/+/refs/heads/master/core/res/res/layout/preference.xml

::: mobile/android/base/resources/layout/fxaccount_remove_account_preference.xml
@@ +4,5 @@
> +    android:layout_height="match_parent">
> +    <TextView
> +        android:id="@+android:id/title"
> +        style="@style/FxAccountTextItem"
> +        android:layout_width="fill_parent"

This is not a review, just a self note.
While declaring the custom layout always define icon, summary and widget_frame which may be accessed programmatically in java code.
You can set the visibility of these elements to gone.

Comment 11

[Kalpesh Krishna [:martianwars]]

Attachment: colorfix.patch

Hey Vivek, did as you said. I hope it looks okay now

Comment 12

[Vivek Balakrishnan[:vivek]]

Comment on attachment 8674367 [details] [diff] [review]
colorfix.patch

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

Kalpesh,

Thanks you for your effort.
I have a few minor nits and one important license comment issue.

I also noticed that the text is a little bigger in comparison (may be it is just me) and it is not aligned with other preferences.
I need time to investigate why it is not aligned correctly. 
I am leaving the review flag to get back to this alignment issue.

Can you please post a screenshot and ask for review from Nick.
While you do that can you post another screenshot with text size set to @style/TextAppearance.Medium
You can post the screenshot as a patch or post a link to your shared dropbox or google drive folder

Feel free to NI me if you need any info or ping me in irc. My irc nickname is vivek
I am usually signed in #mobile channel.

::: mobile/android/base/resources/layout/fxaccount_remove_account_preference.xml
@@ +1,4 @@
> +<?xml version="1.0" encoding="utf-8"?>
> +<!-- This Source Code Form is subject to the terms of the Mozilla Public
> +   - License, v. 2.0. If a copy of the MPL was not distributed with this
> +   - file, You can obtain one at http://mozilla.org/MPL/2.0/. -->

As we copied this file from android source, don't change the license comment to MPL. That is copy it as it is.

@@ +6,5 @@
> +<LinearLayout xmlns:android="http://schemas.android.com/apk/res/android"
> +android:layout_width="match_parent"
> +android:layout_height="wrap_content"
> +android:minHeight="?android:attr/listPreferredItemHeight"
> +android:gravity="center_vertical"

Please indent with 4 spaces.
Also align the start of android: attribute so that they are aligned with xmlns: attribute
And use line break to separate different element, improves readability :)

@@ +15,5 @@
> +    android:layout_height="wrap_content"
> +    android:visibility="gone"
> +    android:layout_gravity="center" />
> +<RelativeLayout
> +    android:layout_width="wrap_content"

use 0dp instead of wrap_content here

Comment 13

[Kalpesh Krishna [:martianwars]]

@vivek :- I'm having issues (like I mentioned in the previous bug) in running Firefox for Android. How do I detect the strings in my project? The project won't build without that. 
Also, is the testing procedure identical to Android apps, where I use USB Debugging to run the app, or am I supposed to run some form of unit / integrated tests like done in other Mozilla projects like Gaia, Toolkit?

Comment 14

[Vivek Balakrishnan[:vivek]]

@Kalpesh, 

can you post a detailed error log in pastebin.mozilla.org? 
Also can you please let me know how you are building fennec, say after adding a string.
BTW, are you using IntelliJ for development? I think you should, it is awesome :)

I usually just use ctr + click key-combo (Linux) in IntelliJ to open the generated string file.
Note, this file is generated and I use it just for checking the string values. If you edit it, it will lost when you build again.

I am not aware what Gaia or toolkit development process is. Ideally, you should run tests to verify that your changes does not break things.
We have unit tests but it is mostly related to background Sync stuff.
We have robocop tests that runs the tests in your phone through USB [1] but running a complete suite takes like half an hour.

I'm usually happy with manual testing. Robocop tests are run in try server anyway ;)

As I said early, please try to hang out in mobile irc channel. The people there are very friendly and somebody will try help to you out even if I am offline.

[1] https://wiki.mozilla.org/Mobile/Fennec/Android/Testing

Comment 15

[Kalpesh Krishna [:martianwars]]

@vivek,

Well I start my IDE (Android Studio) using the command `sudo ./mach ide androidstudio`
It takes ~4-5 minutes to open up. Everything is fine except the strings. I keep getting the @string references highlighted in red due to a missing strings.xml file. As a result, I can't really test my work.

If it isn't clear, I'll post links to pictures of my screen in my next comment.

I have asked my query on the IRC as well. In the case I figure out the correct procedure before your reply, I will post the updated patch :)

Comment 16

[Kalpesh Krishna [:martianwars]]

@vivek :- Should I switch to IntelliJ IDEA? I had the wrong impression that IntelliJ and AndroidStudio are exactly the same thing.

Comment 17

[Nick Alexander :nalexander [he/him]]

(In reply to Kalpesh Krishna from comment #16)
> @vivek :- Should I switch to IntelliJ IDEA? I had the wrong impression that
> IntelliJ and AndroidStudio are exactly the same thing.

They should work about the same.  I think your issues should be fixed (with strings.xml) if you run |mach build mobile/android| and then in the IDE, choose the Gradle view (View > Tool windows > Gradle) and refresh.  That should get the strings in place.

Unfortunately, I don't think you can see the strings in the design view -- see Bug 1074258.

Comment 18

[Kalpesh Krishna [:martianwars]]

I don't think it is working. This is how my screen looks after refreshing Gradle :- http://picpaste.com/Screenshot_from_2015-10-17_01_56_52-7n6obwv5.png

Would running the command |mach build| and selecting "Firefox for Android" produce a different result when compared to |mach build mobile/android| ? I've done the former

Comment 19

[Kalpesh Krishna [:martianwars]]

Well I got the app to work on my phone after a successful build :) Sorry for the trouble. Do let me know whether the picpaste.com image is something to worry about. I will submit the new patch with the screenshots soon.

Comment 20

[Kalpesh Krishna [:martianwars]]

Attachment: colorfix.patch

Well this method seems to give a good result. Posting screenshots in subsequent posts

Comment 21

[Kalpesh Krishna [:martianwars]]

Attachment: How Disconnect... looks.

Comment 22

[Nick Alexander :nalexander [he/him]]

(In reply to Kalpesh Krishna from comment #18)
> I don't think it is working. This is how my screen looks after refreshing
> Gradle :-
> http://picpaste.com/Screenshot_from_2015-10-17_01_56_52-7n6obwv5.png
> 
> Would running the command |mach build| and selecting "Firefox for Android"
> produce a different result when compared to |mach build mobile/android| ?
> I've done the former

Kalpesh -- I think you meant "|mach bootstrap| and selecting "Firefox for Android"", not mach build.  That configures your environment but doesn't actually do the build.  The |mach build mobile/android| after that will update the files, including making sure strings.xml gets generated.

Comment 23

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8675139 [details] [diff] [review]
colorfix.patch

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

I don't think a custom preference layout is necessary: I think you should be able to do this with a custom preference class and the regular layout.  Then you just update the text color in code, and don't worry about the spacing, etc.  (The title IDs are guaranteed to be fixed since Android expects them.)

See http://stackoverflow.com/a/14054989.

Comment 24

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8675143 [details] [diff] [review]
How Disconnect... looks.

Let's try to get the visual style just right -- see my earlier comment about using a custom preference, not custom layout.

Comment 25

[Vivek Balakrishnan[:vivek]]

Hi Kalpesh,

I'm sorry to have lead you down the wrong path with custom preference layouts.
I was convinced earlier that this will be a simpler fix with custom layouts but seems like 
it is actually quite difficult to get the visual styling right. 
At least this was a good learning experience for me. Hope it was same for you.

As Nick demonstrated in the previous comment, we can solve this bug with custom preference class .

Are you still interested in seeing this bug to completion?

Please feel free to NI/ping me in case if you have some queries.

Comment 26

[Kalpesh Krishna [:martianwars]]

Yes absolutely! Contributing brings cheer up my day :D This was a good learning experience for me as well. 
I have a few queries though :- 
1. What was the exact issue with the custom layout? The latest patch I sent you had the appropriate text size and style (when compared to other menu options) in my opinion.
2. Could you tell me about some files implementing a custom preference which will assist me while I re-work on this bug? Or an online guide to the same. 

Hoping to do a good job!

Comment 27

[Vivek Balakrishnan[:vivek]]

Kalpesh,

Thanks for asking the right questions.

1) The issue with custom layout is that the android has two variants of preference layout -> one we copied and the another for holo themes named preference_holo [1]. Preference_holo defines a padding left on the top level linear layout of 8dip which is not there in other regular layout. Also, the text appearance is textAppearanceMedium not textAppearanceLarge. 

In Fennec, on pre-21 android devices preferences inherit from Themes.holo.Light style [2]. So, that's why was I was having the preference misalignment  with my Kitkat device while you didn't have any such issue with your lollypop. Here is a screen grab from my phone for the regular and holo layouts [3].

This google group discussion covers on this issue [4].

Once again thanks for asking this question, this was good learning experience for me :).
Please let me know if something was not clear.

I will answer your second question as a separate comment.


[1] https://android.googlesource.com/platform/frameworks/base/+/refs/heads/master/core/res/res/layout/preference_holo.xml#25

[2] http://mxr.mozilla.org/mozilla-central/search?string=GeckoPreferencesBase&find=&findi=&filter=^[^\0]*%24&hitlimit=&tree=mozilla-central

[3] https://www.dropbox.com/sh/fszv7xw4qfmu5f1/AACNNvF1ZO5ReFre_38C2p-ha?dl=0

[4] https://groups.google.com/forum/#!topic/android-developers/R8DrriOnuGQ

Comment 28

[Kalpesh Krishna [:martianwars]]

Hi Vivek, yeah it's pretty much clear now. Thanks for the effort. 
Eagerly waiting for the second answer.

Comment 29

[Vivek Balakrishnan[:vivek]]

@Kalpesh,

Please refer CustomCheckBoxPreference class for pointer on how to solve this bug. [1]

Lets us not hard code the color info in Java file. So define some custom attributes and apply the text color in onBindView() [2]. Also, Lets do this correct so that it is reusable i.e apply separate colors for both title and summary.

As always please feel free to NI me if something is not clear.

[1] http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/preferences/CustomCheckBoxPreference.java#21
[2] http://developer.android.com/training/custom-views/create-view.html#customattr

Comment 30

[Kalpesh Krishna [:martianwars]]

Attachment: colorfix.patch

Hey Vivek, I tried to use the links you sent me. I guess I still have to tell the Preference about this custom layout class. I saw a few examples doing them via Java. How could I do the same? Also, do let me know the corrections in the patch I'm submitting.

Comment 31

[Vivek Balakrishnan[:vivek]]

Comment on attachment 8678346 [details] [diff] [review]
colorfix.patch

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

@Kalpesh,

I am yet to test your patch on a real device, I will wait for an updated version :)
Please see my review comments below.

In the prefscreen.xml you can directly call this preference with the full qualified package name like 
<org.mozilla.gecko.preferences.RemoveAccountPreference
            android:editable="false"
            android:key="remove_account"
            android:persistent="false"
            gecko:titleColor="@color/rejection_red"
            android:title="@string/fxaccount_remove_account" />

You should declare the xml namespace for gecko in PreferenceScreen element [1].
Check the AlignRightLinkPreference example [2].
It is fine if you want to drop the namespace for these custom attributes like we do in LinkPreference.


[1] http://developer.android.com/training/custom-views/create-view.html#customattr
[2] http://mxr.mozilla.org/mozilla-central/search?string=LinkPreference
Also, note that you need to add the java class to moz.build like here https://dxr.mozilla.org/mozilla-central/source/mobile/android/base/moz.build#417

::: mobile/android/base/preferences/RemoveAccountPreference.java
@@ +1,1 @@
> +package org.mozilla.gecko.preferences;

You forgot the Mozilla license comment :)

@@ +7,5 @@
> +import android.view.View;
> +import android.widget.TextView;
> +
> +
> +class RemoveAccountPreference extends Preference {

Please rename this class to CustomColoredPreference or something similar.

@@ +13,5 @@
> +    private int mTitleColor, mSummaryColor;
> +    public RemoveAccountPreference(Context context) {
> +        super(context);
> +    }
> +    public RemoveAccountPreference(Context context, AttributeSet attrs) {

Please add the third constructor variant with (Context context, AttributeSet attrs, int defStyle). 
Also, to avoid code duplication you move the come code to a init() function.

@@ +24,5 @@
> +
> +    @Override
> +    protected void onBindView(View view) {
> +        super.onBindView(view);
> +        title = (TextView) view.findViewById(android.R.id.title);

I am not convinced that a null check is needed here, so I guess this is fine.

::: mobile/android/base/resources/values/attrs.xml
@@ +167,5 @@
>          <attr name="inactiveTextColor" format="color" />
>          <attr name="titlebarFill" format="boolean" />
>      </declare-styleable>
>  
> +    <declare-styleable name="RemoveAccountPreference">

Change CustomColorPreference or something similar

Comment 32

[Vivek Balakrishnan[:vivek]]

@Kalpesh,
Please attach a screen grab with you patch and ask review from Nick

Comment 33

[Kalpesh Krishna [:martianwars]]

Well I am getting the following error in my Android Studio :- 

[ 504769]   WARN - g4idea.command.HgStatusCommand - Could not execute hg status command [abort: path 'obj-arm-linux-androideabi/mobile/android/gradle/base/src/main/java/org/mozilla/gecko/preferences/RemoveAccountPreference.java' traverses symbolic link 'obj-arm-linux-androideabi/mobile/android/gradle/base/src/main/java/org/mozilla/gecko'] 

Can't really build the project due to this.

Comment 34

[Kalpesh Krishna [:martianwars]]

Well slight error above, the error message is shown here :- https://pastebin.mozilla.org/8850302

Comment 35

[Kalpesh Krishna [:martianwars]]

Attachment: colorfix.patch

I haven't really tested it due to the Gradle build error. I have built my project using |sudo mach build mobile/android|

Comment 36

[Kalpesh Krishna [:martianwars]]

Attachment: colorfix.patch

Sorry, messed up :/ Had forgotten to add the renamed file.

Comment 37

[Vivek Balakrishnan[:vivek]]

Comment on attachment 8678450 [details] [diff] [review]
colorfix.patch

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

@Kalpesh,

Your patch looks good except for the couple of changes I have asked for.

Regarding the build failure, You need to add an entry for CustomColorPreference.java in mobile/android/base/moz.build file. 
This is the metadata file for the build process and I suspect this is the root cause for your build failure.

Please address the following review comments.

::: mobile/android/base/preferences/CustomColorPreference.java
@@ +10,5 @@
> +import android.util.AttributeSet;
> +import android.view.View;
> +import android.widget.TextView;
> +
> +import org.w3c.dom.Attr;

Please remove this import.

::: mobile/android/base/resources/xml/fxaccount_status_prefscreen.xml
@@ +106,3 @@
>              android:editable="false"
>              android:key="remove_account"
>              android:persistent="false"

Please use the custom attribute "titleColor" to define the color.
As a part of this change, you need to add a namespace for the custom attribute

@@ +106,5 @@
>              android:editable="false"
>              android:key="remove_account"
>              android:persistent="false"
>              android:title="@string/fxaccount_remove_account" />
> +

Nit. Remove this new line

Comment 38

[Vivek Balakrishnan[:vivek]]

Comment on attachment 8678450 [details] [diff] [review]
colorfix.patch

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

::: mobile/android/base/preferences/CustomColorPreference.java
@@ +32,5 @@
> +
> +    }
> +
> +    public void init(Context context,AttributeSet attrs) {
> +        TypedArray a = context.obtainStyledAttributes(attrs, R.styleable.CustomColorPreference);

Just realized that you are also missing the import for R.java

Comment 39

[Kalpesh Krishna [:martianwars]]

My moz.build file :- https://pastebin.mozilla.org/8850949

Comment 40

[Vivek Balakrishnan[:vivek]]

@Kalpesh,

Can you post only the relevant section of your moz.build file. Sorry ,that pastebin link doesn't include your relevant changes.

Comment 41

[Kalpesh Krishna [:martianwars]]

@vivek :- I'm sorry, I copied the wrong file. I've added the entry, I'll attach the updated patch soon and let you know about the build status.

Comment 42

[Kalpesh Krishna [:martianwars]]

Attachment: colorfix.patch

I am getting the following build error now :( :- 
https://pastebin.mozilla.org/8850956

Comment 43

[Vivek Balakrishnan[:vivek]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=ea74edef0997

Comment 44

[Vivek Balakrishnan[:vivek]]

Comment on attachment 8681256 [details] [diff] [review]
colorfix.patch

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

@Kalpesh,

You patch did compile fine for me and it did not fail in try as well :). 
Can you please try a clobber build?

Overall the patch looks good except for a few minor nits.
Also, note that the patch did not apply cleanly against fx-team.
Can you please submit a new patch rebased against latest fx-team branch addressing those nits.

::: mobile/android/base/preferences/CustomColorPreference.java
@@ +2,5 @@
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +package org.mozilla.gecko.preferences;
> +import org.mozilla.gecko.R;

Add a new line between package and import

@@ +11,5 @@
> +import android.util.AttributeSet;
> +import android.view.View;
> +import android.widget.TextView;
> +
> +

Can you add a comment explaining what this class is about.
Also, kill the extra lines.

@@ +16,5 @@
> +
> +class CustomColorPreference extends Preference {
> +    private TextView title, summary;
> +    private int mTitleColor, mSummaryColor;
> +    public CustomColorPreference(Context context) {

Add a new line between fields and constructor

@@ +27,5 @@
> +    }
> +
> +    public CustomColorPreference(Context context, AttributeSet attrs, int defStyle) {
> +        super(context, attrs, defStyle);
> +        init(context,attrs);

nit: space between arguments

@@ +31,5 @@
> +        init(context,attrs);
> +
> +    }
> +
> +    public void init(Context context,AttributeSet attrs) {

nit: space between parameters

::: mobile/android/base/resources/xml/fxaccount_status_prefscreen.xml
@@ +1,3 @@
>  <?xml version="1.0" encoding="utf-8"?>
>  <PreferenceScreen xmlns:android="http://schemas.android.com/apk/res/android"
> +    xmlns:gecko="http://schemas.android.com/apk/res-auto"

Can you please align it so that this line and the next start right below xmlns:android

Comment 45

[Kalpesh Krishna [:martianwars]]

Attachment: colorfix.patch

I hope it is okay now. 
|./mach clobber| worked successfully for me. If this isn't the correct command for the clobber build, do let me know.

Comment 46

[Vivek Balakrishnan[:vivek]]

Comment on attachment 8681788 [details] [diff] [review]
colorfix.patch

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

@Kalpesh,

Thanks for being patient with my pedantic reviews.
We are almost there, can you please address the following comments.

::: mobile/android/base/preferences/CustomColorPreference.java
@@ +12,5 @@
> +import android.view.View;
> +import android.widget.TextView;
> +
> +/**
> + * This class is used to assign the rejection red color to a

change this to something like "This preference is used to define custom colors both title and summary texts. placeholder grey color is used as fallback color for both title and summary."

@@ +33,5 @@
> +
> +    public CustomColorPreference(Context context, AttributeSet attrs, int defStyle) {
> +        super(context, attrs, defStyle);
> +        init(context,attrs);
> +

nit: remove this extra line

@@ +38,5 @@
> +    }
> +
> +    public void init(Context context,AttributeSet attrs) {
> +        TypedArray a = context.obtainStyledAttributes(attrs, R.styleable.CustomColorPreference);
> +        mTitleColor = a.getColor(R.styleable.CustomColorPreference_titleColor, R.color.rejection_red);

use placeholder_grey as fallback for title

::: mobile/android/base/resources/xml/fxaccount_status_prefscreen.xml
@@ +1,4 @@
>  <?xml version="1.0" encoding="utf-8"?>
>  <PreferenceScreen xmlns:android="http://schemas.android.com/apk/res/android"
> +                  xmlns:gecko="http://schemas.android.com/apk/res-auto"
> +    android:key="status_screen">

Please align this. Sorry for not mentioning it earlier

Comment 47

[Kalpesh Krishna [:martianwars]]

@vivek :- No it's important to be pedantic. I didn't get what you mean by fallback for title?

Comment 48

[Kalpesh Krishna [:martianwars]]

Attachment: colorfix.patch

Fixing the nits

Comment 49

[Vivek Balakrishnan[:vivek]]

Comment on attachment 8681983 [details] [diff] [review]
colorfix.patch

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

@Kalpesh,

Can you please change the class comment as mentioned below and upload a new patch.
Apart from that it looks good to me.
Thanks for your effort.

Also, requesting @Nick for a super review.

::: mobile/android/base/preferences/CustomColorPreference.java
@@ +14,5 @@
> +
> +/**
> + * This preference is used to define custom
> + * colors for both title and summary texts.
> + * Placeholder grey color is used as the 

Can you please replace the entire comment section with this

 /**
 * This preference is used to define custom  colors for both title and summary texts.
 * Color code #777777 (placeholder_grey) is used as the fallback color for both title and summary.
 */

Note that line width for fennec is 120 characters.Also avoid trailing spaces at end of a line.

Comment 50

[Kalpesh Krishna [:martianwars]]

Attachment: colorfix.patch with comment update

Comment 51

[Vivek Balakrishnan[:vivek]]

Comment on attachment 8682041 [details] [diff] [review]
colorfix.patch with comment update

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

@Kalpesh,

Thanks for the quick turn around. 
I'll wait for Nick's review before marking landing it.

Comment 52

[Kalpesh Krishna [:martianwars]]

@vivek :- That's excellent :) Any good bug I could work on next?

Comment 53

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8682041 [details] [diff] [review]
colorfix.patch with comment update

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

Looking good!  A few nits (small details) and one real thing (no need to save the View elements).

::: mobile/android/base/preferences/CustomColorPreference.java
@@ +15,5 @@
> + /**
> + * This preference is used to define custom  colors for both title and summary texts.
> + * Color code #777777 (placeholder_grey) is used as the fallback color for both title and summary.
> + */
> +

nit: no newline.

@@ +17,5 @@
> + * Color code #777777 (placeholder_grey) is used as the fallback color for both title and summary.
> + */
> +
> +class CustomColorPreference extends Preference {
> +    private TextView title, summary;

nit: one field per line.  Subsequent changes (removals) don't touch more than the one element they concern.

@@ +34,5 @@
> +        super(context, attrs, defStyle);
> +        init(context,attrs);
> +    }
> +
> +    public void init(Context context,AttributeSet attrs) {

nit: space after ,.

@@ +44,5 @@
> +
> +    @Override
> +    protected void onBindView(View view) {
> +        super.onBindView(view);
> +        title = (TextView) view.findViewById(android.R.id.title);

There's no need to save these as fields, since they are only set and never read.

::: mobile/android/base/resources/values/attrs.xml
@@ +167,5 @@
>          <attr name="inactiveTextColor" format="color" />
>          <attr name="titlebarFill" format="boolean" />
>      </declare-styleable>
>  
> +    <declare-styleable name="CustomColorPreference">

Nifty!  I learned something.

Comment 54

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8681983 [details] [diff] [review]
colorfix.patch

I've reviewed a later version.

Comment 55

[Kalpesh Krishna [:martianwars]]

@nalexander,

I didn't quite get what you meant by "save these as fields" :(

Comment 56

[Nick Alexander :nalexander [he/him]]

(In reply to Kalpesh Krishna from comment #55)
> @nalexander,
> 
> I didn't quite get what you meant by "save these as fields" :(

No need to store them as member variables.  That is, get rid of

 private TextView title, summary;

Comment 57

[Kalpesh Krishna [:martianwars]]

Okay so what would I do to 
> title = (TextView) view.findViewById(android.R.id.title);
> summary = (TextView) view.findViewById(android.R.id.summary);
> title.setTextColor(mTitleColor);
> summary.setTextColor(mSummaryColor);

Do I need to setTextColor at all? or should I do it like this :- 
> ( (TextView) view.findViewById(android.R.id.summary)).setTextColor(mSummaryColor);

Comment 58

[Nick Alexander :nalexander [he/him]]

(In reply to Kalpesh Krishna from comment #57)
> Okay so what would I do to 
> > title = (TextView) view.findViewById(android.R.id.title);
> > summary = (TextView) view.findViewById(android.R.id.summary);
> > title.setTextColor(mTitleColor);
> > summary.setTextColor(mSummaryColor);
> 
> Do I need to setTextColor at all? or should I do it like this :- 
> > ( (TextView) view.findViewById(android.R.id.summary)).setTextColor(mSummaryColor);

Could do.  Up to you if you want an intermediate:

final TextView title = (TextView) ...
title.setTextColor(...)

Comment 59

[Kalpesh Krishna [:martianwars]]

Attachment: colorfix.patch, removing member variables

I hope it is okay now @nalexander

Comment 60

[Vivek Balakrishnan[:vivek]]

https://hg.mozilla.org/integration/fx-team/rev/373ee6962940137f8e71ea423ec456ff17849da8
Bug 1212596 - Highlight Remove account in rejection red. r=vivek

Comment 61

[Kalpesh Krishna [:martianwars]]

Should I add the "checkin-needed"?

Comment 62

[Vivek Balakrishnan[:vivek]]

No need. I landed this for you :)

Comment 63

[Kalpesh Krishna [:martianwars]]

Marking this as resolved hence. :)

Comment 64

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/373ee6962940

Comment 65

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8682776 [details] [diff] [review]
colorfix.patch, removing member variables

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

vivek landed this, all good.