fix($location): $locationchangesuccess not fires with browser back when path ends with '/#'. #12175

[rrsivabalan]

In line #930, $location.absUrl() returns "path/", and newUrl returns "path/#". with browser back, so if condition is positive and returns. For fix I used trimEmptyHash function to trim # from newUrl, the same function is used in browser forward scenario @line#947.

[rrsivabalan]

This Pull Request fixes #12175

[rrsivabalan]

@petebacondarwin Can you please review changes(closes #12175)

[petebacondarwin]

@rrsivabalan - thanks for this PR.
First, it needs a unit test to demonstrate the bug you are fixing.
Also, it seems wasteful to trim the hash from the newUrl twice. Shouldn't we just do it once and store it in a variable?

[gggritso]

This issue has been stale for about a month now, I'm hoping to get it pushed through; this is a somewhat important problem for me. I think this unit test shows the bug:

it('should should detect location changes to bare hash',
  inject(function($location, $browser, $rootScope, $log) {

    $rootScope.$on('$locationChangeSuccess', function(event, newUrl, oldUrl) {
      $log.info('changed', newUrl, oldUrl);
    });

    $browser.url('http://server/#/');
    $browser.poll();

    expect($log.info.logs.shift()).
      toEqual(['changed', 'http://server/#/', 'http://server/']);

    $browser.url('http://server/#');
    $browser.poll();

    expect($log.info.logs.shift()).
      toEqual(['changed', 'http://server/#', 'http://server/#/']);

    expect($browser.url()).toEqual('http://server/#');
  })
);

Unfortunately, I don't think the code in this PR correctly covers this case. I would love to submit a PR myself, but right now I don't have enough Angular knowledge to propose a good solution. Is there anything else I can do to help out here?

[petebacondarwin]

We are working our way through the 1.4.x bugs

[gggritso]

That’s great to hear, thank you for your hard work!

[rrsivabalan]

@petebacondarwin sorry for delay, i have fixed code review comments and added UT to demonstrate issue
New PR: #13251
Please note that i got merge conflict issue in existing PR, so raised new PR
@gggrisot my UT is pretty similar to your UT, i think my PR covers your test case.