823644 - Layout and Styles should have a clear demarcation

Status: RESOLVED INCOMPLETE

(Firefox for Android Graveyard :: General, defect)

Description

[Sriram Ramasubramanian [:sriram]]

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.

Comment 1

[Richard Newman [:rnewman]]

+1 for improving clarity. No objection here.

Comment 2

[Sriram Ramasubramanian [:sriram]]

Attachment: Part 1: Themes

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.

Comment 3

[Sriram Ramasubramanian [:sriram]]

Attachment: Part 2: Remove "Screen"

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

Comment 4

[Sriram Ramasubramanian [:sriram]]

Attachment: Part 3: ListView styles

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.

Comment 5

[Sriram Ramasubramanian [:sriram]]

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

Comment 6

[Sriram Ramasubramanian [:sriram]]

Attachment: Part 4: TextAppearance

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

Comment 7

[Sriram Ramasubramanian [:sriram]]

Attachment: Part 5: TextAppearance - about:home

Making about:home colors depend on global colors.

Comment 8

[Sriram Ramasubramanian [:sriram]]

Attachment: WIP: TabsTray

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 9

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

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 10

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

Comment on attachment 725109 [details] [diff] [review]
WIP: TabsTray

Seems sane to me. Let's get Lucas to take a look too.

Comment 11

[Lucas Rocha (:lucasr)]

Comment on attachment 725109 [details] [diff] [review]
WIP: TabsTray

Review of attachment 725109 [details] [diff] [review]:
-----------------------------------------------------------------

Makes sense.

Comment 12

[Sriram Ramasubramanian [:sriram]]

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

Comment 13

[Phil Ringnalda (:philor)]

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

Comment 14

[Sriram Ramasubramanian [:sriram]]

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

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/a23ff3177682

Comment 16

[Sriram Ramasubramanian [:sriram]]

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.

Comment 17

[Sriram Ramasubramanian [:sriram]]

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

Comment 18

[Sriram Ramasubramanian [:sriram]]

Attachment: Part 3: ListView styles

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

Comment 19

[Sriram Ramasubramanian [:sriram]]

Attachment: Part 4: TextAppearance

(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 20

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

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 21

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

Comment on attachment 735598 [details] [diff] [review]
Part 4: TextAppearance

Make sure these changes are tested on many different devices

Comment 22

[Sriram Ramasubramanian [:sriram]]

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

Comment 23

[Sriram Ramasubramanian [:sriram]]

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

Comment 24

[Sriram Ramasubramanian [:sriram]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f85629692c59

Comment 25

[Ryan VanderMeulen [:RyanVM]]

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

Comment 26

[Sriram Ramasubramanian [:sriram]]

Attachment: Part 6: More cleanup

Comment 27

[BMO Automation]

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.