Use system theme colors in nsLookAndFeel on Android

RESOLVED FIXED in Firefox 6

Status

()

Core
Widget: Android
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: alexp, Assigned: alexp)

Tracking

(Blocks: 1 bug)

Trunk
mozilla6
All
Android
Points:
---
Bug Flags:
in-testsuite ?

Firefox Tracking Flags

(firefox6 fixed, fennec6+)

Details

Attachments

(1 attachment, 7 obsolete attachments)

(Assignee)

Description

6 years ago
Current implementation of nsLookAndFeel on Android has all the colors hard-coded.
We should get them from the system instead.
(Assignee)

Updated

6 years ago
Assignee: nobody → alexp
Status: NEW → ASSIGNED
tracking-fennec: --- → 2.0next+
(Assignee)

Comment 1

6 years ago
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.
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;
    }
(Assignee)

Comment 3

6 years ago
(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.
Duplicate of this bug: 610632
(Assignee)

Comment 5

6 years ago
Created attachment 520408 [details] [diff] [review]
[WIP] Get some system colors
(Assignee)

Comment 6

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

Comment 9

6 years ago
Created attachment 523060 [details] [diff] [review]
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.
Attachment #520408 - Attachment is obsolete: true
(Assignee)

Updated

6 years ago
Attachment #523060 - Attachment description: [WIP] Use the colors from the system → Use the colors from the system
Attachment #523060 - Flags: review?(blassey.bugs)
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
Attachment #523060 - Flags: review?(blassey.bugs) → review+
(Assignee)

Comment 11

6 years ago
Created attachment 525120 [details] [diff] [review]
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.
Attachment #523060 - Attachment is obsolete: true
Whiteboard: [fennec-6]
(Assignee)

Comment 12

6 years ago
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.
Attachment #525120 - Attachment is obsolete: true
Attachment #527723 - Flags: review?(blassey.bugs)
(Assignee)

Comment 13

6 years ago
Created attachment 528501 [details] [diff] [review]
[WIP] Made it work in the content process

Call LookAndFeel::GetColor remotely when in the content process.
(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]?
(Assignee)

Comment 15

6 years ago
Created attachment 528754 [details] [diff] [review]
Fix v5

Remote call from the content process now gets all the colors in one batch.
Combined both patches into one.
Attachment #527723 - Attachment is obsolete: true
Attachment #528501 - Attachment is obsolete: true
Attachment #527723 - Flags: review?(blassey.bugs)
Attachment #528754 - Flags: review?(blassey.bugs)
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
Attachment #528754 - Flags: review?(blassey.bugs) → review-
(Assignee)

Comment 17

6 years ago
Created attachment 529254 [details] [diff] [review]
Fix v6

Used a structure instead of an array.
Attachment #528754 - Attachment is obsolete: true
Attachment #529254 - Flags: review?(blassey.bugs)
Comment on attachment 529254 [details] [diff] [review]
Fix v6

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

r=blassey
Attachment #529254 - Flags: review?(blassey.bugs) → review+
tracking-fennec: 2.0next+ → 6+
Whiteboard: [fennec-6]
(Assignee)

Comment 19

6 years ago
Created attachment 530424 [details] [diff] [review]
Fix v6 (final)

Updated to the latest code.
r=blassey
Attachment #529254 - Attachment is obsolete: true
Attachment #530424 - Flags: review+
(Assignee)

Updated

6 years ago
Keywords: checkin-needed
Keywords: checkin-needed
Whiteboard: [fixed in cedar]
Version: unspecified → Trunk
Pushed http://hg.mozilla.org/mozilla-central/rev/d6d104661d30
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Flags: in-testsuite?
Resolution: --- → FIXED
Whiteboard: [fixed in cedar]
Target Milestone: --- → mozilla6

Comment 21

6 years ago
This landed some days ago and still I'm not seeing Fennec matching the colours of my theme for some reason.
(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

6 years ago
Thanks for clearing that up. Could you, if time permits, erect a dependency tree?
Depends on: 643032
No longer depends on: 643032
Blocks: 647338
status-firefox6: --- → fixed
You need to log in before you can comment on or make changes to this bug.