Closed Bug 1185002 Opened 4 years ago Closed 4 years ago

Replace HomePagerTabStrip titles in First Run Pane

Categories

(Firefox for Android :: Awesomescreen, defect)

All
Android
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 44
Tracking Status
firefox43 + fixed
firefox44 --- fixed

People

(Reporter: sebastian, Assigned: liuche, Mentored)

References

Details

(Whiteboard: [lang=java])

Attachments

(6 files, 2 obsolete files)

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.
Whiteboard: [lang=java]
Blocks: 1199859
Summary: Replace HomePagerTabStrip in First Run Pane → Replace HomePagerTabStrip titles in First Run Pane
Exposing decor interface.
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.
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).
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.
Assignee: nobody → liuche
Status: NEW → ASSIGNED
Flags: needinfo?(alam)
(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.
Flags: needinfo?(alam) → needinfo?(liuche)
Comment on attachment 8664591 [details]
MozReview Request: Bug 1185002 - Remove unused HomePagerTabStrip code. r=mcomella

Bug 1185002 - Use TabMenuStrip in firstrun.
Attachment #8664591 - Attachment description: MozReview Request: Bug 1185002 - Switching to TabMenuStrip. → MozReview Request: Bug 1185002 - Use TabMenuStrip in firstrun.
Comment on attachment 8664592 [details]
MozReview Request: Bug 1185002 - Add custom active/inactive colors. r=sebastian

Add custom active/inactive colors.
Attachment #8664592 - Attachment description: MozReview Request: Exposing decor interface. → MozReview Request: Add custom active/inactive colors.
Select first panel if it exists.
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.
Flags: needinfo?(liuche)
Attachment #8664591 - Flags: feedback?(s.kaspari)
Attachment #8664592 - Flags: feedback?(s.kaspari)
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.
Attachment #8667579 - Flags: feedback?(s.kaspari)
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.
Attachment #8667596 - Flags: feedback?(alam)
Comment on attachment 8667596 [details]
Screenshot: Two panels with colored titles

Alright then, looks OK to me!
Attachment #8667596 - Flags: feedback?(alam) → feedback+
Actually, we currently have a "disabled" color for this label. Let's just stick to the current color.
Flags: needinfo?(liuche)
Attachment #8667596 - Flags: feedback+ → feedback-
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?
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.
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?
Attachment #8664591 - Flags: feedback?(s.kaspari) → feedback+
Attachment #8664592 - Flags: feedback?(s.kaspari) → feedback+
Attachment #8667579 - Flags: feedback?(s.kaspari) → feedback+
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. :)
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 on attachment 8664591 [details]
MozReview Request: Bug 1185002 - Remove unused HomePagerTabStrip code. r=mcomella

Bug 1185002 - Use TabMenuStrip in firstrun. r=sebastian
Attachment #8664591 - Attachment description: MozReview Request: Bug 1185002 - Use TabMenuStrip in firstrun. → MozReview Request: Bug 1185002 - Use TabMenuStrip in firstrun. r=sebastian
Attachment #8664591 - Flags: feedback+ → review?(s.kaspari)
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
Attachment #8664592 - Attachment description: MozReview Request: Add custom active/inactive colors. → MozReview Request: Bug 1185002 - Add custom active/inactive colors. r=sebastian
Attachment #8664592 - Flags: feedback+ → review?(s.kaspari)
Attachment #8667579 - Attachment description: MozReview Request: Select first panel if it exists. → MozReview Request: Bug 1185002 - Select first panel if it exists. r=sebastian
Attachment #8667579 - Flags: feedback+ → review?(s.kaspari)
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
I realized that I am missing one patch - remove all the HomePagerTabStrip code!
Flags: needinfo?(liuche)
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?
Attachment #8664591 - Flags: review?(s.kaspari) → review+
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! :)
Attachment #8664592 - Flags: review?(s.kaspari) → review+
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?
Attachment #8667579 - Flags: review?(s.kaspari) → review+
(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? :)
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.)
(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
Flags: needinfo?(s.kaspari)
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.
Keywords: leave-open
Comment on attachment 8664591 [details]
MozReview Request: Bug 1185002 - Remove unused HomePagerTabStrip code. r=mcomella

Bug 1185002 - Remove unused HomePagerTabStrip code. r=mcomella
Attachment #8664591 - Attachment description: MozReview Request: Bug 1185002 - Use TabMenuStrip in firstrun. r=sebastian → MozReview Request: Bug 1185002 - Remove unused HomePagerTabStrip code. r=mcomella
Attachment #8664591 - Flags: review?(michael.l.comella)
Attachment #8664592 - Attachment is obsolete: true
Attachment #8667579 - Attachment is obsolete: true
Attachment #8664591 - Flags: review?(michael.l.comella) → review+
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.
Keywords: leave-open
Duplicate of this bug: 1185034
Flags: needinfo?(s.kaspari)
Depends on: 1211412
Blocks: 1211412
No longer depends on: 1211412
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
Attachment #8674493 - Flags: review+
Attachment #8674493 - Flags: approval-mozilla-aurora?
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
Attachment #8674495 - Flags: review+
Attachment #8674493 - Attachment is patch: true
Attachment #8674495 - Flags: approval-mozilla-aurora?
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
Attachment #8674496 - Flags: review+
Attachment #8674496 - Flags: approval-mozilla-aurora?
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.
Attachment #8674493 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
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.
Attachment #8674495 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #8674496 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
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!
Flags: qe-verify+
Yes! I can check this (and bug 1185002) out on aurora.
Hi Liz,

Is the qe-verify+ flag still valid?

Thanks!
Flags: needinfo?(lhenry)
Since it's been 3 years without a duplicate bug I don't think we need to verify. Thanks!
Flags: qe-verify-
Flags: qe-verify+
Flags: needinfo?(lhenry)
You need to log in before you can comment on or make changes to this bug.