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

Status: RESOLVED FIXED

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

Description

[Peter Retzer (:pretzer)]

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.

Comment 1

[Wesley Johnston (:wesj)]

Attachment: Patch

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.

Comment 2

[Wesley Johnston (:wesj)]

Attachment: Patch v2

Talked to sriram on irc. Updated to do this for all these different rows.

Comment 3

[Wesley Johnston (:wesj)]

Attachment: 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.

Comment 4

[Sriram Ramasubramanian [: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*.

Comment 5

[Sriram Ramasubramanian [:sriram]]

Attachment: Patch

Sorry Wes, I stole this bug from you ;)

Comment 6

[Wesley Johnston (:wesj)]

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.

Comment 7

[Sriram Ramasubramanian [:sriram]]

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.

Comment 8

[Wesley Johnston (:wesj)]

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

Comment 9

[Sriram Ramasubramanian [:sriram]]

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

[Sriram Ramasubramanian [:sriram]]

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.

Comment 11

[Kartikaya Gupta (email:[e-mail])]

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?

Comment 12

[Sriram Ramasubramanian [:sriram]]

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.

Comment 13

[Kartikaya Gupta (email:[e-mail])]

Attachment: Patch w/ style refactoring

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.

Comment 14

[Kartikaya Gupta (email:[e-mail])]

Attachment: Patch w/ style refactoring

Updated as per IRC discussion to keep layoutish attributes in the layout file and stylish attributes in the style file.

Comment 15

[Sriram Ramasubramanian [: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/>

Comment 16

[Kartikaya Gupta (email:[e-mail])]

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

[Wesley Johnston (:wesj)]

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.

Comment 18

[Kartikaya Gupta (email:[e-mail])]

(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 :)

Comment 19

[Kartikaya Gupta (email:[e-mail])]

Attachment: Patch w/ style refactoring (v2)

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.

Comment 20

[Sriram Ramasubramanian [:sriram]]

Comment on attachment 730141 [details] [diff] [review]
Patch w/ style refactoring (v2)

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

Looks good to me. Thanks :)

Comment 21

[Kartikaya Gupta (email:[e-mail])]

https://hg.mozilla.org/integration/mozilla-inbound/rev/6644525153e8

Comment 22

[Kartikaya Gupta (email:[e-mail])]

Attachment: Missed another layout_height

I found another 47dip that should be reusing the attribute value added in the previous patch (I think).

Comment 23

[Kartikaya Gupta (email:[e-mail])]

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

Comment 24

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/6644525153e8

Comment 25

[Ryan VanderMeulen [:RyanVM]]

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