Make TabStripView a RecyclerView

RESOLVED FIXED in Firefox 53

Status

()

Firefox for Android
General
P3
normal
RESOLVED FIXED
9 months ago
6 months ago

People

(Reporter: Tom Klein, Assigned: Tom Klein)

Tracking

(Blocks: 1 bug)

unspecified
Firefox 53
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox53 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(3 attachments)

Comment hidden (empty)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 4

9 months ago
A couple notes:

*) The current TabStripView does a smoothScrollToPosition (i.e. an animated scroll) in some situations, including when a new tab is added using the "+" button in the tab strip, or when the TabStripView becomes visible after viewing the tabs panel.  When you add a tab, the animation is from the current position to the last position, and the time it takes is linear in the number of tabs you have to scroll through.  So, if you have 50 tabs open and you've scrolled to the first tab, and then tap "+" for a new tab, you have to wait a couple of seconds for the animation to run (on an emulator at least, but I expect it's basically the same on a real device).  So I've switched to just using scrollToPosition, and I think the animations look good, but if we really want the smooth animated scroll back just let me know.

*) The one issue I'm aware of that is sort of hackily resolved right now is: if you have a bunch of tabs opened and the last one is the selected tab, when you change orientation we're supposed to scroll to make sure the currently selected tab is in view.  But when you switch to portrait orientation from landscape, sometimes the scroll doesn't quite go far enough - it leaves about 1.25 tabs unscrolled, so the last tab isn't visible.  For all but the last two tabs it doesn't seem to be an issue :/  Removing the item decoration has no effect, so that's not the issue this time, and post()ing the scroll doesn't help.  Delaying that scroll for 50msec does seem to fix it though, so that's what's currently being done.  I don't see any flashing (I assume it's happening while the orientation animation is running?), but I don't have a physical tablet, so I've only been able to test on emulators.

*) Lastly, the current TabStripView uses an AnimatorListener to 'setLayerType(View.LAYER_TYPE_HARDWARE);' when it runs its remove, add, and restore animations.

https://dxr.mozilla.org/mozilla-central/source/mobile/android/base/java/org/mozilla/gecko/tabs/TabStripView.java#431

With RecyclerView the remove and add animations have been moved into RecyclerView animations, which don't set the layer type (I assume), so is it really still worth keeping the AnimatorListener stuff just for the restore animation?  My preference would be to remove it, but I don't really know how much of a difference it makes.  What do you think?
Great!

I do not have access to a tablet this week. But I'll try this and review it next week! :)
Priority: -- → P3

Comment 6

9 months ago
mozreview-review
Comment on attachment 8810564 [details]
Bug 1317446 - 1. Make TabStripView a RecyclerView.

https://reviewboard.mozilla.org/r/92808/#review92950

::: mobile/android/base/java/org/mozilla/gecko/tabs/TabStripAdapter.java:73
(Diff revision 1)
> +            }
> +        }
> -        }
> +    }
>  
> -        return tabs.indexOf(tab);
> +    /* package */ void removeTab(Tab tab) {
> +        final int position =  getPositionForTab(tab);

nit: Two whitespaces after '='
Attachment #8810564 - Flags: review?(s.kaspari) → review+

Comment 7

9 months ago
mozreview-review
Comment on attachment 8810565 [details]
Bug 1317446 - 2. Add TabStripItemAnimator for TabStripView item animations.

https://reviewboard.mozilla.org/r/92810/#review92996
Attachment #8810565 - Flags: review?(s.kaspari) → review+

Comment 8

9 months ago
mozreview-review
Comment on attachment 8810566 [details]
Bug 1317446 - 3. Add TabStripDividerItem for TabStripView dividers and spacing.

https://reviewboard.mozilla.org/r/92812/#review92998
Attachment #8810566 - Flags: review?(s.kaspari) → review+
Code-wise this looks great. I can give you more feedback next week and try this on my tablets.
I just remembered that I wanted to test those patches on my tablet. Sorry! Setting NI flag to remember doing that later (Tablet is charging now!).
Flags: needinfo?(s.kaspari)
Just tested it on my N9 and it's great. I think this can land!
Flags: needinfo?(s.kaspari) → needinfo?(twointofive)
Blocks: 1324034
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

8 months ago
Flags: needinfo?(twointofive)

Comment 15

8 months ago
Pushed by twointofive@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/2ccfe8d2a65a
1. Make TabStripView a RecyclerView. r=sebastian
https://hg.mozilla.org/integration/autoland/rev/7931c003514e
2. Add TabStripItemAnimator for TabStripView item animations. r=sebastian
https://hg.mozilla.org/integration/autoland/rev/a4eff01501e5
3. Add TabStripDividerItem for TabStripView dividers and spacing. r=sebastian

Comment 16

8 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/2ccfe8d2a65a
https://hg.mozilla.org/mozilla-central/rev/7931c003514e
https://hg.mozilla.org/mozilla-central/rev/a4eff01501e5
Status: NEW → RESOLVED
Last Resolved: 8 months ago
status-firefox53: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 53

Updated

8 months ago
Blocks: 925108

Updated

6 months ago
Depends on: 1339066
(Assignee)

Updated

6 months ago
Depends on: 1340929
You need to log in before you can comment on or make changes to this bug.