Skip to content
This repository has been archived by the owner on Apr 12, 2024. It is now read-only.

Commit

Permalink
fix($location): make legacy browsers behave like modern ones in html5…
Browse files Browse the repository at this point in the history
…Mode

Previously, LocationHashbangInHtml5Url, which is used when html5Mode is enabled
in browsers which do not support the history API (IE8/9), would behave very
inconsistently WRT relative URLs always being resolved relative to the app root
url.

This fix enables these legacy browsers to behave like history enabled browsers,
by processing href attributes in order to resolve urls correctly.

Closes #6162
Closes #6421
Closes #6899
Closes #6832
Closes #6834
  • Loading branch information
Richard Collins authored and caitp committed Apr 18, 2014
1 parent b6eb5fd commit e020366
Showing 1 changed file with 42 additions and 0 deletions.
42 changes: 42 additions & 0 deletions src/ng/location.js
Original file line number Diff line number Diff line change
Expand Up @@ -268,6 +268,16 @@ function LocationHashbangInHtml5Url(appBase, hashPrefix) {
return appBaseNoFile;
}
};

this.$$compose = function() {
var search = toKeyValue(this.$$search),
hash = this.$$hash ? '#' + encodeUriSegment(this.$$hash) : '';

this.$$url = encodePath(this.$$path) + (search ? '?' + search : '') + hash;
// include hashPrefix in $$absUrl when $$url is empty so IE8 & 9 do not reload page because of removal of '#'
this.$$absUrl = appBase + hashPrefix + this.$$url;
};

}


Expand Down Expand Up @@ -642,6 +652,38 @@ function $LocationProvider(){
absHref = urlResolve(absHref.animVal).href;
}

// Make relative links work in HTML5 mode for legacy browsers (or at least IE8 & 9)
// The href should be a regular url e.g. /link/somewhere or link/somewhere or ../somewhere or somewhere#anchor or http://example.com/somewhere
if (LocationMode === LocationHashbangInHtml5Url) {
// get the actual href attribute - see http://msdn.microsoft.com/en-us/library/ie/dd347148(v=vs.85).aspx
// TODO check browser is in standards mode
var href = elm[0].getAttribute('href');

if (href.indexOf('://' == -1)) { // Ignore absolute URLs
if (href[0] == '/') {
// absolute path - replace old path
absHref = serverBase(absHref) + href;
} else if (href[0] == '#') {
// local anchor
absHref = serverBase(absHref) + $location.path() + href;
} else {
// relative path - join with current path
var stack = $location.path().split("/"),
parts = href.split("/");
stack.pop(); // remove top file
for (var i=0; i<parts.length; i++) {
if (parts[i] == ".")
continue;
else if (parts[i] == "..")
stack.pop();
else
stack.push(parts[i]);
}
absHref = serverBase(absHref) + stack.join("/");
}
}
}

var rewrittenUrl = $location.$$rewrite(absHref);

if (absHref && !elm.attr('target') && rewrittenUrl && !event.isDefaultPrevented()) {
Expand Down

13 comments on commit e020366

@tleruitte
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this commit (and commit 24f7999) should be reverted. @caitp @richardcrichardc

In the initial issue #6162, @richardcrichardc said:

The link subdir/ from http://angular-relative-links.richardc.net/ goes to http://angular-relative-links.richardc.net/subdir/ and,
the link subdir/ from http://angular-relative-links.richardc.net/subdir/ goes to http://angular-relative-links.richardc.net/subdir/subdir/ ...

@richardcrichardc are you using a base href tag in the head of you document? I don't think you would have this issue if you use it. Your test case is not online anymore, can you put it back?

However, here is my test case: I have an application which use html5 mode, and is served from domain.com/base. It declares a base href="base/" tag, and have a link with href="page1". With angular 1.2.16, on new browsers (Chrome), the link redirects to domain.com/base/page1, and on old browsers (IE8-9), to domain.com/base/#!/page1 as expected. This commit breaks this behaviour.

Besides, I tried to add this test case to test the behaviour in angular 1.2.17 (where the commit has been introduced):

it('should rewrite relative links relative to current path when history enabled on new browser', function() {
      configureService('link', true, true, true);
      inject(
        initBrowser(),
        initLocation(),
        function($browser, $location) {
          $location.path('/some');
          browserTrigger(link, 'click');
          expectRewriteTo($browser, 'http://host.com/base/some/link');
        }
      );
    });

And this test case failed.

@caitp
Copy link
Contributor

@caitp caitp commented on e020366 Jul 3, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is actually a collection of commits, this change wasn't all squished into this particular one.

I am open to reverting because I recognize that there have been some problems introduced with it, but just to be sure, you should test this on the subsequent changes.

@tleruitte
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, I did the tests on the HEAD of the branch v1.2.x.

As far as I can tell, this shouldn't hurt IE8 or IE9, which this CL was meant to fix, and as far as anyone could tell this didn't break modern browsers, since the real change shouldn't affect those browsers which supported history.

My point is that this fix is not needed, because if we use a base href tag, the behaviour is indeed the same in old and new browsers.

@caitp
Copy link
Contributor

@caitp caitp commented on e020366 Jul 3, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My point is that this fix is not needed, because if we use a base href tag, the behaviour is indeed the same in old and new browsers.

That's not actually true, relative URL resolution has no concept of our virtual urls

Yep, I did the tests on the HEAD of the branch v1.2.x.

We need to do better than that to figure out if a regression was introduced somewhere, a bisect would be more appropriate

@tleruitte
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hello @caitp ,

I have more time this week so I made this demo of the issue.

Angular 1.2.16 + route
code: http://fiddle.jshell.net/tleruitte/5NYbv/
demo: http://fiddle.jshell.net/tleruitte/5NYbv/show/
It works great on latest Chrome and IE8+.

Angular 1.2.19 + route
code: http://jsfiddle.net/tleruitte/25JNU/ (same code than previously)
demo: http://fiddle.jshell.net/tleruitte/25JNU/show/
It works great on latest Chrome, but breaks on IE8-9.
(What I mean by break is that when you click on the links "partial 1" and "partial 2", nothing happens...)

I still think (but I may be wrong of course) that this commit, and commit 24f7999 are responsible of this regression and should be reverted. Can you take a look at this?

Thanks.

@ykvarts
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@caitp, @tleruitte
I faced the same issue when upgraded angularjs, so had to downgrade to 1.2.16. This kind of critical issue for us because it makes all internal links broken in IE8 & IE9.
Is there open issue for this or solution/workaround?

@caitp
Copy link
Contributor

@caitp caitp commented on e020366 Jul 30, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You should be able to use links like "/absolute/path" and that should work consistently.

There isn't much else we can do other than revert the commits, I've been thinking about that.

@tleruitte
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You should be able to use links like "/absolute/path" and that should work consistently.

Yes, but if we use a base, the absolute path becomes /base/absolute/path, and it's cumbersome to rewrite each link like that.

There isn't much else we can do other than revert the commits, I've been thinking about that.

Do revert the commit please, I think it's the right solution to this problem.���

@notatestuser
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this commit is responsible for breaking my anchor tags that use the ngClick directive alongside an href="javascript:void(0)" attribute in IE 8 and 9.

Upon updating to v1.2.17 from v1.2.16 the browser appears to try to navigate to http://hostna.me/app/#!/javascript:void(0) when these links are clicked, bouncing the user to the 404 route. Prior behaviour would have the browser execute the expression in the ng-click attribute and ignore the href altogether.

@caitp
Copy link
Contributor

@caitp caitp commented on e020366 Aug 7, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it's responsible for a lot of nonsense, but probably not that. See #8492 so that we can add tests for things we broke with that CL, thanks!

@notatestuser
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can confirm that my links work again when the above code is removed.

@mpenttila
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I also ran into this while testing with IE9. As @tleruitte said, there's no easy way to run the app from an arbitrary base directory if you have to use absolute paths in all links. In 1.2.16 everything works perfectly if you set the base href correctly.

I can't quite understand how the issue mentioned in #6162 was fixed by this and/or related commits, because with 1.2.21 relative links are not possible with IE9, only absolute paths work, and thus also a relative link "subdir" in /absolute/path does not work correctly (it goes to /subdir).

But based on #8492 it now seems that the revert is going ahead, that's great!

@caitp
Copy link
Contributor

@caitp caitp commented on e020366 Aug 7, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

send a PR against my revert branch with test cases that should be included, thanks

Please sign in to comment.