Skip to content
This repository has been archived by the owner on May 29, 2019. It is now read-only.

fix(tabs): Initial tab selection #834

Closed
wants to merge 1 commit into from

Conversation

chrisirhc
Copy link
Contributor

  • 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 (setActive assigns 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.


This fix is ugly because the logic for the two-way binding of active and the getActive is complex. Consider refactoring the implementation for maintaining the two-way binding. It looks like this sort of binding is exactly what $compile already does in compile.js#L973-L1001 . AngularJS 1.1.4 introduced the feature in angular/angular.js@ac899d0 to allow optional binding of variables in the isolate scope that handles this very cleanly just by declaring scope: {active: '=? active'} for the directive.

I didn't investigate into why the previous test with ng-repeat didn't expose this problem. It's possible it's because it had an additional digest cycle between linking such that the digest cycles didn't go in the order that would expose the problem.

Fixes #747.

@@ -234,9 +239,6 @@ function($parse, $http, $templateCache, $compile) {
scope.$on('$destroy', function() {
tabsetCtrl.removeTab(scope);
});
if (scope.active) {
setActive(scope.$parent, true);
}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

This will happen automatically, because the watcher for active should run asynchronously on the later (see comment on line 216) even if the active variable hasn't changed.

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

---

This fix is ugly because the logic for the two-way binding of `active`
and the `getActive` is complex. Consider refactoring the
implementation for maintaining the two-way binding (see ngModel for
inspiration).

Fixes angular-ui#747.
@ajoslin ajoslin closed this Sep 23, 2013
@ajoslin
Copy link
Contributor

ajoslin commented Sep 23, 2013

thanks @chrisirhc - finally merged it! Fortunately we will soon be replacing the binding with '=?' as you said!

ajoslin pushed a commit that referenced this pull request Sep 23, 2013
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.
@chrisirhc chrisirhc deleted the feature/fix-747 branch September 23, 2013 21:29
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Tab initial selection
2 participants