Custom Tab: Switch theme based on background color?

RESOLVED FIXED in Firefox 53

Status

()

defect
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: sebastian, Assigned: mail)

Tracking

(Blocks 1 bug)

unspecified
Firefox 53
All
Android
Points:
---
Dependency tree / graph

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

3 years 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

3 years ago
Posted patch 1317933-friedger.patch (obsolete) — Splinter Review
Tested on Android 23
Assignee

Comment 4

3 years ago
dark color
Assignee

Comment 5

3 years ago
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

3 years ago
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+

Comment 10

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/721b8d0c2401
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 53
You need to log in before you can comment on or make changes to this bug.