createUrlObject not working in IE

[tschaub]

Word is that createURLObject is causing trouble in IE. This is causing the WMSGetFeatureInfo control to fail. I'll try to put together a test that fails in IE. Easy solution is to use === (again) to compare urls.

Andreas, I'm assigning to you in case you happen to be looking for something to mess around with (in IE). I won't have access to IE again until Tuesday.

[tschaub]

And, dwinslow will probably have to provide the necessary detail. I think he said createURLObject was failing on relative urls like "/foo/bar". The tests cover relative urls, so there might be some detail missing.

[ahocevar]

fix for Util.createUrlObject

[ahocevar]

 2060.patch adds a test which shows the problem with createUrlObject in IE and a fix for Util.js

dwinslow, please report if this fixes your issues with Control.GetFeatureInfo.

After applying the fix, tests pass for me in IE6, FF3, Opera9, Safari3. Thanks for any review.

[tschaub]

Andreas, thanks for the fix! This looks exactly right.

Please commit.

[ahocevar]

fixed in r9320

[crschmidt]

Brought to trunk in r9348.

[crschmidt]

Introduces regression in tests in IE. See #2098.

[crschmidt]

Note that this also causes a failure when tests are run on  file:/// URLs, but that this is unrelated to the actual failure, and should not be treated as a blocker for any fix, since most browsers don't suppot running the full set of tests on file: URLs anyway. (Just don't want people to follow that red herring like I did.)

[tschaub]

more robust createUrlObject

[tschaub]

So, the createUrlObject could use a bit more refactoring, but this fixes things for now.

Tests pass in IE6, IE7, IE8, and FF3. It looks like Opera is choking on our OpenLayers.String.contains - so failures there appear unrelated.

Thanks for review.

[crschmidt]

Looks okay, tests pass. I'm happy.

[tschaub]

(In [9404]) Making it so createUrlObject casts all urls to absolute ones first. r=crschmidt (pullup #2060)

[crschmidt]

(In [9406]) Pullups for OL 2.8 RC3.

jQuery lib fix (Closes #1391) getRenderedSize regression (Closes #1906) element.scrolls error with panzoombar (Closes #2054) createUrlObject bug (Closes #2060) google layer in late rendered maps (Closes #2075) IE6/Lang.nb bug (Closes #2093) Layer.TMS/TileCache bugs (Closes #2099) (Closes #2100) Graphic names issues (Closes #2101)

[elemoine]

With [9404] I get an error in IE7 at the line

urlObject.host = a.host || window.location.host;

The error is on a.host.

[elemoine]

Replying to elemoine:

With [9404] I get an error in IE7 at the line {{{

urlObject.host = a.host window.location.host; }}} The error is on a.host.

Replacing loc.host by "localhost" makes the error go away (see 2060.diff ), so reading from window.location.host seems to cause problem and make the object returned by document.createElement() invalid.

[elemoine]

setting a.href before reading from windows.location.host makes the problem go away!!

[tschaub]

Can you provide an example that fails? We don't want to set the host to localhost in all cases.

At r9410, the Util.html (trunk) tests pass on IE7. I'm running these from a virtual host (so the window.location.href is something like  http://192.168.1.7/path/to/tests).

Where are you seeing failures.

[elemoine]

Replying to tschaub:

Can you provide an example that fails? We don't want to set the host to localhost in all cases. At r9410, the Util.html (trunk) tests pass on IE7. I'm running these from a virtual host (so the window.location.href is something like  http://192.168.1.7/path/to/tests). Where are you seeing failures.

I'm obviously not saying that you should use "localhost". The patch I've come up with (see parch-2060-r9410-A0.diff) sets a.href before reading from windows.location.host - I observed that when setting a.href after reading windows.location.host the a object gets corrupted and we can no longer access a.host, a.port etc.

I'll try to come up with a small example demonstrating the issue.

[elemoine]

Replying to elemoine:

Replying to tschaub:

Can you provide an example that fails? We don't want to set the host to localhost in all cases. At r9410, the Util.html (trunk) tests pass on IE7. I'm running these from a virtual host (so the window.location.href is something like  http://192.168.1.7/path/to/tests). Where are you seeing failures.

I'm obviously not saying that you should use "localhost". The patch I've come up with (see parch-2060-r9410-A0.diff) sets a.href before reading from windows.location.host - I observed that when setting a.href after reading windows.location.host the a object gets corrupted and we can no longer access a.host, a.port etc. I'll try to come up with a small example demonstrating the issue.

Any OpenLayers example using a port different from 80 fails.

[crschmidt]

I believe that I can confirm that examples on ports other than 80 fail. I'm debugging right now.

[crschmidt]

So, looking at it in Visual Studio, it seems that *all* of the URL related properties are unset/'invalid argument' at the poiunt where the 'rulObject.host = a.hostwindow.location.host' is called. Ah, I see why: If I'm running on a different port, then the url set is " http://localhost:8080:8080/", which is clearly not what we're looking for (and is likely an invalid URL through and through). However, I believe this is a different issue than the other one poeople have encountered, which I haven't reproduced yet.

[crschmidt]

So,  http://trac.openlayers.org/attachment/ticket/2109/accessible.html is the other example that was given, and I can't reproduce an error with that code in IE7,  http://crschmidt.net/mapping/popup/openlayers/examples/2060.html .

[elemoine]

Chris, have you tried my patch? It seems to fix it, although I don't understand why.

[crschmidt]

Your patch changes the order of things -- specifically, the regex-based code (on which Tim says:

11:50:04 < tschaub> and I don't think any of that is needed now anyway
11:50:16 < tschaub> hostname is the property that includes host:port
11:50:29 < tschaub> I think that whole chunk of code was wrong

) so that it isn't used. Instead, url is set directly on the 'a' object before Tim's code has a chance to run, and therefore, the URL isn't the broken :8080:8080 that you see above.

So, I believe, based on what I understand:

• This has only been a problem running on different ports (or presumably :80 in the URl, but IE7 at least seems to just replace that)
• The other users reporting this problem both mention running under "Tomcat", which defaults (I believe) to running on :8080, so this is likely the same issue
• The code that is in place breaks when running on a different port, and can be reproduced in the tests
• Tim thinks that the code that is causing this problem is 'wrong'

I believe that the solution for this is for Tim to rework the code so that the regex-iffed section of code is replaced by something that isn't wrong, and we continue from there. Eric, can yuou confirm that you have *only* had this issue on ports other than 80?

[elemoine]

Replying to crschmidt:

Your patch changes the order of things -- specifically, the regex-based code (on which Tim says: {{{ 11:50:04 < tschaub> and I don't think any of that is needed now anyway 11:50:16 < tschaub> hostname is the property that includes host:port 11:50:29 < tschaub> I think that whole chunk of code was wrong }}} ) so that it isn't used. Instead, url is set directly on the 'a' object before Tim's code has a chance to run, and therefore, the URL isn't the broken :8080:8080 that you see above. So, I believe, based on what I understand: * This has only been a problem running on different ports (or presumably :80 in the URl, but IE7 at least seems to just replace that) * The other users reporting this problem both mention running under "Tomcat", which defaults (I believe) to running on :8080, so this is likely the same issue * The code that is in place breaks when running on a different port, and can be reproduced in the tests * Tim thinks that the code that is causing this problem is 'wrong' I believe that the solution for this is for Tim to rework the code so that the regex-iffed section of code is replaced by something that isn't wrong, and we continue from there. Eric, can yuou confirm that you have *only* had this issue on ports other than 80?

I confirm.

[elemoine]

Replying to crschmidt:

Your patch changes the order of things -- specifically, the regex-based code (on which Tim says:

11:50:04 < tschaub> and I don't think any of that is needed now anyway
11:50:16 < tschaub> hostname is the property that includes host:port
11:50:29 < tschaub> I think that whole chunk of code was wrong

) so that it isn't used. Instead, url is set directly on the 'a' object before Tim's code has a chance to run, and therefore, the URL isn't the broken :8080:8080 that you see above. So, I believe, based on what I understand:

• This has only been a problem running on different ports (or presumably :80 in the URl, but IE7 at least seems to just replace that)
• The other users reporting this problem both mention running under "Tomcat", which defaults (I believe) to running on :8080, so this is likely the same issue
• The code that is in place breaks when running on a different port, and can be reproduced in the tests
• Tim thinks that the code that is causing this problem is 'wrong'

I believe that the solution for this is for Tim to rework the code so that the regex-iffed section of code is replaced by something that isn't wrong, and we continue from there.

This reasoning makes sense to me. I understand now why changing the order of things made the problem go away.

[tschaub]

Ok, so I did the refactoring I should have done before. Quite a bit of the old function was unnecessary (and causing issues/adding complication).

So, again, tests pass. I've tested in the following browsers/conditions. If you find an issue with this function, *please* note carefully the conditions that cause failure.

IE6, IE7, IE8:  http://192.168.1.7/path/to/tests and  http://192.168.1.7:88/path/to/tests

FF3:  http://localhost/path/to/tests,  http://localhost:88/path/to/tests, and  file:///path/to/tests

Opera 9.62:  http://localhost/path/to/tests and  http://localhost:88/path/to/tests (other tests fail on Opera, but the createUrlObject related ones pass)

[tschaub]

working createUrlObject

[tschaub]

Tests passing in Safari 3 as well (only tested  http://localhost/path/to/tests).

[tschaub]

And regarding my "that whole chunk of code was wrong", I was referring to parts shown in red in the patch - not the regex stuff I added in r9404.

[elemoine]

Patch looks great, and works for me. Sorry for not being precise in my reports, but it took me a while to figure out that the problem occurred when using a port other than 80.

[tschaub]

(In [9413]) Making createUrlObject more reliable. Tests demonstrate what to expect. r=elemoine (pullup #2060)

[crschmidt]

(In [9414]) Pull up changes for 2.8.

Fix for ArcIMS GetFeatureInfo broken (Closes #2110) createUrlObject not working in IE (Closes #2060)