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

feat($location): test $location enhancements from #6421 and fix them #6899

Closed
wants to merge 3 commits into from

Conversation

caitp
Copy link
Contributor

@caitp caitp commented Mar 28, 2014

This is temporary, just to run a travis build and hopefully make the patch easier to review.

@caitp caitp mentioned this pull request Mar 28, 2014
@caitp caitp added cla: yes and removed cla: no labels Mar 28, 2014
@@ -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) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I feel like it would make sense to have a "preprocessing" method in LocationMode, so that this code could be removed from here, and more easily extended for the other location modes. I guess it's sort of like a special version of $$rewrite that wants the relative url rather than the absolute

@caitp caitp modified the milestones: 1.3.0-beta.5, 1.3.0 Mar 31, 2014
Richard Collins and others added 3 commits April 9, 2014 17:08
…x them

This CL fixes problems and adds test cases for changes from angular#6421. Changes
include fixing the algorithm for preprocessing href attribute values, as
well as supporting xlink:href attributes.

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;
Copy link
Contributor

Choose a reason for hiding this comment

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

how do we cause the page reload here? when an app uses routing, the path is always set, so this override shouldn't change anything.

if the path is not set then we should investigate why.

the original thinking was that we don't want to append '#' in hashbang mode unless you actually use $location, otherwise you end up with apps that change the url needlessly, which is something we wanted to avoid.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This line was added by the original PR, so I think we have to ask them. I don't have access to IE9 to debug that easily except on SL, so it's hard for me to say.

@IgorMinar
Copy link
Contributor

ok, I think we resolved everything in person. let's get this in.

@caitp
Copy link
Contributor Author

caitp commented Apr 17, 2014

Closed by 3f04770

caitp pushed a commit to caitp/angular.js that referenced this pull request Apr 17, 2014
…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 angular#6162
Closes angular#6421
Closes angular#6899
Closes angular#6832
Closes angular#6834
caitp pushed a commit that referenced this pull request Apr 18, 2014
…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
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants