Active tab not shown in tab strip on return from Panorama

RESOLVED FIXED in Firefox 11

Status

Firefox Graveyard
Panorama
RESOLVED FIXED
6 years ago
a year ago

People

(Reporter: ael, Assigned: raymondlee)

Tracking

9 Branch
Firefox 11

Details

Attachments

(1 attachment, 3 obsolete attachments)

(Reporter)

Description

6 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.
Can't reproduce with the latest Nightly. Does the problem still exist if you disable all add-ons?
(Reporter)

Comment 2

6 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.
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?
(Reporter)

Comment 4

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

Comment 5

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

Comment 6

6 years ago
Created attachment 576503 [details] [diff] [review]
v1
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.
(Assignee)

Comment 8

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

Comment 10

6 years ago
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.
Attachment #576676 - Attachment is obsolete: true
Attachment #576676 - Flags: review?(ttaubert)
Attachment #576926 - Flags: review?(ttaubert)
(Assignee)

Comment 11

6 years ago
Passed Try
https://tbpl.mozilla.org/?tree=Try&rev=916886db9a6a
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+
(Reporter)

Comment 13

6 years ago
Thanks for fixing this!
(Assignee)

Comment 14

6 years ago
Created attachment 578967 [details] [diff] [review]
Patch for checkin
Attachment #576926 - Attachment is obsolete: true
(Assignee)

Comment 15

6 years ago
Passed Try
https://tbpl.mozilla.org/?tree=Try&rev=91cb9d689673
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
Last Resolved: 6 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Depends on: 792806
Product: Firefox → Firefox Graveyard
You need to log in before you can comment on or make changes to this bug.