fix($anchorScroll): fix scrolling with element as yOffset in Firefox, IE

gkalpak · gkalpak · commit cfa684497bb2 · 2014-10-09T14:53:05.000+03:00
Firefox and IE use the `&lt;html&gt;` element as the viewport, while Chrome uses
the `&lt;body&gt;`. This caused scrolling to not work correctly when an element
was set as `yOffset`. Using `window.pageYOffset` instead, seems to work
consistently across all supported browsers.
diff --git a/src/ng/anchorScroll.js b/src/ng/anchorScroll.js
@@ -222,10 +222,10 @@ function $AnchorScrollProvider() {
           // case for elements near the bottom of the page.
           // In such cases we do not need to scroll the whole `offset` up, just the fraction of the
           // offset that is necessary to align the top of `elem` at the desired position.
-          var body = document.body;
-          var bodyRect = body.getBoundingClientRect();
-          var elemRect = elem.getBoundingClientRect();
-          var necessaryOffset = offset - (elemRect.top - (bodyRect.top + body.scrollTop));
+          var elemTop = elem.getBoundingClientRect().top;
+          var bodyTop = document.body.getBoundingClientRect().top;
+          var scrollTop = $window.pageYOffset;
+          var necessaryOffset = offset - (elemTop - (bodyTop + scrollTop));
 
           $window.scrollBy(0, -1 * necessaryOffset);
         }
diff --git a/test/ng/anchorScrollSpec.js b/test/ng/anchorScrollSpec.js
@@ -3,14 +3,20 @@
 describe('$anchorScroll', function() {
 
   var elmSpy;
+  var windowSpies;
 
   function addElements() {
     var elements = sliceArgs(arguments);
 
     return function($window) {
       forEach(elements, function(identifier) {
-        var match = identifier.match(/(\w* )?(\w*)=(\w*)/),
-            jqElm = jqLite('<' + (match[1] || 'a ') + match[2] + '="' + match[3] + '"/>'),
+        var match = identifier.match(/(?:(\w*) )?(\w*)=(\w*)/),
+            nodeName = match[1] || 'a',
+            tmpl = '<' + nodeName + ' ' + match[2] + '="' + match[3] + '">' +
+                      match[3] +   // add some content or else Firefox and IE place the element
+                                   // in weird ways that break yOffset-testing.
+                   '</' + nodeName + '>',
+            jqElm = jqLite(tmpl),
             elm = jqElm[0];
 
         elmSpy[identifier] = spyOn(elm, 'scrollIntoView');
@@ -20,7 +26,7 @@ describe('$anchorScroll', function() {
   }
 
   function callAnchorScroll() {
-    return function ($anchorScroll) {
+    return function($anchorScroll) {
       $anchorScroll();
     };
   }
@@ -33,7 +39,7 @@ describe('$anchorScroll', function() {
   }
 
   function changeHashTo(hash) {
-    return function ($anchorScroll, $location, $rootScope) {
+    return function($anchorScroll, $location, $rootScope) {
       $rootScope.$apply(function() {
         $location.hash(hash);
       });
@@ -73,14 +79,29 @@ describe('$anchorScroll', function() {
     return expectScrollingTo(NaN);
   }
 
+  function resetAllSpies() {
+    function resetSpy(spy) {
+      spy.reset();
+    }
+
+    return function($window) {
+      forEach(elmSpy, resetSpy);
+      forEach(windowSpies, resetSpy);
+    };
+  }
+
 
   beforeEach(module(function($provide) {
     elmSpy = {};
+    windowSpies = [];
+    var i = 0;
+
     $provide.value('$window', {
-      scrollTo: jasmine.createSpy('$window.scrollTo'),
-      scrollBy: jasmine.createSpy('$window.scrollBy'),
+      scrollTo: (windowSpies[i++] = jasmine.createSpy('$window.scrollTo')),
+      scrollBy: (windowSpies[i++] = jasmine.createSpy('$window.scrollBy')),
       document: document,
       navigator: {},
+      pageYOffset: 0,
       getComputedStyle: function(elem) {
         return getComputedStyle(elem);
       }
@@ -251,7 +272,10 @@ describe('$anchorScroll', function() {
         expectScrollingTo(identifierCountMap)($window);
         expect($window.scrollBy.calls.length).toBe(list.length);
         forEach(list, function(offset, idx) {
-          expect($window.scrollBy.calls[idx].args).toEqual([0, -1 * offset]);
+          // Due to sub-pixel rendering, there is a +/-1 error margin in the actual offset
+          var args = $window.scrollBy.calls[idx].args;
+          expect(args[0]).toBe(0);
+          expect(Math.abs(offset + args[1])).toBeLessThan(1);
         });
       };
     }
@@ -270,11 +294,17 @@ describe('$anchorScroll', function() {
     }
 
     function setYOffset(yOffset) {
-      return function ($anchorScroll) {
+      return function($anchorScroll) {
         $anchorScroll.yOffset = yOffset;
       };
     }
 
+    function updateMockPageYOffset() {
+      return function($window) {
+        $window.pageYOffset = window.pageYOffset;
+      };
+    }
+
     beforeEach(inject(setupBodyForOffsetTesting()));
 
     afterEach(inject(function($document) {
@@ -323,16 +353,20 @@ describe('$anchorScroll', function() {
               'padding:0',
               ''].join(';');
 
-            forEach($window.document.body.children, function (elem) {
+            forEach($window.document.body.children, function(elem) {
               elem.style.cssText = cssText;
             });
 
-            // Make sure scrolling does actually take place (it is necessary for this test)
-            forEach(elmSpy, function (spy, identifier) {
+            // Make sure scrolling does actually take place
+            // (this is necessary for the current test)
+            forEach(elmSpy, function(spy, identifier) {
               elmSpy[identifier] = spy.andCallThrough();
             });
           },
           changeHashTo('some2'),
+          updateMockPageYOffset(),
+          resetAllSpies(),
+          callAnchorScroll(),
           expectScrollingWithOffset('id=some2', targetAdjustedOffset));
       });
     });
@@ -380,7 +414,7 @@ describe('$anchorScroll', function() {
 
         var jqElem = jqLite('<div style="' + cssText + '"></div>');
 
-        return function ($anchorScroll, $window) {
+        return function($anchorScroll, $window) {
           jqLite($window.document.body).append(jqElem);
           $anchorScroll.yOffset = jqElem;
         };
