Closed Bug 1067388 Opened 10 years ago Closed 10 years ago

Scroll newly opened tabs into position

Categories

(Firefox for Android Graveyard :: General, defect)

35 Branch
ARM
Android
defect
Not set
normal

Tracking

(Not tracked)

VERIFIED FIXED
Firefox 36

People

(Reporter: aaronmt, Assigned: lucasr)

References

Details

(Keywords: reproducible)

Attachments

(2 files)

When new tabs are opened in excess to the available estate given in the new tab-bar, the tab-bar should automatically scroll to the new tab (and perhaps, stack the other tabs akin to the side to make room for the new tab).

Steps to replicate on Nightly (09/15): keep opening new tabs; see that tabs are off-screen. The tab-bar should scroll.
Attached image screenshot.png
The new tab should also fit entirely within view. The cut-off tab should not mingle with the tab button
Yeah, there's a lot of work to be done around managing tab visibility in the strip.
Assignee: nobody → lucasr.at.mozilla
Comment on attachment 8511997 [details] [diff] [review]
Scroll ensure selected tab is always fully visible (r=mcomella)

Ensures the selected tab in the strip becomes full visible.
Attachment #8511997 - Flags: review?(michael.l.comella)
Comment on attachment 8511997 [details] [diff] [review]
Scroll ensure selected tab is always fully visible (r=mcomella)

Review of attachment 8511997 [details] [diff] [review]:
-----------------------------------------------------------------

r+ with the suggested change (if you agree).

::: mobile/android/base/tabs/TabStripView.java
@@ +85,5 @@
> +        treeObserver.addOnPreDrawListener(new OnPreDrawListener() {
> +            @Override
> +            public boolean onPreDraw() {
> +                if (treeObserver.isAlive()) {
> +                    treeObserver.removeOnPreDrawListener(this);

tldr; Get the view tree observer again instead of keeping the reference.

Looking at the Android source, it looks like the observer only gets killed when it is merged into another observer [1]. This only seems to occur with the View.mFloatingTreeObserver getting merged (and killed) into the View.mAttachInfo.mTreeObserver [2]. The mFloatingTreeObserver seems temporary until the mAttachInfo is initialized (View.dispatchAttachedToWindow [3]) and a new instance will be constructed when mAttachInfo is cleared and the view tree observer is needed (View.dispatchDetachedFromWindow [4]).

The AttachInfo where the observer is attached to the ViewRootImpl [5] so this AttachInfo will be re-used each time we're attached.

I *assume* that we will never have onPreDraw called when we're not attached so we should always be getting an observer that has our listener when calling getViewTreeObserver (it's either on the initial floating tree observer, or stuck forever to the attachInfo). However, I don't think our long-lived reference is necessarily valid.

[1]: http://androidxref.com/4.4.4_r1/xref/frameworks/base/core/java/android/view/ViewTreeObserver.java#378
[2]: http://androidxref.com/4.4.4_r1/xref/frameworks/base/core/java/android/view/View.java#12577
[3]: http://androidxref.com/4.4.4_r1/xref/frameworks/base/core/java/android/view/View.java#12569
[4]: http://androidxref.com/4.4.4_r1/xref/frameworks/base/core/java/android/view/View.java#12640
[5]: http://androidxref.com/4.4.4_r1/xref/frameworks/base/core/java/android/view/ViewRootImpl.java#365
Attachment #8511997 - Flags: review?(michael.l.comella) → review+
(In reply to Michael Comella (:mcomella) from comment #5)
> Comment on attachment 8511997 [details] [diff] [review]
> Scroll ensure selected tab is always fully visible (r=mcomella)
> 
> Review of attachment 8511997 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r+ with the suggested change (if you agree).
> 
> ::: mobile/android/base/tabs/TabStripView.java
> @@ +85,5 @@
> > +        treeObserver.addOnPreDrawListener(new OnPreDrawListener() {
> > +            @Override
> > +            public boolean onPreDraw() {
> > +                if (treeObserver.isAlive()) {
> > +                    treeObserver.removeOnPreDrawListener(this);
> 
> tldr; Get the view tree observer again instead of keeping the reference.
> 
> Looking at the Android source, it looks like the observer only gets killed
> when it is merged into another observer [1]. This only seems to occur with
> the View.mFloatingTreeObserver getting merged (and killed) into the
> View.mAttachInfo.mTreeObserver [2]. The mFloatingTreeObserver seems
> temporary until the mAttachInfo is initialized
> (View.dispatchAttachedToWindow [3]) and a new instance will be constructed
> when mAttachInfo is cleared and the view tree observer is needed
> (View.dispatchDetachedFromWindow [4]).
> 
> The AttachInfo where the observer is attached to the ViewRootImpl [5] so
> this AttachInfo will be re-used each time we're attached.
> 
> I *assume* that we will never have onPreDraw called when we're not attached
> so we should always be getting an observer that has our listener when
> calling getViewTreeObserver (it's either on the initial floating tree
> observer, or stuck forever to the attachInfo). However, I don't think our
> long-lived reference is necessarily valid.
> 
> [1]:
> http://androidxref.com/4.4.4_r1/xref/frameworks/base/core/java/android/view/
> ViewTreeObserver.java#378
> [2]:
> http://androidxref.com/4.4.4_r1/xref/frameworks/base/core/java/android/view/
> View.java#12577
> [3]:
> http://androidxref.com/4.4.4_r1/xref/frameworks/base/core/java/android/view/
> View.java#12569
> [4]:
> http://androidxref.com/4.4.4_r1/xref/frameworks/base/core/java/android/view/
> View.java#12640
> [5]:
> http://androidxref.com/4.4.4_r1/xref/frameworks/base/core/java/android/view/
> ViewRootImpl.java#365

Fair enough, done. Great digging btw :-)
https://hg.mozilla.org/mozilla-central/rev/3e842e9a5746
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 36
Status: RESOLVED → VERIFIED
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: