Angular.js: $location issue for decoded and unencoded urls

[dmartres]

I'm submitting a ...

• [x] bug report
• [ ] feature request
• [ ] other (Please do not submit support requests here (see above))

Current behavior:

Hi guys.
After a discussion in a PR that I've created to fix an error in $location service for url that contains apostrophes @gkalpak notice that potencially apostrophes in url is not the main problem instead the problem is using decoded and undecoded urls.

see the PR for more details. https://github.com/angular/angular.js/pull/16098

Expected / new behavior:
undecoded and decoded url should work using $location service

Angular version: 1.x.

Browser: all

Anything else:

How to fix it

In src/ng/browser.js there is a function fireStateOrUrlChange which has this validation

if (lastBrowserUrl === self.url() && prevLastHistoryState === cachedState) {{
return;
}
The main problem seems to be that we are comparing self.url() to lastBrowserUrl, but lastBrowserUrl can be set from different sources, not always with the same encoding/decoding

[jbedard]

@gkalpak you mean (from your comment) doing something such as url = urlResolve(url).href within $browser.url or changing all calls to $browser.url(val) to be consistent? Adding the urlResolve seems to work, although changes a few tests (some URLs now end in /).

[gkalpak]

@jbedard, I didn't think through the implementation details. I was referring to the end result :grin:

[jbedard]

@dmartres can you confirm if this is still an issue in 1.7.3? I think aee7d53a6b5d3d7bc0a1124fd3df9b263777e72e (fixing #16592) may have fixed this?

[jbedard]

We've partially fixed this using the browser <a href> to normalize URLs in a few key areas (https://github.com/angular/angular.js/commit/e68697e2e30695f509e6c2c1e43c2c02b7af41f0, https://github.com/angular/angular.js/commit/2f72a69ded53a122afad3ec28d91f9bd2f41eb4f). However no browser normalizes everything with this method. To truly fix this we would need to implement the normalizing ourselves.

I implemented a POC to do that normalization manually, however we decided this is too big of a change at this point. FYI 15 of the the tested added in that commit currently fail in chrome.

For these reasons we are going to put this in a "wont fix" state.