fix($location/$browser): prevent infinite digests on empty hash changes

[petebacondarwin]

Fixes #9635

[petebacondarwin]

@tbosch is unavailable to to review.
@caitp - Can you take a look at this - I've managed to add unit tests.

[petebacondarwin]

Actually @lgalfaso - here is one that you could review for me.

[lgalfaso]

There is one thing that I do not fully like about this patch. Even when
there is no such a thing as empty fragment, browsers behave slightly
different when moving from some-url#foo to some-url and some-url#. In
the first case they stay at the page position that they are and in the
second they move to the top

This is, by adding a # at the end, we may end up with a situation that
the user is not expecting, this is that the page position moves to the top

[tbosch]

AFAIK: moving from some-url#foo to some-url always does a page reload
On Thu, Nov 6, 2014 at 8:01 AM Lucas Galfasó [email protected]
wrote:

There is one thing that I do not fully like about this patch. Even when
there is no such a thing as empty fragment, browsers behave slightly
different when moving from some-url#foo to some-url and some-url#. In
the first case they stay at the page position that they are and in the
second they move to the top

This is, by adding a # at the end, we may end up with a situation that
the user is not expecting, this is that the page position moves to the top

—
Reply to this email directly or view it on GitHub
#9923 (comment).

[petebacondarwin]

I believe that @tbosch is correct. This is the main cause of the problem. When we go from a URL with a hash to one without the browser does a page reload but doesn't update the location.href (since the page will be reloading anyway - it believes).

In non-HTML5 mode we always leave a # symbol in place so the page does not reload.

In this case, I don't think that this fix is a breaking change unless someone is trying to force a page reload by removing the hash from the URL. Previously removing the hash would cause a page reload and so the browser would reset to the top of the page anyway.

The workaround if you intended to reload the page would be to add an additional call to location.reload();

What do you think?

[lgalfaso]

@petebacondarwin is this still the case when we do not really go to the page but just push the new url to the browser history?

One more question: Will people think that always adding a # to the end pollutes the URL?

[lgalfaso]

I think this will not do the right thing for URLs of the form http://example.com/foo#bar# (check that there are two #)

[petebacondarwin]

Hmm. Good point.

[petebacondarwin]

Although strictly unescaped # characters are not allowed inside fragments, I guess we have to deal with that case.

[lgalfaso]

I agree that strictly speaking, this is not supported, but I tested this with Chrome and Safari and in both cases location.hash was unescaped

[petebacondarwin]

One more question: Will people think that always adding a # to the end pollutes the URL?

Good question. I was wondering about this myself. I think we could try to be clever and only add the empty hash in if we are moving to a URL that has the same base and no hash, while the original did have a hash fragment?

[lgalfaso]

@petebacondarwin if the # was added only when moving from a URL that already had a fragment, then that would be perfect

[petebacondarwin]

I will try to implement that tomorrow but unfortunately I am running a workshop all day so I might miss the 1.3.2 cut-off

[NevilleS]

Hey there, I just wanted to mention that this seems to happen when you navigate to any URL with an empty hash, not just when making a change. It might already be covered by this fix, but I'm not sure - the test cases seem to test things like /path#123 --> /path#, whereas I can reproduce the issue by just navigating to /#. You can see this happening at https://docs.angularjs.org/api#, for example.

[petebacondarwin]

This does appear to be fixed by this: https://ci.angularjs.org/job/angular.js-pete/711/artifact/build/docs/api#

[NevilleS]

OK good 😄

[petebacondarwin]

if the # was added only when moving from a URL that already had a fragment, then that would be perfect

@lgalfaso - actually this is what the code already does!

Do we really need to cope with unescaped # chars in the fragment in HTML5 mode? It would only be a make a difference if moving from a/b/c#frag# to a/b/c#frag. The new algorithm would assume these to be the same URL and not update the browser URL or hash.

[Narretz]

There is one case which is fixed by #9903, but not by this PR:

• hashbang mode must be enabled
• you need a hashprefix
• your first route is this: http://localhost:1337/ (angular will not add #! to this)
• now you link to a route that is http://localhost:1337/#!/whatever
• in this route you link back with <a href="/">Home</a>'
• before this bug, angular would keep '#!'; since this bug the hash and hashPrefix are stripped -> infinite digest
• with fix($location/$browser): prevent infinite digests on empty hash changes #9903, it works like before
• with this PR, the hash stays, but not the path prefix, so angular will throw Error: [$location:ihshprfx] Invalid url "http://localhost:1337/#", missing hash prefix "#!"

I don't know if the previous behavior was explicitly supported, but it made sense to me, since initial loading of a site works without !#.

[lgalfaso]

Do we really need to cope with unescaped # chars in the fragment in HTML5 mode? It would only be a make a difference if moving from a/b/c#frag# to a/b/c#frag

The main issue is that the browser does not escape the second # (at least Chrome nor Safari do). The code will not be any less efficient as something like return url.replace(/(#.+)|#$/, '$1'); should make it

[petebacondarwin]

@lgalfaso - OK, I will change the regex.
@Narretz - I think this is a bug in LocationHashbangUrl or LocationHashbangInHtml5Url highlighted by fixing $browser. In hashbang mode, we ought to catch changes to the $location that would cause the hashbang to be stripped unnecessarily and add it back in before it gets to the $browers.url method. What do you think?

[petebacondarwin]

• in this route you link back with <a href="/">Home</a>

I believe that this link should be rewritten to <a href="/#!'>Home</a>??

In other words, is this test actually invalid: https://github.com/angular/angular.js/blob/master/test/ng/locationSpec.js#L1972-L1978

    it("should not set hash if one was not originally specified", function() {
      location = new LocationHashbangUrl('http://server/pre/index.html', '#');

      location.$$parse('http://server/pre/index.html');
      expect(location.url()).toBe('');
      expect(location.absUrl()).toBe('http://server/pre/index.html');
    });

[petebacondarwin]

Actually, @Narretz - this PR doesn't actually have the problem you describe. See https://gist.github.com/petebacondarwin/3a3bab60cb00941874f4

[Narretz]

@petebacondarwin Okay, I will test again and see what the difference between your gist and my test is.

[petebacondarwin]

In my Gist, the browser actually does a reload - because the new URL without the hash bang is treated as outside the app.

Is this desired? It seems to me that this is about right. If you want to stay within the application you should include the hash bang. But this may not be what people expect.

[lgalfaso]

In my Gist, the browser actually does a reload - because the new URL without the hash bang is treated as outside the app.

Is this also the case in html5 mode?

[petebacondarwin]

@lgalfaso - so in HTML5 mode it is a little different as we have no hash-bang situation but it does appear that going from http://some/url# to http://some/url does cause a page reload. I am now confused as to whether this is expected or not.

[petebacondarwin]

OK, so that reload does not happen in 1.2.20 so it should not happen here. It may be that a variant on #9903 is better

[lgalfaso]

LGTM

[petebacondarwin]

This doesn't actually fully solve the problem either. Check back on #9635 for further analysis