fix(mdToolbar): Better fix for ng-if that allows ng-controller.

topherfangio · kennethcachia · commit 9dcdf73432dd · 2015-09-23T14:40:36.000-07:00
After modifying the `<md-toolbar>` directive to allow both `md-scroll-shrink` and `ng-if`, we accidentally broke support for also using `ng-controller` as well as some layout problems due to the transclusion and a wrapping `<div>`. This reverts those changes but still allows for `ng-if` to work. Fixes angular#4144. Closes angular#4423.
diff --git a/src/components/toolbar/demoScrollShrink/index.html b/src/components/toolbar/demoScrollShrink/index.html
@@ -1,9 +1,9 @@
 <div ng-controller="AppCtrl" layout="column" style="height:600px">
 
-  <md-toolbar md-scroll-shrink>
+  <md-toolbar md-scroll-shrink ng-if="true" ng-controller="TitleController">
     <div class="md-toolbar-tools">
       <h3>
-        <span>My App Title</span>
+        <span>{{title}}</span>
       </h3>
     </div>
   </md-toolbar>
diff --git a/src/components/toolbar/demoScrollShrink/script.js b/src/components/toolbar/demoScrollShrink/script.js
@@ -1,5 +1,9 @@
 var app = angular.module('toolbarDemo2', ['ngMaterial']);
 
+app.controller('TitleController', function($scope) {
+  $scope.title = 'My App Title';
+});
+
 app.controller('AppCtrl', function($scope) {
   var imagePath = 'img/list/60.jpeg';
 
diff --git a/src/components/toolbar/toolbar.js b/src/components/toolbar/toolbar.js
@@ -47,8 +47,10 @@ angular.module('material.components.toolbar', [
  *
  * @param {boolean=} md-scroll-shrink Whether the header should shrink away as
  * the user scrolls down, and reveal itself as the user scrolls up.
- * Note: for scrollShrink to work, the toolbar must be a sibling of a
- * `md-content` element, placed before it. See the scroll shrink demo.
+ * _**Note (1):** for scrollShrink to work, the toolbar must be a sibling of a
+ * `md-content` element, placed before it. See the scroll shrink demo._
+ * _**Note (2):** The `md-scroll-shrink` attribute is only parsed on component
+ * initialization, it does not watch for scope changes._
  *
  *
  * @param {number=} md-shrink-speed-factor How much to change the speed of the toolbar's
@@ -61,84 +63,66 @@ function mdToolbarDirective($$rAF, $mdConstant, $mdUtil, $mdTheming, $animate) {
 
   return {
     restrict: 'E',
-    template: '<div ng-transclude></div>',
-    transclude: true,
-    controller: angular.noop,
-    scope: {
-      scrollShrink: '=?mdScrollShrink'
-    },
+
     link: function(scope, element, attr) {
 
       $mdTheming(element);
 
       setupScrollShrink();
 
       function setupScrollShrink() {
+
+        var toolbarHeight;
+        var contentElement;
+        var disableScrollShrink = angular.noop;
+
         // Current "y" position of scroll
-        var y = 0;
         // Store the last scroll top position
+        var y = 0;
         var prevScrollTop = 0;
-
         var shrinkSpeedFactor = attr.mdShrinkSpeedFactor || 0.5;
 
-        var toolbarHeight;
-        var contentElement;
-
         var debouncedContentScroll = $$rAF.throttle(onContentScroll);
         var debouncedUpdateHeight = $mdUtil.debounce(updateToolbarHeight, 5 * 1000);
 
-        var disableScrollShrink;
-
         // Wait for $mdContentLoaded event from mdContent directive.
         // If the mdContent element is a sibling of our toolbar, hook it up
         // to scroll events.
+
         scope.$on('$mdContentLoaded', onMdContentLoad);
 
         // If the toolbar is used inside an ng-if statement, we may miss the
         // $mdContentLoaded event, so we attempt to fake it if we have a
         // md-content close enough.
-        scope.$watch('scrollShrink', onChangeScrollShrink);
+
+        attr.$observe('mdScrollShrink', onChangeScrollShrink);
 
         // If the scope is destroyed (which could happen with ng-if), make sure
         // to disable scroll shrinking again
-        scope.$on('$destroy', function() {
-          disableScrollShrink();
-        });
 
-        function onChangeScrollShrink(scrollShrink) {
+        scope.$on('$destroy', disableScrollShrink );
+
+        /**
+         *
+         */
+        function onChangeScrollShrink(shrinkWithScroll) {
           var closestContent = element.parent().find('md-content');
 
           // If we have a content element, fake the call; this might still fail
           // if the content element isn't a sibling of the toolbar
+
           if (!contentElement && closestContent.length) {
             onMdContentLoad(null, closestContent);
           }
 
-          if (contentElement) {
-            // Disable only if the attribute's expression evaluates to false
-            if (scrollShrink === false) {
-              disableScrollShrink();
-            } else {
-              enableScrollShrink();
-            }
-          }
-        }
-
-        function enableScrollShrink() {
-          contentElement.on('scroll', debouncedContentScroll);
-          contentElement.attr('scroll-shrink', 'true');
-
-          $$rAF(updateToolbarHeight);
+          // Disable only if the attribute's expression evaluates to false
 
-          return function disableScrollShrink() {
-            contentElement.off('scroll', debouncedContentScroll);
-            contentElement.attr('scroll-shrink', 'false');
-
-            $$rAF(updateToolbarHeight);
-          }
+         if ( shrinkWithScroll )  disableScrollShrink = enableScrollShrink();
         }
 
-
+        /**
+         *
+         */
         function onMdContentLoad($event, newContentEl) {
           // Toolbar and content must be siblings
           if (newContentEl && element.parent()[0] === newContentEl.parent()[0]) {
@@ -152,24 +136,9 @@ function mdToolbarDirective($$rAF, $mdConstant, $mdUtil, $mdTheming, $animate) {
           }
         }
 
-        function updateToolbarHeight() {
-          toolbarHeight = element.prop('offsetHeight');
-          // Add a negative margin-top the size of the toolbar to the content el.
-          // The content will start transformed down the toolbarHeight amount,
-          // so everything looks normal.
-          //
-          // As the user scrolls down, the content will be transformed up slowly
-          // to put the content underneath where the toolbar was.
-          var margin = (-toolbarHeight * shrinkSpeedFactor) + 'px';
-
-          contentElement.css({
-            "margin-top": margin,
-            "margin-bottom": margin
-          });
-
-          onContentScroll();
-        }
-
+        /**
+         *
+         */
         function onContentScroll(e) {
           var scrollTop = e ? e.target.scrollTop : prevScrollTop;
 
@@ -197,6 +166,46 @@ function mdToolbarDirective($$rAF, $mdConstant, $mdUtil, $mdTheming, $animate) {
 
         }
 
+        /**
+         *
+         */
+        function enableScrollShrink() {
+          if ( !contentElement )     return angular.noop;           // no md-content
+
+          contentElement.on('scroll', debouncedContentScroll);
+          contentElement.attr('scroll-shrink', 'true');
+
+          $$rAF(updateToolbarHeight);
+
+          return function disableScrollShrink() {
+            contentElement.off('scroll', debouncedContentScroll);
+            contentElement.attr('scroll-shrink', 'false');
+
+            $$rAF(updateToolbarHeight);
+          }
+        }
+
+        /**
+         *
+         */
+        function updateToolbarHeight() {
+          toolbarHeight = element.prop('offsetHeight');
+          // Add a negative margin-top the size of the toolbar to the content el.
+          // The content will start transformed down the toolbarHeight amount,
+          // so everything looks normal.
+          //
+          // As the user scrolls down, the content will be transformed up slowly
+          // to put the content underneath where the toolbar was.
+          var margin = (-toolbarHeight * shrinkSpeedFactor) + 'px';
+
+          contentElement.css({
+            "margin-top": margin,
+            "margin-bottom": margin
+          });
+
+          onContentScroll();
+        }
+
       }
 
     }
diff --git a/src/components/toolbar/toolbar.spec.js b/src/components/toolbar/toolbar.spec.js
@@ -3,7 +3,11 @@ describe('<md-toolbar>', function() {
   var pageScope, element, controller;
   var $rootScope, $timeout;
 
-  beforeEach(module('material.components.toolbar'));
+  beforeEach(function() {
+    module('material.components.toolbar', function($controllerProvider) {
+      $controllerProvider.register('MockController', function() {});
+    });
+  });
   beforeEach(inject(function(_$rootScope_, _$timeout_) {
     $rootScope = _$rootScope_;
     $timeout = _$timeout_;
@@ -45,7 +49,8 @@ describe('<md-toolbar>', function() {
     // Manually link so we can give our own elements with spies on them
     mdToolbarDirective[0].link($rootScope, toolbar, {
       mdScrollShrink: true,
-      mdShrinkSpeedFactor: 1
+      mdShrinkSpeedFactor: 1,
+      $observe: function() {}
     });
 
     $rootScope.$apply();
@@ -103,44 +108,37 @@ describe('<md-toolbar>', function() {
     // It starts out undefined
     expect(element.find('md-content').attr('scroll-shrink')).toEqual(undefined);
 
-    // Change the ng-if to add the toolbar
+    // Change the ng-if to add the toolbar which then injects a scrollShrink
+    // on the mdContent
     pageScope.$apply('shouldShowToolbar = true');
     expect(element.find('md-content').attr('scroll-shrink')).toEqual('true');
 
     // Change the ng-if to remove the toolbar
     pageScope.$apply('shouldShowToolbar = false');
-    expect(element.find('md-content').attr('scroll-shrink')).toEqual('false');
+    expect(element.find('md-toolbar').length).toBe(0);
   }));
 
-  it('enables scroll shrink when the attribute has no value', function() {
+  // The toolbar is like a container component, so we want to make sure it works with ng-controller
+  it('works with ng-controller', inject(function($exceptionHandler) {
     build(
       '<div>' +
-      '  <md-toolbar md-scroll-shrink></md-toolbar>' +
+      '  <md-toolbar md-scroll-shrink ng-controller="MockController"></md-toolbar>' +
       '  <md-content></md-content>' +
       '</div>'
     );
 
-    expect(element.find('md-content').attr('scroll-shrink')).toEqual('true');
-  });
+    // Expect no errors
+    expect($exceptionHandler.errors).toEqual([]);
+  }));
 
-  it('watches the value of scroll shrink', function() {
+  it('enables scroll shrink when the attribute has no value', function() {
     build(
       '<div>' +
-      '  <md-toolbar md-scroll-shrink="shouldShrink"></md-toolbar>' +
+      '  <md-toolbar md-scroll-shrink></md-toolbar>' +
       '  <md-content></md-content>' +
       '</div>'
     );
 
-    // It starts out undefined which SHOULD add the scroll shrink because it acts as if no value
-    // was specified
-    expect(element.find('md-content').attr('scroll-shrink')).toEqual('true');
-
-    // Change the scrollShink to false
-    pageScope.$apply('shouldShrink = false');
-    expect(element.find('md-content').attr('scroll-shrink')).toEqual('false');
-
-    // Change the scrollShink to true
-    pageScope.$apply('shouldShrink = true');
     expect(element.find('md-content').attr('scroll-shrink')).toEqual('true');
   });
 
