Closed
Bug 1185002
Opened 6 years ago
Closed 5 years ago
Replace HomePagerTabStrip titles in First Run Pane
Categories
(Firefox for Android Graveyard :: Awesomescreen, defect)
Tracking
(firefox43+ fixed, firefox44 fixed)
RESOLVED
FIXED
Firefox 44
People
(Reporter: sebastian, Assigned: liuche, Mentored)
References
Details
(Whiteboard: [lang=java])
Attachments
(6 files, 2 obsolete files)
40 bytes,
text/x-review-board-request
|
sebastian
:
review+
mcomella
:
review+
|
Details |
140.12 KB,
image/png
|
Details | |
142.34 KB,
image/png
|
antlam
:
feedback-
|
Details |
14.07 KB,
patch
|
liuche
:
review+
lizzard
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
7.73 KB,
patch
|
liuche
:
review+
lizzard
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
1.37 KB,
patch
|
liuche
:
review+
lizzard
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
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.
Reporter | ||
Updated•6 years ago
|
Whiteboard: [lang=java]
Assignee | ||
Updated•5 years ago
|
Summary: Replace HomePagerTabStrip in First Run Pane → Replace HomePagerTabStrip titles in First Run Pane
Assignee | ||
Comment 1•5 years ago
|
||
Bug 1185002 - Switching to TabMenuStrip.
Assignee | ||
Comment 2•5 years ago
|
||
Exposing decor interface.
Assignee | ||
Comment 3•5 years ago
|
||
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.
Reporter | ||
Comment 4•5 years ago
|
||
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).
Assignee | ||
Comment 5•5 years ago
|
||
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)
Comment 6•5 years ago
|
||
(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)
Assignee | ||
Comment 7•5 years ago
|
||
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.
Assignee | ||
Comment 8•5 years ago
|
||
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.
Assignee | ||
Comment 9•5 years ago
|
||
Select first panel if it exists.
Assignee | ||
Comment 10•5 years ago
|
||
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)
Assignee | ||
Updated•5 years ago
|
Attachment #8664592 -
Flags: feedback?(s.kaspari)
Assignee | ||
Comment 11•5 years ago
|
||
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)
Assignee | ||
Comment 12•5 years ago
|
||
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 13•5 years ago
|
||
Comment on attachment 8667596 [details]
Screenshot: Two panels with colored titles
Alright then, looks OK to me!
Attachment #8667596 -
Flags: feedback?(alam) → feedback+
Comment 14•5 years ago
|
||
Actually, we currently have a "disabled" color for this label. Let's just stick to the current color.
Flags: needinfo?(liuche)
Updated•5 years ago
|
Attachment #8667596 -
Flags: feedback+ → feedback-
Reporter | ||
Comment 15•5 years ago
|
||
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?
Reporter | ||
Comment 16•5 years ago
|
||
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.
Reporter | ||
Comment 17•5 years ago
|
||
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?
Reporter | ||
Updated•5 years ago
|
Attachment #8664591 -
Flags: feedback?(s.kaspari) → feedback+
Reporter | ||
Updated•5 years ago
|
Attachment #8664592 -
Flags: feedback?(s.kaspari) → feedback+
Reporter | ||
Updated•5 years ago
|
Attachment #8667579 -
Flags: feedback?(s.kaspari) → feedback+
Reporter | ||
Comment 18•5 years ago
|
||
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. :)
Assignee | ||
Comment 19•5 years ago
|
||
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.
Assignee | ||
Comment 20•5 years ago
|
||
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)
Assignee | ||
Comment 21•5 years ago
|
||
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)
Assignee | ||
Updated•5 years ago
|
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)
Assignee | ||
Comment 22•5 years ago
|
||
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
Assignee | ||
Comment 23•5 years ago
|
||
I realized that I am missing one patch - remove all the HomePagerTabStrip code!
Flags: needinfo?(liuche)
Reporter | ||
Comment 24•5 years ago
|
||
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+
Reporter | ||
Comment 25•5 years ago
|
||
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+
Reporter | ||
Comment 26•5 years ago
|
||
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+
Reporter | ||
Comment 27•5 years ago
|
||
(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? :)
Assignee | ||
Comment 28•5 years ago
|
||
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.)
Assignee | ||
Comment 29•5 years ago
|
||
(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)
Comment 30•5 years ago
|
||
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
Assignee | ||
Comment 31•5 years ago
|
||
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
Assignee | ||
Comment 32•5 years ago
|
||
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)
Assignee | ||
Updated•5 years ago
|
Attachment #8664592 -
Attachment is obsolete: true
Assignee | ||
Updated•5 years ago
|
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.
Assignee | ||
Updated•5 years ago
|
Keywords: leave-open
Comment 35•5 years ago
|
||
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
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
status-firefox44:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 44
Reporter | ||
Updated•5 years ago
|
Flags: needinfo?(s.kaspari)
Assignee | ||
Updated•5 years ago
|
Assignee | ||
Comment 37•5 years ago
|
||
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?
Assignee | ||
Comment 38•5 years ago
|
||
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+
Assignee | ||
Updated•5 years ago
|
Attachment #8674493 -
Attachment is patch: true
Assignee | ||
Updated•5 years ago
|
Attachment #8674495 -
Flags: approval-mozilla-aurora?
Assignee | ||
Comment 39•5 years ago
|
||
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!
Assignee | ||
Comment 43•5 years ago
|
||
Yes! I can check this (and bug 1185002) out on aurora.
Comment 44•5 years ago
|
||
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•2 years ago
|
||
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)
Updated•26 days 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
•