601228 - App tabs overlapping again

Status: RESOLVED FIXED

(Firefox :: Tabbed Browser, defect)

Description

[Marcia Knous [:marcia]]

Attachment: Screenshot of tab overlap

Yesterday while doing some testing on Mac using the 20100930 build, I snapped the attached screenshot: .  But I haven't yet been able to reproduce - I might have been coming out of Private Browsing mode. 

Bug 579869 has been fixed so in theory this problem should be gone.

Comment 1

[Marcia Knous [:marcia]]

Attachment: Screenshot from latest nightly after restarting

Comment 2

[Marcia Knous [:marcia]]

I will need to try to see if I can reproduce this again as it has been intermittent. I have seen it on my Mac laptop but not using any of the QA lab machines.

Comment 3

[Marcia Knous [:marcia]]

For me this seems to happen after session restore, and is most easily reproduced with Gmail pinned. I just was able to reproduce this with a clean profile.

STR:
1. Open a tab with Gmail in it. Pin it.
2. Open a tab with Google calendar. Don't pin it.
3. Restart Firefox.
4. Observe that the tabs overlap.

Comment 4

[juan becerra [:juanb]]

Probably a dupe of Bug 579869

Comment 6

[Dão Gottwald [:dao]]

Bug 579869 was fixed months ago. This is a regression and we need a regression range.

Comment 7

[Marcia Knous [:marcia]]

Paul: Are you running with any extensions?

Comment 8

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

(In reply to comment #7)
> Paul: Are you running with any extensions?

dom inspector 2.0.8, https everywhere 0.2.3.development.1,  flashblock 1.5.14.2

Comment 9

[Marcia Knous [:marcia]]

I have the following installed:
*Instant [e-mail]
*Add-on Compatibility [e-mail]
*[e-mail]
*NoScript2.0.4false{73a6fe31-595d-460b-a920-fcc0f8843232}
*Adblock Plus1.2.2false{d10d0bf8-f5b5-c8b4-a8b2-2b9879e08c5d}

In the profile I can consistently repro the issue using today's nightly, even if I launch Minefield in safe mode.

I have tried with a clean profile but I cannot yet reproduce it on the latest trunk or on earlier builds.

Comment 10

[Luke Iliffe (Harlequin99)]

This is not Mac specific, it happens on Windows too, see Bug 601930 which is the same AFAICS.

STR from bug 601930:
1. Start Minefield with new profile
2. Open 2 browser windows.
3. Open a AppTab and a regular tab in each window.
4. Alt > File > Exit, Choose "Save and Quit" on Quit Minefield dialog.
5. Start Browser again

Actual Results:
 AppTab overlaps "regular" tab on the Tab Bar in one of the restored windows.

Expected Results:
 Should not.

I will try to locate a regression range over the weekend.

Comment 11

[Luke Iliffe (Harlequin99)]

Regression range on Windows:
works	2010-09-14-04	http://hg.mozilla.org/mozilla-central/rev/5588c9796f0b
broken	2010-09-15-07	http://hg.mozilla.org/mozilla-central/rev/0caec4ddff74

http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=5588c9796f0b&tochange=0caec4ddff74

This is using the STR of comment 10 on Windows 7, it would be helpful if someone with access to a Mac can verify if this is the same range, then we can be sure this and bug 601930 are the same.

Comment 12

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

(In reply to comment #11)
> Regression range on Windows:
> works    2010-09-14-04   
> http://hg.mozilla.org/mozilla-central/rev/5588c9796f0b
> broken    2010-09-15-07   
> http://hg.mozilla.org/mozilla-central/rev/0caec4ddff74
> 
> http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=5588c9796f0b&tochange=0caec4ddff74

Seem to be the case for OS X too. Nothing in there seems very suspicious. The closest would be bug 593967. There was a TM merge in there too, but that doesn't seem likely.

Comment 14

[Dão Gottwald [:dao]]

Could somebody who's able to reproduce this try to find the first bad changeset using hg bisect?

Comment 15

[Alice0775 White]

STR of comment #0 of Bug 601930
Regression window:
Works:
http://hg.mozilla.org/mozilla-central/rev/017d16925aa1
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0b7pre) Gecko/20100914 Firefox/4.0b7pre ID:20100915000003
Fails:
http://hg.mozilla.org/mozilla-central/rev/3077db085f3c
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0b7pre) Gecko/20100915 Firefox/4.0b7pre ID:20100915021250
Pushlog:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=017d16925aa1&tochange=3077db085f3c

Comment 16

[Alice0775 White]

In adittion to comment 15,
Build from 875f1912a091: Fails
Build from 90d644e0930d: works

Regressed by
875f1912a091	Dão Gottwald — Bug 593967 - Add more elements into tabbrowser tabs for easier stylability. r=gavin

Comment 17

[Dão Gottwald [:dao]]

Thanks. Unfortunately, I don't see yet what the problem with the patch from bug 593967 might be. (Also, I still haven't seen this happen with various profiles on Win XP / 7 / Linux.)

Comment 18

[Alice0775 White]

(In reply to comment #17)
> Thanks. Unfortunately, I don't see yet what the problem with the patch from bug
> 593967 might be. (Also, I still haven't seen this happen with various profiles
> on Win XP / 7 / Linux.)

Umm I can 100% repro:
Windows7 Classic , Aero Basic and Aero.
WindowsXP Classic and Luna.
Ubuntu8.04 Human.

Comment 19

[Alice0775 White]

After restoring window, 
As a matter of course, execute the following code, then the position of the tab becomes normal.
document.getElementById("tabbrowser-tabs")._positionPinnedTabs();

Comment 20

[Dão Gottwald [:dao]]

Attachment: wild guess

Does this help?

Comment 21

[Alice0775 White]

(In reply to comment #20)
> Created attachment 487534 [details] [diff] [review]
> wild guess
> 
> Does this help?

Nothing changed.
"transitionend" event does not fire while restoring window.
However pinTab is called the number of times of the app tab while restoring window.

WORKAROUND:
      <method name="pinTab">
        <parameter name="aTab"/>
        <body><![CDATA[
          if (aTab.pinned)
            return;

          if (aTab.hidden)
            this.showTab(aTab);

          this.moveTabTo(aTab, this._numPinnedTabs);
          aTab.setAttribute("pinned", "true");
-         this.tabContainer._positionPinnedTabs();
+         //xxx width of aTab is not changed immediately
+         setTimeout(function(self){self.tabContainer._positionPinnedTabs();},0, this);
          this.tabContainer.adjustTabstrip();

Comment 22

[Johnathan Nightingale [:johnath]]

I know it's hard going trying to track this down, but we need to understand and fix it.

Dao, is there anything else Alice can give you to help chase this? Any chance there's localstore strangeness in her profile, any chance your profile has stuff in it that is preventing you from reproducing the problem?

Comment 23

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

fwiw, 100% reproducible here for me too on OSX & Win7, new & existing profiles. I don't see it during a normal session restore, but it always see it after restoring a recently closed window.

Dao - let me know if you need me to try anything if you still can't reproduce.

Comment 24

[Dão Gottwald [:dao]]

I see it when restoring a recently closed window indeed.

Comment 25

[Dão Gottwald [:dao]]

Attachment: hack to prevent the bug

For some reason the favicon doesn't take up any space initially, although width:16px is applied. This patch works around the issue but shouldn't be needed. Seems to be a layout bug so far.

I haven't tested the patch on OS X, the min-width might be off there.

Comment 26

[Dão Gottwald [:dao]]

In _positionPinnedTabs, the computed CSS visibility of the favicon is "collapse" when the bug occurs, but nothing seems to be setting that...
As expected it's "visible" when the bug doesn't occur.

Comment 27

[Dão Gottwald [:dao]]

It's not clear to me what was going on here, but bug 582950 appears to have fixed it.

Comment 28

[Dão Gottwald [:dao]]

(In reply to comment #27)
> It's not clear to me what was going on here, but bug 582950 appears to have
> fixed it.

It hid the issue via bug 614290, which is fixed now, exposing this bug again.

Comment 30

[Dão Gottwald [:dao]]

I figure this needs to be a hard blocker.

Comment 31

[Dão Gottwald [:dao]]

Attachment: patch

This "fixes" the issue by not positioning app tabs when the tab bar doesn't overflow. This is really a workaround as far as this bug is concerned, but one we would want to apply anyway since it avoids doing unnecessary work.

Depends on the current patch in bug 617506.

Comment 32

[Justin Dolske [:Dolske]]

Comment on attachment 504166 [details] [diff] [review]
patch

Stealing review. *yoink*

Looks fine, but one minor style this you may or may not want to make... _positionPinnedTabs is a bit hard to read through because there are so many things conditional on |doPosition|. I tried quickly refactoring so just split everything into a single |if (doPosition) { ... } else { ... }|. Seems a little easier to follow, at the cost of duplicating the (trivial) for-loop.

See http://pastebin.mozilla.org/949280

r+ either way.

Comment 33

[Dão Gottwald [:dao]]

http://hg.mozilla.org/mozilla-central/rev/5e3c0dc7434b

Comment 35

[rob64rock]

Is anyone else experienced this using Win32 Nightly build 20110117 Firefox/4.0b10pre?

Screenshots: http://i54.tinypic.com/2n1ryao.jpg
http://i54.tinypic.com/ajqno9.jpg

App Tab (Pin Tab) more than one tab then Unpin one of them and the other App Tabs become non-visible and part of the Unpined Tab is cut off, this happens using the default theme or with a Personas theme applied. Didn't happen on Win32 Nightly build 20110116 Firefox/4.010pre.

Maybe a regression since this bug landed: https://bugzilla.mozilla.org/show_bug.cgi?id=601228

Please confirm if you can...
Thanks... :D