641906 - Use system theme colors in nsLookAndFeel on Android

Status: RESOLVED FIXED

(Core Graveyard :: Widget: Android, defect)

Description

[Alex Pakhotin (:alexp)]

Current implementation of nsLookAndFeel on Android has all the colors hard-coded.
We should get them from the system instead.

Comment 1

[Alex Pakhotin (:alexp)]

My first research shows that not many colors from the Android theme actually seem to be easily accessible. I am sure we won't find most of the colors defined in nsLookAndFeel list. I guess we need to match the base colors, and select our pre-defined theme which goes well with those.

What colors and other metrics defined in nsLookAndFeel list are really important, so they must correspond to the system theme?
http://mxr.mozilla.org/mozilla-central/source/widget/public/nsILookAndFeel.h
http://mxr.mozilla.org/mozilla-central/source/widget/src/android/nsLookAndFeel.cpp

At this point I can distinguish between the dark and light color schemes. We could use this information to select one of two our own schemes.

Comment 2

[Brad Lassey [:blassey] (use needinfo?)]

Alex, here's an example of how to get the system's highlight color. This approach should be extendable to getting the other stuff nsLookAndFeel needs.

    public static int getHighlightColor() {
        int color = Color.rgb(0x83, 0xCF, 0xF1); // a default
        final ContextThemeWrapper contextThemeWrapper = 
            new ContextThemeWrapper(GeckoApp.mAppContext,
                                    android.R.style.TextAppearance);
        final TypedArray appearance = contextThemeWrapper.getTheme().
            obtainStyledAttributes(new int[]
            {android.R.attr.textColorHighlight});
        
        if (0 < appearance.getIndexCount()) {
            int attr = appearance.getIndex(0);
            color = appearance.getColor(attr, color);
            Log.i("GeckoResources", attr + " color: " + Color.red(color) +
                  ", " + Color.green(color) + ", " + Color.blue(color));
        }
        appearance.recycle();
        return color;
    }

Comment 3

[Alex Pakhotin (:alexp)]

(In reply to comment #2)
> Alex, here's an example of how to get the system's highlight color.

Interesting... I was very close - tried basically the same approach, but in my case obtainStyledAttributes() returned some weird array.

Thank you - I'll use this way.

Comment 5

[Alex Pakhotin (:alexp)]

Attachment: [WIP] Get some system colors

Comment 6

[Alex Pakhotin (:alexp)]

I did more experiments and investigation (including other systems). Right now it looks like only two colors from the nsLookAndFeel could be used in Fennec UI: eColor_graytext, and eColor__moz_dialog (actually used for the pages background before they load), when not overridden in the preferences. The rest seem to be defined in CSS.
We need to decide what colors we really need from the system, and get them specifically.

I'd recommend to get the colors on case-by-case basis, as some of them in Android theme are defined through attribute references, so they require additional processing in Java to get the actual color value. I'm sure we want to reduce the Java part of this as much as possible, as this code runs at startup, so we should process only necessary colors.

Comment 7

[Mark Finkle (:mfinkle) (use needinfo?)]

I would get the code in place to handle any of the nsILookAndFeel colors or constants.

Yes, we do not use many "named" colors in Fennec at the moment. It's a chicken/egg problem. When you get a selection of Android colors available, we can start to use them by changing the CSS.

Off the top of my head, some colors and constants we care about:
* Font sizes - does Android tell us what the system font sizes for different UIs?
* Button text and background colors
* Dialog text and background colors (any other dialog styling too)
* Window test and background colors (preferences lists for example)
* Selection text and background colors
* Toaster popup text size, color and background color
* Progress bar color

These are the things that come to mind quickly.

Comment 8

[Matt Brubeck (:mbrubeck)]

We'd also like to know background/foreground/highlight colors for the menu (the one that appears when you press the "Menu" system key), though I realize that like much of the Android skin these are partly images rather than flat colors.

Comment 9

[Alex Pakhotin (:alexp)]

Attachment: Use the colors from the system

Get the colors from the system theme in nsLookAndFeel, and make them available through nsILookAndFeel interface.

This is not an ideal implementation yet. The light theme currently used by Fennec has some inconsistencies, for example, both panelColorForeground and panelColorBackground are black, so apparently they cannot be used together. That's why in the current implementation colorForeground and colorBackground are used instead.

Comment 10

[Brad Lassey [:blassey] (use needinfo?)]

Comment on attachment 523060 [details] [diff] [review]
Use the colors from the system

one nit, do the red and blue swap in the AndroidBridge method

Comment 11

[Alex Pakhotin (:alexp)]

Attachment: Fix v3

Moved color conversion to the AndroidBridge as suggested.

However, this still needs some debugging, as for some reason this patch (and the previous version as well) applied to the latest code somehow causes plugin-container process to crash.

Comment 12

[Alex Pakhotin (:alexp)]

Attachment: Fix v4 - no crash

It doesn't yet work in the content process (though not crashing anymore), but I think it'd be better to make that a separate bug, because it will require e10s-specific changes. A remote call implementation turned out to be a bit more tricky than expected - some interface changes might be needed.

Brad, if this patch looks good for you, let's submit this version, and I'll file a follow-up bug.

Comment 13

[Alex Pakhotin (:alexp)]

Attachment: [WIP] Made it work in the content process

Call LookAndFeel::GetColor remotely when in the content process.

Comment 14

[Brad Lassey [:blassey] (use needinfo?)]

(In reply to comment #12)
> Created attachment 527723 [details] [diff] [review]
> Fix v4 - no crash
> 
> It doesn't yet work in the content process (though not crashing anymore), but I
> think it'd be better to make that a separate bug, because it will require
> e10s-specific changes. A remote call implementation turned out to be a bit more
> tricky than expected - some interface changes might be needed.
> 
> Brad, if this patch looks good for you, let's submit this version, and I'll
> file a follow-up bug.

I don't see a reason to land a patch that doesn't work for the content process at this point. Is there anything wrong with attachment 528501 [details] [diff] [review]?

Comment 15

[Alex Pakhotin (:alexp)]

Attachment: Fix v5

Remote call from the content process now gets all the colors in one batch.
Combined both patches into one.

Comment 16

[Brad Lassey [:blassey] (use needinfo?)]

Comment on attachment 528754 [details] [diff] [review]
Fix v5

Review of attachment 528754 [details] [diff] [review]:

this is mostly good, but I'd like see a patch that uses a struct of nscolors before r+

::: dom/ipc/ContentParent.cpp
@@ +107,5 @@
 #include "nsISupportsPrimitives.h"
 static NS_DEFINE_CID(kCClipboardCID, NS_CLIPBOARD_CID);
 static const char* sClipboardTextFlavors[] = { kUnicodeMime };
 
+static NS_DEFINE_CID(kLookAndFeelCID, NS_LOOKANDFEEL_CID);

I don't think you need to include nsILookAndFeel.h or define kLookAndFeeelCID

::: widget/src/android/AndroidBridge.cpp
@@ +681,5 @@
     GetJNIForThread()->CallStaticVoidMethod(mGeckoAppShellClass, jSetSelectedLocale, jLocale);
 }
 
 void
+AndroidBridge::GetSystemColors(nscolor* aColors, PRUint32& aLen)

might me nice to define a struct of nscolors to use here so the length is defined and the order is set

::: widget/src/android/nsLookAndFeel.cpp
@@ +71,5 @@
+    eSysColor_panelColorBackground,
+    eSysColor_count
+};
+
+static nscolor sSystemColors[eSysColor_count];

you'd use that struct of nscolors here rather than an array and an enumeration

@@ +84,5 @@
+#define ANDROID_HIGHLIGHT_COLOR                sSystemColors[eSysColor_textColorHighlight]
+#define ANDROID_FG_NORMAL_COLOR                sSystemColors[eSysColor_colorForeground]
+#define ANDROID_BG_NORMAL_COLOR                sSystemColors[eSysColor_colorBackground]
+#define ANDROID_FG_PANEL_COLOR                 sSystemColors[eSysColor_panelColorForeground]
+#define ANDROID_BG_PANEL_COLOR                 sSystemColors[eSysColor_panelColorBackground]

these defines seem unnecessary

Comment 17

[Alex Pakhotin (:alexp)]

Attachment: Fix v6

Used a structure instead of an array.

Comment 18

[Brad Lassey [:blassey] (use needinfo?)]

Comment on attachment 529254 [details] [diff] [review]
Fix v6

Review of attachment 529254 [details] [diff] [review]:

r=blassey

Comment 19

[Alex Pakhotin (:alexp)]

Attachment: Fix v6 (final)

Updated to the latest code.
r=blassey

Comment 20

[Boris Zbarsky [:bzbarsky]]

Pushed http://hg.mozilla.org/mozilla-central/rev/d6d104661d30

Comment 21

[Paul [pwd]]

This landed some days ago and still I'm not seeing Fennec matching the colours of my theme for some reason.

Comment 22

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to comment #21)
> This landed some days ago and still I'm not seeing Fennec matching the
> colours of my theme for some reason.

Indeed. This patch only made it possible for the application to use the system colors. We still need to land other work (in other bugs) to actually make use of the system colors.

Comment 23

[Paul [pwd]]

Thanks for clearing that up. Could you, if time permits, erect a dependency tree?