Closed Bug 696602 Opened 13 years ago Closed 13 years ago

Active tab not shown in tab strip on return from Panorama

Tracking

(Not tracked)

Status:

RESOLVED FIXED

Milestone:

Firefox 11

People

(Reporter: aelilea, Assigned: raymondlee)

References

Details

Attachments

(1 file, 3 obsolete files)

v1 13 years ago Raymond Lee [:raymondlee] 5.45 KB, patch		Details \| Diff \| Splinter Review
v2 13 years ago Raymond Lee [:raymondlee] 5.79 KB, patch		Details \| Diff \| Splinter Review
v3 13 years ago Raymond Lee [:raymondlee] 5.58 KB, patch	ttaubert : review+	Details \| Diff \| Splinter Review
Patch for checkin 13 years ago Raymond Lee [:raymondlee] 5.85 KB, patch		Details \| Diff \| Splinter Review

ael

Reporter

Description

•

13 years ago

User Agent: Mozilla/5.0 (X11; Linux i686; rv:9.0a2) Gecko/20111022 Firefox/9.0a2 Build ID: 20111022042018 Steps to reproduce: Select a tab in Panorama from a tab group containing so many tabs that they will not all simultaneusly fit in the tab strip. Actual results: The correct tab is displayed in the browser, but the tab itself is often not shown, because the tab strip is not scrolled to the correct position (such that the active tab is actually visible) Expected results: The active tab should always be visible after selecting a new tab.

Tim Taubert [:ttaubert] (inactive)

Comment 1

•

13 years ago

Can't reproduce with the latest Nightly. Does the problem still exist if you disable all add-ons?

ael

Reporter

Comment 2

•

13 years ago

Yes, I had reproduced it in a clean profile before posting the bug. It doesn't happen every time, i.e. I haven't been able to figure out what exactly it depends upon.

Virgil Dicu [:virgil] [QA]

Comment 3

•

13 years ago

Mozilla/5.0 (X11; Linux x86_64; rv:10.0a1) Gecko/20111108 Firefox/10.0a1 Can't reproduce either on a Nightly. ael, do you have any updates on this issue? Did you switch between different groups when this happened? Hoe many tabs did you have open in the tab group when you reproduced with the clean profile?

ael

Reporter

Comment 4

•

13 years ago

Yes, it happens on switching between different groups; in my test cases each with >20 tabs in them. It is sometimes noticeable that the tab bar _does_ scroll (as this is animated), but not to the right position. I have a suspicion that it mainly (only?) happens when the selected tab is one of the last ones in the group, but I am not sure about this.

Raymond Lee [:raymondlee]

Assignee

Comment 5

•

13 years ago

STR: 1) open first group with 15+ tabs (15+ tabs should be more than enough to make the scrollbar left and right buttons on the tabbar) 2) open second group with 15+ tabs 3) Click on the last tab of first group 4) Go back into Panorama 5) Click on the last tab of second group 6) Repeat 3), 4) and 5) and you should see the issue.

Status: UNCONFIRMED → NEW

Ever confirmed: true

OS: Linux → All

Raymond Lee [:raymondlee]

Assignee

Comment 6

•

13 years ago

Attached patch v1 (obsolete) — Details — Splinter Review

Assignee: nobody → raymond

Status: NEW → ASSIGNED

Attachment #576503 - Flags: review?(ttaubert)

Tim Taubert [:ttaubert] (inactive)

Comment 7

•

13 years ago

Comment on attachment 576503 [details] [diff] [review] v1 Review of attachment 576503 [details] [diff] [review]: ----------------------------------------------------------------- Isn't this more a combination of some 'gBrowser.selectedTab = tab' calls combined with 'gBrowser.showOnlyTheseTabs()'? What I want to say is that I don't think we should work around this in Panorama but fix it in <tabbrowser> if that's the bug's source.

Raymond Lee [:raymondlee]

Assignee

Comment 8

•

13 years ago

Attached patch v2 (obsolete) — Details — Splinter Review

Moved some code to the showOnlyTheseTabs(). However, still need to disable mTabstrip.smoothScroll in Panorama to avoid mTabstrip.ensureElementIsVisible() showing scroll animation on tabSelect event, hence the tab visible check code in showOnlyTheseTabs can work correctly.

Attachment #576503 - Attachment is obsolete: true

Attachment #576503 - Flags: review?(ttaubert)

Attachment #576676 - Flags: review?(ttaubert)

Dão Gottwald [:dao]

Comment 9

•

13 years ago

Comment on attachment 576676 [details] [diff] [review] v2 >diff --git a/browser/base/content/tabbrowser.xml b/browser/base/content/tabbrowser.xml >--- a/browser/base/content/tabbrowser.xml >+++ b/browser/base/content/tabbrowser.xml >@@ -1958,16 +1958,22 @@ > <body> > <![CDATA[ > Array.forEach(this.tabs, function(tab) { > if (aTabs.indexOf(tab) == -1) > this.hideTab(tab); > else > this.showTab(tab); > }, this); >+ >+ let scrollRect = this.tabContainer.mTabstrip.scrollClientRect; >+ let tab = this.selectedTab.getBoundingClientRect(); >+ >+ if (scrollRect.left > tab.left || tab.right > scrollRect.right) >+ this.tabContainer.mTabstrip.ensureElementIsVisible(this.selectedTab, false); These checks look redundant, ensureElementIsVisible should be doing them.

Raymond Lee [:raymondlee]

Assignee

Comment 10

•

13 years ago

Attached patch v3 (obsolete) — Details — Splinter Review

(In reply to Dão Gottwald [:dao] from comment #9) > Comment on attachment 576676 [details] [diff] [review] [diff] [details] [review] > v2 > > >diff --git a/browser/base/content/tabbrowser.xml b/browser/base/content/tabbrowser.xml > >--- a/browser/base/content/tabbrowser.xml > >+++ b/browser/base/content/tabbrowser.xml > >@@ -1958,16 +1958,22 @@ > > <body> > > <![CDATA[ > > Array.forEach(this.tabs, function(tab) { > > if (aTabs.indexOf(tab) == -1) > > this.hideTab(tab); > > else > > this.showTab(tab); > > }, this); > >+ > >+ let scrollRect = this.tabContainer.mTabstrip.scrollClientRect; > >+ let tab = this.selectedTab.getBoundingClientRect(); > >+ > >+ if (scrollRect.left > tab.left || tab.right > scrollRect.right) > >+ this.tabContainer.mTabstrip.ensureElementIsVisible(this.selectedTab, false); > > These checks look redundant, ensureElementIsVisible should be doing them. Removed that code.

Attachment #576676 - Attachment is obsolete: true

Attachment #576676 - Flags: review?(ttaubert)

Attachment #576926 - Flags: review?(ttaubert)

Raymond Lee [:raymondlee]

Assignee

Comment 11

•

13 years ago

Passed Try https://tbpl.mozilla.org/?tree=Try&rev=916886db9a6a

Tim Taubert [:ttaubert] (inactive)

Comment 12

•

13 years ago

Comment on attachment 576926 [details] [diff] [review] v3 Review of attachment 576926 [details] [diff] [review]: ----------------------------------------------------------------- Nice, thanks Raymond! ::: browser/components/tabview/ui.js @@ +517,5 @@ > this._isChangingVisibility = true; > > + // store tab strip smooth scroll value and disable it. > + this._originalSmoothScroll = gBrowser.tabContainer.mTabstrip.smoothScroll; > + gBrowser.tabContainer.mTabstrip.smoothScroll = false; Nit: let tabStrip = gBrowser.tabContainer.mTabstrip;

Attachment #576926 - Flags: review?(ttaubert) → review+

ael

Reporter

Comment 13

•

13 years ago

Thanks for fixing this!

Raymond Lee [:raymondlee]

Assignee

Comment 14

•

13 years ago

Attached patch Patch for checkin — Details — Splinter Review

Attachment #576926 - Attachment is obsolete: true

Raymond Lee [:raymondlee]

Assignee

Comment 15

•

13 years ago

Passed Try https://tbpl.mozilla.org/?tree=Try&rev=91cb9d689673

Tim Taubert [:ttaubert] (inactive)

Comment 16

•

13 years ago

https://hg.mozilla.org/integration/fx-team/rev/69b62700aa75

Hardware: x86 → All

Whiteboard: [fixed-in-fx-team]

Target Milestone: --- → Firefox 11

Tim Taubert [:ttaubert] (inactive)

Comment 17

•

13 years ago

https://hg.mozilla.org/mozilla-central/rev/69b62700aa75

Status: ASSIGNED → RESOLVED

Closed: 13 years ago

Resolution: --- → FIXED

Whiteboard: [fixed-in-fx-team]

Tim Taubert [:ttaubert] (inactive)

Updated

•

12 years ago

Depends on: 792806

Nobody; OK to take it and work on it

Updated

•

9 years ago

Product: Firefox → Firefox Graveyard

You need to log in before you can comment on or make changes to this bug.