1185002 - Replace HomePagerTabStrip titles in First Run Pane

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Awesomescreen, defect)

Description

[Sebastian Kaspari (:sebastian; :pocmo)]

In bug 1073053 we are going to replace the HomePagerTabStrip with the tab strip layout we are using on tablets, aka TabMenuStripLayout. Our first run pane is the last piece using HomePagerTabStrip. Let's unify the look & feel of our tabs and use TabMenuStripLayout here too. Or maybe even something else because right now this screen just has one tab.

Comment 1

[Chenxia Liu [:liuche]]

Attachment: MozReview Request: Bug 1185002 - Remove unused HomePagerTabStrip code. r=mcomella

Bug 1185002 - Switching to TabMenuStrip.

Comment 2

[Chenxia Liu [:liuche]]

Attachment: MozReview Request: Bug 1185002 - Add custom active/inactive colors. r=sebastian

Exposing decor interface.

Comment 3

[Chenxia Liu [:liuche]]

Just hacking around in here, so some WIP commits - this looks like some refactoring is in order because HomePager holds a lot of interfaces that TabMenuStrip uses, but if we want to uplift this with 44, the refactor should be done after. I'll get this working and then sort out if/how we want to refactor in this bug.

Comment 4

[Sebastian Kaspari (:sebastian; :pocmo)]

Yeah, I remember looking into this when I filed the bug and stopped when I saw all these interfaces being needed. In the long term it would be nice to replace all this with TabLayout from the Android design library (bug 1189306).

Comment 5

[Chenxia Liu [:liuche]]

Attachment: Screenshot: titlebar + highlight

Antlam, what color would you like the active/inactive titles to be? In your mock they look like they are different sp, but that's not how the homepager titlebar works.

This is almost done - I just need to set the colors, and figure out a way to update the orange highlight width during onConfigurationChange.

Comment 6

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

(In reply to Chenxia Liu [:liuche] from comment #5)
> Created attachment 8666979 [details]
> Screenshot: titlebar + highlight
> 
> Antlam, what color would you like the active/inactive titles to be? In your
> mock they look like they are different sp, but that's not how the homepager
> titlebar works.

You're right. I had used different font weights. Same size, but Light for inactive, and Regular for active.

If not possible, we could just stick with white for active and the guideline 70% opacity for inactive. Does that work?

> This is almost done - I just need to set the colors, and figure out a way to
> update the orange highlight width during onConfigurationChange.

Comment 7

[Chenxia Liu [:liuche]]

Comment on attachment 8664591 [details]
MozReview Request: Bug 1185002 - Remove unused HomePagerTabStrip code. r=mcomella

Bug 1185002 - Use TabMenuStrip in firstrun.

Comment 8

[Chenxia Liu [:liuche]]

Comment on attachment 8664592 [details]
MozReview Request: Bug 1185002 - Add custom active/inactive colors. r=sebastian

Add custom active/inactive colors.

Comment 9

[Chenxia Liu [:liuche]]

Attachment: MozReview Request: Bug 1185002 - Select first panel if it exists. r=sebastian

Select first panel if it exists.

Comment 10

[Chenxia Liu [:liuche]]

Comment on attachment 8664591 [details]
MozReview Request: Bug 1185002 - Remove unused HomePagerTabStrip code. r=mcomella

Actually, it's about time I ask for feedback here :P Making a lot of these interfaces static works just fine, but maybe the right thing is to just move them somewhere shared.

Comment 11

[Chenxia Liu [:liuche]]

Comment on attachment 8667579 [details]
MozReview Request: Bug 1185002 - Select first panel if it exists. r=sebastian

Again, not the best thing to hard code this, but HomePager does something similar.

I still need to special case the single title, so the bar is calculated to be the width of the title + margin, instead of filling the whole width of the screen.

Comment 12

[Chenxia Liu [:liuche]]

Attachment: Screenshot: Two panels with colored titles

Hey Anthony, since we're basically using the HomePager code, I'm keeping this as setting colors (instead of opacity), so that they're consistent. This uses disabled_grey for the disabled state.

Comment 13

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

Comment on attachment 8667596 [details]
Screenshot: Two panels with colored titles

Alright then, looks OK to me!

Comment 14

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

Actually, we currently have a "disabled" color for this label. Let's just stick to the current color.

Comment 15

[Sebastian Kaspari (:sebastian; :pocmo)]

Sorry for the feedback delay!(In reply to Chenxia Liu [:liuche] from comment #10)

> Actually, it's about time I ask for feedback here :P Making a lot of these
> interfaces static works just fine, but maybe the right thing is to just move
> them somewhere shared.

Yeah, I think moving them is a good idea. With TabMenuStrip being the only class implementing this Decor interface and probably no other class ever going to do that, I wonder if we should just get rid of it altogether and just use TabMenuStrip references? Having an interface is maybe a bit over-engineered here. What do you think?

Comment 16

[Sebastian Kaspari (:sebastian; :pocmo)]

FYI: I just filed bug 1210371 for switching to TabLayout (Android Design Library) eventually. I used it once and it was dead simple - almost no additional code needed.

Comment 17

[Sebastian Kaspari (:sebastian; :pocmo)]

https://reviewboard.mozilla.org/r/20011/#review18835

::: mobile/android/base/home/HomeAdapter.java:32
(Diff revision 2)
> -    interface OnAddPanelListener {
> +    public static interface OnAddPanelListener {

Btw. "public" should be enough. Interfaces are always static.

::: mobile/android/base/home/HomePager.java:117
(Diff revision 2)
> -    interface OnTitleClickListener {
> +    public static interface OnTitleClickListener {

I guess we could move this to TabMenuStrip now that this is the only class handling "titles"?

::: mobile/android/base/resources/layout/tab_menu_strip.xml:9
(Diff revision 2)
> +          android:layout_weight="1"

With TabMenuStripLayout being a LinearLayout: Should we set layout_width to match_parent instead?

Comment 18

[Sebastian Kaspari (:sebastian; :pocmo)]

You just asked for feedback but I'd actually r+ all this so far. Cleaning up the interface etc. would be nice though. But this could be deferred to bug 1210371 too. Overall I'm happy that we can remove HomePagerTabStrip and everything related soon. :)

Comment 19

[Chenxia Liu [:liuche]]

https://reviewboard.mozilla.org/r/20011/#review18835

> With TabMenuStripLayout being a LinearLayout: Should we set layout_width to match_parent instead?

Good call - I actually just changed this to 0dp because the weight should handle the measurements, and then we save a measurement step. For some reason I remembered it not working before, but whatever.

Comment 20

[Chenxia Liu [:liuche]]

Comment on attachment 8664591 [details]
MozReview Request: Bug 1185002 - Remove unused HomePagerTabStrip code. r=mcomella

Bug 1185002 - Use TabMenuStrip in firstrun. r=sebastian

Comment 21

[Chenxia Liu [:liuche]]

Comment on attachment 8664592 [details]
MozReview Request: Bug 1185002 - Add custom active/inactive colors. r=sebastian

Bug 1185002 - Add custom active/inactive colors. r=sebastian

Comment 22

[Chenxia Liu [:liuche]]

Comment on attachment 8667579 [details]
MozReview Request: Bug 1185002 - Select first panel if it exists. r=sebastian

Bug 1185002 - Select first panel if it exists. r=sebastian

Comment 23

[Chenxia Liu [:liuche]]

I realized that I am missing one patch - remove all the HomePagerTabStrip code!

Comment 24

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8664591 [details]
MozReview Request: Bug 1185002 - Remove unused HomePagerTabStrip code. r=mcomella

https://reviewboard.mozilla.org/r/20011/#review18955

::: mobile/android/base/resources/layout/tab_menu_strip.xml:9
(Diff revision 3)
> +          android:layout_weight="1"

> Good call - I actually just changed this to 0dp because the weight should
> handle the measurements, and then we save a measurement step. For some
> reason I remembered it not working before, but whatever.

What's the advantage of using the weight? Shouldn't match_parent be even more efficient? We always have just one item horizontally, don't we?

Comment 25

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8664592 [details]
MozReview Request: Bug 1185002 - Add custom active/inactive colors. r=sebastian

https://reviewboard.mozilla.org/r/20013/#review18957

::: mobile/android/base/resources/layout/firstrun_pane.xml:26
(Diff revision 3)
> +                                             gecko:inactiveTextColor="@color/disabled_grey" />

Adding them as attributes: Nice! :)

Comment 26

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8667579 [details]
MozReview Request: Bug 1185002 - Select first panel if it exists. r=sebastian

https://reviewboard.mozilla.org/r/20785/#review18959

::: mobile/android/base/firstrun/FirstrunPager.java:144
(Diff revision 2)
> +            if (panels.size() > 0) {
> +                mDecor.onPageSelected(0);
> +            }

Seems to be reasonable. But why was it needed?

Comment 27

[Sebastian Kaspari (:sebastian; :pocmo)]

(In reply to Chenxia Liu [:liuche] from comment #23)
> I realized that I am missing one patch - remove all the HomePagerTabStrip
> code!

Isn't that the most fun? :)

Comment 28

[Chenxia Liu [:liuche]]

https://reviewboard.mozilla.org/r/20011/#review18955

> > Good call - I actually just changed this to 0dp because the weight should
> > handle the measurements, and then we save a measurement step. For some
> > reason I remembered it not working before, but whatever.
> 
> What's the advantage of using the weight? Shouldn't match_parent be even more efficient? We always have just one item horizontally, don't we?

Ah okay, I see the confusion. This layout is actually not the strip itself, but rather the TextView that gets added for each strip. TabMenuStripLayout is actually a LinearLayout that just adds TextViews added to it. So this file should actually be named tab_menu_item. Since we want to the text views to resize as they're added, we need the layout_weight. (I tried it with match_parent, and what happens is that each title becomes the full width of the bar.)

Comment 29

[Chenxia Liu [:liuche]]

(In reply to Sebastian Kaspari (:sebastian) from comment #26)
> Comment on attachment 8667579 [details]
> MozReview Request: Bug 1185002 - Select first panel if it exists. r=sebastian
> 
> https://reviewboard.mozilla.org/r/20785/#review18959
> 
> ::: mobile/android/base/firstrun/FirstrunPager.java:144
> (Diff revision 2)
> > +            if (panels.size() > 0) {
> > +                mDecor.onPageSelected(0);
> > +            }
> 
> Seems to be reasonable. But why was it needed?

Ahhh! I realize I lost my comment that I meant to post about this.

During some debugging, I realized that the code to "draw the selection bar when views are loaded" is actually not running when it is expected to (after the layout is updated). [1] This isn't running at all when you load a single page.

What's actually selecting the titles is the strip.setBounds in onPageScrolled [2], which is called when adding new pages to the pager.

Do you have suggestions on what listener should actually be used instead of onGlobalLayoutListener? Or is that being used incorrectly? I'll look at this some more too.

[1] 
http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/home/TabMenuStripLayout.java#85
[2] http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/home/TabMenuStripLayout.java#139

Comment 30

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/036f1ab8464f
https://hg.mozilla.org/integration/fx-team/rev/a3f5dc4d8e90
https://hg.mozilla.org/integration/fx-team/rev/9e9ae23bcefe

Comment 31

[Chenxia Liu [:liuche]]

Marking leave-open because I want to let the patches that fix this to get on nightly in case they need to be uplifted. Last patch removing code forthcoming.

Comment 32

[Chenxia Liu [:liuche]]

Comment on attachment 8664591 [details]
MozReview Request: Bug 1185002 - Remove unused HomePagerTabStrip code. r=mcomella

Bug 1185002 - Remove unused HomePagerTabStrip code. r=mcomella

Comment 33

[Michael Comella (:mcomella) [needinfo or I won't see it]]

Comment on attachment 8664591 [details]
MozReview Request: Bug 1185002 - Remove unused HomePagerTabStrip code. r=mcomella

https://reviewboard.mozilla.org/r/20011/#review19031

Assuming HomePagerTabStrip is unused, lgtm.

Comment 34

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/7e3b4ebc1720

Comment 35

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/036f1ab8464f
https://hg.mozilla.org/mozilla-central/rev/a3f5dc4d8e90
https://hg.mozilla.org/mozilla-central/rev/9e9ae23bcefe
https://hg.mozilla.org/mozilla-central/rev/7e3b4ebc1720

Comment 37

[Chenxia Liu [:liuche]]

Attachment: Aurora patch: Part 1 - Use TabMenuStrip in first run

Approval Request Comment
[Feature/regressing bug #]: Bug 1199859
[User impact if declined]: User will not be able to see length of first run experience
[Describe test coverage new/current, TreeHerder]: baked on nightly for a week, green Aurora try run https://treeherder.mozilla.org/#/jobs?repo=try&revision=bf090ef5130f
[Risks and why]: low, baked on Nightly
[String/UUID change made/needed]: none

Comment 38

[Chenxia Liu [:liuche]]

Attachment: Aurora patch: Part 2 - Add active/inactive colors

Approval Request Comment
[Feature/regressing bug #]: Bug 1199859
[User impact if declined]: User will not be able to see length of first run experience
[Describe test coverage new/current, TreeHerder]: baked on nightly for a week, green Aurora try run https://treeherder.mozilla.org/#/jobs?repo=try&revision=bf090ef5130f
[Risks and why]: low, baked on Nightly
[String/UUID change made/needed]: none

Comment 39

[Chenxia Liu [:liuche]]

Attachment: Aurora patch: Part 3 - Select first panel

Approval Request Comment
[Feature/regressing bug #]: Bug 1199859
[User impact if declined]: User will not be able to see length of first run experience
[Describe test coverage new/current, TreeHerder]: baked on nightly for a week, green Aurora try run https://treeherder.mozilla.org/#/jobs?repo=try&revision=bf090ef5130f
[Risks and why]: low, baked on Nightly
[String/UUID change made/needed]: none

Comment 40

[Liz Henry (:lizzard Please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8674493 [details] [diff] [review]
Aurora patch: Part 1 - Use TabMenuStrip in first run

OK to uplift, looks ok on m-c and aurora try run.

Comment 41

[Liz Henry (:lizzard Please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8674495 [details] [diff] [review]
Aurora patch: Part 2 - Add active/inactive colors

OK to uplift, looks ok on m-c and aurora try run.

Comment 42

[Liz Henry (:lizzard Please n-i to RyanVM, jcristau, or pascal)]

Tracking this for 43 since it's a fairly large change late in aurora. 
liuche or sebastian, can you help to verify that this is working correctly once it lands in aurora? Thanks!

Comment 43

[Chenxia Liu [:liuche]]

Yes! I can check this (and bug 1185002) out on aurora.

Comment 44

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/58436ea95190
https://hg.mozilla.org/releases/mozilla-aurora/rev/fe4f5ad3215f
https://hg.mozilla.org/releases/mozilla-aurora/rev/3b5587971ba2

Comment 45

[miralobontiu]

Hi Liz,

Is the qe-verify+ flag still valid?

Thanks!

Comment 46

[Liz Henry (:lizzard Please n-i to RyanVM, jcristau, or pascal)]

Since it's been 3 years without a duplicate bug I don't think we need to verify. Thanks!