Ticket #2398 (closed bug: fixed)

Opened 3 years ago

Last modified 3 years ago

var grandchild is undefined

Reported by: ahocevar Owned by: ahocevar
Priority: blocker Milestone: 2.9 Release
Component: Format.KML Version: 2.9 RC2
Keywords: Cc:
State: Complete

Description

The following fails:

<description><![CDATA[...]]>
</description>

Also:

<description>
<![CDATA[...]]></description>

The following works:

<description>
<![CDATA[...]]>
</description>

Also:

<description><![CDATA[...]]></description>

The proposed patch should fix the issue. Tests still pass in FF3.5. Please review

Attachments

openlayers-2398.patch Download (2.2 KB) - added by ahocevar 3 years ago.
openlayers-2398.2.patch Download (1.0 KB) - added by ahocevar 3 years ago.

Change History

Changed 3 years ago by ahocevar

  Changed 3 years ago by fredj

  • state set to Review

  Changed 3 years ago by bartvde

  • milestone changed from 2.10 Release to 2.9 Release

  Changed 3 years ago by bartvde

  • owner changed from tschaub to ahocevar
  • state changed from Review to Commit

Looks good Andreas, please commit. I can confirm tests pass in IE6 as well.

  Changed 3 years ago by ahocevar

  • status changed from new to closed
  • state changed from Commit to Complete
  • resolution set to fixed

(In [10138]) fixed KML parsing for cases where a line break is only before or after a CDATA block. r=bartvde (closes #2398)

  Changed 3 years ago by Zed

  • status changed from closed to reopened
  • type changed from feature to bug
  • summary changed from Parsing of KML attributes gets confused with line breaks to var grandchild is undefined
  • priority changed from minor to blocker
  • state changed from Complete to Needs More Work
  • version changed from 2.8 to 2.9 RC2
  • resolution fixed deleted

Code now breaks at line 905 - KML data fails to display:

if(grandchild.nodeType == 3 grandchild.nodeType == 4)

fails with var "grandchild" is "undefined". Var grandchild is declared twice (along with a few other variables) in the code, which may be the problem? Line 900 also has a redundant fall through to the default statement. It may also be good to change some of the comparisions from == to === and != to !==

I would change the code but I don't feel fully qualified enough to do so.

  Changed 3 years ago by bartvde

  • state changed from Needs More Work to Awaiting User Feedback

Zed, do you have an example page to reproduce your problem? Also which browser are you using?

  Changed 3 years ago by Zed

  • state changed from Awaiting User Feedback to Needs More Work

firefox 3.6.2 with firebug and IE 7.0.5730.11 (both fail)

This page is an example:  http://www.bigyak.net.au/dev/bikeflows.htm

You need to be centred on Melbourne or Sydney, Australia (it should default to Melbourne). Turn off the overlay labelled "Edit Layer" before clicking on map for any reason, as clicks are designed to do different stuff that may confuse. The overlays labeled "Empty Layer: x" should say something else when bug is fixed.

Let me know if I can help further.

  Changed 3 years ago by elemoine

I'm wondering whether

if(grandchildren.length >= 1 || grandchildren.length <= 3) {

shouldn't be:

if(grandchildren.length >= 1 && grandchildren.length <= 3) {

?

follow-up: ↓ 10   Changed 3 years ago by bartvde

I remember at one point in the review I had the same thought.

in reply to: ↑ 9   Changed 3 years ago by ahocevar

Replying to bartvde:

I remember at one point in the review I had the same thought.

I guess you are both right. Must have missed that.

Changed 3 years ago by ahocevar

  Changed 3 years ago by ahocevar

  • state changed from Needs More Work to Review

With the above patch, Zed's data will be parsed in Safari, but Firefox chokes on the too long CDATA blocks. Anyway, the patch fixes the regression. Please review.

  Changed 3 years ago by ahocevar

Oh, after uploading the patch in Safari I see that Firefox has finished parsing the KML and it also works. Just takes ages.

  Changed 3 years ago by bartvde

  • state changed from Review to Commit

Looks good, please commit in trunk and I will pull this up to the 2.9 branch later on.

  Changed 3 years ago by ahocevar

  • state changed from Commit to Pullup

  Changed 3 years ago by bartvde

  • status changed from reopened to closed
  • resolution set to fixed

Pulled up in r10205

  Changed 3 years ago by bartvde

  • state changed from Pullup to Awaiting User Feedback

Zed, are you able to confirm that this fix (r10205) works for you? Do I need to get you a new OpenLayers.js file for you to test, or do you build yourself? If you need a new build, please let me know your e-mail address.

  Changed 3 years ago by bartvde

Zed, I've put up a new version of OpenLayers with this fix included on my website, can you please verify that the problem you reported has been fixed. TIA!

 http://www.osgis.nl/openlayers/OpenLayers.js

  Changed 3 years ago by Zed

  • state Awaiting User Feedback deleted

No - I can't test it, although the fix makes sense. Please send: ancestors .. bigyak"net'au and I'll try it. Thanks for your work.

  Changed 3 years ago by Zed

  • state set to Review

OK - using script on your server - problem appears to be fixed - great. To review you need to be looking at Sydney, Australia - see pull down list at bottom left of screen.

 http://www.bigyak.net.au/dev/bikeflows.html

However the layer switcher and the attribution and lat/lon strings are all in the top left of the screen now (FireFox & IE)! Which is perhaps a new bug (nothing do to do with the KML) or the update requires CSS changes?

It doesn't happen at this url (url has no trailing l as seen in the above url)

 http://www.bigyak.net.au/dev/bikeflows.htm

which is using the release candidate:

http://openlayers.org/api/2.9-rc2/OpenLayers.js

Any ideas?

  Changed 3 years ago by bartvde

  • state changed from Review to Complete

Hi Zed, that's a different issue, because I don't have the OpenLayers style css files on my webserver. So it is a configuration issue, nothing to worry about. Thanks for your test and report back.

Note: See TracTickets for help on using tickets.