Closed
Bug 823644
Opened 12 years ago
Closed 4 years ago
Layout and Styles should have a clear demarcation
Categories
(Firefox for Android Graveyard :: General, defect)
Tracking
(Not tracked)
RESOLVED
INCOMPLETE
People
(Reporter: sriram, Unassigned)
References
Details
(Whiteboard: [leave open])
Attachments
(8 files, 1 obsolete file)
(deleted),
patch
|
mfinkle
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
mfinkle
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
mfinkle
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
mfinkle
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
mfinkle
:
feedback+
lucasr
:
feedback+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
mfinkle
:
review+
mfinkle
:
approval-mozilla-aurora-
|
Details | Diff | Splinter Review |
(deleted),
patch
|
mfinkle
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
Details | Diff | Splinter Review |
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•12 years ago
|
||
+1 for improving clarity. No objection here.
Reporter | ||
Comment 2•12 years ago
|
||
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.
Attachment #724634 -
Flags: review?(mark.finkle)
Reporter | ||
Comment 3•12 years ago
|
||
(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.
Attachment #724640 -
Flags: review?(mark.finkle)
Reporter | ||
Comment 4•12 years ago
|
||
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.
Attachment #724690 -
Flags: review?(mark.finkle)
Reporter | ||
Comment 5•12 years ago
|
||
<item name="android:cacheColorHint">#EEF1F5</item> Should be transparent to make it generic. I'll push it with that change, once I get r+.
Reporter | ||
Comment 6•12 years ago
|
||
This adds the basic list of color and the text-styles we use.
Attachment #725107 -
Flags: review?(mark.finkle)
Reporter | ||
Comment 7•12 years ago
|
||
Making about:home colors depend on global colors.
Attachment #725108 -
Flags: review?(mark.finkle)
Reporter | ||
Comment 8•12 years ago
|
||
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).
Attachment #725109 -
Flags: feedback?(mark.finkle)
Reporter | ||
Updated•12 years ago
|
Attachment #725108 -
Attachment description: Part 4: TextAppearance - about:home → Part 5: TextAppearance - about:home
Updated•12 years ago
|
Attachment #724634 -
Flags: review?(mark.finkle) → review+
Updated•12 years ago
|
Attachment #724640 -
Flags: review?(mark.finkle) → review+
Comment 9•12 years ago
|
||
Comment on attachment 724690 [details] [diff] [review]
Part 3: ListView styles
Looks like this should be tested in prompts as well as regular lists.
Attachment #724690 -
Flags: review?(mark.finkle) → review+
Updated•12 years ago
|
Attachment #725107 -
Flags: review?(mark.finkle) → review+
Updated•12 years ago
|
Attachment #725108 -
Flags: review?(mark.finkle) → review+
Comment 10•12 years ago
|
||
Comment on attachment 725109 [details] [diff] [review]
WIP: TabsTray
Seems sane to me. Let's get Lucas to take a look too.
Attachment #725109 -
Flags: feedback?(mark.finkle)
Attachment #725109 -
Flags: feedback?(lucasr.at.mozilla)
Attachment #725109 -
Flags: feedback+
Comment 11•12 years ago
|
||
Comment on attachment 725109 [details] [diff] [review]
WIP: TabsTray
Review of attachment 725109 [details] [diff] [review]:
-----------------------------------------------------------------
Makes sense.
Attachment #725109 -
Flags: feedback?(lucasr.at.mozilla) → feedback+
Reporter | ||
Comment 12•12 years ago
|
||
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
Whiteboard: [leave open]
Comment 13•12 years ago
|
||
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
Reporter | ||
Comment 14•12 years ago
|
||
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•12 years ago
|
||
Reporter | ||
Comment 16•12 years ago
|
||
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.
Reporter | ||
Comment 17•12 years ago
|
||
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
Reporter | ||
Comment 18•12 years ago
|
||
(Mild changes)
I had to add styles for Button, TextView and EditText as Froyo crashes when try to get the standard ones.
Attachment #724690 -
Attachment is obsolete: true
Attachment #735596 -
Flags: review?(mark.finkle)
Reporter | ||
Comment 19•12 years ago
|
||
(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.
Attachment #735598 -
Flags: review?(mark.finkle)
Comment 20•12 years ago
|
||
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?
Attachment #735596 -
Flags: review?(mark.finkle)
Attachment #735596 -
Flags: review+
Attachment #735596 -
Flags: approval-mozilla-aurora-
Comment 21•12 years ago
|
||
Comment on attachment 735598 [details] [diff] [review]
Part 4: TextAppearance
Make sure these changes are tested on many different devices
Attachment #735598 -
Flags: review?(mark.finkle) → review+
Reporter | ||
Comment 22•12 years ago
|
||
(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.
Reporter | ||
Comment 23•12 years ago
|
||
Reporter | ||
Comment 24•12 years ago
|
||
Comment 25•12 years ago
|
||
Reporter | ||
Comment 26•12 years ago
|
||
Comment 27•4 years ago
|
||
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.
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → INCOMPLETE
Assignee | ||
Updated•4 years ago
|
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•