See screenshot.

Yeah, our tab strip on tablets is just an ordinary LinearLayout. We need to add side-scrolling support by enclosing it in a HorizontalScrollview or something.

The instanceof business here feels hacky, but this was the least invasive way I could think of fixing this. I also experimented with making TabMenuStrip itself into a HorizontalScrollView instead of a LinearLayout, but that would require changing a bunch of its logic. However, we still need to do something here to automatically scroll the tab strip when the user selects a page off to the side, since it doesn't automatically move over.

Comment on attachment 8393844 [details] [diff] [review] (WIP) Scroll the tablet tab strip Review of attachment 8393844 [details] [diff] [review]: ----------------------------------------------------------------- This is a good direction but, as you pointed out, we need to sync the HorizontalScrollView's scroll position with the strip selection as you scroll around. Google has published a pretty good sample implementation for the very same kind of widget. It does more than what we need here though. So I suggest you to have a look at: http://developer.android.com/samples/SlidingTabsBasic/project.html http://developer.android.com/samples/SlidingTabsBasic/src/com.example.android.common/view/SlidingTabLayout.html http://developer.android.com/samples/SlidingTabsBasic/src/com.example.android.common/view/SlidingTabStrip.html And borrow only the code that we need for our UI.

This patch turns TabMenuStrip into a HorizontalScrollView, and splits out the current LinearLayout logic into a new class called SlidingTabStrip. It looks like a lot has changed, but most of it is copy/pasting to this new file. I don't love how I'm just passing along calls from TabMenuStrip to SlidingTabStrip, but I wasn't sure of a good way to change this without doing more refactoring.

Comment on attachment 8401648 [details] [diff] [review] Scroll the tablet tab strip Review of attachment 8401648 [details] [diff] [review]: ----------------------------------------------------------------- Looks good (assuming there are not functional changes here). ::: mobile/android/base/home/SlidingTabStrip.java @@ +18,5 @@ > +import android.view.accessibility.AccessibilityEvent; > +import android.widget.LinearLayout; > +import android.widget.TextView; > + > +class SlidingTabStrip extends LinearLayout nit: remove trailing space.

I assume you're still going to post the patches implementing the final behaviour?

(In reply to Lucas Rocha (:lucasr) from comment #6) > I assume you're still going to post the patches implementing the final > behaviour? Sorry, I included those changes in the patch that was included. Basically, the only functional change is the scrollToTab call added in TabMenuStrip's onPageScrolled method. However, the logic in SlidingTabStrip entirely came from the current TabMenuStrip code.

Comment on attachment 8401648 [details] [diff] [review] Scroll the tablet tab strip Review of attachment 8401648 [details] [diff] [review]: ----------------------------------------------------------------- Tried the APK a bit. This is implementing the same behaviour than, say, the Google Play store strip. it unconditionally moves the selected tab to align close to the left with an offset. I personally find this behaviour a bit confusing. Maybe because the scroll position moves too fast? It would be nice to get some input from ibarlow before landing. I'm finding hard to separate the functional changes from the code that is just being moved around. I might be missing out some important implementation details in my review here.

(In reply to Lucas Rocha (:lucasr) from comment #8) > Comment on attachment 8401648 [details] [diff] [review] > Scroll the tablet tab strip > > Review of attachment 8401648 [details] [diff] [review]: > ----------------------------------------------------------------- > > Tried the APK a bit. This is implementing the same behaviour than, say, the > Google Play store strip. it unconditionally moves the selected tab to align > close to the left with an offset. I personally find this behaviour a bit > confusing. Maybe because the scroll position moves too fast? It would be > nice to get some input from ibarlow before landing. > > I'm finding hard to separate the functional changes from the code that is > just being moved around. I might be missing out some important > implementation details in my review here. I can split the patch up, and I'll post a build for ibarlow to try. This code is kinda confusing as it is, since we have this Decor interface, which does more than the OnPageChangeListener interface. I feel like there is room to improve the readability of this logic, but we should do that in a follow-up.

This patch just factors out the current LinearLayout TabMenuStrip logic to a new class called TabMenuStripLayout, and turns TabMenuStrip into a HorizontalScrollView that holds this new layout as its only child view. If you compare TabMenuStripLayout to the current TabMenuStrip code on m-c, they are essentially the same.

This patch adds the logic to scroll the tab strip when the user scrolls the home pager. This is the patch we may want to tweak depending on desired UX.

Here's a build I made with some test panels hacked in for demonstration purposes: https://dl.dropboxusercontent.com/u/3358452/fennec-tab-strip.apk The main question is, should we scroll the tab strip whenever the user scrolls the home pager, or should we only scroll the tab strip when the user scrolls to a tab that is currently not completely visible?

Comment on attachment 8402800 [details] [diff] [review] (Part 1) Turn TabMenuStrip into a HorizontalScrollView, moving LinearLayout logic to TabMenuStripLayout Review of attachment 8402800 [details] [diff] [review]: ----------------------------------------------------------------- Thanks for splitting the patches! Really appreciate it :-) ::: mobile/android/base/home/TabMenuStripLayout.java @@ +27,5 @@ > + implements View.OnFocusChangeListener { > + > + private HomePager.OnTitleClickListener mOnTitleClickListener; > + private Drawable mStrip; > + private View mSelectedView; While at it, fix the the coding style inconsistencies in both classes (in a separate patch). Some members are using the 'm' prefix, others are not. @@ +57,5 @@ > + button.setText(title.toUpperCase()); > + > + addView(button); > + button.setOnClickListener(new ViewClickListener(getChildCount() - 1)); > + button.setOnFocusChangeListener(this); Nothing to do with this patch but I'm not a fan of exposing public methods by using 'this' as listener. It breaks encapsulation. Not a big deal though. Maybe file a (mentored) follow-up bug?

Comment on attachment 8402802 [details] [diff] [review] (Part 2) Scroll tab strip when HomePager is scrolled Review of attachment 8402802 [details] [diff] [review]: ----------------------------------------------------------------- Patch looks good but I'd like to get input from ibarlow before giving r+. I feel we should not unconditionally move the selected title to "left + mTitleOffset". We should only make sure the selected title is always fully visible. This means: 1. Don't scroll the strip if the title is already fully visible when you tap on it. 2. Scroll to make the selected title fully visible when you tap on it. The behaviour feels correct for when you're just swiping across panels. ::: mobile/android/base/home/TabMenuStrip.java @@ +77,5 @@ > + return; > + } > + > + final View selectedChild = mLayout.getChildAt(tabIndex); > + if (selectedChild != null) { Don't you check if the tabIndex is valid above? This null check seems unnecessary. @@ +82,5 @@ > + int targetScrollX = selectedChild.getLeft() + positionOffset; > + > + if (tabIndex > 0 || positionOffset > 0) { > + // If we're not at the first child and are mid-scroll, make sure we obey the offset > + targetScrollX -= mTitleOffset; The first child's width might be smaller than mTitleOffset leading to off bounds scroll value here. Shouldn't be an issue as HorizontalScrollView's scrollTo() clamps the passed values: https://github.com/android/platform_frameworks_base/blob/master/core/java/android/widget/HorizontalScrollView.java#L1583

(In reply to :Margaret Leibovic from comment #12) > Here's a build I made with some test panels hacked in for demonstration > purposes: > > https://dl.dropboxusercontent.com/u/3358452/fennec-tab-strip.apk > > The main question is, should we scroll the tab strip whenever the user > scrolls the home pager, or should we only scroll the tab strip when the user > scrolls to a tab that is currently not completely visible? Thanks for posting this build, Margaret. Having tried it, I think the tab strip should scroll whenever the user scrolls the home pager, it just feels a little nicer somehow. This seems to be the behaviour in the build you posted when you scroll from the left most panel to the right most. The behaviour seems to be different when you scroll from the far right to the far left though, so we should probably adjust that so it behaves consistently in both directions.

A follow-up to Part 1 to clean up prefixes.

Comment on attachment 8404794 [details] [diff] [review] (Part 1.5) Remove m prefixes Review of attachment 8404794 [details] [diff] [review]: ----------------------------------------------------------------- Yep.

(In reply to Ian Barlow (:ibarlow) from comment #15) > (In reply to :Margaret Leibovic from comment #12) > > Here's a build I made with some test panels hacked in for demonstration > > purposes: > > > > https://dl.dropboxusercontent.com/u/3358452/fennec-tab-strip.apk > > > > The main question is, should we scroll the tab strip whenever the user > > scrolls the home pager, or should we only scroll the tab strip when the user > > scrolls to a tab that is currently not completely visible? > > Thanks for posting this build, Margaret. > > Having tried it, I think the tab strip should scroll whenever the user > scrolls the home pager, it just feels a little nicer somehow. > > This seems to be the behaviour in the build you posted when you scroll from > the left most panel to the right most. > > The behaviour seems to be different when you scroll from the far right to > the far left though, so we should probably adjust that so it behaves > consistently in both directions. I was playing around with this yesterday, and I was having a hard time coming up with rules for how this should behave. Like, what should happen while scrolling back-and-forth within the middle of the tab strip when it has room to overflow in both directions? I'd be more inclined to go with lucasr's suggestion, since it seems easier to implement. The rule in that case would be "only scroll the tab strip if scrolling required to see the entire selected tab". However, for the record, I just took the behavior in the patch/build I posted from the SlidingTabsBasic project that lucasr linked to in comment 3, and it is in fact consistent with what the play store does, so maybe we should just go with my original approach and be platform consistent ;)

This is an updated version of my first approach. I spent some more time today trying to play around with changing the behavior, but I wasn't able to come up with something that worked well. I'm probably being a lazy developer, but I think being consistent with the play store app is good enough for me :) Or we can continue to investigate polishing this interaction more in a follow-up, but we should definitely land a basic fix sooner rather than later, since this is currently quite busted.

Comment on attachment 8404935 [details] [diff] [review] (Part 2 v2) Scroll tab strip when HomePager is scrolled Review of attachment 8404935 [details] [diff] [review]: ----------------------------------------------------------------- (In reply to :Margaret Leibovic from comment #19) > Created attachment 8404935 [details] [diff] [review] > (Part 2 v2) Scroll tab strip when HomePager is scrolled > > This is an updated version of my first approach. I spent some more time > today trying to play around with changing the behavior, but I wasn't able to > come up with something that worked well. > > I'm probably being a lazy developer, but I think being consistent with the > play store app is good enough for me :) > > Or we can continue to investigate polishing this interaction more in a > follow-up, but we should definitely land a basic fix sooner rather than > later, since this is currently quite busted. Patch looks good but, to be honest, I don't really like the implemented behaviour. I actually complained about it back when it first appeared on the Google Play app :-) But it's up to ibarlow. I won't oppose if he's fine with it. Giving f+ for now. I'll flip to r+ once he gives his thumbs up.

(In reply to Lucas Rocha (:lucasr) from comment #20) > Patch looks good but, to be honest, I don't really like the implemented > behaviour. I actually complained about it back when it first appeared on the > Google Play app :-) But it's up to ibarlow. I won't oppose if he's fine with > it. Giving f+ for now. I'll flip to r+ once he gives his thumbs up. Do you have a suggestion for an alternative behaviour? Now would be the time to bring it up :)

(In reply to Ian Barlow (:ibarlow) from comment #21) > (In reply to Lucas Rocha (:lucasr) from comment #20) > > > Patch looks good but, to be honest, I don't really like the implemented > > behaviour. I actually complained about it back when it first appeared on the > > Google Play app :-) But it's up to ibarlow. I won't oppose if he's fine with > > it. Giving f+ for now. I'll flip to r+ once he gives his thumbs up. > > Do you have a suggestion for an alternative behaviour? Now would be the time > to bring it up :) See comment #14 :-)

I see what happened here. Margaret and I were trying this by swiping from panel to panel, and Lucas was tapping the tabs which causes them to jump around if we use the scroll interaction I was favoring. Tried the build again by tapping the tabs, and I think that just made me change my mind, and now I agree with Lucas that we should try to keep them still, and only scroll the tab list if we are moving to one where the tab is partially cut off.

Here's an updated patch that only scrolls the tab strip when it's about to overflow. A build is available here: https://dl.dropboxusercontent.com/u/3358452/fennec-tab-strip.apk

Comment on attachment 8406216 [details] [diff] [review] (Part 2 v3) Scroll tab strip to ensure selected tab is visible Review of attachment 8406216 [details] [diff] [review]: ----------------------------------------------------------------- Looks good to me, could you post a build with the patch? Just want to make sure I'm not missing any edge case.

(In reply to Lucas Rocha (:lucasr) from comment #25) > Comment on attachment 8406216 [details] [diff] [review] > (Part 2 v3) Scroll tab strip to ensure selected tab is visible > > Review of attachment 8406216 [details] [diff] [review]: > ----------------------------------------------------------------- > > Looks good to me, could you post a build with the patch? Just want to make > sure I'm not missing any edge case. I included a link in the last comment, although I realized I didn't make that build with any test panels thrown in. I just updated it to have some test panels: https://dl.dropboxusercontent.com/u/3358452/fennec-tab-strip.apk

Lucas pointed out that my previous patch wasn't handling touch events on the tab strip properly. With this new version, we scroll the home pager as the user scrolls the tab strip, which is the behavior we're currently shipping. Updated build with test panels here: https://dl.dropboxusercontent.com/u/3358452/fennec-tab-strip.apk

(In reply to :Margaret Leibovic from comment #27) > Created attachment 8407650 [details] [diff] [review] > (Part 2 v4) Scroll tab strip to ensure selected tab is visible > > Lucas pointed out that my previous patch wasn't handling touch events on the > tab strip properly. With this new version, we scroll the home pager as the > user scrolls the tab strip, which is the behavior we're currently shipping. My comment on the previousbuild was more about the tap behaviour after scrolling the strip around (the tapped title would jump either to the left+offset or right-offset). The rest of the interaction felt right to me. This new version is consistent with phones somehow but you lose the ability to jump directly to specific tabs (e.g. straight to first or last tabs) which was a nice thing imo. I'd like to hear from ibarlow before reviewing the patch.

For the record, on IRC ibarlow said the most recent patch is the behavior he expected.

Comment on attachment 8407650 [details] [diff] [review] (Part 2 v4) Scroll tab strip to ensure selected tab is visible Review of attachment 8407650 [details] [diff] [review]: ----------------------------------------------------------------- ::: mobile/android/base/home/TabMenuStrip.java @@ +38,3 @@ > private final TabMenuStripLayout layout; > > + private HomePager.OnTouchEventListener onTouchEventListener; nit: I'd go with simply touchEventListener.

(In reply to Lucas Rocha (:lucasr) from comment #30) > Comment on attachment 8407650 [details] [diff] [review] > (Part 2 v4) Scroll tab strip to ensure selected tab is visible > > Review of attachment 8407650 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: mobile/android/base/home/TabMenuStrip.java > @@ +38,3 @@ > > private final TabMenuStripLayout layout; > > > > + private HomePager.OnTouchEventListener onTouchEventListener; > > nit: I'd go with simply touchEventListener. I went with onTouchEventListener to be consistent with onTitleClickListener (the other listener used with the Decor interface).

Ugh, I was testing this some more, and I found that if the tab strip isn't overfowed but you make a gesture on it, we can run into this crash: E/GeckoAppShell( 739): >>> REPORTING UNCAUGHT EXCEPTION FROM THREAD 1 ("main") E/GeckoAppShell( 739): java.lang.IllegalArgumentException: pointerIndex out of range E/GeckoAppShell( 739): at android.view.MotionEvent.nativeGetAxisValue(Native Method) E/GeckoAppShell( 739): at android.view.MotionEvent.getX(MotionEvent.java:1979) E/GeckoAppShell( 739): at android.support.v4.view.MotionEventCompatEclair.getX(MotionEventCompatEclair.java:32) E/GeckoAppShell( 739): at android.support.v4.view.MotionEventCompat$EclairMotionEventVersionImpl.getX(MotionEventCompat.java:91) E/GeckoAppShell( 739): at android.support.v4.view.MotionEventCompat.getX(MotionEventCompat.java:219) E/GeckoAppShell( 739): at android.support.v4.view.ViewPager.onTouchEvent(ViewPager.java:1966) E/GeckoAppShell( 739): at android.view.View.dispatchTouchEvent(View.java:7706) E/GeckoAppShell( 739): at android.view.ViewGroup.dispatchTransformedTouchEvent(ViewGroup.java:2210) E/GeckoAppShell( 739): at android.view.ViewGroup.dispatchTouchEvent(ViewGroup.java:1945) E/GeckoAppShell( 739): at org.mozilla.gecko.home.HomePager.dispatchTouchEvent(HomePager.java:342) E/GeckoAppShell( 739): at android.view.ViewGroup.dispatchTransformedTouchEvent(ViewGroup.java:2216) E/GeckoAppShell( 739): at android.view.ViewGroup.dispatchTouchEvent(ViewGroup.java:1959) E/GeckoAppShell( 739): at android.view.ViewGroup.dispatchTransformedTouchEvent(ViewGroup.java:2216) E/GeckoAppShell( 739): at android.view.ViewGroup.dispatchTouchEvent(ViewGroup.java:1959) E/GeckoAppShell( 739): at android.view.ViewGroup.dispatchTransformedTouchEvent(ViewGroup.java:2216) E/GeckoAppShell( 739): at android.view.ViewGroup.dispatchTouchEvent(ViewGroup.java:1959) E/GeckoAppShell( 739): at android.view.ViewGroup.dispatchTransformedTouchEvent(ViewGroup.java:2216) E/GeckoAppShell( 739): at android.view.ViewGroup.dispatchTouchEvent(ViewGroup.java:1959) E/GeckoAppShell( 739): at android.view.ViewGroup.dispatchTransformedTouchEvent(ViewGroup.java:2216) E/GeckoAppShell( 739): at android.view.ViewGroup.dispatchTouchEvent(ViewGroup.java:1959) E/GeckoAppShell( 739): at android.view.ViewGroup.dispatchTransformedTouchEvent(ViewGroup.java:2216) E/GeckoAppShell( 739): at android.view.ViewGroup.dispatchTouchEvent(ViewGroup.java:1959) E/GeckoAppShell( 739): at android.view.ViewGroup.dispatchTransformedTouchEvent(ViewGroup.java:2216) E/GeckoAppShell( 739): at android.view.ViewGroup.dispatchTouchEvent(ViewGroup.java:1959) E/GeckoAppShell( 739): at com.android.internal.policy.impl.PhoneWindow$DecorView.superDispatchTouchEvent(PhoneWindow.java:2068) E/GeckoAppShell( 739): at com.android.internal.policy.impl.PhoneWindow.superDispatchTouchEvent(PhoneWindow.java:1515) E/GeckoAppShell( 739): at android.app.Activity.dispatchTouchEvent(Activity.java:2458) E/GeckoAppShell( 739): at com.android.internal.policy.impl.PhoneWindow$DecorView.dispatchTouchEvent(PhoneWindow.java:2016) E/GeckoAppShell( 739): at android.view.View.dispatchPointerEvent(View.java:7886) E/GeckoAppShell( 739): at android.view.ViewRootImpl$ViewPostImeInputStage.processPointerEvent(ViewRootImpl.java:3954) E/GeckoAppShell( 739): at android.view.ViewRootImpl$ViewPostImeInputStage.onProcess(ViewRootImpl.java:3833) E/GeckoAppShell( 739): at android.view.ViewRootImpl$InputStage.deliver(ViewRootImpl.java:3399) E/GeckoAppShell( 739): at android.view.ViewRootImpl$InputStage.onDeliverToNext(ViewRootImpl.java:3449) E/GeckoAppShell( 739): at android.view.ViewRootImpl$InputStage.forward(ViewRootImpl.java:3418) E/GeckoAppShell( 739): at android.view.ViewRootImpl$AsyncInputStage.forward(ViewRootImpl.java:3525) E/GeckoAppShell( 739): at android.view.ViewRootImpl$InputStage.apply(ViewRootImpl.java:3426) E/GeckoAppShell( 739): at android.view.ViewRootImpl$AsyncInputStage.apply(ViewRootImpl.java:3582) E/GeckoAppShell( 739): at android.view.ViewRootImpl$InputStage.deliver(ViewRootImpl.java:3399) E/GeckoAppShell( 739): at android.view.ViewRootImpl$InputStage.onDeliverToNext(ViewRootImpl.java:3449) E/GeckoAppShell( 739): at android.view.ViewRootImpl$InputStage.forward(ViewRootImpl.java:3418) E/GeckoAppShell( 739): at android.view.ViewRootImpl$InputStage.apply(ViewRootImpl.java:3426) E/GeckoAppShell( 739): at android.view.ViewRootImpl$InputStage.deliver(ViewRootImpl.java:3399) E/GeckoAppShell( 739): at android.view.ViewRootImpl.deliverInputEvent(ViewRootImpl.java:5602) E/GeckoAppShell( 739): at android.view.ViewRootImpl.doProcessInputEvents(ViewRootImpl.java:5582) E/GeckoAppShell( 739): at android.view.ViewRootImpl.enqueueInputEvent(ViewRootImpl.java:5553) E/GeckoAppShell( 739): at android.view.ViewRootImpl$WindowInputEventReceiver.onInputEvent(ViewRootImpl.java:5682) E/GeckoAppShell( 739): at android.view.InputEventReceiver.dispatchInputEvent(InputEventReceiver.java:185) E/GeckoAppShell( 739): at android.view.InputEventReceiver.nativeConsumeBatchedInputEvents(Native Method) E/GeckoAppShell( 739): at android.view.InputEventReceiver.consumeBatchedInputEvents(InputEventReceiver.java:176) E/GeckoAppShell( 739): at android.view.ViewRootImpl.doConsumeBatchedInput(ViewRootImpl.java:5655) E/GeckoAppShell( 739): at android.view.ViewRootImpl$ConsumeBatchedInputRunnable.run(ViewRootImpl.java:5701) I did some googling, and it looks like this is actually a bug in the support library: https://code.google.com/p/android/issues/detail?id=64553 https://github.com/chrisbanes/PhotoView/issues/31 http://stackoverflow.com/questions/16459196/java-lang-illegalargumentexception-pointerindex-out-of-range-exception-dispat These posts suggest overriding the ViewPager's onTouchEvent method to just eat this exception, but that doesn't feel so great. I don't want to land these patches until I address this, since this crash is really reproducible. Thoughts? I'm going to investigate this more to see if we should change the way we're passing this event to make sure it doesn't tickle this crash.

Okay, back to scrolling the tab strip, but this version fixes the issue where the tab strip scrolls itself after the user scrolls it then taps on an item. This patch is actually simpler than the previous version because instead of caching the scrollX value (that's what was causing that jumping around problem), we can just ask the view for its scrollX value when we're checking to see if the selected tab is too far to the left or right. Build here: https://dl.dropboxusercontent.com/u/3358452/fennec-tab-strip-scroll.apk

Comment on attachment 8411071 [details] [diff] [review] (Part 2 v5) Scroll tab strip to ensure selected tab is visible. Review of attachment 8411071 [details] [diff] [review]: ----------------------------------------------------------------- Tried the build, works fine overall. We'll probably have to tweak the behaviour for when you tap on a title while the previously selected title is off-screen (right now, it feels a bit jumpy).

Landed a folded patch: https://hg.mozilla.org/integration/fx-team/rev/feb85b7e78f5

[Approval Request Comment] Bug caused by (feature/regressing bug #): ability to add more home panels (new APIs in Fx30) User impact if declined: about:home tab strip will be broken if more panels are added Testing completed (on m-c, etc.): just landed on fx-team, tested locally by a few people Risk to taking this patch (and alternatives if risky): low-risk, layout change to about:home tab strip on tablets String or IDL/UUID changes made by this patch: none

Verified as fixed in builds: - 32.0a1 (2014-05-08); - 31.0a2 (2014-05-08); - 30 beta 2; Device: Asus Transformer (Android 4.0.3).

Setting P1 hub bugs to block hub v1 EPIC bug (targeting fx30 release). Filter on epic-hub-bugs.