882322 - Find in page bar does not change selection color when set to private mode

Status: RESOLVED INCOMPLETE

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

Description

[Shilpan Bhagat]

Attachment: Find in bar during private mode

Private mode has its own color scheme when selected. However the find in page bar does not seem to follow that.

Comment 1

[Shilpan Bhagat]

Attachment: 'Find in bar' appearance changes based on the selected tab's mode (private/normal)

- Made changes in the find_in_page_content.xml to remove hard coded value of the text color to make it change based on the states
- Made changes in the FindInPageBar.java to make the appearance of the bar change based on the selected tab's state

Comment 2

[Sriram Ramasubramanian [:sriram]]

Comment on attachment 761740 [details] [diff] [review]
'Find in bar' appearance changes based on the selected tab's mode (private/normal)

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

r+ with these changes.

::: mobile/android/base/FindInPageBar.java
@@ +13,5 @@
>  import android.view.View;
>  import android.view.inputmethod.InputMethodManager;
>  import android.widget.LinearLayout;
>  
> +public class FindInPageBar extends LinearLayout implements TextWatcher, View.OnClickListener, Tabs.OnTabsChangedListener {

Please move it to individual lines. BrowserApp is a good example.

@@ +66,5 @@
> +        Tab tab = Tabs.getInstance().getSelectedTab();
> +        if (tab != null && tab.isPrivate() && !mFindText.isPrivateMode())
> +            mFindText.setPrivateMode(true);
> +        else if(tab != null && !tab.isPrivate() && mFindText.isPrivateMode())
> +            mFindText.setPrivateMode(false);    

White spaces.

Also, CustomEditText takes care of not propagating "setPrivateMode()", when passed with same value. So, you could just do,

if (tab != null) {
    mFindText.setPrivateMode(tab.isPrivate());
}

@@ +143,5 @@
> +            case SELECTED:
> +                if (tab != null && tab.isPrivate() && !mFindText.isPrivateMode())
> +                    mFindText.setPrivateMode(true);
> +                else if(tab != null && !tab.isPrivate() && mFindText.isPrivateMode())
> +                    mFindText.setPrivateMode(false);    

Same here.

Comment 3

[Shilpan Bhagat]

Attachment: 'Find in bar' appearance changes based on the selected tab's mode (private/normal)

Does as mentioned above, did some refactoring.

Comment 4

[Shilpan Bhagat]

Attachment: Revision: Changes appearance of 'find in bar' based on the selected tab's mode (private/normal)

More refactoring.

Comment 5

[Sriram Ramasubramanian [:sriram]]

Comment on attachment 761774 [details] [diff] [review]
Revision: Changes appearance of 'find in bar' based on the selected tab's mode (private/normal)

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

Looks good to me.

Comment 6

[Wesley Johnston (:wesj)]

Comment on attachment 761774 [details] [diff] [review]
Revision: Changes appearance of 'find in bar' based on the selected tab's mode (private/normal)

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

So, I kinda want all of these theming things to run through our special GeckoView.java.frag class (which is not in any way at all related to GeckoView.java). In fact, it looks from this like the textbox here is already a CustomEditText class, which inherits from GeckoEditText (which is a GeckoView.java.frag). Because of that, it should automatically set the private mode state when its in private mode:

http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/GeckoView.java.frag#53

and its drawable background should pick that up and use the right image:

http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/resources/drawable/address_bar_url.xml#22

I'm curious why this isn't working already?

Comment 7

[Shilpan Bhagat]

Hey, it seems that you need to set the private mode by calling setPrivateMode. That's how awesomebar does it atleast.

http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/AwesomeBar.java#150

Comment 8

[Shilpan Bhagat]

I am going to put all of this in a viewstub :)

Comment 9

[Shilpan Bhagat]

Please see the above comments

Comment 10

[Wesley Johnston (:wesj)]

Comment on attachment 761774 [details] [diff] [review]
Revision: Changes appearance of 'find in bar' based on the selected tab's mode (private/normal)

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

I wish this was more automagic. i.e.

http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/BrowserToolbar.java#1153

makes me kinda sad, and its guaranteed that at some point we'll add something and forget to update it. Can you file a bug to make this work the way lightweight themes do?

::: mobile/android/base/FindInPageBar.java
@@ +144,5 @@
> +    // Tabs.OnTabsChangedListener implementation for setting the appearance on change of tab
> +    @Override
> +    public void onTabChanged(Tab tab, Tabs.TabEvents msg, Object data) {
> +        switch(msg) {
> +            case SELECTED:

Can you verify that this fires for tabs added or restore?

Comment 11

[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.