Skip to content
This repository was archived by the owner on Apr 12, 2024. It is now read-only.

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

Closed
wants to merge 1 commit into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 24 additions & 6 deletions src/ng/location.js
Original file line number Diff line number Diff line change
Expand Up @@ -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) === '/' ?
Expand Down Expand Up @@ -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);
Expand All @@ -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);

Expand Down Expand Up @@ -675,7 +676,8 @@ function $LocationProvider() {
enabled: false,
requireBase: true,
rewriteLinks: true
};
},
fixHashFragmentLinks = false;

/**
* @ngdoc method
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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();
Expand Down
12 changes: 12 additions & 0 deletions test/e2e/fixtures/ng/location/hashFragmentScrolling/README.md
Original file line number Diff line number Diff line change
@@ -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);
}
```
13 changes: 13 additions & 0 deletions test/e2e/fixtures/ng/location/hashFragmentScrolling/index.html
Original file line number Diff line number Diff line change
@@ -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>
7 changes: 7 additions & 0 deletions test/e2e/fixtures/ng/location/hashFragmentScrolling/script.js
Original file line number Diff line number Diff line change
@@ -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
});
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
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() {
Copy link
Contributor

Choose a reason for hiding this comment

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

This test is causing an error in Travis (and looks like it's causing all subsequent tests to fail).

Copy link
Author

Choose a reason for hiding this comment

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

Does this reproduce? A similar line already exists in this test, so I don't see a reason why it would break here...

var initialScrollTop = null;
// Firefox requires window.pageYOffset (document.body.scrollTop is always 0)
browser.executeScript('return document.body.scrollTop||window.pageYOffset;').then(function(scrollTop) {
initialScrollTop = scrollTop;
});
element(by.id('click-me')).click();
browser.executeScript('return document.body.scrollTop||window.pageYOffset;').then(function(scrollTop) {
expect(scrollTop).toBeGreaterThan(initialScrollTop);
});
expect(browser.executeScript('return document.location.hash;')).toBe('##some-section');
});
});
28 changes: 28 additions & 0 deletions test/ng/locationSpec.js
Original file line number Diff line number Diff line change
Expand Up @@ -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() {
Expand Down