Closed Bug 585785 Opened 12 years ago Closed 11 years ago

Tab closing animation fails if the opening animation hasn't moved yet

Categories

(Firefox :: Tabbed Browser, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 4.0b7
Tracking Status
blocking2.0 --- betaN+

People

(Reporter: andreasjunghw, Assigned: dao)

References

Details

Attachments

(3 files, 2 obsolete files)

User-Agent:       Mozilla/5.0 (Windows NT 5.1; rv:2.0b4pre) Gecko/20100809 Minefield/4.0b4pre
Build Identifier: Mozilla/5.0 (Windows NT 5.1; rv:2.0b4pre) Gecko/20100809 Minefield/4.0b4pre

It seems the fadein attribute is not set on some tabs.
These Tabs are displayed with a width of 1px, because of:
browser.css, line 35:
.tabbrowser-tab:not([pinned]):not([fadein]) {
  max-width: 1px;
  min-width: 1px;
}
It is possible to move these tabs, but not possible to detach them


Reproducible: Sometimes

Steps to Reproduce:
1. Open tabs (in different ways)
   (Tabs created by Flash seem to cause this bug more reliably...)

Actual Results:  
Tabs with 1px width are created


Expected Results:  
Tabs should have the usual width
Attached image Screenshot
> It is possible to move these tabs, but not possible to detach them
It's also not possible to close them.

Adding the fadein attribute with DOM Inspector makes the tabs appear, but the problems mentioned above persist.
Blocks: 380960
Keywords: qawanted
look like this problem occurred because we don't pass the tab item to setTimeout

            if (aSkipAnimation ||
                this.tabContainer.getAttribute("overflow") == "true" ||
                !Services.prefs.getBoolPref("browser.tabs.animate")) {
              t.setAttribute("fadein", "true");
-             setTimeout(function (tabContainer) {
-               tabContainer._handleNewTab(t);
-             }, 0, this.tabContainer);
+             setTimeout(function (tabContainer, tab) {
+               tabContainer._handleNewTab(tab);
+             }, 0, this.tabContainer, t);
            } else {
-             setTimeout(function (tabContainer) {
-               if (t.pinned)
-                 tabContainer._handleNewTab(t);
-               else
-                 t.setAttribute("fadein", "true");
-             }, 0, this.tabContainer);
+             setTimeout(function (tabContainer, tab) {
+               if (tab.pinned)
+                 tabContainer._handleNewTab(tab);
+               else
+                 tab.setAttribute("fadein", "true");
+             }, 0, this.tabContainer, t);
            }

I can't post a patch
Status: UNCONFIRMED → NEW
Ever confirmed: true
(In reply to comment #3)
> look like this problem occurred because we don't pass the tab item to
> setTimeout

Any idea why this would make a difference? Doesn't seem to make sense to me...
There seem to be two ways to end up with these broken tabs:
1) When a tab is opened:
   The tab is created, but the fadein attribute is never set. (described above)
2) When a tab is closed:
   The fadein attribute is removed, but the tab is never destroyed / 
   completely closed. (see also bug 380960 comment 100)

It seems the latter case can be provoked by closing several tabs in fast succession.
(In reply to comment #4)
> (In reply to comment #3)
> > look like this problem occurred because we don't pass the tab item to
> > setTimeout
> 
> Any idea why this would make a difference? Doesn't seem to make sense to me...

when many tabs open fast, the after the timeout t in addtab can be another tab then the one that call the timeout
(In reply to comment #6)
> (In reply to comment #4)
> > (In reply to comment #3)
> > > look like this problem occurred because we don't pass the tab item to
> > > setTimeout
> > 
> > Any idea why this would make a difference? Doesn't seem to make sense to me...
> 
> when many tabs open fast, the after the timeout t in addtab can be another tab
> then the one that call the timeout

t is declared above that code within addTab. It's static within each addTab call and shouldn't change in closures. At least, this is how things /should/ work, as far as I know.
Attached patch workaround (obsolete) — Splinter Review
Avoiding the closure is a good thing anyway, so I guess we should just try this. I can't tell right now if this will help, as I couldn't reproduce this bug.
Assignee: nobody → dao
Status: NEW → ASSIGNED
Attachment #464732 - Flags: review?(gavin.sharp)
(In reply to comment #3)
> look like this problem occurred because we don't pass the tab item to
> setTimeout

(In reply to comment #6)
> when many tabs open fast, the after the timeout t in addtab can be another tab
> then the one that call the timeout

Are these guesses, or actual observations from debugging? Can you reliably reproduce this issue? Does Dao's patch fix it?

The current code should be working fine from what I can see, so I don't like the idea of taking shots in the dark without trying to understand this better. If the closure is failing to produce the correct tab, then that would be a core bug that we need to fix.
This might actually just be bug 583757.
Yes, this should be working, but there's no reason to use a closure either.
Was this fixed by bug 583757? Bug 588981 seems to indicate otherwise...
Duplicate of this bug: 588981
I just had this happen again.  About:config is once again 1 px.
Duplicate of this bug: 591650
Duplicate of this bug: 593786
Duplicate of this bug: 596540
Duplicate of this bug: 593713
I can reliably reproduce this bug by quickly pressing CTRL+T (to open a new tab) and then CTRL+W (to close that new tab). For me this really happens all the time, I just need to to this so fast that the new tab is barely showing up at all.
Duplicate of this bug: 598034
(In reply to comment #19)

This is to reproduce a problem with tabs shrinking to 1px after closing them, as described in bug 597366. I am not sure if that is discussed in this bug at all, but the existing duplicate bugs (like bug 591650) made me think that. So is it, or is it not?
Yes it is
bug 597366 is a duplicate
Another part of weird problem.
Plus button (to open new tab) is impossible to click on if there are some ghost tabs
(In reply to comment #23)
> Another part of weird problem.
> Plus button (to open new tab) is impossible to click on if there are some ghost
> tabs

Confirmed... happens everytime with the Ghost tabs around.
I can reproduce it by fast pressing CTRL+T and CTRL+W too.
Normally have disabled the new tab button in tab bar, so I re-added it with TMP to reproduce it's behaviour, but after re-adding the new tab button all ghost tabs got their normal size but without any text and still uncloseable, but i also can't reproduce the ghost tabs any more !?

By the way bug 533924 comment 2 and 3 are about the same problem.
Hope this gets fixed soon.
Duplicate of this bug: 603783
Assignee: dao → general
Component: Tabbed Browser → JavaScript Engine
Product: Firefox → Core
QA Contact: tabbed.browser → general
Version: unspecified → Trunk
Attachment #464732 - Attachment description: patch?! → workaround
blocking2.0: --- → ?
Status: ASSIGNED → NEW
OS: Windows XP → All
Hardware: x86 → All
Why is this a JS bug?
(In reply to comment #27)
> Why is this a JS bug?

There's nothing wrong in the frontend code as far as I can see. Avoiding the closures mentioned in comment 3 supposedly makes the bug go away.
In the failure mode, is t undefined, then?  I agree that shouldn't happen, if so...
blocking2.0: ? → betaN+
(In reply to comment #29)
> In the failure mode, is t undefined, then?

I guess so. Unfortunately I don't know how to reproduce this bug.
1. Can anyone reproduce this now?

2. If yes to #1, could you bisect this using Tracemonkey nightlies?

3. Also if yes to #1, please repost STR. I tried pressing Ctrl+T,Ctrl+W quickly, but nothing special happened. I'm not sure if I was supposed to get the browser in a certain state first, and I'm not sure exactly what's supposed to happen.
Duplicate of this bug: 597366
@David Mandelin - for my fast pressing Ctrl+T and Ctrl+W creates "ghost tabs"
only addon i have is :
adblock plus 1.3a (always up-to-date)
addon compatibility reporter (always up-to-date)
(In reply to comment #31)
> 1. Can anyone reproduce this now?

I am using Windows Vista and (on the same PC) Win 7 as a VM, and I can still reproduce it on both using latest-trunk.

> 2. If yes to #1, could you bisect this using Tracemonkey nightlies?

I just installed latest-tracemonkey on my Win 7 VM and I can still reproduce it.

> 3. Also if yes to #1, please repost STR. I tried pressing Ctrl+T,Ctrl+W
> quickly, but nothing special happened. I'm not sure if I was supposed to get
> the browser in a certain state first, and I'm not sure exactly what's supposed
> to happen.

STR: still by pressing Ctrl+T -> Ctrl+W (do not even release Ctrl and press T W nearly at the same time, a few times)
clean profile, first startup, works in every situation for me

What happens, for example: It looks as if there is only one tab open (close other visible tabs first), but when you exit Firefox, the "Save and Quit" dialog comes up. Starting Firefox again then restores more than one tab. The "ghost tab" itself might be hard to notice.

Is there a build containing the workaround?
Yes, this bug is still reproducible.

STR didn't change, to open or close tabs as fast as possible is the probably easiest way.
Sometimes this bug is reproducible by clicking on flash ads, trying to close a tab that didn't finish opening (i.e. is still expanding), or adding/closing tabs while the browser is busy (i.e. high CPU usage), or it just happens randomly, but it is luck to reproduce it this way.

The problem with these STR is that it seems that it is not always possible to reproduce the bug with them. If that's the case continue browsing normally and try it again later.

Since this bug was reported by me 2 months ago, the screenshot isn't completely valid anymore, because these ghost tabs don't appear in the "List all tabs" menu anymore.
They only appear on the tab bar as vertical dark lines and sometimes, though very rarely, in TabCandy.

The ghost tab can be moved around, but cannot be closed and cannot be accessed (Except it's the rare case that one of them appears in TabCandy: In this case you can click on it in TabCandy to access it).

If there are ghost tabs and one regular tab open, Firefox will complain on exit, that more than one tab is open and when you choose "Save and Quit", the regular tab and the ghost tabs will be restored on next start as regular tabs.
OK, I think I get it now.

STR: (On Win7) Press Ctrl+T,Ctrl+W rapidly and repeatedly.

Expected result: If pressing fast enough, pretty much nothing should happen.

Actual result: If you look closely at the "+" tab at the right, you will see it moving to the right about a pixel on each Ctrl+T and then back left on Ctrl+W. If you repeat the sequence for a while, when you are done you will notice that the space between the last tab and the "+" is slightly bigger than the space between the other adjacent pairs of tabs. The extra space is the "ghost tab". It doesn't show up in the tabs list, but it can be moved within the tab strip like a regular tab.

----

Now, if someone wouldn't mind pinpointing the first TM nightly build on which this occurs, that would be really helpful.
(In reply to comment #36)
> Now, if someone wouldn't mind pinpointing the first TM nightly build on which
> this occurs, that would be really helpful.

I downloaded firefox-4.0b*pre.en-US.win32.zip

2010-08-06-04-mozilla-central / 2010-08-07-04-tracemonkey
- could not reproduce

2010-08-07-04-mozilla-central / 2010-08-08-04-tracemonkey
- easy to reproduce (those have closing tab animation)
(In reply to comment #37)
> (In reply to comment #36)
> > Now, if someone wouldn't mind pinpointing the first TM nightly build on which
> > this occurs, that would be really helpful.
> 
> I downloaded firefox-4.0b*pre.en-US.win32.zip
> 
> 2010-08-06-04-mozilla-central / 2010-08-07-04-tracemonkey
> - could not reproduce
> 
> 2010-08-07-04-mozilla-central / 2010-08-08-04-tracemonkey
> - easy to reproduce (those have closing tab animation)

Thanks! It's clear that the regressing change hit m-c before TM, so it is probably not JS (although conceivable some new code exposed an old bug), and closing tab animation looks like the most closely related change to me, too.
Assignee: general → nobody
Component: JavaScript Engine → Tabbed Browser
Product: Core → Firefox
QA Contact: general → tabbed.browser
(In reply to comment #36)
> OK, I think I get it now.
> 
> STR: (On Win7) Press Ctrl+T,Ctrl+W rapidly and repeatedly.

I can reproduce it with these steps. The problem here is that we're not getting the transitionend event for the shrinking tab when the transition started before the opening transition finished. Not sure if that's expected. I could work around it.

I'm not sure this covers all aspects of this bug either; it doesn't seem like people always followed the above steps when they got the 1px wide tabs.
Attachment #464732 - Flags: review?(gavin.sharp)
Attached patch another fix or workaround (obsolete) — Splinter Review
Per comment 39, this is solely for the steps in comment 36.
(In reply to comment #39)
> (In reply to comment #36)
> > OK, I think I get it now.
> > 
> > STR: (On Win7) Press Ctrl+T,Ctrl+W rapidly and repeatedly.
> 
> I can reproduce it with these steps. The problem here is that we're not getting
> the transitionend event for the shrinking tab when the transition started
> before the opening transition finished. Not sure if that's expected.

dbaron, can you shed some light on this?
(In reply to comment #39)
> I'm not sure this covers all aspects of this bug either; it doesn't seem like
> people always followed the above steps when they got the 1px wide tabs.

Correct: it seems to be able to happen whenever the closing tab animation occurs, though the chance is low.

The only thing about the ctrl-T,ctrl-W method is a way to speed up triggering the closing tab animation.

It's true that the animation might ALSO be more likely to trigger the bug when executed rapidly like that. However, that's certainly not the only circumstances under which it can trigger the bug.
Comment on attachment 464732 [details] [diff] [review]
workaround

This might have been relevant when bug 583757 existed, but I think it was mostly just confusing things, and the main issue is the one fixed by the new workaround (and caused by bug 380960).
Attachment #464732 - Attachment is obsolete: true
Attachment #484177 - Flags: review?(gavin.sharp)
Attachment #484177 - Flags: feedback?(dbaron)
(In reply to comment #41)
> (In reply to comment #39)
> > (In reply to comment #36)
> > > OK, I think I get it now.
> > > 
> > > STR: (On Win7) Press Ctrl+T,Ctrl+W rapidly and repeatedly.
> > 
> > I can reproduce it with these steps. The problem here is that we're not getting
> > the transitionend event for the shrinking tab when the transition started
> > before the opening transition finished. Not sure if that's expected.
> 
> dbaron, can you shed some light on this?

transitionend isn't fired when the transition is interrupted by another style change.  That's what the spec says to do.

We should probably add an additional event for transitionchanged (or cancelled, or something).  (I think WebKit has implemented more events than those in the spec.)
Attachment #484177 - Flags: feedback?(dbaron)
(In reply to comment #44)
> transitionend isn't fired when the transition is interrupted by another style
> change.  That's what the spec says to do.

It's not an arbitrary style change, though, but a new transition, for which we don't get the event either. Still expected?
You should get an end event for the new transition assuming there is a new transition.

But if the first transition hasn't even started moving yet and you set the style to exactly where it was, there won't be a second transition at all.
Comment on attachment 484177 [details] [diff] [review]
another fix or workaround

It surprises me that this can be triggered this easily, but like David said it seems that in the failure cases, the fade-in transition effectively hasn't started yet, so the max-width is still 1px and thus can't make a transition back down.
Attachment #484177 - Attachment is obsolete: true
Attachment #484177 - Flags: review?(gavin.sharp)
Attached patch patchSplinter Review
Attachment #485111 - Flags: review?(gavin.sharp)
Comment on attachment 485111 [details] [diff] [review]
patch

Add a "/* transition hasn't started yet */" comment?

I think a transitionchange/cancelled event would be a cleaner solution - followup bug(s)?

You can actually reproduce this programmatically using:
var tab = gBrowser.addTab();
executeSoon(function () {
  gBrowser.removeTab(tab, {animate:true});
});

which means this should be testable... Though I don't think there's a good way to know when to check whether the tab was actually removed (i.e. transitionend fired), which I suppose is kind of annoying.
Attachment #485111 - Flags: review?(gavin.sharp) → review+
(In reply to comment #49)
> I think a transitionchange/cancelled event would be a cleaner solution -
> followup bug(s)?

I'm not sure how that would help. The problem is not that we're cancelling the fade-in transition but that the fade-out transition has nothing to do...
We are cancelling it, by quickly undoing the change that would otherwise trigger it. The backend should be able to detect that and notify us.
(In reply to comment #51)
> We are cancelling it, by quickly undoing the change that would otherwise
> trigger it.

There's a difference between "cancelled before it could move" and "cancelled somewhere in the middle". Only the former is problematic, but an event exclusively for that case seems a bit strange to me.
Attached patch test addedSplinter Review
http://hg.mozilla.org/mozilla-central/rev/b6c9c4833613

I have seen a few 1px-wide tabs not using the STR from comment 36 a week or so ago but not since then. Optimistically marking fixed. I'll hold off landing bug 604735, as that would make it harder to observe the bug.
Assignee: nobody → dao
Status: NEW → RESOLVED
Closed: 11 years ago
Flags: in-testsuite+
Keywords: qawanted
Resolution: --- → FIXED
Summary: fadein=true not set on some tabs, causes tabs to be displayed with a width of 1px → Tab closing animation fails if the opening animation hasn't moved yet
Target Milestone: --- → Firefox 4.0b8
Depends on: 606567
Target Milestone: Firefox 4.0b8 → Firefox 4.0b7
(In reply to comment #54)
> http://hg.mozilla.org/mozilla-central/rev/b6c9c4833613
> 
> I have seen a few 1px-wide tabs not using the STR from comment 36 a week or so
> ago but not since then. Optimistically marking fixed. I'll hold off landing bug
> 604735, as that would make it harder to observe the bug.

It just happened again, see bug 608589.
You need to log in before you can comment on or make changes to this bug.