fix($location): fix handling of hash fragment links

urish · urish · commit 6172383e8112 · 2014-11-22T14:57:42.000-08:00
This commit adds a new `fixHashFragmentLinks` method to `$locationProvider`. This method fixes incorrect rewriting of hash fragment links. In order to use in your project, you need to enable the new behavior: ```js myApp.config(function($locationProvider) { $locationProvider.fixHashFragmentLinks(true); }); ``` In addition, you need to inject the `$anchorScroll` service to one of your controllers/services, to enable automatic anchor scrolling. Closes angular#8675
diff --git a/src/ng/location.js b/src/ng/location.js
@@ -31,10 +31,11 @@ function parseAbsoluteUrl(absoluteUrl, locationObj) {
 }
 
 
-function parseAppUrl(relativeUrl, locationObj) {
+function parseAppUrl(relativeUrl, locationObj, fixHashFragmentLinks) {
   var prefixed = (relativeUrl.charAt(0) !== '/');
+  var hashPrefix = (fixHashFragmentLinks && relativeUrl.charAt(0) !== '#') ? '#' : '';
   if (prefixed) {
-    relativeUrl = '/' + relativeUrl;
+    relativeUrl = '/' + hashPrefix + relativeUrl;
   }
   var match = urlResolve(relativeUrl);
   locationObj.$$path = decodeURIComponent(prefixed && match.pathname.charAt(0) === '/' ?
@@ -166,7 +167,7 @@ function LocationHtml5Url(appBase, basePrefix) {
  * @param {string} appBase application base URL
  * @param {string} hashPrefix hashbang prefix
  */
-function LocationHashbangUrl(appBase, hashPrefix) {
+function LocationHashbangUrl(appBase, hashPrefix, fixHashFragmentLinks) {
   var appBaseNoFile = stripFile(appBase);
 
   parseAbsoluteUrl(appBase, this);
@@ -189,7 +190,7 @@ function LocationHashbangUrl(appBase, hashPrefix) {
       throw $locationMinErr('ihshprfx', 'Invalid url "{0}", missing hash prefix "{1}".', url,
           hashPrefix);
     }
-    parseAppUrl(withoutHashUrl, this);
+    parseAppUrl(withoutHashUrl, this, fixHashFragmentLinks);
 
     this.$$path = removeWindowsDriveName(this.$$path, withoutHashUrl, appBase);
 
@@ -675,7 +676,8 @@ function $LocationProvider() {
         enabled: false,
         requireBase: true,
         rewriteLinks: true
-      };
+      },
+      fixHashFragmentLinks = false;
 
   /**
    * @ngdoc method
@@ -736,6 +738,22 @@ function $LocationProvider() {
     }
   };
 
+  /**
+   * @ngdoc method
+   * @name $locationProvider#fixHashFragmentLinks
+   * @description
+   * @param {boolean=} [enable=false] If set to true, will replace hashes in the URL with double hashes
+   * @returns {*} current value if used as getter or itself (chaining) if used as setter
+   */
+  this.fixHashFragmentLinks = function(value) {
+    if (isDefined(value)) {
+      fixHashFragmentLinks = !!value;
+      return this;
+    } else {
+      return fixHashFragmentLinks;
+    }
+  };
+
   /**
    * @ngdoc event
    * @name $location#$locationChangeStart
@@ -794,7 +812,7 @@ function $LocationProvider() {
       appBase = stripHash(initialUrl);
       LocationMode = LocationHashbangUrl;
     }
-    $location = new LocationMode(appBase, '#' + hashPrefix);
+    $location = new LocationMode(appBase, '#' + hashPrefix, fixHashFragmentLinks);
     $location.$$parseLinkUrl(initialUrl, initialUrl);
 
     $location.$$state = $browser.state();
diff --git a/test/e2e/fixtures/ng/location/hashFragmentScrolling/README.md b/test/e2e/fixtures/ng/location/hashFragmentScrolling/README.md
@@ -0,0 +1,12 @@
+Regression test for [#8675](https://github.com/angular/angular.js/issues/8675).
+
+Makes sure that hash fragment links actually jump to the relevant document fragment when `$location`
+is injected and configured to operate in hashbang mode. In order to use this fix, you need to inject
+the `$anchorScroll` service somewhere in your application, and also add the following config block
+to your application:
+
+```js
+function($locationProvider) {
+  $locationProvider.fixHashFragmentLinks(true);
+}
+```
diff --git a/test/e2e/fixtures/ng/location/hashFragmentScrolling/index.html b/test/e2e/fixtures/ng/location/hashFragmentScrolling/index.html
@@ -0,0 +1,13 @@
+<!DOCTYPE html>
+<html ng-app="test">
+<div ng-controller="TestCtrl">
+    <a id="click-me" href="#some-section">Click me</a>
+
+    <div style="height:9999px;"></div>
+
+    <h1 id="some-section" style="height:500px">Should scroll here</h1>
+</div>
+
+<script src="/build/angular.js"></script>
+<script src="script.js"></script>
+</html>
diff --git a/test/e2e/fixtures/ng/location/hashFragmentScrolling/script.js b/test/e2e/fixtures/ng/location/hashFragmentScrolling/script.js
@@ -0,0 +1,7 @@
+angular.module("test", [])
+  .config(function($locationProvider) {
+    $locationProvider.fixHashFragmentLinks(true);
+  })
+  .controller("TestCtrl", function($scope, $anchorScroll) {
+    // $anchorScroll is required for handling automatic scrolling for hash fragment links
+  });
diff --git a/test/e2e/tests/ng/location/hashFragmentScrolling/hashFragmentScrollingSpec.js b/test/e2e/tests/ng/location/hashFragmentScrolling/hashFragmentScrollingSpec.js
@@ -0,0 +1,15 @@
+describe('Hash Fragment Scrolling', function () {
+  beforeEach(function () {
+    loadFixture("ng/location/hashFragmentScrolling").andWaitForAngular();
+  });
+
+  it('should scroll to the element whose id appears in the hash part of the link', function () {
+    var initialScrollTop = null;
+    browser.executeScript('return document.body.scrollTop;').then(function (scrollTop) {
+      initialScrollTop = scrollTop;
+    });
+    element(by.id('click-me')).click();
+    expect(browser.executeScript('return document.body.scrollTop;')).toBeGreaterThan(initialScrollTop);
+    expect(browser.executeScript('return document.location.hash;')).toBe('##some-section');
+  });
+});
diff --git a/test/ng/locationSpec.js b/test/ng/locationSpec.js
@@ -2151,6 +2151,34 @@ describe('$location', function() {
       expect(location.absUrl()).toBe('http://server/pre/index.html#/not-starting-with-slash');
     });
 
+    describe('fixHashFragmentLinks(true)',function() {
+      it("should prefix hash url with another hash sign if it did not start with a /", function () {
+        location = new LocationHashbangUrl('http://server/pre/index.html', '#', true);
+
+        location.$$parse('http://server/pre/index.html#not-starting-with-slash');
+        expect(location.url()).toBe('#not-starting-with-slash');
+        expect(location.path()).toBe('');
+        expect(location.hash()).toBe('not-starting-with-slash');
+        expect(location.absUrl()).toBe('http://server/pre/index.html##not-starting-with-slash');
+      });
+
+      it("should not modify the url is it already starts with a /", function () {
+        location = new LocationHashbangUrl('http://server/pre/index.html', '#', true);
+
+        location.$$parse('http://server/pre/index.html#/starting-with-slash');
+        expect(location.url()).toBe('/starting-with-slash');
+        expect(location.path()).toBe('/starting-with-slash');
+        expect(location.hash()).toBe('');
+        expect(location.absUrl()).toBe('http://server/pre/index.html#/starting-with-slash');
+      });
+
+      it("should not append another hash sign if the existing url already starts with a hash sign", function () {
+        location = new LocationHashbangUrl('http://server/pre/index.html', '#', true);
+        location.$$parse('http://server/pre/index.html##digesting-unicorn');
+        expect(location.path()).toBe('');
+        expect(location.hash()).toBe('digesting-unicorn');
+      });
+    });
 
     it('should not strip stuff from path just because it looks like Windows drive when it\'s not',
         function() {
