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

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

Closed
wants to merge 2 commits into from

Conversation

caitp
Copy link
Contributor

@caitp caitp commented Aug 5, 2014

These commits have caused more problems than they've been worth, and we get to remove some code, too.

Reverts: 3f04770, 49e7c32

Opinions on reverting these would be appreciated! /cc @IgorMinar

attrs = relLink;
relLink = false;
}
function configureService(linkHref, html5Mode, supportHist, attrs, content) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

can't revert this I don't think, some newer tests are actually using this feature, it's proven to be somewhat useful

…n html5Mode

This reverts commit 3f04770.

Conflicts:
	src/ng/location.js
@IgorMinar
Copy link
Contributor

What issues are we solving with these reverts?

We should add info and maybe tests so that we don't reintroduce the same issues in the future.

@caitp
Copy link
Contributor Author

caitp commented Aug 6, 2014

There are discussions about that at e020366, and I'm pretty sure some other issues which have been opened are attributable to those shas. It seems to break peoples relative links in hashbang html5mode

@caitp
Copy link
Contributor Author

caitp commented Aug 6, 2014

I will look at putting together some tests to see if reverting fixes anything

@caitp
Copy link
Contributor Author

caitp commented Aug 6, 2014

Fairly sure they're related:
#8126
#8233 (would be rendered invalid by reverting this, but this CL might solve some of these issues?)

Possibly related (but I'm not positive):
#8478

I thought there were more of these, so I'm probably not using the right search queries (location, html5mode, relative), but the issues brought up in the comments on the commit are somewhat telling IMO.

caitp referenced this pull request Aug 7, 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
@notatestuser
Copy link

Links with both ng-click and href="javascript:void(0)" are broken by these changes. Having test coverage for this would be ideal.

@caitp
Copy link
Contributor Author

caitp commented Aug 7, 2014

Hi @notatestuser --- despite not being a test user, it would be helpful if you could write some test cases so that we know what we broke and what to not break in the future.

Could you send me a PR for this branch adding some tests for things that were broken but fixed with the revert?

@notatestuser
Copy link

Sure, I'll give it a go

@caitp
Copy link
Contributor Author

caitp commented Aug 11, 2014

@notatestuser hows it coming? @IgorMinar it's possible that #8233 might actually make reverting this unnecessary, but I don't have a good way to verify that.

However, there have been a number of issues and comments about this, so we want to fix it before 1.3 ships IMO.

@notatestuser
Copy link

Yeah I'm just going to have to spend some time figuring out how I'm going to run this in ancient IE when I exclusively use macs. Is running the suite in a VM a normal thing to do?

@caitp
Copy link
Contributor Author

caitp commented Aug 12, 2014

Yeah I'm just going to have to spend some time figuring out how I'm going to run this in ancient IE when I exclusively use macs. Is running the suite in a VM a normal thing to do?

I'm in pretty much the same boat, so I know the feeling =P You can get VMs for running IE from https://github.com/xdissent/ievms/ --- and you might be able to use the VMs provided by https://modern.ie/en-us. Getting the dev environment set up in the vms is kind of a pain, but doable.

@magrabow
Copy link

We are experiencing the same issues with IE9 as mentioned in this comment.
Is there a chance this revert PR can be included in any of the nearest release of 1.2.x branch, apart from the 1.3.x? This would be really helpful.

@caitp
Copy link
Contributor Author

caitp commented Aug 14, 2014

Basically as soon as it gets an LGTM, yeah

@zachsnow
Copy link
Contributor

Looks like this will remove the code that is causing the issue in #7721 as well.

@caitp
Copy link
Contributor Author

caitp commented Aug 22, 2014

Closing in favour of #8233

@caitp caitp closed this Aug 22, 2014
tbosch added a commit to tbosch/angular.js that referenced this pull request Aug 29, 2014
…` 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 added a commit to tbosch/angular.js that referenced this pull request Oct 4, 2014
… master

Backport of 2294880 without enforcing the `<base>` tag and without the new handling for links that only contain hash fragments.

Related to angular#6162
Closes angular#8492
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants