We've been using layout_weight and layout_height in layout/xyz.xml and styles.xml interchangeably. There are few drawbacks here. Even though styles.xml can hold these values, once we remove "layout_xxx" attributes from the layout files, the inherent value of the layout file is lost. It's so hard to find where an element will be placed relative to its parent, if the layout_xxx are not available in the layout files. Also, "layout_toLeftOf" and other rules for RelativeLayout messes up if they are specified in the styles.xml. Proposal: 1. The layout/xyz.xml should have the basic skeleton of the layout. This means, the layout_xxx attributes better be specified in the layout file. The padding and margin values can go into dimens.xml and can be used here, provided they are common for a lot of entries -- like a url-bar button. Exceptions (may be): * The ImageButtons used on the url-bar tend to be a square. They can have their width and height properties in styles.xml, and the style can be referred in layout file. This makes it easy to understand, with a style like "BrowserToolbar.SquareButton". * Padding of such elements are going to be the same. So are the layout_centerVertical attributes. 2. Styles should be more generic. And, it's better to specify a "Styleable" scheme, specify it in themes.xml (for global values) and use this attribute in layouts. Also, its better to stylize the "styleable" values only. Like a textAppearance would hold textColor, textSize, typeface and few more. So, a textAppearance like "TextAppearance.Primary.Bold" would make it easy to understand in the layout as the element to use the primary text color in bold. In themes.xml <item name="android:textAppearance">@style/BrowserToolbar.TextAppearance</item> In styles.xml, definition of various styleable values for the TextAppearance. And in the element, <UIElement android:textAppearance="?android:textAppearance"/> or, if we need a styleable that is specified for URL-bar. <UIElement android:textAppearance="?urlBarTextAppearance"/> or, a generic style as, <UIElement android:textAppearance="@style/BrowserToolbar.TextAppearance.Primary"/> We can create custom styleable attributes and use them too (our attrs.xml actually does this! All we would need to do is, take those values and feed the super() in Java). This makes the layout and styles simpler.

+1 for improving clarity. No objection here.

This cleans up the themes.xml. The ADT generated standard styles.xml as follows. <style name="Base" theme="_android_theme_"> -- api based overrides -- </style> <style name="App" parent="Base"> -- activity/application based overrides -- </style> which makes sure that there aren't much App/Application level overrides that needs to be copied across different values-xxx/ folders. The "Base" theme can be Theme.Dark or Theme.Holo based on version of the values/ folder. I've used the same approach here. The only little difference we have is, we have to copy few attributes for Gecko.App. This is because, listViewStyle controls more-menu in GB phones. panelBackground gives the background for the menu. Until we change it to use our custom styles, we need this override for the Activity. Notes: * Ideally "Gecko" should have a blue background. But since Sync is using it, I don't want to change it there. * All styles, like buttons, list-views will subsequently go into "Gecko" theme. Proposal: * Could we just have "Gecko" instead of "Gecko.App"? I feel it redundant.

(He who started it ends it) "Screen" removes if the Layout is horizontal or vertical. In cases like ScrollView, there is no orientation attribute. It's better to remove this.

All listviews use same color scheme (from custom menu to awesomebar lists). It's better to consolidate them, and have it as the default style of the Activity. By doing this, any new ListView created will have default styles. Also, layout doesn't need to have an explicit "style" attribute. (Ah! So.. cleannn) Tablets needed a padding, and that's been added in dimens file. dimens file is lacking comments. I've started adding them as I edit.

<item name="android:cacheColorHint">#EEF1F5</item> Should be transparent to make it generic. I'll push it with that change, once I get r+.

This adds the basic list of color and the text-styles we use.

Making about:home colors depend on global colors.

This is one of the first patches to make use of text-styles. Basically <TextView/> as a widget can have globally defined attributes. A "Widget.TabRow" is a "kind of TextView" with a different set of global attributes. The "android:textAppearance" is defined in the styles and the layout holds just the layout. (If we ever make it a widget like org.mozilla.gecko.TabRow, we can define a global attribute like tabRowStyle, and we don't have to specify a "style" attribute in layout file. But since this is a one-time use only, I don't want to take that much of a generic approach).

Comment on attachment 724690 [details] [diff] [review] Part 3: ListView styles Looks like this should be tested in prompts as well as regular lists.

Comment on attachment 725109 [details] [diff] [review] WIP: TabsTray Seems sane to me. Let's get Lucas to take a look too.

Comment on attachment 725109 [details] [diff] [review] WIP: TabsTray Review of attachment 725109 [details] [diff] [review]: ----------------------------------------------------------------- Makes sense.

https://hg.mozilla.org/integration/mozilla-inbound/rev/2f24c9525f57 https://hg.mozilla.org/integration/mozilla-inbound/rev/809b5e32d3a2 https://hg.mozilla.org/integration/mozilla-inbound/rev/db52418726e6 https://hg.mozilla.org/integration/mozilla-inbound/rev/f0c492121470 https://hg.mozilla.org/integration/mozilla-inbound/rev/a23ff3177682

Backed out the whole push in https://hg.mozilla.org/integration/mozilla-inbound/rev/5eb7efc431c8 for mochitest-1 and robocop-2 errors about "Error inflating class android.widget.Button" like https://tbpl.mozilla.org/php/getParsedLog.php?id=20799813&tree=Mozilla-Inbound and https://tbpl.mozilla.org/php/getParsedLog.php?id=20798777&tree=Mozilla-Inbound

That could have been a clobbering issue I guess. My try server push with same patches didnt show that error: https://tbpl.mozilla.org/?tree=Try&rev=8772e5059823

The Android 2.2 convention is too strict! It cannot refer to an attribute value for "android:textColorHighlight" in textAppearance. And it's strict that it wants the textColor to be of ColorStateList (with enabled/disabled states) and not a single color. E/GeckoAppShell( 985): Caused by: android.content.res.Resources$NotFoundException: Resource is not a ColorStateList (color or path): TypedValue{t=0x2/d=0x1010350 a=3} E/GeckoAppShell( 985): at android.content.res.Resources.loadColorStateList(Resources.java:1804) E/GeckoAppShell( 985): at android.content.res.TypedArray.getColorStateList(TypedArray.java:342) E/GeckoAppShell( 985): at android.widget.TextView.<init>(TextView.java:401) E/GeckoAppShell( 985): at android.widget.EditText.<init>(EditText.java:55) E/GeckoAppShell( 985): at android.widget.EditText.<init>(EditText.java:51) E/GeckoAppShell( 985): at org.mozilla.gecko.GeckoEditText.<init>(GeckoEditText.java:25) E/GeckoAppShell( 985): ... 31 more I guess this needs more investigation with Froyo devices. I would like to hold off landing this before this merge.

Android 2.2 tries to find the file and throws ResourceNotFound. However the file is there in the APK. Something similar: http://stackoverflow.com/questions/5580290/setting-textcolor-from-a-theme-fails-only-on-htc-desire-hd

(Mild changes) I had to add styles for Button, TextView and EditText as Froyo crashes when try to get the standard ones.

(Mild changes) Froyo crashes if the textColorPrimary is not a ColorStateList. So, creating a dummy colorstatelist. However, this is not a performance regression. Android tries to cache colorstatelist. So, if we use (and reuse) same ones, it will always be in memory and wouldn't have to parse XML files.

Comment on attachment 735596 [details] [diff] [review] Part 3: ListView styles >diff --git a/mobile/android/base/resources/layout/awesomebar_tabs.xml b/mobile/android/base/resources/layout/awesomebar_tabs.xml > <android.support.v4.view.ViewPager > android:id="@+id/tabviewpager" > android:layout_width="fill_parent" >- android:layout_height="fill_parent"/> >+ android:layout_height="fill_parent" >+ android:background="#EEF1F5" Should the color be in a style?

Comment on attachment 735598 [details] [diff] [review] Part 4: TextAppearance Make sure these changes are tested on many different devices

(In reply to Mark Finkle (:mfinkle) from comment #20) > Comment on attachment 735596 [details] [diff] [review] > Part 3: ListView styles > > >diff --git a/mobile/android/base/resources/layout/awesomebar_tabs.xml b/mobile/android/base/resources/layout/awesomebar_tabs.xml > > > <android.support.v4.view.ViewPager > > android:id="@+id/tabviewpager" > > android:layout_width="fill_parent" > >- android:layout_height="fill_parent"/> > >+ android:layout_height="fill_parent" > >+ android:background="#EEF1F5" > > Should the color be in a style? Yes, but we haven't styles for this tag yet. When we do that, this will eventually move in there.

https://hg.mozilla.org/integration/mozilla-inbound/rev/0b212f5b6bbc https://hg.mozilla.org/integration/mozilla-inbound/rev/625041f40a97 https://hg.mozilla.org/integration/mozilla-inbound/rev/50ddb97f5723 https://hg.mozilla.org/integration/mozilla-inbound/rev/13606f290f3b

https://hg.mozilla.org/mozilla-central/rev/0b212f5b6bbc https://hg.mozilla.org/mozilla-central/rev/625041f40a97 https://hg.mozilla.org/mozilla-central/rev/50ddb97f5723 https://hg.mozilla.org/mozilla-central/rev/13606f290f3b https://hg.mozilla.org/mozilla-central/rev/f85629692c59

We have completed our launch of our new Firefox on Android. The development of the new versions use GitHub for issue tracking. If the bug report still reproduces in a current version of [Firefox on Android nightly](https://play.google.com/store/apps/details?id=org.mozilla.fenix) an issue can be reported at the [Fenix GitHub project](https://github.com/mozilla-mobile/fenix/). If you want to discuss your report please use [Mozilla's chat](https://wiki.mozilla.org/Matrix#Connect_to_Matrix) server https://chat.mozilla.org and join the [#fenix](https://chat.mozilla.org/#/room/#fenix:mozilla.org) channel.