fix($location): consider baseHref in relative link for legacy browsers #8233

thenikso · 2014-07-17T10:09:56Z

Request Type: bug

How to reproduce:

In a legacy browser that doesn't support HTML5 push state (ie: IE9) have the following:

$locationProvider.html5Mode(true).hashPrefix('!'); in app configuration;
a number of routes such as /foo, /bar in the $routeProvider configuration;
<base href="/en/"> in page headers;
<a href="/en/foo"> in a link.

When clicking a link, it is rewritten as: http://host/en/#!/en/foo
Expected: http://host/en/#!/foo

Component(s): $location

Impact: medium

Complexity: small

This issue is related to:

Other Comments:

I couldn't yet devise any test for covering this case. This fix concern a click handler on the $rootElement which (afaik) is not covered in the current tests.

Please let me know if you find it reasonable and if you have ideas on how to test for it.
Thanks!

mary-poppins · 2014-07-17T10:10:00Z

Thanks for the PR! Please check the items below to help us merge this faster. See the contributing docs for more information.

Uses the issue template (#8233)

If you need to make changes to your pull request, you can update the commit with git commit --amend.
Then, update the pull request with git push -f.

Thanks again for your help!

timraymond · 2014-07-17T11:38:29Z

@thenikso It's certainly a tricky thing to get tests around, but it looks like there's plenty of helpers in locationSpec for setting this kind of stuff up. This spec is similar in spirit to what you're trying to test... have you tried something similar? https://github.com/angular/angular.js/blob/master/test/ng/locationSpec.js#L1162-L1190

jeffbcross · 2014-07-21T22:13:29Z

Hi @thenikso, would you mind putting together a complete plnkr demonstrating this? And is this issue only reproducible in IE9?

winsontam · 2014-07-22T07:42:27Z

src/ng/location.js

@@ -668,6 +668,13 @@ function $LocationProvider(){
        if (href.indexOf('://') < 0) {         // Ignore absolute URLs
          var prefix = '#' + hashPrefix;
          if (href[0] == '/') {
+            // Account for base href already present in appBase


// use baseHref variable is fine

if (!baseHref && href.indexOf(baseHref) === 0) {
href = href.substr(baseHref.length);
if (!href || href[0] != '/') {
href = '/' + href;
}
}

thanks! I've updated accordingly

winsontam · 2014-07-22T08:04:48Z

Hi Jeffbcross,

I have made a pinkr.
http://plnkr.co/edit/NSNqVnvbysC8A170Ax4V?p=preview
please take a look in browser that don't support window.history.

Thanks

…tml5-fallback-base

thenikso · 2014-07-29T10:50:05Z

I've added a test thanks to @timraymond suggestion.
I confirm that the @winsontam plunkr also reproduces the issue on IE9.

jeffbcross · 2014-08-11T23:58:41Z

@thenikso I created a build of angular and angular-route from your commits, and the issue persists on my fork of @winsontam's plnkr on IE9: plnkr.

jeffbcross · 2014-08-12T18:33:33Z

There are several issues regarding html5Mode in legacy browsers, which should be fixed with @caitp's PR to revert offending commits: #8492

caitp · 2014-08-19T22:24:44Z

I was talking with Igor about this today, there were a few nits regarding this, but I think we're good to just land this as is.

With this, and a really tiny fix to prevent the can't invoke method "indexOf" of undefined issue, we should be good.

I'm going to check this in today.

caitp · 2014-08-20T01:53:59Z

src/ng/location.js

@@ -668,6 +668,13 @@ function $LocationProvider(){
        if (href.indexOf('://') < 0) {         // Ignore absolute URLs
          var prefix = '#' + hashPrefix;
          if (href[0] == '/') {
+            // Account for base href already present in appBase
+            if (baseHref && href.indexOf(baseHref) === 0) {


I don't think this is quite right.

Assuming we have a base tag like <base href="http://www.google.com/foo/bar">,

<a href="/baz" id="a">baz</a> should resolve to http://www.google.com/baz,
<a href="baz" id="a2">baz2</a> should resolve to http://www.google.com/foo/baz (not /foo/bar/baz as you might expect, because the base tag doesnt' end with a slash)

The issues are, 1) we're not really correctly resolving relative to the baseURI for absolute references, and 2) we're not resolving relative URIs correctly this way at all, because this branch is only ever evaluated for absolute uris (uris beginning with a /).

We need to fix this up

…port spec

…-base absolute references

thenikso · 2014-08-20T11:15:35Z

@caitp you are right, the relative URI case is not fixed at all in this PR. The logic for it should be to consider if the base tag ends with a / and resolve accordingly? Considering the hash prefix that would be added for a legacy browser, how does this list looks?

<base href="http://www.google.com/foo/bar"> and <a href="baz" id="a2">baz2</a> should resolve to http://www.google.com/foo/#baz
<base href="http://www.google.com/foo/bar/"> and <a href="baz" id="a2">baz2</a> should resolve to http://www.google.com/foo/bar/#baz

You also mentioned a can't invoke method "indexOf" of undefined but I believe it will be fixed somewhere else, am I right?

@jeffbcross I'll take a look at your updated fiddle within the day.

caitp · 2014-08-20T13:21:04Z

You also mentioned a can't invoke method "indexOf" of undefined but I believe it will be fixed somewhere else, am I right?

There are 2 bugs which cause that --- one of them is fixed in master right now, but the other one will happen whenever you try to rewrite a url that doesn't include the base href of the app (base href must be the same as app base, unfortunately :()

caitp · 2014-08-23T03:25:10Z

The frustrating thing is that, because the appBase and baseHref are so closely tied in angular, it's not really possible for us to properly emulate relative urls WRT a base url --- to do so, the base path would need to be part of the virtual URL rather than part of the appBase --- otherwise there's no way we can correctly handle things like href="./relative/path" --- I mean we could just prepend baseHref to it with any file/search query removed, but it's not quite going to give the right result, and it the right combination of hacks is sort of complicated and really gross code.

I kinda feel like just reverting would have been better. I'll take another look at it on monday

NevilleS · 2014-08-27T18:32:38Z

FYI I modified @winsontam's plunkr in a way that reproduces this without requiring you to use IE9 here: http://plnkr.co/edit/UGPBOpHlHPghl6Q6wqXl?p=preview

I also have a more detailed reproduction that shows the behaviour of relative paths and absolute paths here: http://stormy-fjord-8496.herokuapp.com/html5_no_history

caitp · 2014-08-27T20:13:42Z

we've got all that, the problem is that this still does not match the behaviour of a history-enabled app.

…` url BREAKING CHANGE (since 1.2.0 and 1.3.0-beta.1): Angular now requires a `<base>` tag when html5 mode of `$location` is enabled. Reasoning: Using html5 mode without a `<base href="...">` tag makes relative links for images, links, ... relative to the current url if the browser supports the history API. However, if the browser does not support the history API Angular falls back to using the `#`, and then all those relative links would be broken. The `<base>` tag is also needed when a deep url is loaded from the server, e.g. `http://server/some/page/url`. In that case, Angular needs to decide which part of the url is the base of the application, and which part is path inside of the application. To summarize: Now all relative links are always relative to the `<base>` tag. Exception (also a breaking change): Link tags whose `href` attribute starts with a `#` will only change the hash of the url, but nothing else (e.g. `<a href="#someAnchor">`). This is to make it easy to scroll to anchors inside a document. Related to angular#6162 Closes angular#8492 BREAKING CHANGE (since 1.2.17 and 1.3.0-beta.10): In html5 mode without a `<base>` tag on older browser that don't support the history API relative paths were adding up. E.g. clicking on `<a href="page1">` and then on `<a href="page2">` would produce `$location.path()==='/page1/page2'. The code that introduced this behavior was removed and Angular now also requires a `<base>` tag to be present when using html5 mode. Closes angular#8172, angular#8233

tbosch · 2014-09-02T17:51:58Z

This should work now as expected as of 1.3.0-rc0, see 2294880

tleruitte · 2014-10-03T16:19:06Z

Hi guys,
Will this fix be backported to angular 1.2.x?

tbosch · 2014-10-04T00:49:11Z

@tleruitte Just landed in v1.2.x branch...

tbosch · 2014-10-04T00:49:30Z

(but without enforcing the <base> tag which would be a breaking change...)

fix($location): consider baseHref in realtive link for legacy browsers

25da859

thenikso added cla: yes and removed cla: no labels Jul 17, 2014

jeffbcross self-assigned this Jul 21, 2014

jeffbcross added browser: IE9 labels Jul 21, 2014

jeffbcross added this to the Purgatory milestone Jul 21, 2014

winsontam reviewed Jul 22, 2014
View reviewed changes

jeffbcross assigned jeffbcross and unassigned jeffbcross Jul 23, 2014

thenikso added 3 commits July 29, 2014 10:45

Merge branch 'master' of https://github.com/angular/angular.js into h…

df05668

…tml5-fallback-base

Use cached value of baseHref instead of calculating it

dfd2959

Add test for html5mode fallback with base path

b6d00f7

jeffbcross modified the milestones: Purgatory, 1.3.0-beta.18 Jul 29, 2014

caitp mentioned this pull request Aug 6, 2014

revert fix($location): make legacy browsers behave like modern ones in html5Mode #8492

Closed

petebacondarwin changed the title ~~fix($location): consider baseHref in realtive link for legacy browsers~~ fix($location): consider baseHref in relative link for legacy browsers Aug 8, 2014

jeffbcross modified the milestones: 1.3.0-beta.18, 1.3.0-beta.19 Aug 12, 2014

caitp mentioned this pull request Aug 20, 2014

fix($location): rewrite relative paths correctly path==='/' in legacy html5Mode #8684

Closed

caitp reviewed Aug 20, 2014
View reviewed changes

thenikso added 2 commits August 20, 2014 12:48

Remove unneccessary setup and cleanup to legacy browser html5mode sup…

5b99a00

…port spec

Augment html5mode spec for legacy browser base support to include non…

b65fd3c

…-base absolute references

btford removed the gh: PR label Aug 20, 2014

caitp force-pushed the master branch from 591bac5 to d713ad1 Compare August 22, 2014 13:51

tbosch modified the milestones: 1.3.0-beta.19, 1.3.0-rc.0 Aug 27, 2014

tbosch assigned tbosch and unassigned caitp Aug 27, 2014

matsko modified the milestones: 1.3.0-rc.0, 1.3.0-rc.1 Aug 30, 2014

tbosch force-pushed the master branch from 6c27b89 to 271572c Compare August 30, 2014 00:57

tbosch closed this Sep 2, 2014

NevilleS mentioned this pull request Sep 5, 2014

$location rewrites absolute paths incorrectly in LocationHashbangInHtml5Url mode #8633

Closed

chrisirhc mentioned this pull request Sep 17, 2014

fix($location): links should respect absolute paths and base href when in html5mode #9126

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix($location): consider baseHref in relative link for legacy browsers #8233

fix($location): consider baseHref in relative link for legacy browsers #8233

thenikso commented Jul 17, 2014

mary-poppins commented Jul 17, 2014

timraymond commented Jul 17, 2014

jeffbcross commented Jul 21, 2014

winsontam Jul 22, 2014

thenikso Jul 29, 2014

winsontam commented Jul 22, 2014

thenikso commented Jul 29, 2014

jeffbcross commented Aug 11, 2014

jeffbcross commented Aug 12, 2014

caitp commented Aug 19, 2014

caitp Aug 20, 2014

thenikso commented Aug 20, 2014

caitp commented Aug 20, 2014

caitp commented Aug 23, 2014

NevilleS commented Aug 27, 2014

caitp commented Aug 27, 2014

tbosch commented Sep 2, 2014

tleruitte commented Oct 3, 2014

tbosch commented Oct 4, 2014

tbosch commented Oct 4, 2014

fix($location): consider baseHref in relative link for legacy browsers #8233

fix($location): consider baseHref in relative link for legacy browsers #8233

Conversation

thenikso commented Jul 17, 2014

mary-poppins commented Jul 17, 2014

timraymond commented Jul 17, 2014

jeffbcross commented Jul 21, 2014

winsontam Jul 22, 2014

Choose a reason for hiding this comment

thenikso Jul 29, 2014

Choose a reason for hiding this comment

winsontam commented Jul 22, 2014

thenikso commented Jul 29, 2014

jeffbcross commented Aug 11, 2014

jeffbcross commented Aug 12, 2014

caitp commented Aug 19, 2014

caitp Aug 20, 2014

Choose a reason for hiding this comment

thenikso commented Aug 20, 2014

caitp commented Aug 20, 2014

caitp commented Aug 23, 2014

NevilleS commented Aug 27, 2014

caitp commented Aug 27, 2014

tbosch commented Sep 2, 2014

tleruitte commented Oct 3, 2014

tbosch commented Oct 4, 2014

tbosch commented Oct 4, 2014