Implement closing tabs animation

VERIFIED FIXED in Firefox 4.0b4

Status

()

enhancement
P3
normal
VERIFIED FIXED
12 years ago
7 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

()

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

12 years ago
Posted patch patch (obsolete) — Splinter Review
No description provided.
Attachment #265072 - Flags: ui-review?(beltzner)
Attachment #265072 - Flags: review?(enndeakin)
Assignee

Updated

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

Updated

12 years ago
Flags: blocking-firefox3?

Comment 1

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

12 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

12 years ago
Posted patch patch v2 (obsolete) — 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)
Assignee

Comment 4

12 years ago
Posted patch patch v2.1 (obsolete) — 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 5

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

12 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

12 years ago
Posted patch patch v2.2 (obsolete) — Splinter Review
changed the pref name.
Attachment #265121 - Flags: review?(enndeakin)
Assignee

Comment 8

12 years ago
Posted patch patch v3 (obsolete) — 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 9

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

12 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

12 years ago
Posted patch patch v4 (obsolete) — 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)
Assignee

Comment 12

12 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

12 years ago
Posted patch patch v5 (obsolete) — Splinter Review
Attachment #265182 - Flags: ui-review?(beltzner)
Attachment #265182 - Flags: review?(gavin.sharp)

Comment 14

12 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

12 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

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

Comment 18

12 years ago
Posted file v5 as extension (obsolete) —
Attachment #273109 - Flags: ui-review?(beltzner)
Assignee

Comment 19

12 years ago
Posted file v5 as extension (obsolete) —
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

12 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

12 years ago
Posted patch patch v5, synced with trunk (obsolete) — Splinter Review
Attachment #265182 - Attachment is obsolete: true
Attachment #276822 - Flags: review?(gavin.sharp)
Attachment #265182 - Flags: review?(gavin.sharp)
Assignee

Comment 23

12 years ago
Posted patch patch v5.1, <field> cleanup (obsolete) — Splinter Review
Attachment #276822 - Attachment is obsolete: true
Attachment #277064 - Flags: review?(gavin.sharp)
Attachment #276822 - Flags: review?(gavin.sharp)

Comment 24

12 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

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

Comment 26

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

Comment 27

12 years ago
Posted patch updated to trunk (obsolete) — Splinter Review
Attachment #277064 - Attachment is obsolete: true
Attachment #278750 - Flags: review?(gavin.sharp)
Attachment #277064 - Flags: review?(gavin.sharp)

Comment 28

12 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

12 years ago
Posted patch patch v6 (obsolete) — 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)
Assignee

Updated

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

Comment 30

12 years ago
Posted patch patch v6.1 (obsolete) — 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)
Assignee

Comment 31

12 years ago
Posted patch 6.2 (obsolete) — 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)
Assignee

Comment 32

12 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

12 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

12 years ago
Posted patch patch v7 (obsolete) — Splinter Review
Attachment #279515 - Flags: review?(mano)

Comment 37

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

Comment 38

12 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

12 years ago
Posted file v7 as extension (obsolete) —
This should do.

Comment 40

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

Updated

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

Comment 41

12 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

12 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

12 years ago
Posted patch patch v8 (obsolete) — Splinter Review
Attachment #279515 - Attachment is obsolete: true
Attachment #281804 - Attachment is obsolete: true
Attachment #289816 - Flags: review?(mconnor)
Attachment #279515 - Flags: review?(mconnor)
Assignee

Updated

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

Comment 45

12 years ago
Posted patch patch v8 (obsolete) — 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]
Assignee

Updated

12 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.
Assignee

Comment 48

12 years ago
Posted patch patch v9 (obsolete) — 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)?
Assignee

Comment 50

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

Comment 51

12 years ago
Posted patch patch v10 (obsolete) — 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)
Attachment #306858 - Flags: approval1.9?
Comment on attachment 306858 [details] [diff] [review]
patch v10

a1.9=beltzner
Attachment #306858 - Flags: approval1.9? → approval1.9+
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: 11 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

11 years ago
Posted patch patch + fix (obsolete) — Splinter Review
superb...
Attachment #306858 - Attachment is obsolete: true
Attachment #310735 - Flags: approval1.9b5?
Attachment #310735 - Flags: approval1.9?

Comment 57

11 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

11 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

11 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

11 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

11 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

11 years ago
Posted patch patch v11 (obsolete) — 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)
Assignee

Comment 65

11 years ago
Posted patch rewritten for trunk (obsolete) — Splinter Review
Attachment #320929 - Attachment is obsolete: true
Attachment #344209 - Flags: review?(gavin.sharp)
Attachment #320929 - Flags: review?(mano)
Assignee

Updated

11 years ago
Attachment #344209 - Attachment description: updated to trunk → rewritten for trunk
Assignee

Comment 66

11 years ago
Posted patch rewritten patch v2 (obsolete) — 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)
Assignee

Updated

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

Updated

11 years ago
Blocks: 347930
Assignee

Comment 67

11 years ago
Posted patch updated to trunk (obsolete) — Splinter Review
Attachment #344270 - Attachment is obsolete: true
Attachment #344617 - Flags: review?(gavin.sharp)
Attachment #344270 - Flags: review?(gavin.sharp)
Assignee

Comment 68

11 years ago
Posted patch v3 (obsolete) — 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)
Assignee

Updated

11 years ago
Blocks: 457096
Assignee

Updated

11 years ago
Blocks: 462673
Assignee

Comment 69

11 years ago
Posted patch updated to trunk (obsolete) — Splinter Review
Attachment #344741 - Attachment is obsolete: true
Attachment #345932 - Flags: review?(gavin.sharp)
Attachment #344741 - Flags: review?(gavin.sharp)
Assignee

Updated

11 years ago
No longer blocks: 462673
Depends on: 462673
Assignee

Updated

11 years ago
No longer blocks: 457096
Assignee

Comment 70

10 years ago
Posted patch updated to trunk (obsolete) — Splinter Review
Attachment #345932 - Attachment is obsolete: true
Attachment #345932 - Flags: review?(gavin.sharp)
Assignee

Updated

10 years ago
No longer blocks: 347930
Assignee

Comment 71

10 years ago
Posted patch updated to trunk (obsolete) — Splinter Review
Attachment #369065 - Attachment is obsolete: true

Comment 72

10 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?
Assignee

Updated

10 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

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

Comment 75

9 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

9 years ago
Posted patch patch (obsolete) — Splinter Review
Attachment #389666 - Attachment is obsolete: true
Attachment #451237 - Flags: review?(gavin.sharp)
Assignee

Updated

9 years ago
No longer depends on: 572114

Comment 77

9 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

9 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

9 years ago
Posted patch patch (obsolete) — Splinter Review
updated to tip
Attachment #451237 - Attachment is obsolete: true
Attachment #453852 - Flags: review?(gavin.sharp)
Attachment #451237 - Flags: review?(gavin.sharp)
Assignee

Comment 85

9 years ago
Posted patch patch (obsolete) — 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)
Assignee

Updated

9 years ago
Blocks: 578373
Assignee

Updated

9 years ago
Duplicate of this bug: 582674
Assignee

Comment 87

9 years ago
Posted patch patch (obsolete) — 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)

Comment 88

9 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

9 years ago
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+
Posted video 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

9 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

9 years ago
Posted patch patch (obsolete) — 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)

Updated

9 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

9 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

9 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

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

9 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

9 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

9 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

9 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

9 years ago
http://hg.mozilla.org/mozilla-central/rev/06b0aaa3623b
Status: REOPENED → RESOLVED
Closed: 11 years ago9 years ago
Flags: in-testsuite? → in-testsuite+
Resolution: --- → FIXED
Target Milestone: Firefox 3.1b2 → Firefox 4.0b4

Comment 105

9 years ago
Right click>close tab = no animation
Assignee

Comment 106

9 years ago
Please file a new bug.

Updated

9 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

9 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

9 years ago
Blocks: 585417
Assignee

Comment 110

9 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

9 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

9 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

9 years ago
Depends on: 585776
Assignee

Updated

9 years ago
Depends on: 585785
Assignee

Updated

9 years ago
Depends on: 585813
Depends on: 585830

Comment 113

9 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

9 years ago
No longer depends on: 585776
Assignee

Updated

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

Updated

9 years ago
Depends on: 633157
Assignee

Updated

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