From a08173ec791c8eb1caf6f4a3574cdb9cb0f96514 Mon Sep 17 00:00:00 2001
From: Chris Chua <chris@thousandeyes.com>
Date: Fri, 16 Aug 2013 17:00:54 -0700
Subject: [PATCH] fix(tabs): initial tab selection

Closes #834, Fixes #747

- Avoid re-initializing `tab.active`
- `setActive` only when all `tab.active` are set up (on the next
  digest cycle)

Before I go to explain, there are up to two expressions that indicate
whether a tab is active:

  1. `active` variable in the isolate scope of the tab directive
  2. The expression from the active attribute (`attrs.active`) if it
     is set, I'll call this `getActive`, as that's the variable that
     refers to it.

During initial linking (adding of tabs), the `active` variable in the
tab's isolate scope tracks the active tab. When the first tab is
added, it's `active` is set to true since there's no way to know if
subsequent tabs are active since they haven't been added yet. As such,
at this point, it is not meaningful to set assign the `getActive`
with the value of `active`. At least not until all the tabs have been
added. Hence, a good time would be to wait until the next $digest
cycle.

A watcher is called asynchronously after initialization even if there
is no change on the expression's value.

As such, we can leave that to the watcher for the `active` expression
to initialize getActive by calling setActive during its initlization
cycle.

However, there is a chance (if the $digest cycles and planets
align...) that the `active` variable gets initialized a second time
using the `getActive` (in the watcher for `getActive`). Since we're
already setting `active` to `getActive`, and the `active` variable
should now be carrying the *truth*. Avoid this re-initialization.
---
 src/tabs/tabs.js          | 14 +++++++----
 src/tabs/test/tabsSpec.js | 51 +++++++++++++++++++++++++++++++++++++++
 2 files changed, 60 insertions(+), 5 deletions(-)

diff --git a/src/tabs/tabs.js b/src/tabs/tabs.js
index db998b0d47..dbda75ca29 100644
--- a/src/tabs/tabs.js
+++ b/src/tabs/tabs.js
@@ -196,8 +196,13 @@ angular.module('ui.bootstrap.tabs', [])
         if (attrs.active) {
           getActive = $parse(attrs.active);
           setActive = getActive.assign;
-          scope.$parent.$watch(getActive, function updateActive(value) {
-            scope.active = !!value;
+          scope.$parent.$watch(getActive, function updateActive(value, oldVal) {
+            // Avoid re-initializing scope.active as it is already initialized
+            // below. (watcher is called async during init with value ===
+            // oldVal)
+            if (value !== oldVal) {
+              scope.active = !!value;
+            }
           });
           scope.active = getActive(scope.$parent);
         } else {
@@ -205,6 +210,8 @@ angular.module('ui.bootstrap.tabs', [])
         }
 
         scope.$watch('active', function(active) {
+          // Note this watcher also initializes and assigns scope.active to the
+          // attrs.active expression.
           setActive(scope.$parent, active);
           if (active) {
             tabsetCtrl.select(scope);
@@ -231,9 +238,6 @@ angular.module('ui.bootstrap.tabs', [])
         scope.$on('$destroy', function() {
           tabsetCtrl.removeTab(scope);
         });
-        if (scope.active) {
-          setActive(scope.$parent, true);
-        }
 
 
         //We need to transclude later, once the content container is ready.
diff --git a/src/tabs/test/tabsSpec.js b/src/tabs/test/tabsSpec.js
index 692d42a9f4..61215db39b 100644
--- a/src/tabs/test/tabsSpec.js
+++ b/src/tabs/test/tabsSpec.js
@@ -102,6 +102,57 @@ describe('tabs', function() {
 
   });
 
+  describe('basics with initial active tab', function() {
+
+    beforeEach(inject(function($compile, $rootScope) {
+      scope = $rootScope.$new();
+
+      function makeTab(active) {
+        return {
+          active: !!active,
+          select: jasmine.createSpy()
+        };
+      }
+      scope.tabs = [
+        makeTab(), makeTab(), makeTab(true), makeTab()
+      ];
+      elm = $compile([
+        '<tabset>',
+        '  <tab active="tabs[0].active" select="tabs[0].select()">',
+        '  </tab>',
+        '  <tab active="tabs[1].active" select="tabs[1].select()">',
+        '  </tab>',
+        '  <tab active="tabs[2].active" select="tabs[2].select()">',
+        '  </tab>',
+        '  <tab active="tabs[3].active" select="tabs[3].select()">',
+        '  </tab>',
+        '</tabset>'
+      ].join('\n'))(scope);
+      scope.$apply();
+    }));
+
+    function expectTabActive(activeTab) {
+      var _titles = titles();
+      angular.forEach(scope.tabs, function(tab, i) {
+        if (activeTab === tab) {
+          expect(tab.active).toBe(true);
+          //It should only call select ONCE for each select
+          expect(tab.select).toHaveBeenCalled();
+          expect(_titles.eq(i)).toHaveClass('active');
+          expect(contents().eq(i)).toHaveClass('active');
+        } else {
+          expect(tab.active).toBe(false);
+          expect(_titles.eq(i)).not.toHaveClass('active');
+        }
+      });
+    }
+
+    it('should make tab titles and set active tab active', function() {
+      expect(titles().length).toBe(scope.tabs.length);
+      expectTabActive(scope.tabs[2]);
+    });
+  });
+
   describe('ng-repeat', function() {
 
     beforeEach(inject(function($compile, $rootScope) {