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

$location rewrites absolute paths incorrectly in LocationHashbangInHtml5Url mode #8633

Closed
NevilleS opened this issue Aug 15, 2014 · 5 comments

Comments

@NevilleS
Copy link
Contributor

Issue Description

I have a web application that uses a handful of Angular SPAs hosted together. The general structure is something like this:

On each app, there is a navigation bar with elements like this:

<a href="/admin/foo">Foo</a>
<a href="/admin/bar">Bar</a>

I'm using $locationProvider.html5mode(true), so the SPA functionality works great... except in IE9! In IE9, these absolute paths are transformed weirdly like this:

admin/foo --> http://example.com/admin/#!/admin/foo  (should be http://example.com/admin/#!/foo)

After some investigation, I traced the issue to be that IE9 doesn't support the history API, so $location switches to LocationHashbangInHtml5Url mode. In this mode, it seems like absolute paths don't work correctly.

Steps to reproduce

I've created a simple demo site here that reproduces this issue. You don't need to use IE9 to test this; you can reproduce this issue by just disabling the history API (I've done this by decorating $sniffer).

My test site is here: http://stormy-fjord-8496.herokuapp.com/html5_no_history/
Source code is here: https://github.com/NevilleS/angular-route-problem

On the test page, clicking on the "abs_path" link will send you to http://stormy-fjord-8496.herokuapp.com/html5_no_history/#!/html_5_no_history/abs_path, which is the issue I'm seeing.

If you try the same style of link with history enabled, it works. You can reproduce this on my test page as well: http://stormy-fjord-8496.herokuapp.com/html5_history/

Affected Versions

Note that this behaviour is relatively new; this used to work! I traced back through the commit history and found that this one seems to be the culprit: 3f04770. This commit was done to fix some issues in IE8 it seems, and references a couple different issues (#6162, #6421, etc.)

This means this was likely introduced in

Fix

I'm not sure of the best way to fix this yet, unfortunately. My current workaround has been to fork Angular locally, reverse that commit, and use my modified version of Angular in production. I don't really want to keep doing that, though! I'd be happy to help work on a fix for this but I'm not sure exactly how best to do so.

Unit Test

In my opinion, there is already a unit test for this exact use case, but I think there is a bug in that test as well 👍. The spec that covers this functionality (absolute path with history disabled) is:
https://github.com/angular/angular.js/blob/master/test/ng/locationSpec.js#L1181

    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 test passes because of this code, which transforms the absolute path into an absolute URL, which bypasses the code that creates this issue:
https://github.com/angular/angular.js/blob/master/test/ng/locationSpec.js#L855

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

Based on some of the commit and issue history I think @caitp might be able to set me straight?

Cheers,

Neville

@caitp
Copy link
Contributor

caitp commented Aug 15, 2014

See #8492 --- just waiting for the greenlight on that.

@NevilleS
Copy link
Contributor Author

Reverting that commit is my current fix, too 😄

Well, my example app helps build the case to merge that PR~

@caitp
Copy link
Contributor

caitp commented Aug 20, 2014

I don't think we're going to end up reverting those CLs, instead we're going to try and fix it. Blocked on one of the PRs mentioned in #8492, which needs a bit more work before it can land.

Sorry for the wait

@caitp caitp added this to the Backlog milestone Aug 20, 2014
@NevilleS
Copy link
Contributor Author

OK, thanks! Let me know if I can help in anyway.

@NevilleS
Copy link
Contributor Author

NevilleS commented Sep 5, 2014

This is fixed by #8233, thanks!

@NevilleS NevilleS closed this as completed Sep 5, 2014
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

2 participants