Proposal: Merge the $anchorScroll service into $location

[gsklee]

This service only offers two things as it currently stands:

• Make the browser be aware of a URL hash change and jump to that anchor; it's like the $scope.$apply() for the URL hash.

  Though unlike $scope.$apply(), which exists so we can work with any arbitrary JavaScript code, I don't see the reasoning in this case: if an element which explicitly says href="#anchor" or $location.hash('anchor') is clicked, and there exists exactly such an anchor named id="anchor" within the document, I fail to see any scenario where this _is not_ implying for an internal link, because this is the way hyperlink has been working since eons.

• Provide a watch mechanism and setting which is very confusing in its current implementation.

  • If I leave the default setting as is, the watch mechanism can be enabled by simply DI'ing $anchorScroll into any place in the code:

    controller('MainCtrl', function(
        $anchorScroll,
        $location
    ){
        this.click = function() {
            $location.hash('anchor');
            // No nothing here! The browser will jump to the `anchor` section automatically.
        };
    })

  • Or, if you happen to have used ngInclude within your template, you don't even need to inject it because ngInclude has done that for you:

    controller('MainCtrl', function(
        $location
    ){
        this.click = function() {
            $location.hash('anchor');
            // No nothing here! The browser will jump to the `anchor` section automatically.
        };
    })

  • Of course, you won't wanna write your code like above with such obscurity, so chances are you'll be authoring your code like this (which I assume is the recommended usage because this is how the doc has it):

    controller('MainCtrl', function(
        $anchorScroll,
        $location
    ){
        this.click = function() {
            $location.hash('anchor');
            $anchorScroll();
        };
    })

    Now if you actually go check it you'll find that you're performing the click two times, one by you and the other the watch mechanism.

  • So if you wanna get everything right (better coding style, only click once), you gotta have it this way:

    config(function(
        $anchorScrollProvider
    ){
        $anchorScrollProvider.disableAutoScrolling();
    }).
    
    controller('MainCtrl', function(
        $anchorScroll,
        $location
    ){
        this.click = function() {
            $location.hash('anchor');
            $anchorScroll();
        };
    })

    Btw, the name of the provider setting doesn't really match the API naming convention as well - think $locationProvider.html5Mode(true).

---

For reasons above I'm proposing a breaking change which will fulfill the following:

• If an element, which explicitly has href="#anchor" or $location.hash('anchor') attached to it, is clicked, and there exists exactly such an anchor name within the document, $location will treat that as an internal page link and jump to that section. Correct implementation will address $location should not intercept #links (in html4 mode) #772.
• A new setting, $locationProvider.anchorScroll(), which can be set as 'normal' or 'smooth' (because I've always been thinking this is what $anchorScroll does, very disappointed to find out the truth yesterday 🙀). The 'smooth' part can be implemented later.
• Another new setting, $locationProvider.anchorScrollOffset(), which addresses $anchorScroll Offset #2070. This can also be implemented later.
  • Alternatively, put these two settings under a separate provider: $anchorProvider.scroll() and $anchorProvider.scrollOffset(). Like this one better but then we're left with a service that does nothing.

---

Would like to look for some input before I send my PR, thanks!

[IgorMinar]

This sounds quite good. Thanks for the thorough explanation.

I scheduled this for 1.3.x and I'd love for you to work on it BUT now is not the time.

There are still bugs we want to fix in 1.2 before forking.

Making a PR now would likely result in a sad rebase later when we work on 1.3

Can you send us a PR when we announce that we branched 1.2.x and are starting to work on 1.3?

Thanks a lot!

[gsklee]

@IgorMinar got it, thanks for explaining!

[tabanliviu]

+1 for $anchorProvider. Potentially later there could be a more than just settings in the provider, there could be a event registering for ex.

[carlosrymer]

+1

[albertboada]

+1

Having had a hard time understanding how the $anchorScroll service exactly worked, I find this refactor proposal very refreshing and necessary.

Worried, tho, about how all this would affect the way ngRoute handles direct navigation to urls with anchors (i.e. #/posts#3: (1) navigates to #/posts first, and (2) scrolls to anchor #3). Right now, this is possible because $anchorScroll is called after updating the view (step (1)).

[petebacondarwin]

@gsklee are you still interested in implementing this? Sorry it has taken a while to get back to it. We are about to start 1.5 so now would be a perfect time for this change.

[petebacondarwin]

It seems that there is no interest in implementing this refactoring.