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

Potential fix for #6162 #6421

Closed
wants to merge 2 commits into from
Closed

Potential fix for #6162 #6421

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
bc71e7b
Potential fix for #6162
Feb 23, 2014
84aa85b
Parse anchors in IE9
Mar 16, 2014
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
42 changes: 42 additions & 0 deletions src/ng/location.js
View file
Open in desktop
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 @@ -621,6 +631,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
Copy link
Contributor

Choose a reason for hiding this comment

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

href.indexOf('://') < 0 --- this currently means href.indexOf(false)

if (href[0] == '/') {
Copy link
Contributor

Choose a reason for hiding this comment

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

Even though old IE is not supported, it's probably better to use charAt() rather than the array index annotation --- and maybe just cache the value so that it doesn't need to be fetched multiple times.

// 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("/"),
Copy link
Contributor

Choose a reason for hiding this comment

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

We really need test cases verifying this (actually, test cases verifying every unit of functionality added in this patch!), but especially this branch.

Please add test cases for this, I'm happy to help if you need assistance.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@caitp. Please reread my comment
#6162 (comment)

On 28 March 2014 11:02, Caitlin Potter notifications@github.com wrote:

In src/ng/location.js:

  •  // 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("/"),

We really need test cases verifying this (actually, test cases verifying
every unit of functionality added in this patch!), but especially this
branch.

Please add test cases for this, I'm happy to help if you need assistance.

Reply to this email directly or view it on GitHubhttps://github.com//pull/6421/files#r11047283
.

Copy link
Contributor

Choose a reason for hiding this comment

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

I can help you fix failing tests and write new ones for your patch, but we do need tests. As for the timeframe this can be merged for, that's hard to say --- but having a mergeable patch will help make that soon.

So, I can pair with you on getting this in, but it's going to take some extra work.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry Caitlin, I just don't have the time to take this further.

What I've supplied your team with is simply the patch I used to get things
working for one particular project, I hope it is helpful.

It's just my opinion, but I think whoever originally wrote location.js and
whoever committed the team to supporting legacy browsers for a stable
release, and for that matter whoever decided to backpedal on supporting IE8
(
a2acd79)
should get together and fix this bug. That is the way we do things around
here - you make a problem you fix the problem.

If the goal is to get location.js into an easy to maintain state. I would
say the best approach is to remove all the clever tricks and workarounds
for resolving urls. Sure, browsers already know how to apply relative and
absolute paths, and every byte counts when you want a quick download, but
the browsers implementation cannot be relied upon in all cases, i.e. legacy
browsers. The dozen or so lines of code to resolve urls are needed for
legacy browsers, so you might as well just use that method all the time.
This will lead to a simpler and easier to maintain implementation. You also
need to consider whether the same bugs exist in 1.3, I expect it does as
1.3 is supposed to support IE9.

Basically, I wouldn't recommend just tidying up and merging my fix.

The disparity between actual and documented browser support, and
back-pedalling on supported browsers for a stable release has convinced me
Angular is not mature enough for real world systems. IE8 still has around
8% of users. It is unlikely I will be using it for future projects. The
real problem is the expectation gap. A expect a lot of people have wasted a
lot of time with Angular as its apparently well known limitations are not
well documented. Not your fault Caitlin I know.

Sorry I cannot be of any help.

On 28 March 2014 12:02, Caitlin Potter notifications@github.com wrote:

In src/ng/location.js:

  •  // 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("/"),

I can help you fix failing tests and write new ones for your patch, but we
do need tests. As for the timeframe this can be merged for, that's hard to
say --- but having a mergeable patch will help make that soon.

So, I can pair with you on getting this in, but it's going to take some
extra work.

Reply to this email directly or view it on GitHubhttps://github.com//pull/6421/files#r11049243
.

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