Closed Bug 696602 Opened 8 years ago Closed 8 years ago

Active tab not shown in tab strip on return from Panorama

Categories

(Firefox Graveyard :: Panorama, defect)

9 Branch
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 11

People

(Reporter: aelilea, Assigned: raymondlee)

References

Details

Attachments

(1 file, 3 obsolete files)

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.
Can't reproduce with the latest Nightly. Does the problem still exist if you disable all add-ons?
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.
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?
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.
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
Attached patch v1 (obsolete) — Splinter Review
Assignee: nobody → raymond
Status: NEW → ASSIGNED
Attachment #576503 - Flags: review?(ttaubert)
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.
Attached patch v2 (obsolete) — 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)
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.
Attached patch v3 (obsolete) — 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)
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+
Thanks for fixing this!
Attachment #576926 - Attachment is obsolete: true
https://hg.mozilla.org/integration/fx-team/rev/69b62700aa75
Hardware: x86 → All
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 11
https://hg.mozilla.org/mozilla-central/rev/69b62700aa75
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Product: Firefox → Firefox Graveyard
You need to log in before you can comment on or make changes to this bug.