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

CB-6299 Strip protocol and leading slashes from XHRLOCAL URL #30

Merged
merged 2 commits into from Mar 19, 2014

Conversation

gabrielschulhof
Copy link
Contributor

This PR submits the fix mentioned in a related jQuery Mobile bug

I've tested this fix on a Nokia Lumia 800 for WP7 and on a Nokia Lumia 520 for WP8.

@gabrielschulhof
Copy link
Contributor Author

@purplecabbage I guess a PR can't hurt, either, eh?

@purplecabbage
Copy link
Contributor

@gabrielschulhof
Can you provide a test case that demonstrates the issue?
I thought this would do it, but it seems to load regardless of whether the patch is applied.

        var xhr = new XMLHttpRequest();
        xhr.onload = function () {
            console.log("loaded");
        }
        var loc = window.location.toString() + "#Hoopla";
        xhr.open("GET", loc, true);
        xhr.send();

@gabrielschulhof
Copy link
Contributor Author

That does indeed work. What doesn't work is xhr.open( "GET", "x-wmapp0:/www/some-other-file.html", true); even if the file is present. The leading slash is causing a 404. Basically, this code is only strips off the leading slash and only performs the path juggling if you have absolute URLs that do not include the protocol. If the URL includes the protocol, then all that code is bypassed. My modification would first strip off the protocol so that that code can do its magic.

@gabrielschulhof
Copy link
Contributor Author

So, there are (probably more than) three cases when retrieving a local file via Ajax:

other-file.html
works, because the URL is correctly assembled by Cordova
x-wmapp0:www/other-file.html
works, because the Uri class used here seems to understand this kind of URL
x-wmapp0:/www/other-file.html
doesn't work, because the code I mention in my previous comment is not having an effect due to the fact that the URL starts with the protocol

Now, granted, the form x-wmapp0:/www/other.html may not occur much in the wild, but performing URL calculations such as creating an absolute URL from a relative URL are impossible when the URL scheme does not have a slash at the root, because you cannot distinguish an absolute URL from a relative URL.

@purplecabbage
Copy link
Contributor

Thanks. I'll add a test to spec and close this up today.

Sent from my iPhone

On Mar 19, 2014, at 7:42 AM, gabrielschulhof notifications@github.com wrote:

So, there are (probably more than) three cases when retrieving a local file via Ajax:

other-file.html
works, because the URL is correctly assembled by Cordova
x-wmapp0:www/other-file.html
works, because the Uri class used here seems to understand this kind of URL
x-wmapp0:/www/other-file.html
doesn't work, because the code I mention in my previous comment is not having an effect due to the fact that the URL starts with the protocol
Now, granted, the form x-wmapp0:/www/other.html may not occur much in the wild, but performing URL calculations such as creating an absolute URL from a relative URL are impossible when the URL scheme does not have a slash at the root, because you cannot distinguish an absolute URL from a relative URL.


Reply to this email directly or view it on GitHub.

@gabrielschulhof
Copy link
Contributor Author

Thanks!

@asfgit asfgit merged commit ca8efaf into apache:master Mar 19, 2014
gabrielschulhof pushed a commit to jquery-archive/jquery-mobile that referenced this pull request Mar 20, 2014
Note: this does indeed represent a fix for gh-6574, but only once a version of
Cordova sporting apache/cordova-wp8#30 is released.

Closes gh-6597
Fixes gh-6574
Fixes gh-6599
gabrielschulhof pushed a commit to jquery-archive/jquery-mobile that referenced this pull request Apr 8, 2014
Note: this does indeed represent a fix for gh-6574, but only once a version of
Cordova sporting apache/cordova-wp8#30 is released.

Closes gh-6597
Fixes gh-6574
Fixes gh-6599
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants