1317933 - Custom Tab: Switch theme based on background color?

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Sebastian Kaspari (:sebastian; :pocmo)]

In bug 1316869 we added code to adapt the text color based on the background color defined by the third-party app. However the back/up icon is still white on light backgrounds. We might need to switch the whole theme of the activity (light/dark).

Comment 1

[friedger]

This seems to work:

private void setThemeFromToolbarColor() {
        if (toolbarColor == NO_COLOR) {
            return;
        }

        if (ColorUtil.getReadableTextColor(toolbarColor) == Color.BLACK) {
            setTheme(android.R.style.Theme_Material_Light_NoActionBar);
        } else {
            setTheme(android.R.style.Theme_Material_NoActionBar);
        }

    }

Comment 2

[Sebastian Kaspari (:sebastian; :pocmo)]

Sounds good for now. Do you want to upload a patch for that?

Comment 3

[friedger]

Attachment: 1317933-friedger.patch

Tested on Android 23

Comment 4

[friedger]

Attachment: device-2016-12-21-115802.png

dark color

Comment 5

[friedger]

Attachment: device-2016-12-21-115715.png

light color

Comment 6

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8820649 [details] [diff] [review]
1317933-friedger.patch

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

::: mobile/android/base/java/org/mozilla/gecko/customtabs/CustomTabsActivity.java
@@ +94,5 @@
> +
> +        if (ColorUtil.getReadableTextColor(toolbarColor) == Color.BLACK) {
> +            setTheme(android.R.style.Theme_Material_Light_NoActionBar);
> +        } else {
> +            setTheme(android.R.style.Theme_Material_NoActionBar);

I guess we would need an appcompat theme here to support 4+?

Comment 7

[friedger]

Attachment: 1317933-friedger_2.patch

Now with appcompat theme

Comment 8

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8826955 [details] [diff] [review]
1317933-friedger_2.patch

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

LGTM. Thank you, Friedger!

::: mobile/android/base/java/org/mozilla/gecko/customtabs/CustomTabsActivity.java
@@ +94,5 @@
> +        if (toolbarColor == NO_COLOR) {
> +            return;
> +        }
> +
> +        if (ColorUtil.getReadableTextColor(toolbarColor) == Color.BLACK) {

Technically this utility method was only used here, so it could return something different than a color now (a boolean? a theme?). However we can file a follow-up bug for that if anyone wants to make this nicer.

Comment 9

[Sebastian Kaspari (:sebastian; :pocmo)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/721b8d0c2401e20cb48a82e7eb7ccc8e96e98e35
Bug 1317933 - CustomTabsActivity: Set theme based on toolbar color. r=sebastian

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/721b8d0c2401