654079 - Reposition app tabs when leaving private browsing mode

Status: VERIFIED FIXED

(Firefox :: Tabbed Browser, defect)

Description

[mcdavis941 (sporadically reading bugmail)]

You can end up with pinned app tabs within the scrolling region of the tabstrip rather than being pinned to the left of it, if you create enough tabs while you're in private browsing and then exit private browsing.

Best stated in an STR:

1 - Start with a fresh profile (zero add-ons, default theme).
2 - Make sure the window is not maximized and not fullscreen.  (This may not matter, but that's how I did it.)
3 - Pin some app tabs (try four).
4 - Create enough other unpinned tabs (try fourteen) to trigger tabstrip scrolling. 
5 - Enter private browsing.
6 - In private browsing, create enough unpinned tabs (try fourteen) to trigger tabstrip scrolling.
7 - Exit private browsing.
8 - Notice that the four pinned app tabs are now scrolled, rather than being to the left of the tab scroll arrow.

Expect results:
- pinned app tabs are always to the left of the left-most tab scroll arrow

Actual results:
- pinned app tabs are sometimes to the right of the left-most tab scroll arrow, and included in the tabs that scroll

Mozilla/5.0 (Windows NT 6.0; WOW64; rv:5.0a2) Gecko/20110426 Firefox/5.0a2

Notes:
- saw this with Aurora build
- did not try with either fx4 or fx6 nightly, so can't say whether it also happens there or not
- was very easy to reproduce
- simply entering private browsing, and then immediately leaving private browsing without creating any new tabs, did not trigger the error
- saw this whether the firefox button was shown or not, and whether tabs on top or not
- did not try on Mac or Linux 

Please check that I picked the right version when filing this bug .. this is my first bug filed since Aurora was added to the mix of versions.

Comment 1

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Have you tried in Safe Mode (http://support.mozilla.com/en-US/kb/Safe+Mode) with all add-ons disabled? Does it still occur and is the issue reproducible? Also please check in Firefox 4.0.1 so we can get a feeling if it is a regression. Thanks.

Comment 2

[mcdavis941 (sporadically reading bugmail)]

(In reply to comment #1)
> Have you tried in Safe Mode (http://support.mozilla.com/en-US/kb/Safe+Mode)
> with all add-ons disabled? Does it still occur and is the issue reproducible?

Yes, it still happens in safe mode, with Fx4.0.1.

The issue is easily reproducible.

> Also please check in Firefox 4.0.1 so we can get a feeling if it is a
> regression. Thanks.

Yes, in both 4.0.1 and in the Aurora build listed above.  Same result in both cases.

Comment 3

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Ok, same here on OS X with Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:6.0a1) Gecko/20110501 Firefox/6.0a1

Comment 4

[Frank Yan (:fryn)]

Attachment: patch

Comment 5

[Dão Gottwald [:dao]]

Comment on attachment 529665 [details] [diff] [review]
patch

This doesn't look like the right fix -- tabbrowser should handle this automatically.

Comment 6

[Frank Yan (:fryn)]

Attachment: patch v2

Comment 7

[Dão Gottwald [:dao]]

Comment on attachment 529679 [details] [diff] [review]
patch v2

Review of attachment 529679 [details] [diff] [review]:

So why is this needed? _positionPinnedTabs is currently called from pinTab and the overflow event.

Comment 8

[Frank Yan (:fryn)]

(In reply to comment #7)

1. Pin one tab.
2. Open enough tabs to overflow.
3. Enter private browsing mode.
4. Open enough tabs to overflow.
5. Exit private browsing mode.

Upon exiting private mode, in sss_restoreWindow's call to pinTab, aTab.pinned is false, so _positionPinnedTabs is called. In that call, this is the state of the tabbrowser:

this.tabContainer.getAttribute("overflow") == "true"
this.tabs.length == 1
this._numPinnedTabs == 1
this.visibleTabs.length == 1
this._removingTabs.length == 0

This results in doPosition being false, because numPinned == visibleTabs.length.
After that sss_restoreWindow for loop finishes, in sss_restoreHistoryPrecursor's call to pinTab, tabbrowser.tabs.length is much larger than 1, but aTab.pinned is true, so it simply returns.

One way to fix this is to move _positionPinnedTabs at the top of pinTab. What do you think?

Comment 9

[Dão Gottwald [:dao]]

(In reply to comment #8)
> this.tabContainer.getAttribute("overflow") == "true"
> this.tabs.length == 1

This seems bogus... Maybe it's because the underflow and overflow events are asynchronous?

> One way to fix this is to move _positionPinnedTabs at the top of pinTab. What
> do you think?

This wouldn't help if sss_restoreHistoryPrecursor (or any other API consumer) checked .pinned before calling pinTab.

Comment 10

[Frank Yan (:fryn)]

(In reply to comment #9)
> (In reply to comment #8)
> > this.tabContainer.getAttribute("overflow") == "true"
> > this.tabs.length == 1
> 
> This seems bogus... Maybe it's because the underflow and overflow events are
> asynchronous?

I thought so too, but upon exiting private browsing mode with the above STR, no underflow or overflow events are dispatched at all.

> > One way to fix this is to move _positionPinnedTabs at the top of pinTab. What
> > do you think?
> 
> This wouldn't help if sss_restoreHistoryPrecursor (or any other API consumer)
> checked .pinned before calling pinTab.

True. Both patch v1 and v2 don't have that problem though.

Comment 11

[Dão Gottwald [:dao]]

Maybe the numPinned == visibleTabs.length check should go away. An overflowing tab bar full of app tabs without a normal tab doesn't seem like a case we should care about. What could make sense instead is to compare the width consumed by all app tabs with the tab container's width, and let them scroll when they hit a certain threshold.

Comment 12

[Frank Yan (:fryn)]

Attachment: patch v3

(In reply to comment #11)
> What could make sense instead is to compare the width
> consumed by all app tabs with the tab container's width, and let them scroll
> when they hit a certain threshold.

Filed followup bug 654604 for that, but it's an edge case, so I don't think it's worth tackling here.

Comment 13

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Can we please get an automated test for this fix? Thanks.

Comment 14

[Frank Yan (:fryn)]

(In reply to comment #13)
> Can we please get an automated test for this fix? Thanks.

I don't think this is really necessary. There are other tab behaviors much more in need of tests that I'm busy writing. If someone else wants to write a test for this, absolutely go ahead!

Pushed:
https://hg.mozilla.org/mozilla-central/rev/ad65ecec07b5

Comment 15

[Henrik Skupin [:whimboo][⌚️UTC+2]]

(In reply to comment #14)
> I don't think this is really necessary. There are other tab behaviors much
> more in need of tests that I'm busy writing. If someone else wants to write
> a test for this, absolutely go ahead!

So the request is still valid and should not be declined. Otherwise no-one else can step in later to create a test for it because it will not appear in the query.

Comment 16

[Vlad [QA]]

Verified Fixed on Mozilla/5.0 (Windows NT 6.1; rv:7.0a1) Gecko/20110525 Firefox/7.0a1

I have tried also on Mozilla/5.0 (Windows NT 6.1; rv:6.0a2) Gecko/20110525 Firefox/6.0a2 and the issue is reproducible following the steps from bug description. After stopping the private browsing, the apptabs are not displayed at all.