Closed Bug 380960 Opened 18 years ago Closed 14 years ago

Implement closing tabs animation

Categories

(Firefox :: Tabbed Browser, enhancement, P3)

Product:
Firefox
Component:
Tabbed Browser
Type:
enhancement

Tracking

()

Status:
VERIFIED FIXED
Milestone:
Firefox 4.0b4

People

(Reporter: dao, Assigned: dao)

References

(
URL
)

Details

Attachments

(2 files, 35 obsolete files)

Tab closing video
(deleted), video/x-msvideo
Details
comments addressed, test added [Checked in: Comment 104]
(deleted), patch
beltzner
: approval2.0+
Details | Diff | Splinter Review
Attached patch patch (obsolete) (deleted) — — Splinter Review
No description provided.
Attachment #265072 - Flags: ui-review?(beltzner)
Attachment #265072 - Flags: review?(enndeakin)
Priority: -- → P3
Whiteboard: PRD:TAB-004b
Flags: blocking-firefox3?
Comment on attachment 265072 [details] [diff] [review] patch Nothing too big: >Index: tabbrowser.xml >+ <body><![CDATA[ >+ if (aTab.localName != "tab") >+ aTab = this.mCurrentTab; I know you just moved this but (aTab.control == this.mTabContainer) is a more accurate way to check if a tab is part of the tabbrowser. Also, add a null check for aTab, and use this.mCurrentTab in that case as well. >+ >+ if (aTab._closing) >+ return; Don't like the use of a magic property here too much. >+ >+ var anim_id = setInterval(processFrame, 60); Looks kind of wierd to have anim_id declared below here when it is referenced above. Is there a need to support animated closing of two tabs at once? If not, you could just use a _closeTabTimerId field instead, and check that if an animation is in progress rather than using _close. Also, there might be a need to clear the timeout in the destructor or elsewhere. + <method name="_removeTab"> Give this a more distinct name to avoid confusion between removeTab.
Attachment #265072 - Flags: review?(enndeakin) → review-
Dao, I have a few comments about this: 1) If I close the currently selected tab, the tab beside it should be selected before the animation starts, not after the animation finishes. Copy the relevant code from _removeTab. The animation looks good but I don't want it getting in the way of my browsing. 2) >+ if (++i >= self._removeTabOpacities.length) { Change the >= to > or else the last frame won't happen at all. 3) There MUST MUST MUST be a pref for this. Don't forget we still support OS/2 (IIRC) and opacity on that platform absolutely murders performance (unless its been fixed? I dunno... there should still be a pref for those who don't like animation personally even on modern machines) Maybe there should be a global pref for all animation? This bug, our smooth scrolling bug, any other animations we may implement...
Attached patch patch v2 (obsolete) (deleted) — — Splinter Review
Attachment #265072 - Attachment is obsolete: true
Attachment #265115 - Flags: ui-review?(beltzner)
Attachment #265115 - Flags: review?(enndeakin)
Attachment #265072 - Flags: ui-review?(beltzner)
Attached patch patch v2.1 (obsolete) (deleted) — — Splinter Review
panel selection was flawed
Attachment #265115 - Attachment is obsolete: true
Attachment #265117 - Flags: ui-review?(beltzner)
Attachment #265117 - Flags: review?(enndeakin)
Attachment #265115 - Flags: ui-review?(beltzner)
Attachment #265115 - Flags: review?(enndeakin)
Comment on attachment 265117 [details] [diff] [review] patch v2.1 Index: modules/libpref/src/init/all.js // xxxbsmedberg more toolkit prefs? // Tab browser preferences. +pref("toolkit.tabbrowser.animations", true); Why is this toolkit and not browser.tabs like the others? Index: toolkit/content/widgets/tabbrowser.xml <method name="removeTab"> + + for each (var tab in this._removingTabs) + if (tab == aTab) + return; You could just do: if (aTab in this._removingTabs) if you used _removingTabs[aTab] = anim_id below. I don't understand the rest of the changes. Why have you moved the removeTab code into two separate destroyTab and releaseTab methods, removed some bits, and rearranged the order of the code within them?
Attachment #265117 - Flags: review?(enndeakin)
(In reply to comment #5) > (From update of attachment 265117 [details] [diff] [review]) > Index: modules/libpref/src/init/all.js > > // xxxbsmedberg more toolkit prefs? > // Tab browser preferences. > +pref("toolkit.tabbrowser.animations", true); > > Why is this toolkit and not browser.tabs like the others? bsmedberg's comment implies that those should be toolkit prefs, doesn't it? > Index: toolkit/content/widgets/tabbrowser.xml > > <method name="removeTab"> > + > + for each (var tab in this._removingTabs) > + if (tab == aTab) > + return; > > You could just do: > if (aTab in this._removingTabs) > if you used _removingTabs[aTab] = anim_id below. Object keys must be serial, so that would be equal to |_removingTabs["[object XULElement]"] = anim_id|. > I don't understand the rest of the changes. Why have you moved the removeTab > code into two separate destroyTab and releaseTab methods, removed some bits, > and rearranged the order of the code within them? See comment 2: "If I close the currently selected tab, the tab beside it should be selected before the animation starts, not after the animation finishes." That's reasonable, imho, but it required to break up the old method.
Attached patch patch v2.2 (obsolete) (deleted) — — Splinter Review
changed the pref name.
Attachment #265121 - Flags: review?(enndeakin)
Attached patch patch v3 (obsolete) (deleted) — — Splinter Review
here comes another behavioral fix: call _fillTrailingGap() when resizing a tab.
Attachment #265117 - Attachment is obsolete: true
Attachment #265121 - Attachment is obsolete: true
Attachment #265125 - Flags: ui-review?(beltzner)
Attachment #265125 - Flags: review?(enndeakin)
Attachment #265121 - Flags: review?(enndeakin)
Attachment #265117 - Flags: ui-review?(beltzner)
Comment on attachment 265125 [details] [diff] [review] patch v3 + <body><![CDATA[ + if (aTab == null || aTab.control != this.mTabContainer) + aTab = this.mCurrentTab; + Actually, you should probably check the tagname too, either that or check it's a nsIDOMXULSelectControlItemElement. + // Find and locate the tab in our list. + var l = this.mTabContainer.childNodes.length; + for (var i = 0; i < l; i++) + if (this.mTabContainer.childNodes[i] == aTab) + index = i; + } You could also just write: index = Array.indexOf(this.mTabContainer.childNodes, aTab); + + // invalidate cache, because mTabContainer is about to change + this._browsers = null; Is there a need to have this here and at the beginning of the method above? - newIndex = (index == l - 1) ? index - 1 : index; + newIndex = (index == l - 1) ? index - 1 : index + 1; + } else { + newIndex = currentIndex; } Don't understand this part. + this.mTabContainer.selectedIndex = newIndex; + this.selectedTab._selected = true; The last line should be removed, as it is already done when changing the selectedIndex. > See comment 2: "If I close the currently selected tab, the tab beside it should > be selected before the animation starts, not after the animation finishes." > That's reasonable, imho, but it required to break up the old method. That doesn't explain why you reaaranged the code in a different order. Why is all the code in destroyTab necessary to be there instead of in releaseTab? Some of the code seems like it should happen when the tab is changed not after the animation is complete. The call to updateCurrentBrowser for instance. In any case, you should also get a review from gavin as well.
Attachment #265125 - Flags: review?(enndeakin)
(In reply to comment #9) > + > + // invalidate cache, because mTabContainer is about to change > + this._browsers = null; > > Is there a need to have this here and at the beginning of the method above? It was two times in the original removeTab, but I too think one is sufficient. > - newIndex = (index == l - 1) ? index - 1 : index; > + newIndex = (index == l - 1) ? index - 1 : index + 1; > + } else { > + newIndex = currentIndex; > } > > Don't understand this part. The old code assumed that the tab is removed, but I'm not removing anything in that method. > + this.mTabContainer.selectedIndex = newIndex; > + this.selectedTab._selected = true; > > The last line should be removed, as it is already done when changing the > selectedIndex. I think there was a bug that required to set _selected explicitly. But maybe that's fixed now, or I just mistake things. > > See comment 2: "If I close the currently selected tab, the tab beside it should > > be selected before the animation starts, not after the animation finishes." > > That's reasonable, imho, but it required to break up the old method. > > That doesn't explain why you reaaranged the code in a different order. The reason for that is that I moved the parts one by one and dropped them where it made sense to me. I can double-check that and maybe restore the original order. > Why is all the code in destroyTab necessary to be there instead of in > releaseTab? Some of the code seems like it should happen when the tab is > changed not after the animation is complete. The call to updateCurrentBrowser > for instance. As far as I remember, updateCurrentBrowser must be called as the old browser is removed. Removing the browser but not the tab would probably cause problems. > In any case, you should also get a review from gavin as well. Ok, I'll attach a new patch and ask gavin for review.
Attached patch patch v4 (obsolete) (deleted) — — Splinter Review
Attachment #265125 - Attachment is obsolete: true
Attachment #265169 - Flags: ui-review?(beltzner)
Attachment #265169 - Flags: review?(gavin.sharp)
Attachment #265125 - Flags: ui-review?(beltzner)
Comment on attachment 265169 [details] [diff] [review] patch v4 There's a problem with |l == 1 && this.mPrefs.getBoolPref("browser.tabs.autoHide")|, i.e. |return| cancels _releaseTab but not the rest of removeTab. New patch coming.
Attachment #265169 - Attachment is obsolete: true
Attachment #265169 - Flags: ui-review?(beltzner)
Attachment #265169 - Flags: review?(gavin.sharp)
Attached patch patch v5 (obsolete) (deleted) — — Splinter Review
Attachment #265182 - Flags: ui-review?(beltzner)
Attachment #265182 - Flags: review?(gavin.sharp)
Dao, nice work, though I'm still concerned about this: > Maybe there should be a global pref for all animation? This bug, our smooth > scrolling bug, any other animations we may implement... This bug, bug 347363, and hopefully future eye-candy bugs should share a pref, something like "general.ui.animations" so its not limited to toolkit either. Why have many animation prefs when most people either want animation or they don't?
(In reply to comment #14) > This bug, bug 347363, and hopefully future eye-candy bugs should share a pref, > something like "general.ui.animations" so its not limited to toolkit either. > Why have many animation prefs when most people either want animation or they > don't? I'm not sure if general.ui would be the right scope, and I don't really want to discuss this here. Also, just in case gavin and beltzner accept v5, I wouldn't want to add a new version only for changing the pref name again. tabbrowser.xml#tabbrowser-tabs has another opacity animation, btw. Bug 366797 would introduce another one, and there's browser.preferences.animateFadeIn. So, just file a new bug to track this issue?
Not a blocker, but I'll swing back around for the ui-r soon ... /me keeps swapping between PM and UE hats
Flags: blocking-firefox3? → blocking-firefox3-
Whiteboard: PRD:TAB-004b → PRD:TAB-004b [wanted-firefox3]
Been playing around with this patch for a few weeks now, and it's been working really nicely for me.
Attachment #265182 - Flags: ui-review?(beltzner)
Attached file v5 as extension (obsolete) (deleted) —
Attachment #273109 - Flags: ui-review?(beltzner)
Attached file v5 as extension (obsolete) (deleted) —
em:id fixed
Attachment #273109 - Attachment is obsolete: true
Attachment #273124 - Flags: ui-review?(beltzner)
Attachment #273109 - Flags: ui-review?(beltzner)
Comment on attachment 273124 [details] v5 as extension Dao, this is really freakin' sweet. I've noticed (on OSX) a few performance issues, especially when the throbber on other tabs are loading. Still, it's a really nice effect, and significantly eases the experience of understanding how the tabstrip has morphed when you close a tab.
Attachment #273124 - Flags: ui-review?(beltzner) → ui-review+
There is no animation for re-arranging tabs, though I am not sure that a priority in the scheme of things...
Attached patch patch v5, synced with trunk (obsolete) (deleted) — — Splinter Review
Attachment #265182 - Attachment is obsolete: true
Attachment #276822 - Flags: review?(gavin.sharp)
Attachment #265182 - Flags: review?(gavin.sharp)
Attached patch patch v5.1, <field> cleanup (obsolete) (deleted) — — Splinter Review
Attachment #276822 - Attachment is obsolete: true
Attachment #277064 - Flags: review?(gavin.sharp)
Attachment #276822 - Flags: review?(gavin.sharp)
I see a ui-review+, so when is this landing on trunk? I used the extension attached in here and its super cool. Would love to see something similar for Tab-dragging and maybe Tab opening (for consistency sake, or use whatever argument you like, but please do it :-) Firefox can sure do with some prettiness!
No, it still needs a code review from Gavin as you can see on the most recent patch.
The code freeze is less than a week away. Why aren't we landing this even though its complete?
Attached patch updated to trunk (obsolete) (deleted) — — Splinter Review
Attachment #277064 - Attachment is obsolete: true
Attachment #278750 - Flags: review?(gavin.sharp)
Attachment #277064 - Flags: review?(gavin.sharp)
Looks great when closing the last tab but when closing any tabs in between other tabs especially the second tab if there are a lot open, seems to lag.
Attached patch patch v6 (obsolete) (deleted) — — Splinter Review
The previous patch caused problems when adjacent tabs were closing concurrently.
Attachment #273124 - Attachment is obsolete: true
Attachment #278750 - Attachment is obsolete: true
Attachment #278750 - Flags: review?(gavin.sharp)
Attachment #279451 - Flags: review?(gavin.sharp)
Attached patch patch v6.1 (obsolete) (deleted) — — Splinter Review
I accidentally reverted a change from 5.1 ...
Attachment #279451 - Attachment is obsolete: true
Attachment #279452 - Flags: review?(gavin.sharp)
Attachment #279451 - Flags: review?(gavin.sharp)
Attached patch 6.2 (obsolete) (deleted) — — Splinter Review
Better yet. (I hope that's the last one for now.) Btw, Gavin, please let me know if you won't have time for this.
Attachment #279452 - Attachment is obsolete: true
Attachment #279454 - Flags: review?(gavin.sharp)
Attachment #279452 - Flags: review?(gavin.sharp)
Comment on attachment 279454 [details] [diff] [review] 6.2 Actually, I think the option to skip the animation (this._removeTabsDontAnimate currently) should be part of the API and the TabClose event should be dispatched in _destroyTab, both of which became clear as I worked on an extension that adds another user interface for closing tabs and also listens to the TabSelect and TabClose events. I'll post a new patch later today.
Attachment #279454 - Attachment is obsolete: true
Attachment #279454 - Flags: review?(gavin.sharp)
(In reply to comment #31) > Btw, Gavin, please let me know if you won't have time for this. I have a few other patches that are higher in my prioritized list of pending review requests at the moment. You might want to ask Neil Deakin or Mano whether they have cycles to review this? I'll try to get to it soon if not.
Oh, I see Neil already reviewed it. Does the patch he reviewed differ significantly from the latest patch? If so, perhaps Mano can just take a quick look.
It didn't change dramatically. There are the mentioned two or three issues that I discovered today, but they are basically fixed by moving code.
Attached patch patch v7 (obsolete) (deleted) — — Splinter Review
Attachment #279515 - Flags: review?(mano)
How can I include the new code (v7) in the extension (v5). Is there a major change?
I can attach an updated extension. Please do not use v5. It accidentally contained an old version of my Ctrl-Tab extension.
Attached file v7 as extension (obsolete) (deleted) —
This should do.
Thanks for the extension update Dao. Hope this lands on trunk soon. Its been quite sometime.
Attachment #279515 - Flags: review?(mano) → review?(mconnor)
Need to bump up the version number for compatibility with 1.9b2pre. Also can we add this to http://wiki.mozilla.org/Firefox3/Beta2CheckinQueue under the Non-Blockers section, or do we need to set the Target Milestone first?
(In reply to comment #41) > Need to bump up the version number for compatibility with 1.9b2pre. just add the boolean pref extensions.checkCompatibility and set it to false. > Also can we add this to http://wiki.mozilla.org/Firefox3/Beta2CheckinQueue It can't be added to the check-in queue without review.
(In reply to comment #42) > It can't be added to the check-in queue without review. ... and approval. :)
Attached patch patch v8 (obsolete) (deleted) — — Splinter Review
Attachment #279515 - Attachment is obsolete: true
Attachment #281804 - Attachment is obsolete: true
Attachment #289816 - Flags: review?(mconnor)
Attachment #279515 - Flags: review?(mconnor)
Whiteboard: PRD:TAB-004b [wanted-firefox3] → PRD:TAB-004b [wanted-firefox3][has patch][has ui-review][needs review]
Attached patch patch v8 (obsolete) (deleted) — — Splinter Review
updated to trunk
Attachment #289816 - Attachment is obsolete: true
Attachment #294948 - Flags: review?(mconnor)
Attachment #289816 - Flags: review?(mconnor)
Flags: wanted-firefox3+
Whiteboard: PRD:TAB-004b [wanted-firefox3][has patch][has ui-review][needs review] → PRD:TAB-004b [has patch][has ui-review][needs review]
Attachment #294948 - Flags: review?(mconnor) → review?(mano)
This patch makes pressing and holding the "close tab" keyboard shortcut in an empty window with tabbar very entertaining. ;) Maybe we shouldn't animate closing the last tab when "Always show Tabbar" is on.
Dão, can you look at comment 46?
Attached patch patch v9 (obsolete) (deleted) — — Splinter Review
addressed comment 46 by disabling the animation for the last tab I also renamed the pref, because a generic name isn't useful anymore at this point.
Attachment #294948 - Attachment is obsolete: true
Attachment #303683 - Flags: review?(mano)
Attachment #294948 - Flags: review?(mano)
I'm a little scared of making removeTab async at this point, any chance you could add an argument to explicitly animate (and set it whenever we call removeTab)?
Yes, that's a simple move. Definitely better for extensions at this point.
Attached patch patch v10 (obsolete) (deleted) — — Splinter Review
I didn't touch all places where we call removeTab since it will default to skipping the animation.
Attachment #303683 - Attachment is obsolete: true
Attachment #306858 - Flags: review?(mano)
Attachment #303683 - Flags: review?(mano)
Comment on attachment 306858 [details] [diff] [review] patch v10 r=mano
Attachment #306858 - Flags: review?(mano) → review+
Attachment #306858 - Flags: approval1.9?
Comment on attachment 306858 [details] [diff] [review] patch v10 a1.9=beltzner
Attachment #306858 - Flags: approval1.9? → approval1.9+
Keywords: checkin-needed
Checking in browser/app/profile/firefox.js; /cvsroot/mozilla/browser/app/profile/firefox.js,v <-- firefox.js new revision: 1.314; previous revision: 1.313 done Checking in browser/base/content/tabbrowser.xml; /cvsroot/mozilla/browser/base/content/tabbrowser.xml,v <-- tabbrowser.xml new revision: 1.268; previous revision: 1.267 done
Status: ASSIGNED → RESOLVED
Closed: 17 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Whiteboard: PRD:TAB-004b [has patch][has ui-review][needs review] → PRD:TAB-004b
Target Milestone: --- → Firefox 3 beta5
Backed out. [Exception... "'[JavaScript Error: "this._fillTrailingGap is not a function" {file: "chrome://browser/content/tabbrowser.xml" line: 2706}]' when calling method: [nsIDOMEventListener::handleEvent]" nsresult: "0x80570021 (NS_ERROR_XPC_JAVASCRIPT_ERROR_WITH_DETAILS)" location: "<unknown>" data: yes]
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Attached patch patch + fix (obsolete) (deleted) — — Splinter Review
superb...
Attachment #306858 - Attachment is obsolete: true
Attachment #310735 - Flags: approval1.9b5?
Attachment #310735 - Flags: approval1.9?
Chrome tests?
Comment on attachment 310735 [details] [diff] [review] patch + fix Per previous comment, let's put some tests in here before landing ...
Attachment #310735 - Flags: approval1.9b5?
Attachment #310735 - Flags: approval1.9b5-
Attachment #310735 - Flags: approval1.9?
Attachment #310735 - Flags: approval1.9-
Flags: in-testsuite?
I'm not really a fan of this feature. First, the animation is slow (and therefore distracting) when Firefox is under heavy load. Second, the feature breaks "click twice quickly to close two tabs", since the close button of the second tab doesn't immediately occupy the space where the first close button used to be.
(In reply to comment #57) I don't see what could be tested here. (In reply to comment #59) You can't actually close two tabs with a double click. You already have to wait with the second click or click three times.
Target Milestone: Firefox 3 beta5 → ---
Dao, are you still working on this or is it too late? As for tests, Litmus tests should do as I don't think any automated test suite can check this. Just click the tab close button and verify that the closing animation happens.
I was planning to reanimate this bug for 1.9.1 or so. This is kind of a new feature and probably impossible to get approved now. Comment 59 is correct in that there's a performance problem, which I would like to address as done in bug 430925. That's quite easy and I could do it right now, but we'd still lack the approval.
(In reply to comment #61) > As for tests, Litmus tests should do as I don't think any automated test suite > can check this. Just click the tab close button and verify that the closing > animation happens. We can at least have an automated test to make sure that the close button still works and closes the tab IMO.
Whiteboard: PRD:TAB-004b → [not needed for 1.9]PRD:TAB-004b
Attached patch patch v11 (obsolete) (deleted) — — Splinter Review
Mano: This is quite similar to what you've already reviewed. I'm using the secret 'lateness' argument in the interval callback to make sure that the animation doesn't take too long, and I've added a very basic test for removeTab().
Attachment #310735 - Attachment is obsolete: true
Attachment #320929 - Flags: review?(mano)
Attached patch rewritten for trunk (obsolete) (deleted) — — Splinter Review
Attachment #320929 - Attachment is obsolete: true
Attachment #344209 - Flags: review?(gavin.sharp)
Attachment #320929 - Flags: review?(mano)
Attachment #344209 - Attachment description: updated to trunk → rewritten for trunk
Attached patch rewritten patch v2 (obsolete) (deleted) — — Splinter Review
Instead of hardcoding the animation steps, I'm now specifying the expected duration and calculating the steps on the fly. This seems to be smoother. Otherwise this is the same patch as before.
Attachment #344209 - Attachment is obsolete: true
Attachment #344270 - Flags: review?(gavin.sharp)
Attachment #344209 - Flags: review?(gavin.sharp)
Whiteboard: [not needed for 1.9]PRD:TAB-004b
Target Milestone: --- → Firefox 3.1b2
Blocks: 347930
Attached patch updated to trunk (obsolete) (deleted) — — Splinter Review
Attachment #344270 - Attachment is obsolete: true
Attachment #344617 - Flags: review?(gavin.sharp)
Attachment #344270 - Flags: review?(gavin.sharp)
Attached patch v3 (obsolete) (deleted) — — Splinter Review
The previous patch didn't successfully skip the animation for the last tab. Fixed that.
Attachment #344617 - Attachment is obsolete: true
Attachment #344741 - Flags: review?(gavin.sharp)
Attachment #344617 - Flags: review?(gavin.sharp)
Blocks: 457096
Blocks: 462673
Attached patch updated to trunk (obsolete) (deleted) — — Splinter Review
Attachment #344741 - Attachment is obsolete: true
Attachment #345932 - Flags: review?(gavin.sharp)
Attachment #344741 - Flags: review?(gavin.sharp)
No longer blocks: 462673
Depends on: 462673
No longer blocks: 457096
Attached patch updated to trunk (obsolete) (deleted) — — Splinter Review
Attachment #345932 - Attachment is obsolete: true
Attachment #345932 - Flags: review?(gavin.sharp)
No longer blocks: 347930
Attached patch updated to trunk (obsolete) (deleted) — — Splinter Review
Attachment #369065 - Attachment is obsolete: true
Has this bug been abandoned? This patch was fixed and finalized almost 2 years ago. Given the kind of UI improvements we are looking at for 3.6/3.7 and 4.0, can we please now land this on trunk, so it can be tested with a larger audience?
Depends on: 543206
Animation for opening tab landed... So can we also get it for closing it ? It will be nice to have a such future like Chromium and Opera. :)
Yeah - 3 yrs in the waiting... this deserves to land now :)
Since the patch has not been touched since mid-late 2009, it probably needs yet another "update to trunk". Still, perhaps most of the code from the opening animation patch can be used?
Attached patch patch (obsolete) (deleted) — — Splinter Review
Attachment #389666 - Attachment is obsolete: true
Attachment #451237 - Flags: review?(gavin.sharp)
No longer depends on: 572114
it has to land for beta 1, shouldn't it?
(In reply to comment #77) > it has to land for beta 1, shouldn't it? No, this isn't a beta 1 blocker.
If we have opening animation, why not closing animation as well? It's weird to me.
(In reply to comment #79) > If we have opening animation, why not closing animation as well? It's weird to > me. We are not saying we don't want it. We are saying we don't need it for beta 1.
What I ment was it will be weird to have in beta opening animation, but not closing. This is only suggestion from purely logical angle of view. :)
(In reply to comment #81) I think that beta 1 is more targeted towards developers, since the UI is only about half-done, so only more advanced users like us will need to put up with it. It'll be in beta 2 when we can expect a more feature-complete UI, so it will be targeted more towards end-users.
I think we can all stop talking about this in the bug - it's not moving it forward. Thanks for your interest, let's keep it on track and only see comments about the ongoing implementation.
Attached patch patch (obsolete) (deleted) — — Splinter Review
updated to tip
Attachment #451237 - Attachment is obsolete: true
Attachment #453852 - Flags: review?(gavin.sharp)
Attachment #451237 - Flags: review?(gavin.sharp)
Attachment #453852 - Flags: review?(mano)
Attached patch patch (obsolete) (deleted) — — Splinter Review
updated to tip
Attachment #453852 - Attachment is obsolete: true
Attachment #456048 - Flags: review?(mano)
Attachment #456048 - Flags: review?(gavin.sharp)
Attachment #453852 - Flags: review?(mano)
Attachment #453852 - Flags: review?(gavin.sharp)
Blocks: 578373
Attached patch patch (obsolete) (deleted) — — Splinter Review
updated to tip
Attachment #456048 - Attachment is obsolete: true
Attachment #461204 - Flags: review?(mano)
Attachment #461204 - Flags: review?(gavin.sharp)
Attachment #461204 - Flags: review?(dolske)
Attachment #456048 - Flags: review?(mano)
Attachment #456048 - Flags: review?(gavin.sharp)
you call _endRemoveTab from transitionend so we get a delay between TabClose event and the actual tab removal and actual "UI responsiveness"
That delay isn't visible to the user, as the closing tab loses focus when the transition starts.
Comment on attachment 461204 [details] [diff] [review] patch Dolske asked me to do a first-pass on this. It looks good functionality-wise. Code-wise: In beginRemoveTab, I'm not sure why you need to attach properties to the tab, instead of returning an array and passing the arguments to endRemoveTab. Nit: beginRemoveTab now returns false, null, and true, when it seems that false and null mean the same thing for the context in which you are using them.
Attachment #461204 - Flags: review+
Attached video Tab closing video (deleted) —
Tested patch with my Win7 build, seems to work fine. Though it seems there is a somewhat jarring glitch when closing the last of many tabs (ie, last in overflow). I'm not sure exactly what's happening, but I'd guess the scrolling of tabs-to-the-left is interacting poorly with the shrinking of the closed tab? I'll try to attach a video I took (60fps with my cheap camera)... VLC is handy for single-frame stepping. Closing the right-most tab, when there are additional tabs overflowed offscreen, seems to be ok. Not sure if bug 465086 will help here, since I don't think the tabs are resizing (I have dozens overflowed to the left). Perhaps the tab-close animation should be suppressed when closing the last tab of overflow?
(In reply to comment #90) > In beginRemoveTab, I'm not sure why you need to attach properties to the tab, > instead of returning an array and passing the arguments to endRemoveTab. Because the transitionend handler can call _endRemoveTab. > Nit: beginRemoveTab now returns false, null, and true, when it seems that false > and null mean the same thing for the context in which you are using them. Yeah, I missed a case where null should be converted to false.
Attached patch patch (obsolete) (deleted) — — Splinter Review
this fixes the inconsistent _endRemoveTab return value and immediately removes animating tabs when getting the underflow event.
Attachment #461204 - Attachment is obsolete: true
Attachment #461869 - Flags: review?(dolske)
Attachment #461204 - Flags: review?(mano)
Attachment #461204 - Flags: review?(gavin.sharp)
Attachment #461204 - Flags: review?(dolske)
Attachment #461204 - Flags: review+ → feedback+
Comment on attachment 461869 [details] [diff] [review] patch >diff --git a/browser/base/content/tabbrowser.xml b/browser/base/content/tabbrowser.xml > <method name="removeTab"> >+ if (!animate || >+ isLastTab || >+ aTab.pinned || >+ this._removingTabs.length > 3 || Is this just an arbitrary "stop animating once we're closing many tabs"? Worth a clarifying comment. > <method name="_endRemoveTab"> >+ if (this._windowIsClosing) { >+ aCloseWindow = false; >+ aNewTab = false; >+ } Why is this needed? Looks unrelated to this patch. > <handler event="underflow"><![CDATA[ >+ tabs.tabbrowser._removingTabs.forEach(tabs.tabbrowser._endRemoveTab, >+ tabs.tabbrowser); Seems like perhaps this should be exposed on tabbrowser (finishRemovingTabs?) to avoiding "reaching into" _removingTabs, but I guess that's not a big deal. The comment above _beginRemoveTab needs updating. I worry that removeTab not being synchronous will lead to trouble, even if you've limited that to only some specific cases. I suppose there isn't much that can be done about that, apart maybe from having this.tabs not actually rely on tabContainer.childNodes or otherwise moving some of the teardown stuff from _endRemoveTab to _beginRemoveTab (though the implications of that on tab detaching would then be problematic...).
Attachment #461869 - Flags: review?(dolske) → review+
We should get some basic event-triggered tests for the methods being converted to animate:true, if there aren't any already.
(In reply to comment #94) > > <method name="_endRemoveTab"> > > >+ if (this._windowIsClosing) { > >+ aCloseWindow = false; > >+ aNewTab = false; > >+ } > > Why is this needed? Looks unrelated to this patch. It compensates to this change: if (aCloseWindow) { this._windowIsClosing = true; while (this._removingTabs.length) - this._endRemoveTab([this._removingTabs[0], false]); + this._endRemoveTab(this._removingTabs[0]);
> > <handler event="underflow"><![CDATA[ > > >+ tabs.tabbrowser._removingTabs.forEach(tabs.tabbrowser._endRemoveTab, > >+ tabs.tabbrowser); > > Seems like perhaps this should be exposed on tabbrowser (finishRemovingTabs?) > to avoiding "reaching into" _removingTabs, but I guess that's not a big deal. I'm going to avoid this, as finishRemovingTabs would wrongly sound like it could be used in the case mentioned in my previous comment. > I worry that removeTab not being synchronous will lead to trouble, even if > you've limited that to only some specific cases. I suppose there isn't much > that can be done about that, apart maybe from having this.tabs not actually > rely on tabContainer.childNodes or otherwise moving some of the teardown stuff > from _endRemoveTab to _beginRemoveTab (though the implications of that on tab > detaching would then be problematic...). We'll find out.
Attachment #461869 - Attachment is obsolete: true
Attachment #463451 - Flags: approval2.0?
Comment on attachment 463451 [details] [diff] [review] comments addressed, test added [Checked in: Comment 104] a=beltzner
Attachment #463451 - Flags: approval2.0? → approval2.0+
Have been using this patch for the last few weeks and had a few minor comments. Closing a tab via a middle click is not currently animated. Adding {animate: true} to the removeCurrentTab call in the _handleKeyEvent method seemed to work OK. Not sure why this patch uses a preference to disable animation, while the open tab animation doesn't, but I guess that's another bug. Also, occasionally after trying to close a tab, the tab itself would shrink down to be several pixels wide but not actually close, and would still appear in the tab list drop-down, but be impossible to close. I haven't been able to reliably reproduce this unfortunately so it is quite possible it only occurred with an earlier version of this patch.
Sorry for the noise, I was horribly confused in the last comment. The actual change for animating on middle click is in <handler event="click"> around line 2760 of tabbrowser.xml
(In reply to comment #101) > Sorry for the noise, I was horribly confused in the last comment. > The actual change for animating on middle click is in <handler event="click"> > around line 2760 of tabbrowser.xml That change is contained in the latest attached patch. You've probably been using an older version where this got lost due to a bad merge or something.
Ah yes, that does seem to be the case sorry. Isn't present in the patch in comment 93, and i missed it when manually updating to the latest patch in comment 98
http://hg.mozilla.org/mozilla-central/rev/06b0aaa3623b
Status: REOPENED → RESOLVED
Closed: 17 years ago → 14 years ago
Flags: in-testsuite? → in-testsuite+
Resolution: --- → FIXED
Target Milestone: Firefox 3.1b2 → Firefox 4.0b4
Right click>close tab = no animation
Please file a new bug.
Blocks: 585294
Depends on: 585361
It's just me or animation isnt smooth on the end ? Its seems like it will hang for a moment at the end.
Verified fixed using Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0b4pre) Gecko/20100807 Minefield/4.0b4pre (In reply to comment #107) > It's just me or animation isnt smooth on the end ? Its seems like it will hang > for a moment at the end. Looks smooth to me. Maybe you think it isn't so smooth because the tab doesn't resize that much so the animation isn't as drastic.
Status: RESOLVED → VERIFIED
I think we could do a better job making the process less blocky as the tab increases in size going from 2 tabs to 1 tab. Otherwise, the animation looks fine to me.
Blocks: 585417
(In reply to comment #109) > I think we could do a better job making the process less blocky as the tab > increases in size going from 2 tabs to 1 tab Not sure what exactly this means, but it sounds like you should file a new bug on it.
No longer blocks: 585294, 585417
Depends on: 585294, 585417
(In reply to comment #107) > It's just me or animation isnt smooth on the end ? Its seems like it will hang > for a moment at the end. Possibly related to bug 579323?
(In reply to comment #111) > (In reply to comment #107) > > It's just me or animation isnt smooth on the end ? Its seems like it will hang > > for a moment at the end. > > Possibly related to bug 579323? no, bug 585393
Depends on: 585776
Depends on: 585785
Depends on: 585813
Depends on: 585830
Adding to the comment 109, the animation's jerky when tabs overflow. It seems to be animated in two steps. Perhaps bug 465086 landing will fix this.
No longer depends on: 585776
Depends on: 593565
Attachment #463451 - Attachment description: comments addressed, test added → comments addressed, test added [Checked in: Comment 104]
Depends on: 556687
Depends on: 629230
No longer depends on: 556687
Depends on: 633157
No longer depends on: 593565
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: