Implement closing tabs animation

VERIFIED FIXED in Firefox 4.0b4

Status

()

Firefox
Tabbed Browser
P3
enhancement
VERIFIED FIXED
10 years ago
5 years ago

People

(Reporter: dao, Assigned: dao)

Tracking

(Depends on: 1 bug)

Trunk
Firefox 4.0b4
Points:
---
Dependency tree / graph
Bug Flags:
blocking-firefox3 -
wanted-firefox3 +
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(2 attachments, 35 obsolete attachments)

1.34 MB, video/x-msvideo
Details
17.61 KB, patch
beltzner
: approval2.0+
Details | Diff | Splinter Review
(Assignee)

Description

10 years ago
Created attachment 265072 [details] [diff] [review]
patch
Attachment #265072 - Flags: ui-review?(beltzner)
Attachment #265072 - Flags: review?(enndeakin)
(Assignee)

Updated

10 years ago
Priority: -- → P3
Whiteboard: PRD:TAB-004b

Updated

10 years ago
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-

Comment 2

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

Comment 3

10 years ago
Created attachment 265115 [details] [diff] [review]
patch v2
Attachment #265072 - Attachment is obsolete: true
Attachment #265115 - Flags: ui-review?(beltzner)
Attachment #265115 - Flags: review?(enndeakin)
Attachment #265072 - Flags: ui-review?(beltzner)
(Assignee)

Comment 4

10 years ago
Created attachment 265117 [details] [diff] [review]
patch v2.1

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

Comment 6

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

Comment 7

10 years ago
Created attachment 265121 [details] [diff] [review]
patch v2.2

changed the pref name.
Attachment #265121 - Flags: review?(enndeakin)
(Assignee)

Comment 8

10 years ago
Created attachment 265125 [details] [diff] [review]
patch v3

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

Comment 10

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

Comment 11

10 years ago
Created attachment 265169 [details] [diff] [review]
patch v4
Attachment #265125 - Attachment is obsolete: true
Attachment #265169 - Flags: ui-review?(beltzner)
Attachment #265169 - Flags: review?(gavin.sharp)
Attachment #265125 - Flags: ui-review?(beltzner)
(Assignee)

Comment 12

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

Comment 13

10 years ago
Created attachment 265182 [details] [diff] [review]
patch v5
Attachment #265182 - Flags: ui-review?(beltzner)
Attachment #265182 - Flags: review?(gavin.sharp)

Comment 14

10 years ago
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?

(Assignee)

Comment 15

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

Updated

10 years ago
Attachment #265182 - Flags: ui-review?(beltzner)
(Assignee)

Comment 18

10 years ago
Created attachment 273109 [details]
v5 as extension
Attachment #273109 - Flags: ui-review?(beltzner)
(Assignee)

Comment 19

10 years ago
Created attachment 273124 [details]
v5 as extension

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+

Comment 21

10 years ago
There is no animation for re-arranging tabs, though I am not sure that a priority in the scheme of things...
(Assignee)

Comment 22

10 years ago
Created attachment 276822 [details] [diff] [review]
patch v5, synced with trunk
Attachment #265182 - Attachment is obsolete: true
Attachment #276822 - Flags: review?(gavin.sharp)
Attachment #265182 - Flags: review?(gavin.sharp)
(Assignee)

Comment 23

10 years ago
Created attachment 277064 [details] [diff] [review]
patch v5.1, <field> cleanup
Attachment #276822 - Attachment is obsolete: true
Attachment #277064 - Flags: review?(gavin.sharp)
Attachment #276822 - Flags: review?(gavin.sharp)

Comment 24

10 years ago
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!

Comment 25

10 years ago
No, it still needs a code review from Gavin as you can see on the most recent patch.

Comment 26

10 years ago
The code freeze is less than a week away. Why aren't we landing this even though its complete?
(Assignee)

Comment 27

10 years ago
Created attachment 278750 [details] [diff] [review]
updated to trunk
Attachment #277064 - Attachment is obsolete: true
Attachment #278750 - Flags: review?(gavin.sharp)
Attachment #277064 - Flags: review?(gavin.sharp)

Comment 28

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

Comment 29

10 years ago
Created attachment 279451 [details] [diff] [review]
patch v6

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

Updated

10 years ago
Attachment #279451 - Flags: review?(gavin.sharp)
(Assignee)

Comment 30

10 years ago
Created attachment 279452 [details] [diff] [review]
patch v6.1

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

Comment 31

10 years ago
Created attachment 279454 [details] [diff] [review]
6.2

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

Comment 32

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

Comment 35

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

Comment 36

10 years ago
Created attachment 279515 [details] [diff] [review]
patch v7
Attachment #279515 - Flags: review?(mano)

Comment 37

10 years ago
How can I include the new code (v7) in the extension (v5). Is there a major change?
(Assignee)

Comment 38

10 years ago
I can attach an updated extension. Please do not use v5. It accidentally contained an old version of my Ctrl-Tab extension.
(Assignee)

Comment 39

10 years ago
Created attachment 281804 [details]
v7 as extension

This should do.

Comment 40

10 years ago
Thanks for the extension update Dao. Hope this lands on trunk soon. Its been quite sometime.
(Assignee)

Updated

10 years ago
Attachment #279515 - Flags: review?(mano) → review?(mconnor)

Comment 41

10 years ago
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?
(Assignee)

Comment 42

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

Comment 44

10 years ago
Created attachment 289816 [details] [diff] [review]
patch v8
Attachment #279515 - Attachment is obsolete: true
Attachment #281804 - Attachment is obsolete: true
Attachment #289816 - Flags: review?(mconnor)
Attachment #279515 - Flags: review?(mconnor)
(Assignee)

Updated

10 years ago
Whiteboard: PRD:TAB-004b [wanted-firefox3] → PRD:TAB-004b [wanted-firefox3][has patch][has ui-review][needs review]
(Assignee)

Comment 45

10 years ago
Created attachment 294948 [details] [diff] [review]
patch v8

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

Updated

10 years ago
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?
(Assignee)

Comment 48

10 years ago
Created attachment 303683 [details] [diff] [review]
patch v9

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

Comment 50

10 years ago
Yes, that's a simple move. Definitely better for extensions at this point.
(Assignee)

Comment 51

10 years ago
Created attachment 306858 [details] [diff] [review]
patch v10

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
Last Resolved: 10 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 → ---
(Assignee)

Comment 56

10 years ago
Created attachment 310735 [details] [diff] [review]
patch + fix

superb...
Attachment #306858 - Attachment is obsolete: true
Attachment #310735 - Flags: approval1.9b5?
Attachment #310735 - Flags: approval1.9?

Comment 57

10 years ago
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?

Comment 59

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

Comment 60

10 years ago
(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 → ---

Comment 61

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

Comment 62

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

Comment 64

9 years ago
Created attachment 320929 [details] [diff] [review]
patch v11

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

Comment 65

9 years ago
Created attachment 344209 [details] [diff] [review]
rewritten for trunk
Attachment #320929 - Attachment is obsolete: true
Attachment #344209 - Flags: review?(gavin.sharp)
Attachment #320929 - Flags: review?(mano)
(Assignee)

Updated

9 years ago
Attachment #344209 - Attachment description: updated to trunk → rewritten for trunk
(Assignee)

Comment 66

9 years ago
Created attachment 344270 [details] [diff] [review]
rewritten patch v2

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

Updated

9 years ago
Whiteboard: [not needed for 1.9]PRD:TAB-004b
Target Milestone: --- → Firefox 3.1b2
(Assignee)

Updated

9 years ago
Blocks: 347930
(Assignee)

Comment 67

9 years ago
Created attachment 344617 [details] [diff] [review]
updated to trunk
Attachment #344270 - Attachment is obsolete: true
Attachment #344617 - Flags: review?(gavin.sharp)
Attachment #344270 - Flags: review?(gavin.sharp)
(Assignee)

Comment 68

9 years ago
Created attachment 344741 [details] [diff] [review]
v3

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

Updated

9 years ago
Blocks: 457096
(Assignee)

Updated

9 years ago
Blocks: 462673
(Assignee)

Comment 69

9 years ago
Created attachment 345932 [details] [diff] [review]
updated to trunk
Attachment #344741 - Attachment is obsolete: true
Attachment #345932 - Flags: review?(gavin.sharp)
Attachment #344741 - Flags: review?(gavin.sharp)
(Assignee)

Updated

9 years ago
No longer blocks: 462673
Depends on: 462673
(Assignee)

Updated

9 years ago
No longer blocks: 457096
(Assignee)

Comment 70

9 years ago
Created attachment 369065 [details] [diff] [review]
updated to trunk
Attachment #345932 - Attachment is obsolete: true
Attachment #345932 - Flags: review?(gavin.sharp)
(Assignee)

Updated

8 years ago
No longer blocks: 347930
(Assignee)

Comment 71

8 years ago
Created attachment 389666 [details] [diff] [review]
updated to trunk
Attachment #369065 - Attachment is obsolete: true

Comment 72

8 years ago
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?

Updated

8 years ago
(Assignee)

Updated

8 years ago
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. :)

Comment 74

7 years ago
Yeah - 3 yrs in the waiting... this deserves to land now :)

Comment 75

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

Comment 76

7 years ago
Created attachment 451237 [details] [diff] [review]
patch
Attachment #389666 - Attachment is obsolete: true
Attachment #451237 - Flags: review?(gavin.sharp)
Depends on: 572114
(Assignee)

Updated

7 years ago
No longer depends on: 572114

Comment 77

7 years ago
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. :)

Comment 82

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

Comment 84

7 years ago
Created attachment 453852 [details] [diff] [review]
patch

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

Comment 85

7 years ago
Created attachment 456048 [details] [diff] [review]
patch

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

Updated

7 years ago
Blocks: 578373
(Assignee)

Updated

7 years ago
Duplicate of this bug: 582674
(Assignee)

Comment 87

7 years ago
Created attachment 461204 [details] [diff] [review]
patch

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)

Comment 88

7 years ago
you call _endRemoveTab from transitionend
so we get a delay between TabClose event and the actual tab removal and actual  "UI responsiveness"
(Assignee)

Comment 89

7 years ago
That delay isn't visible to the user, as the closing tab loses focus when the transition starts.

Comment 90

7 years ago
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+
Created attachment 461845 [details]
Tab closing video

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

Comment 92

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

Comment 93

7 years ago
Created attachment 461869 [details] [diff] [review]
patch

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)

Updated

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

Comment 96

7 years ago
(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]);
(Assignee)

Comment 97

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

Comment 98

7 years ago
Created attachment 463451 [details] [diff] [review]
comments addressed, test added
[Checked in: Comment 104]
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+

Comment 100

7 years ago
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.

Comment 101

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

Comment 102

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

Comment 103

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

Comment 104

7 years ago
http://hg.mozilla.org/mozilla-central/rev/06b0aaa3623b
Status: REOPENED → RESOLVED
Last Resolved: 10 years ago7 years ago
Flags: in-testsuite? → in-testsuite+
Resolution: --- → FIXED
Target Milestone: Firefox 3.1b2 → Firefox 4.0b4

Comment 105

7 years ago
Right click>close tab = no animation
(Assignee)

Comment 106

7 years ago
Please file a new bug.

Updated

7 years ago
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.

Comment 108

7 years ago
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.

Updated

7 years ago
Blocks: 585417
(Assignee)

Comment 110

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

Updated

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

Comment 112

7 years ago
(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

Updated

7 years ago
Depends on: 585776
(Assignee)

Updated

7 years ago
Depends on: 585785
(Assignee)

Updated

7 years ago
Depends on: 585813
Depends on: 585830

Comment 113

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

Updated

7 years ago
No longer depends on: 585776
(Assignee)

Updated

7 years ago
Depends on: 593565
Attachment #463451 - Attachment description: comments addressed, test added → comments addressed, test added [Checked in: Comment 104]
http://hg.mozilla.org/mozilla-central/rev/f23f7c54c080
Bug 380960 follow-up fix for test_bug511449.html
Depends on: 556687
Depends on: 629230
No longer depends on: 556687
(Assignee)

Updated

7 years ago
Depends on: 633157
(Assignee)

Updated

7 years ago
No longer depends on: 593565
You need to log in before you can comment on or make changes to this bug.