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

Commit

Permalink
fix($location): fix and test html5Mode url-parsing algorithm for lega…
Browse files Browse the repository at this point in the history 
…cy browsers

This CL fixes problems and adds test cases for changes from #6421. Changes
include fixing the algorithm for preprocessing href attribute values, as
well as supporting xlink:href attributes. Credit for the original URL
parsing algorithm still goes to @richardcrichardc.

Good work, champ!
  • Loading branch information
@caitp
caitp committed Apr 18, 2014
1 parent e020366 commit 24f7999
Show file tree
Hide file tree
Showing 2 changed files with 68 additions and 15 deletions.
21 changes: 11 additions & 10 deletions src/ng/location.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -653,33 +653,34 @@ function $LocationProvider(){
}

// 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
// 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');
// get the actual href attribute - see
// http://msdn.microsoft.com/en-us/library/ie/dd347148(v=vs.85).aspx
var href = elm.attr('href') || elm.attr('xlink:href');

if (href.indexOf('://' == -1)) { // Ignore absolute URLs
if (href.indexOf('://') < 0) { // Ignore absolute URLs
var prefix = '#' + hashPrefix;
if (href[0] == '/') {
// absolute path - replace old path
absHref = serverBase(absHref) + href;
absHref = appBase + prefix + href;

This comment has been minimized.

Sign in to view

Copy link
@vinntreus

vinntreus Oct 30, 2014

Now using 1.2.26 and change on this row breaks routing when having a base href and clicking on link in IE8 standards mode. No error is show - nothing seem to happen when click links (route is not matching).
} else if (href[0] == '#') {
// local anchor
absHref = serverBase(absHref) + $location.path() + href;
absHref = appBase + prefix + ($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
else if (parts[i].length)
stack.push(parts[i]);
}
absHref = serverBase(absHref) + stack.join("/");
absHref = appBase + prefix + stack.join('/');
}
}
}
Expand Down
62 changes: 57 additions & 5 deletions test/ng/locationSpec.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -779,15 +779,22 @@ describe('$location', function() {

var root, link, originalBrowser, lastEventPreventDefault;

function configureService(linkHref, html5Mode, supportHist, attrs, content) {
function configureService(linkHref, html5Mode, supportHist, relLink, attrs, content) {
if (typeof relLink !== "boolean") {
content = attrs;
attrs = relLink;
relLink = false;
}
module(function($provide, $locationProvider) {
attrs = attrs ? ' ' + attrs + ' ' : '';

// fake the base behavior
if (linkHref[0] == '/') {
linkHref = 'http://host.com' + linkHref;
} else if(!linkHref.match(/:\/\//)) {
linkHref = 'http://host.com/base/' + linkHref;
if (!relLink) {

This comment has been minimized.

Sign in to view

Copy link
@chrisirhc

chrisirhc Sep 17, 2014

Contributor

@caitp this doesn't look right. When the link begins with a /, the link must always be treated as absolute and coming from the root (domain).
This condition should instead be if (!relLink || linkHref[0] == '/'). There shouldn't be a concept of relative links that begin with /, because those are "relative" to the root of the path -> they are absolute.

This comment has been minimized.

Sign in to view

Copy link
@chrisirhc

chrisirhc Sep 17, 2014

Contributor

I see that if this is done, the new code doesn't get tested at all because the links will be treated as absolute links.

This comment has been minimized.

Sign in to view

Copy link
@caitp

caitp Sep 17, 2014

Author Contributor

We know it's not right, we've already redone the solution for this. It's landed in 1.3 and will be backported to 1.2 once we think it's relatively safe (there are some issues with it that people have complained about like breaking SVG clip-paths, so it's not totally perfect, but it resolves urls better)

This comment has been minimized.

Sign in to view

Copy link
@caitp

caitp Sep 17, 2014

Author Contributor

This condition should instead be if (!relLink || linkHref[0] == '/'). There shouldn't be a concept of relative links that begin with /, because those are "relative" to the root of the path -> they are absolute.

No, not quite.

So by default we turn the link into a fully qualified URL --- the relLink flag says "don't qualify this URL", basically. You could call it a misnomer because it's "absolute relative to the base URI", but yeah, it just means not fully qualified.

Anyways, I think you're worrying too much about this =)

This comment has been minimized.

Sign in to view

Copy link
@chrisirhc

chrisirhc Sep 17, 2014

Contributor

I'm trying to fix the tests now using the existing solution, which seems okay, as an safer alternative, and I'll push a PR when I'm done.

I'm aware that the current solution is to force base tags, and that there are plans to add directives for SVG attributes to remedy this situation. However, this won't work too well for those who are rendering through d3. Well, there are other possible issues but I'll leave that for another place/time.

Anyways, I'll see how far I get with a safer/non-compatibility-breaking solution and post a PR.

This comment has been minimized.

Sign in to view

Copy link
@caitp

caitp Sep 17, 2014

Author Contributor

There's nothing to "fix" in the tests really, the tests are running the right codepaths. All the flag does is say "don't add the scheme and hostname and port and base path to the href attribute", like it's not clear what you're even talking about. What are you talking about?

@caitp this doesn't look right. When the link begins with a /, the link must always be treated as absolute and coming from the root (domain).

From the base path

This condition should instead be if (!relLink || linkHref[0] == '/'). There shouldn't be a concept of relative links that begin with /, because those are "relative" to the root of the path -> they are absolute.

No, we only want to qualify the URL if we're testing qualified URLs ---- the link starting with / is unrelated to this.

You are confused about this whole thing :(

This comment has been minimized.

Sign in to view

Copy link
@caitp

caitp Sep 17, 2014

Author Contributor

In short, the code is basically "if we get an href that doesn't contain a qualified URL, lets try and resolve it relative to the appBase" --- so what these tests are doing is saying "hey test suite, don't make all of these links qualified urls, because we want to test that the href parser actually sort of works".

Needless to say, the href parser doesn't really work very well in a number of cases :( But the tests are still actually testing this correctly

This comment has been minimized.

Sign in to view

Copy link
@chrisirhc

chrisirhc Sep 17, 2014

Contributor

Sorry ignore this comment thread, I'm not referring to this line (relLink) that's wrong. Read the comment below: 24f7999#commitcomment-7815179

This comment has been minimized.

Sign in to view

Copy link
@chrisirhc

chrisirhc Sep 17, 2014

Contributor

I understand the reasons for the relLink check, I wrote that in 24f7999#commitcomment-7815653 . I'm referring to the below test cases in a newer comment.

This comment has been minimized.

Sign in to view

Copy link
@caitp

caitp Sep 17, 2014

Author Contributor

I see --- that test was added almost 3 years ago, and the code has transformed considerably since it was added. There was no relLink flag back then, because the url was never qualified at all in the test suite back then.

So yeah, it would make sense to set that flag to true to reflect the original behaviour of that test

This comment has been minimized.

Sign in to view

Copy link
@chrisirhc

chrisirhc Sep 17, 2014

Contributor

Sorry for the confusion, it's hopefully easier to explain through code changes I'm proposing: #9126 .
Thanks for the super quick responses though. 😄
if (linkHref[0] == '/') {
linkHref = 'http://host.com' + linkHref;
} else if(!linkHref.match(/:\/\//)) {
linkHref = 'http://host.com/base/' + linkHref;
}
}

link = jqLite('<a href="' + linkHref + '"' + attrs + '>' + content + '</a>')[0];
Expand Down Expand Up @@ -1067,6 +1074,51 @@ describe('$location', function() {
});


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

This comment has been minimized.

Sign in to view

Copy link
@tleruitte

tleruitte Jul 3, 2014

Contributor

This test case fails:

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');
        }
      );
    });

This comment has been minimized.

Sign in to view

Copy link
@caitp

caitp Jul 3, 2014

Author Contributor

So, your test fails when you have history enabled? What browsers does this fail for?

This comment has been minimized.

Sign in to view

Copy link
@caitp

caitp Jul 3, 2014

Author Contributor

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.

I will look at this later today.

it('should replace current path when link begins with "/" and history disabled', function() {
configureService('/link', true, false, true);
inject(
initBrowser(),
initLocation(),
function($browser, $location) {
$location.path('/some');
browserTrigger(link, 'click');
expectRewriteTo($browser, 'http://host.com/base/index.html#!/link');
}
);
});

This comment has been minimized.

Sign in to view

Copy link
@chrisirhc

chrisirhc Sep 17, 2014

Contributor

I've been reading this test case over and over, and it still doesn't look right to me. I must be misunderstanding something? Help me out here.

This contradicts the behavior outlined in lines 1009-1020 of the same file. (Sidenote, these added test cases aren't named correctly, history is enabled but it's on an old browser, so this test case is actually testing the same condition as the test case in those lines, except this is a "relative" link).

In one test case:
Current url: http://host.com/base/index.html
Click on: /other_base/link ➡️ Rewritten in configureService to ➡️ http://host.com/other_base/link
End up on (since no rewriting): http://host.com/other_base/link

In this test case:
Current url: http://host.com/base/index.html#/some
Click on: /link (no rewriting)
End up on (yes, rewriting): http://host.com/base/index.html#!/link

This looks contradictory. Both hrefs begin with / but one is treated differently from the other.

This comment has been minimized.

Sign in to view

Copy link
@chrisirhc

chrisirhc Sep 17, 2014

Contributor

This also contradicts lines 915-925, because in that case:

Current url: http://host.com/base/index.html
Click on: /base/link?a#b ➡️ Rewritten in configureService to ➡️ http://host.com/base/link?a#b
End up on (with rewriting): http://host.com/base/index.html#!/link?a#b

So a link with href of /base/link goes to http://host.com/base/index.html#!/some, while a link of href /link goes to the same place? That doesn't seem right.


it('should replace current hash fragment when link begins with "#" history disabled', function() {
configureService('#link', true, false, true);
inject(
initBrowser(),
initLocation(),
function($browser, $location) {
// Initialize browser URL
$location.path('/some');
$location.hash('foo');
browserTrigger(link, 'click');
expect($location.hash()).toBe('link');
expectRewriteTo($browser, 'http://host.com/base/index.html#!/some#link');
}
);
});


// don't run next tests on IE<9, as browserTrigger does not simulate pressed keys
if (!(msie < 9)) {

Expand Down

0 comments on commit 24f7999

Please sign in to comment.