injecting $location rewrites and breaks anchor links

[lanterndev]

Simply injecting $location anywhere in your app breaks anchor links (which up till now have been working for 20 years!). Here is example code demonstrating the issue: https://gist.github.com/skivvies/7125406

Here is a rawgithub link so you can actually run this code: https://rawgithub.com/skivvies/7125406/raw/b42a135b7a40db7dc00a7726fa8efd080e14b7b7/index.html

Please keep an eye on the browser's address bar when you click "link to anchor". Compare this to almost the exact same code, with the only difference being that $location is not injected: https://gist.github.com/skivvies/7125477

Again, here is a rawgithub link to this version so you can run it: https://rawgithub.com/skivvies/7125477/raw/1b4a4d2692616c42b92b813b4c7d1e26ffe38c9b/index.html

Reproduction steps:

1. Open a new browser window directly to an anchor on a page with an Angular app that injects $location, e.g. https://rawgithub.com/skivvies/7125406/raw/b42a135b7a40db7dc00a7726fa8efd080e14b7b7/index.html#anchor
2. Angular hijacks the url's hash and rewrites it to add a leading slash, e.g. https://rawgithub.com/skivvies/7125406/raw/b42a135b7a40db7dc00a7726fa8efd080e14b7b7/index.html#/anchor
3. The browser does not scroll the page down to the linked-to anchor
4. Clicking any links on the page to the anchor (e.g. the "link to anchor" link in the example code) also fails to scroll the page to the anchor

(I hit this because I'm using angular-bootstrap's dropdown toggle directive which injects $location, I don't ever actually inject it into my app, but simply depending on angular-bootstrap is enough to break my anchor links. Thanks to this post for tipping me off. /cc @pkozlowski-opensource)

http://www.benlesh.com/2013/02/angular-js-scrolling-to-element-by-id.html looked like a promising workaround but ultimately didn't work. Update: See comments below.

[jasonswearingen]

i am also seeing this issue.

[joshualinog]

seeing the exact same behavior.

[lanterndev]

Might have some time to hack on a PR for this coming up. Any pointers from the core team on this one before I dive in?

[lanterndev]

Noticed some other $location-related bug fixes have gone into the last couple point releases, nice! I'll try to spend some time on this fix this weekend. Angular team, please ping me if that's not a good idea or if you have any other input.

[lanterndev]

Here is a gist with target="_self": https://gist.github.com/skivvies/8430668

And here is a rawgithub link so you can try it:
https://rawgithub.com/skivvies/8430668/raw/e17b8cae60faaec3a07084d657c0eeaf47f8b3f3/index.html

When the link is clicked, the viewport is successfully scrolled to the
targeted anchor, but the url hash is changed to #/anchor rather than
#anchor, and worse, Angular has now broken the browser's back button. (If
you have a page in your history before this one, clicking the back button
won't take you to it as it should, but rather now does nothing but cause a
momentary flicker.)

So this isn't a complete workaround, but is still somewhat of an
improvement. Thanks for the tip!

On Tue, Jan 14, 2014 at 9:38 PM, Llyle van Schalkwyk <
notifications@github.com> wrote:

It seems that using target="_self" works around the issue.

—
Reply to this email directly or view it on GitHubhttps://github.com//issues/4608#issuecomment-32329769
.

[benlesh]

It's probably because, when not in html5Mode, the $location is assuming that you're using Angular's routing and treating all "#" links as such.

So there are two options/workarounds:

1. If you use an ng-click to update the $location.hash and call $anchorScroll in a $timeout, it seems to fix the issue: http://plnkr.co/edit/nzTBmWFKOMVkxrcJSiPW?p=preview
2. Use html5Mode, and that also solves the issue: http://plnkr.co/edit/qAH0bPAMdUKRYblW6I6k?p=preview

[benlesh]

I guess what I'm saying is, that at the point that $location is being used, if you're not in HTML5 mode, everything after the "#" is Angular routing's domain, and it's going to behave differently than what you might expect.

[lanterndev]

Thanks for the comments, @Blesh.

I think the first proposed workaround of changing every href to an ng-click which updates $location.hash and calls $anchorScroll in a $timeout is ugly. It requires undoing your valid html for an unrobust attempt at reimplementing the correct behavior in javascript. Given this has been working in browsers -- without needing javascript -- for the last 20 years, I think an acceptable workaround would have to not go against the grain of the web so drastically. Let's not make Tim Berners-Lee want to come kill us in the night!

As for the second proposal, I tested it, and it's not sufficient to just set html5Mode to true, you also have to update all your anchor links to add target="_self" for it to work, as you've done in your plunker. This, at least, is a complete workaround (i.e. with only target="_self" and no html5Mode, the url hash would incorrectly be prefixed with /, as noted earlier), and doesn't break the web the way the first proposal does. It's too bad you have to update every anchor link in your application with some magic markup that looks like it doesn't accomplish anything to someone else who looks at the code (or yourself, 3 months later), but IMO it's far better than living with the bug or using the first proposal.

Thanks again for the comments. And still hoping to take a crack at a patch some time if no one else beats me to it.

[lanterndev]

Just wanted to add, another metric for a workaround is what you would do once the bug being worked around is actually fixed. With the first proposal, to benefit from the bug being fixed, you'd have to change all the ng-clicks back to proper hrefs. With the second, you could still benefit from the fix without having to take out all the target="_self" attributes, which could be done at a later time if there was more pressing work to do.

[benlesh]

I'm not sure how this could be patched without a large refactor of routing, which might be messy. Best of luck.

[benlesh]

I could be wrong of course, I haven't looked too closely at that code.

[benlesh]

Okay, so there's one more workaround...

<a href="#/#anchor">Go to the id, "anchor"</a>

I got to thinking about it, and I realized, $location is only to be used with routing, really, and when you're routing you can scroll to ids on a page with links by a "hash" on the route. so the link url is like http://hostname/path/to/app/#/angular/route/#hashOnPage, which is the same as /path/to/app/#/angular/route/#hashOnPage where hashOnPage is the ID of what you want to scroll to. Depending on where you're at, where the app lives, and what route your on in it, you can use that, plust a second # to add a hash to something on the screen... If that makes sense.

Does that more effectively solve your problem?

[benlesh]

In the end, you really shouldn't use $location unless you're using routing. Since that's sort of what it's built around.

[lanterndev]

Thanks for the additional idea, @Blesh. In the case where I hit this, I wasn't using routes, and had no need for $location at all. I was simply using angular-bootstrap for something totally unrelated to routing. However, since one of its directives used $location, it spread like a virus, breaking anchor links not just within my Angular app, but even to ones outside the DOM element with ng-app applied. Ideally the area affected by using $location would be more constrained, so it wouldn't affect code in far-removed and unsuspecting places.

So your second workaround is still the best fit for this case, where you have no need for $location or routing, but some dependency using it is messing up your anchor links.

It looks like angular-bootstrap is no longer using $location as of angular-ui/bootstrap@35c93076 (which just might also fix angular-ui/bootstrap#619 -- see how $location's current behavior can cause bugs like this everywhere?), so for other angular-bootstrap users who don't need $location themselves (show of hands?), upgrading to the latest version should also get rid of this bug, as long as nothing else they touch uses $location.

Thanks also for your warnings that fixing this would entail a large refactor of routing, which may be messy. If that's true, it probably makes sense for a core developer to chime in on this thread before a less experienced community contributor spends a lot of time on a patch.

[jasonswearingen]

any updates on resolving this? I also have module dependencies that include $location so all my anchor links are broken.

[lanterndev]

Still hoping to hear from a core dev before taking a shot at a PR since it sounds like it will require coordination to get merged.

Also just noticed that the milestone was changed from "1.2.x" to "backlog" at some point. Does that mean this won't make it into a release any time soon? /cc @btford

[caitp]

@Skivvies, the backlog milestone really just means we aren't actively pushing to get it into the next release, but if anyone in the community cares to investigate and provide more information about this, we can make a decision about how serious the issue is and prioritize it.

A good start with this is to provide a test in angular core which fails, reproducing the issue in a minimal fashion. Otherwise, provide a reproduction of the issue on plnkr or jsbin or something. I think in an issue tracker like chromium you'd label this "unconfirmed", we really need to see that this isn't working as expected.

(This is just my personal opinion and does not necessarily reflect the opinion of the Angular team)

[jasonswearingen]

fyi this is my workaround. back links still work, and no need to change my html.

the only issue is that it creates verbosity in the querystring. ex: mySite.com/page.html#/anchor#anchor

put inside of $scope-on-location-change-start:

if($location.hash() && $location.hash().length>=1)
{
var path = $location.path();
var potentialAnchor = path.substring(path.lastIndexOf("/")+1);
if ($("#" + potentialAnchor).length > 0) {                          
    $location.hash(potentialAnchor);
    $anchorScroll();
}
}

edit: and i tested it down to ie8, still works there.

[lanterndev]

@caitp wrote:

A good start with this is to provide a test in angular core which fails, reproducing the issue in a minimal fashion. Otherwise, provide a reproduction of the issue on plnkr or jsbin or something. I think in an issue tracker like chromium you'd label this "unconfirmed", we really need to see that this isn't working as expected.

Sounds like you may have missed the ticket description. You'll see links there to a minimal reproduction. This issue is definitely not unconfirmed. I believe @btford confirmed it, and was the one who originally slated it for 1.2.x.

Submitting a PR with a failing unit test is a great idea though. I spent a lot of time preparing the description of the problem already. Can anyone else step up and write the test?

Thanks.

@jasons-novaleaf wrote:

fyi this is my workaround. back links still work, and no need to change my html. the only issue is that it creates verbosity in the querystring. ex: mySite.com/page.html#/anchor#anchor

If you use this workaround, unless you want to break your links after the issue is fixed (or possibly write legacy code to redirect them back), you'll have to commit to continuing to use these verbose query strings even after the issue is fixed. So I prefer the workaround of adding target="_self" to anchor links and making sure html5Mode is true. That way you get nice, clean, normal-looking anchor links both before and after the fix. (And the back button still works.)

Thank you for sharing, though!

[caitp]

@Skivvies, actually I've been paying attention to this thread for some time now, and I still haven't really been convinced of anything truly confirming this. I still can't decide if this is an application error or not, which is why I say I need to see a more minimal, exact reproduction.

But yeah, the reason you aren't seeing a PRs plz! status is generally because this hasn't been shown to be a real problem yet. If we know for sure that this is an issue, then we'll definitely be saying "please contribute a fix for this, we might not be working hard to get this into the next release, but if someone comes up with a fix that doesn't break other things badly, they're probably good"

Again, I'm paraphrasing, but this is really what I've been seeing from this issue for the past few months.

[lanterndev]

Wow, ok, this is totally coming as a surprise to me. https://gist.github.com/skivvies/7125406 looks to me like as minimal a reproduction as you can get. And the behavior when you run that seems incontrovertibly broken.

I don't know how this could be any more minimal or any more clearly broken, but if you could help me to understand, I'd be happy to do more to make a clearer case.

At the very least, would you agree the behavior of https://gist.github.com/skivvies/7125406 is unexpected enough to warrant some documentation warning users about it?

[lanterndev]

Take out $location and it's fine, add it back in (or depend on something (that depends on something...) that adds it in), and all of a sudden your app's anchor links are broken, and you probably have no idea why. This has clearly bitten all the people that chimed in here, and seems to me like the definition of a subtle and undesired bug.

[caitp]

I guess you could say it's a bug not to instantiate $location on bootstrap, but that's sort of unrelated

[lanterndev]

Yes it is, since you'll still see the bug if you instantiate $location on
bootstrap. So not doing so makes for a more minimal reproduction.

On Fri, Feb 7, 2014 at 1:19 PM, Caitlin Potter notifications@github.comwrote:

I guess you could say it's a bug not to instantiate $location on
bootstrap, but that's sort of unrelated

Reply to this email directly or view it on GitHubhttps://github.com//issues/4608#issuecomment-34482945
.

[caitp]

Instantiating location on bootstrap wouldn't make the link to the named anchor work as expected either, though, because the rewritten URL falls within the SPA and prevents the default behaviour. In html5mode we would see the hash fragment correctly (the talks about melding $anchorScroll into $location could fix this), but it would work inconsistently from hashbang mode, which is why that is sort of problematic

[lanterndev]

Instantiating location on bootstrap wouldn't make the link to the named anchor work as expected either

Right, that's what I meant by "you'll still see the bug if you instantiate $location on bootstrap", but thanks for the additional info and clarity. Good to make it clearer that the workaround that requires setting html5Mode to true won't work on browsers that don't support html5Mode.

Interesting there's talk of merging $anchorScroll and $location, hadn't heard that.

[shawn-simon]

This should really be fixed. Didn't realizing adding angular bootstrap to my site would break our site navigation. I can't seem to figure out a workaround either.

[RichardLitt]

Aight. Shall I open a ticket for that?

[caitp]

sure

[samccone]

Is there any update on this bug, and or is the core team still open to a PR to fix this functionality (even tho this is a wildly breaking change)?

[caitp]

@samccone, @tbosch provided a fix for this.

[samccone]

@caitp is there a link to the resolved solution? I must be missing it. Thanks

[caitp]

2294880 (and a bunch of other related commits) are the big fix

[caitp]

Wait, which bug are we talking about? That's the "big" location bug, but there are lots of other ones as well, many have been fixed

[samccone]

ah yes it is fixed there, just waiting on the 1.2.x release of that.

Thanks!

[Rutix]

Oh! >.< I spend 2 days figuring out why it didn't work. I am glad this is also coming to 1.2.x 👍 . Any idea when it is going to be released though?

[jainarpit014]

set target="_self" in every anchor tag.This workaround is working fine for me.

[jeromebrule]

set target="_self" fix my problem. thx :)

[wobine]

setting target="_self" only fixes the problem internally to the site itself. If you want to link from outside to an anchor link, there is still that horrible slash (angular's hijacking) sitting right inside your hyperlink. Its terrible!

[wobine]

I'm using version 1.3.9 and it doesn't work yet. Why are folks saying it was fixed in 1.2??

[Rutix]

@wobine We didn't say it was fixed in 1.2 . It has been fixed and it will probably be merged into 1.2.x but that hasn't happened yet.

[iobaixas]

I managed to get anchors working correctly using the following setup (it only considers id anchors and probably messes up angular routing for more complex use cases)

module.config([ '$anchorScrollProvider', function($anchorScrollProvider) {
    $anchorScrollProvider.disableAutoScrolling();
}])
.run(['$rootScope', '$location', function($rootscope, $location) {
    // mimic the anchor behaviour, avoid using the $anchorScroll service
    $rootscope.$on('$locationChangeSuccess', function() {
        var element = $('#' + $location.url().replace('/', ''));
        if(element.length > 0) element[0].scrollIntoView();
    });
}]);

[manutalcual]

Hi,

I've found a solution to this nightmare, but I'm new to AngularJS and don't know if it is a correct solution.
I've enabled html5mode like this:

saApp.config (function($locationProvider) {
    $locationProvider.html5Mode({
    enabled : true,
    requireBase: false,
    rewriteLinks : false
    });
});

Now all links work as expected and there are no errors on js.

[lydiastepanek]

I got around this by using a ng-click event with a window.open(url) command:

<a class="viewLink" href ng-click="linkViewed()">

$scope.mediaViewed = function linkViewed() { window.open($scope.link) }

[cwi-ezoutas]

I can confirm that manutalcual answer works as expected with AngularJS v1.3.13.

The only problem is that when I directly load e.g. http://site.com/#test and I click again on #test the final URL is http://site.com/#test#test.

[brunoamancio]

@manutalcual, your solution solved it.

[tsibley]

Another +1 for @manutalcual's solution. Given the implication of angular-bootstrap, it's also probably worth noting that I'm using 0.13.0 of that, which includes this change mentioned in the conversation above.

[cesardariogar]

#14315 $anchorScroll breaks anchor links with html5mode enabled

[VincentLoy]

This is my workaround:
Stop event propagation directive with high priority

var app = angular.module('my.module', []);

app.directive('externalAnchorLink', [function () {
    return {
        restrict: 'A',
        priority: 0,
        link: function (scope, el) {
            /*jslint unparam: true*/
            el.on('click', function (event) {
                event.stopPropagation();
            });
        }
    };
}]);

Then in HTML

<a href="/test/example/#my-anchor" external-anchor-link>link text</a>

[gkalpak]

Regarding the original problem ($location affecting the default behavior of anchor links):

Indeed Angular uses # (potentially with a hash-prefix after #) to identify a "client-side" route (in non-HTML5 mode). It is $location's job to interact with hash part of the URL. So, since it is not possible to distinguish a #some-client-side-route-path for #i-want-to-scroll-to-an-element, it has to always assume it is a client-side route path.

There are several solutions available (most of them discussed above) - especially if you don't use $location/client-side routing in your app (but might have dependencies that instantiate it):

1. Use an empty path and a client-side route hash: href="##i-want-to-scroll-to-an-element".
2. Configure $locationProvider to use HTML5 mode
3. Change the hash-prefix (which is empty by default), so that #foo is not recognized as a client-side route path. E.g. with $locationProvider.hashPrefix('!'), a client-side route needs to start with #! (e.g. #!foo). Thus #foo is not recognized as a path and the normal browser behavior applies.

Note: Starting with v1.6 the default hash-prefix will be !, so OP's usecase will work out-of-the-box (without the need to manually call $locationProvider.hashPrefix('!')). In older versions, you need to configure it yourself.

I understand there may be several roblems discussed in this issue, but this does not help with assessing, investigating and hopefully fixing them, so (since the original problem falls under the fixed/works as expected category), I am going to close this issue. If you are still running into different problems (that are not already reported), please open separate issues.