Closed Bug 1317933 Opened 8 years ago Closed 7 years ago

Custom Tab: Switch theme based on background color?

Categories

(Firefox for Android Graveyard :: General, defect)

All
Android
defect
Not set
normal

Tracking

(firefox53 fixed)

RESOLVED FIXED
Firefox 53
Tracking Status
firefox53 --- fixed

People

(Reporter: sebastian, Assigned: mail)

References

Details

Attachments

(3 files, 1 obsolete file)

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).
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);
        }

    }
Sounds good for now. Do you want to upload a patch for that?
Attached patch 1317933-friedger.patch (obsolete) — Splinter Review
Tested on Android 23
dark color
light color
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+?
Attachment #8820649 - Flags: feedback+
Now with appcompat theme
Attachment #8820649 - Attachment is obsolete: true
Attachment #8826955 - Flags: review?(s.kaspari)
Assignee: nobody → mail
Status: NEW → ASSIGNED
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.
Attachment #8826955 - Flags: review?(s.kaspari) → review+
https://hg.mozilla.org/mozilla-central/rev/721b8d0c2401
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 53
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

Creator:
Created:
Updated:
Size: