Closed Bug 839767 Opened 12 years ago Closed 12 years ago

[about:home] Featured addon rows don't show highlight color anymore

Categories

(Firefox for Android Graveyard :: Theme and Visual Design, defect)

19 Branch
All
Android
defect
Not set
normal

Tracking

(firefox18 unaffected, firefox19 affected, firefox20 affected, firefox21 affected)

RESOLVED FIXED
Firefox 22
Tracking Status
firefox18 --- unaffected
firefox19 --- affected
firefox20 --- affected
firefox21 --- affected

People

(Reporter: pretzer, Assigned: kats)

References

Details

(Keywords: regression)

Attachments

(2 files, 6 obsolete files)

The rows within the featured addons section on about:home don't show a highlight color anymore, when selecting them. This is a regression in FF19 because FF18 shows the color. The problem probably also affects the remote tabs section on about:home but I can't test that right now.
Attached patch Patch (obsolete) (deleted) — Splinter Review
duplicateParentState isn't what we want here. I also replace the margin with padding so that the highlight extends the width of the entire row.
Attachment #716672 - Flags: review?
Attachment #716672 - Flags: review? → review?(sriram)
Attached patch Patch v2 (obsolete) (deleted) — Splinter Review
Talked to sriram on irc. Updated to do this for all these different rows.
Attachment #716672 - Attachment is obsolete: true
Attachment #716672 - Flags: review?(sriram)
Attachment #716815 - Flags: review?
Attachment #716815 - Flags: review? → review?(sriram)
Attached patch WIP v2 (obsolete) (deleted) — Splinter Review
That patch broke lightweight themes. I think we should do something like this instead sriram. This removes all the duplicateParentState stuff, and switches these views to be GeckoTextView's so that they update when the theme changes. I moved bunch of repeated code into our GeckoView code, including attaching listeners, so that these views are automatically updated when the theme changes. I also made the theme stuff a singleton, because it really doesn't need to be tied to ANY activity that I can see. Just needs access to resources? I also moved all the duplicate onAttachedToWindow, onDetachedFromWindow, and onLayout code. Need to test this more and fix whatever comes up.
Attachment #716815 - Attachment is obsolete: true
Attachment #716815 - Flags: review?(sriram)
Attachment #717016 - Flags: feedback?(sriram)
(In reply to Wesley Johnston (:wesj) from comment #3) > Created attachment 717016 [details] [diff] [review] > WIP v2 > > That patch broke lightweight themes. I think we should do something like > this instead sriram. This removes all the duplicateParentState stuff, and > switches these views to be GeckoTextView's so that they update when the > theme changes. > > I moved bunch of repeated code into our GeckoView code, including attaching > listeners, so that these views are automatically updated when the theme > changes. I also made the theme stuff a singleton, because it really doesn't > need to be tied to ANY activity that I can see. Just needs access to > resources? I also moved all the duplicate onAttachedToWindow, > onDetachedFromWindow, and onLayout code. > > Need to test this more and fix whatever comes up. LightweightTheme is already a singleton. There is just once instance of it in Application, and GeckoActivity returns that. I don't want to have state inside LightweightTheme to make a singleton pattern. My idea was to have GeckoView not know about LightweightTheme. As, it was created for Private browsing, and later enhanced with the themes. But I guess I am fine with it now. That justifies cleaning up other views, that inherit Gecko*View*. I would suggest splitting the patch into two. One cleans up the onAttach/onDetach, and that enhances the Gecko*View* to know about LightweightTheme. The other enhancing the about:home views to use Gecko*View*.
Depends on: 845572
Attached patch Patch (obsolete) (deleted) — Splinter Review
Sorry Wes, I stole this bug from you ;)
Attachment #721360 - Flags: review?(wjohnston)
Comment on attachment 721360 [details] [diff] [review] Patch Review of attachment 721360 [details] [diff] [review]: ----------------------------------------------------------------- Now I can be picky! ::: mobile/android/base/resources/layout/abouthome_addon_row.xml @@ +11,5 @@ > + android:textAppearance="@style/AboutHome.TextAppearance.Title" > + android:background="@drawable/action_bar_button" > + android:gravity="center_vertical" > + android:singleLine="true" > + android:ellipsize="middle"/> We should move all this stuff into styles. Especially since most of it is duplicated between these different rows with very small differences.
Attachment #721360 - Flags: review?(wjohnston) → review-
Already a bug is filed for that: https://bugzilla.mozilla.org/show_bug.cgi?id=823644, and I don't like refactoring code as a part of another bug.
(In reply to Sriram Ramasubramanian [:sriram] from comment #7) > Already a bug is filed for that: > https://bugzilla.mozilla.org/show_bug.cgi?id=823644, and I don't like > refactoring code as a part of another bug. I don't think we can/want to do that move in one step. Its too much for anyone to hope to write/review well. I'd rather do it in pieces as we touch little bits of it, and this seems like as good a place as any to me.
(In reply to Wesley Johnston (:wesj) from comment #8) > (In reply to Sriram Ramasubramanian [:sriram] from comment #7) > > Already a bug is filed for that: > > https://bugzilla.mozilla.org/show_bug.cgi?id=823644, and I don't like > > refactoring code as a part of another bug. > > I don't think we can/want to do that move in one step. Its too much for > anyone to hope to write/review well. I'd rather do it in pieces as we touch > little bits of it, and this seems like as good a place as any to me. We cannot do that in one go for sure. Bits and pieces is better. But again, I don't want this bug to track style/layout changes. May be a separate bug can be filed and tagged with the meta bug 823644.
Comment on attachment 717016 [details] [diff] [review] WIP v2 Review of attachment 717016 [details] [diff] [review]: ----------------------------------------------------------------- Most things have been done in Bug 845572. Please post a new patch.
Attachment #717016 - Flags: feedback?(sriram) → feedback-
Comment on attachment 721360 [details] [diff] [review] Patch I would like this patch for these elements on about:home to be usable on ouya. Anything I can do to help here?
Attachment #721360 - Flags: feedback+
Comment on attachment 721360 [details] [diff] [review] Patch This patch cannot be uplifted to aurora, as the "GeckoView should support LWTheme" is pretty complex and would require pulling in more and more patches. I guess, if this is going to land only for 22, this UI is going to change for 23. I don't find a reason to move anything in this to styles. Also, the textAppearance is already a style. We are moving towards having layout information with the layout. Hence, this patch doesn't need any more changes for 22.
Attachment #721360 - Flags: review- → review?(wjohnston)
Attached patch Patch w/ style refactoring (obsolete) (deleted) — Splinter Review
I stole sriram's patch and moved a bunch of the common style attributes into a new style, which is what I think you wanted. I also set android:focusable to be true on the rows so that I can put focus on them via gamepad navigation.
Attachment #721360 - Attachment is obsolete: true
Attachment #721360 - Flags: review?(wjohnston)
Attachment #729694 - Flags: review?(wjohnston)
Attached patch Patch w/ style refactoring (obsolete) (deleted) — Splinter Review
Updated as per IRC discussion to keep layoutish attributes in the layout file and stylish attributes in the style file.
Attachment #729694 - Attachment is obsolete: true
Attachment #729694 - Flags: review?(wjohnston)
Attachment #729699 - Flags: review?(wjohnston)
Attachment #729699 - Flags: review?(sriram)
Comment on attachment 729699 [details] [diff] [review] Patch w/ style refactoring Review of attachment 729699 [details] [diff] [review]: ----------------------------------------------------------------- Looks good to me. r+ with the change, if needed. ::: mobile/android/base/resources/layout/abouthome_last_tabs_row.xml @@ +36,5 @@ > + android:layout_below="@id/last_tab_title" > + android:textSize="12sp" > + android:singleLine="true" > + android:textColor="@color/abouthome_section_subtitle" > + android:focusable="true"/> The focusable should be for entire row right? Then probably this should be with the <RelativeLayout/>
Attachment #729699 - Flags: review?(sriram) → review+
(In reply to Sriram Ramasubramanian [:sriram] from comment #15) > The focusable should be for entire row right? > Then probably this should be with the <RelativeLayout/> Yes, you're right. I just tested it and I need to move the focusable to the RelativeLayout. Done locally.
Comment on attachment 729699 [details] [diff] [review] Patch w/ style refactoring Review of attachment 729699 [details] [diff] [review]: ----------------------------------------------------------------- I feel like the main reason to do this is to share as much as we possibly can. If something is the same for all the different rows, I'd like to at least share it with attributes (but I still personally feel like we should move at least padding and gravity, if not margin parameters). I'll clear the review and we can talk about this here or on irc if you've got an opposite opinion. ::: mobile/android/base/resources/layout/abouthome_addon_row.xml @@ +4,5 @@ > - file, You can obtain one at http://mozilla.org/MPL/2.0/. --> > > +<Gecko.TextView xmlns:android="http://schemas.android.com/apk/res/android" > + android:layout_width="fill_parent" > + android:layout_height="46dip" At the very least, we need to move things like this to an attribute. i.e. see abouthome_last_tabs_row, which I assume should be the same but is actually 47dip, but we should share the value to make upkeep simpler. @@ +5,5 @@ > > +<Gecko.TextView xmlns:android="http://schemas.android.com/apk/res/android" > + android:layout_width="fill_parent" > + android:layout_height="46dip" > + android:paddingLeft="12dip" Same. We can share this padding, which is the same in all these layouts. ::: mobile/android/base/resources/layout/abouthome_last_tabs_row.xml @@ +22,5 @@ > + android:layout_toRightOf="@id/last_tab_favicon" > + android:layout_marginTop="2dip" > + android:layout_marginLeft="12dip" > + android:layout_marginRight="12dip" > + android:textSize="15sp" And this text size. Since most of this is the same as the other style, I'd suggest we have two something like: @style/AboutHome.RowItem - singleLine, textAppearance, ellipsize stuff @style/AboutHome.RowItem.Row - background, and focusable @@ +35,5 @@ > + android:layout_toRightOf="@id/last_tab_favicon" > + android:layout_below="@id/last_tab_title" > + android:textSize="12sp" > + android:singleLine="true" > + android:textColor="@color/abouthome_section_subtitle" This stuff should be moved to a style of its own.
Attachment #729699 - Flags: review?(wjohnston)
(In reply to Wesley Johnston (:wesj) from comment #17) > I'll clear the > review and we can talk about this here or on irc if you've got an opposite > opinion. > I don't really have a strong opinion either way - I think you and sriram should hash it out and figure out what's acceptable to the both of you :)
Attached patch Patch w/ style refactoring (v2) (deleted) — Splinter Review
Ok, I updated the patch with some more refactoring: - added abouthome_rowitem_height and abouthome_rowitem_left_padding to dimens.xml - extracted a AboutHome.RowItem.TextRow style from the AboutHome.RowItem style - pulled out styles for AboutHome.LastTabRow.Title and AboutHome.LastTabRow.Url (In reply to Wesley Johnston (:wesj) from comment #17) > > +<Gecko.TextView xmlns:android="http://schemas.android.com/apk/res/android" > > + android:layout_width="fill_parent" > > + android:layout_height="46dip" > > At the very least, we need to move things like this to an attribute. i.e. > see abouthome_last_tabs_row, which I assume should be the same but is > actually 47dip, but we should share the value to make upkeep simpler. Done (extracted to abouthome_rowitem_height, which is 46dip. This also changes the height for abouthome_last_tabs_row to be 46dip. > @@ +5,5 @@ > > > > +<Gecko.TextView xmlns:android="http://schemas.android.com/apk/res/android" > > + android:layout_width="fill_parent" > > + android:layout_height="46dip" > > + android:paddingLeft="12dip" > > Same. We can share this padding, which is the same in all these layouts. Extracted the padding and used it in all the layouts. In abouthome_last_tabs_row it actually gets assigned as a margin value. > ::: mobile/android/base/resources/layout/abouthome_last_tabs_row.xml > @@ +22,5 @@ > > + android:layout_toRightOf="@id/last_tab_favicon" > > + android:layout_marginTop="2dip" > > + android:layout_marginLeft="12dip" > > + android:layout_marginRight="12dip" > > + android:textSize="15sp" > > And this text size. Since most of this is the same as the other style, I'd > suggest we have two something like: I don't see this text size shared with anything, so I didn't move it into an attribute. > @style/AboutHome.RowItem - singleLine, textAppearance, ellipsize stuff > @style/AboutHome.RowItem.Row - background, and focusable Because of style inheritance I had to do this slightly differently but I think it captures the intent of what you wanted. > @@ +35,5 @@ > > + android:layout_toRightOf="@id/last_tab_favicon" > > + android:layout_below="@id/last_tab_title" > > + android:textSize="12sp" > > + android:singleLine="true" > > + android:textColor="@color/abouthome_section_subtitle" > > This stuff should be moved to a style of its own. Done. I also moved the style properties from the title into their own style block.
Attachment #729699 - Attachment is obsolete: true
Attachment #730141 - Flags: review?(wjohnston)
Attachment #730141 - Flags: review?(sriram)
Assignee: nobody → bugmail.mozilla
Comment on attachment 730141 [details] [diff] [review] Patch w/ style refactoring (v2) Review of attachment 730141 [details] [diff] [review]: ----------------------------------------------------------------- Looks good to me. Thanks :)
Attachment #730141 - Flags: review?(sriram) → review+
Attachment #730141 - Flags: review?(wjohnston) → review+
Attachment #717016 - Attachment is obsolete: true
Attached patch Missed another layout_height (deleted) — Splinter Review
I found another 47dip that should be reusing the attribute value added in the previous patch (I think).
Attachment #730783 - Flags: review?(sriram)
Attachment #730783 - Flags: review?(sriram) → review+
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 22
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: