Custom Tab: Switch theme based on background color?

RESOLVED FIXED in Firefox 53

Status

()

Firefox for Android
General
RESOLVED FIXED
7 months ago
5 months ago

People

(Reporter: sebastian, Assigned: friedger)

Tracking

(Blocks: 1 bug)

unspecified
Firefox 53
All
Android
Points:
---

Firefox Tracking Flags

(firefox53 fixed)

Details

Attachments

(3 attachments, 1 obsolete attachment)

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).
(Assignee)

Comment 1

6 months ago
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?
(Assignee)

Comment 3

6 months ago
Created attachment 8820649 [details] [diff] [review]
1317933-friedger.patch

Tested on Android 23
(Assignee)

Comment 4

6 months ago
Created attachment 8820651 [details]
device-2016-12-21-115802.png

dark color
(Assignee)

Comment 5

6 months ago
Created attachment 8820652 [details]
device-2016-12-21-115715.png

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+
(Assignee)

Comment 7

5 months ago
Created attachment 8826955 [details] [diff] [review]
1317933-friedger_2.patch

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/integration/mozilla-inbound/rev/721b8d0c2401e20cb48a82e7eb7ccc8e96e98e35
Bug 1317933 - CustomTabsActivity: Set theme based on toolbar color. r=sebastian

Comment 10

5 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/721b8d0c2401
Status: ASSIGNED → RESOLVED
Last Resolved: 5 months ago
status-firefox53: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 53
You need to log in before you can comment on or make changes to this bug.