Last Comment Bug 696602 - Active tab not shown in tab strip on return from Panorama
: Active tab not shown in tab strip on return from Panorama
Status: RESOLVED FIXED
:
Product: Firefox Graveyard
Classification: Graveyard
Component: Panorama (show other bugs)
: 9 Branch
: All All
: -- normal
: Firefox 11
Assigned To: Raymond Lee [:raymondlee]
:
Mentors:
Depends on: 792806
Blocks:
  Show dependency treegraph
 
Reported: 2011-10-22 13:04 PDT by ael
Modified: 2016-04-12 14:00 PDT (History)
2 users (show)
See Also:
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
v1 (5.45 KB, patch)
2011-11-23 07:55 PST, Raymond Lee [:raymondlee]
no flags Details | Diff | Splinter Review
v2 (5.79 KB, patch)
2011-11-23 20:12 PST, Raymond Lee [:raymondlee]
no flags Details | Diff | Splinter Review
v3 (5.58 KB, patch)
2011-11-25 07:00 PST, Raymond Lee [:raymondlee]
ttaubert: review+
Details | Diff | Splinter Review
Patch for checkin (5.85 KB, patch)
2011-12-04 18:34 PST, Raymond Lee [:raymondlee]
no flags Details | Diff | Splinter Review

Description ael 2011-10-22 13:04:39 PDT
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.
Comment 1 Tim Taubert [:ttaubert] (on PTO, back Aug 29th) 2011-10-22 13:11:37 PDT
Can't reproduce with the latest Nightly. Does the problem still exist if you disable all add-ons?
Comment 2 ael 2011-10-22 15:51:52 PDT
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.
Comment 3 Virgil Dicu [:virgil] [QA] 2011-11-09 02:43:18 PST
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?
Comment 4 ael 2011-11-09 04:45:29 PST
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.
Comment 5 Raymond Lee [:raymondlee] 2011-11-23 00:58:02 PST
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.
Comment 6 Raymond Lee [:raymondlee] 2011-11-23 07:55:00 PST
Created attachment 576503 [details] [diff] [review]
v1
Comment 7 Tim Taubert [:ttaubert] (on PTO, back Aug 29th) 2011-11-23 15:45:18 PST
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.
Comment 8 Raymond Lee [:raymondlee] 2011-11-23 20:12:39 PST
Created attachment 576676 [details] [diff] [review]
v2

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.
Comment 9 Dão Gottwald [:dao] 2011-11-25 04:09:16 PST
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.
Comment 10 Raymond Lee [:raymondlee] 2011-11-25 07:00:45 PST
Created attachment 576926 [details] [diff] [review]
v3

(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.
Comment 11 Raymond Lee [:raymondlee] 2011-11-25 17:59:55 PST
Passed Try
https://tbpl.mozilla.org/?tree=Try&rev=916886db9a6a
Comment 12 Tim Taubert [:ttaubert] (on PTO, back Aug 29th) 2011-12-02 06:03:57 PST
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;
Comment 13 ael 2011-12-02 06:57:27 PST
Thanks for fixing this!
Comment 14 Raymond Lee [:raymondlee] 2011-12-04 18:34:38 PST
Created attachment 578967 [details] [diff] [review]
Patch for checkin
Comment 15 Raymond Lee [:raymondlee] 2011-12-04 18:47:29 PST
Passed Try
https://tbpl.mozilla.org/?tree=Try&rev=91cb9d689673
Comment 16 Tim Taubert [:ttaubert] (on PTO, back Aug 29th) 2011-12-05 02:14:22 PST
https://hg.mozilla.org/integration/fx-team/rev/69b62700aa75
Comment 17 Tim Taubert [:ttaubert] (on PTO, back Aug 29th) 2011-12-06 00:07:52 PST
https://hg.mozilla.org/mozilla-central/rev/69b62700aa75

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