The default bug view has changed. See this FAQ.

Reposition app tabs when leaving private browsing mode

VERIFIED FIXED in Firefox 7

Status

()

Firefox
Tabbed Browser
VERIFIED FIXED
6 years ago
3 years ago

People

(Reporter: mcdavis941 (sporadically reading bugmail), Assigned: fryn)

Tracking

Trunk
Firefox 7
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite ?

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

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.
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.
(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.
Version: 5 Branch → 4.0 Branch
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
Blocks: 563730
OS: Windows Vista → All
Hardware: x86_64 → All
Summary: on return from private browsing, apptabs are in the tab scrolling region instead of outside it → Leaving private browsing mode with tab scrolling active, app tabs are part of the tab scrolling region
(Assignee)

Updated

6 years ago
Assignee: nobody → fryn
Status: NEW → ASSIGNED
Summary: Leaving private browsing mode with tab scrolling active, app tabs are part of the tab scrolling region → Reposition app tabs when leaving private browsing mode
Version: 4.0 Branch → Trunk
(Assignee)

Comment 4

6 years ago
Created attachment 529665 [details] [diff] [review]
patch
Attachment #529665 - Flags: review?(dao)
Comment on attachment 529665 [details] [diff] [review]
patch

This doesn't look like the right fix -- tabbrowser should handle this automatically.
Attachment #529665 - Flags: review?(dao) → review-
(Assignee)

Comment 6

6 years ago
Created attachment 529679 [details] [diff] [review]
patch v2
Attachment #529665 - Attachment is obsolete: true
Attachment #529679 - Flags: review?(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.
(Assignee)

Comment 8

6 years ago
(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?
(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.
(Assignee)

Comment 10

6 years ago
(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.
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.
(Assignee)

Updated

6 years ago
Blocks: 654604
(Assignee)

Comment 12

6 years ago
Created attachment 529876 [details] [diff] [review]
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.
Attachment #529679 - Attachment is obsolete: true
Attachment #529679 - Flags: review?(dao)
Attachment #529876 - Flags: review?(dao)

Updated

6 years ago
Attachment #529876 - Flags: review?(dao) → review+
Can we please get an automated test for this fix? Thanks.
Flags: in-testsuite?
(Assignee)

Comment 14

6 years ago
(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
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Flags: in-testsuite? → in-testsuite-
Resolution: --- → FIXED
(Assignee)

Updated

6 years ago
Target Milestone: --- → Firefox 7
(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.
Flags: in-testsuite- → in-testsuite?

Comment 16

6 years ago
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.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.