OpenLayers OpenLayers

Ticket #1479 (closed feature: fixed)

Opened 8 months ago

Last modified 8 months ago

Popup.FramedCloud reports error in setContentHTML

Reported by: elemoine Assigned to: euzuro
Priority: minor Milestone: 2.6 Release
Component: general Version: 2.6 RC1
Keywords: Cc:
State: Complete

Description

The error:

"position has no properties" Framed.js line 254

My code looks like that: 

popup = new OpenLayers.Popup.FramedCloud(...);
popup.setContentHTML(html);
map.addPopup(popup);

The error occurs in setContentHTML.

The code problem is: setContentHTML -> setSize -> updateBlocks -> BUG! because this.relativePosition is null at this point. popup.moveTo hasn't been called yet.

The workaround is just to not use setContentHTML and always pass the html to the constructor.

Attachments

createBlocks.patch (1.6 kB) - added by crschmidt on 04/02/08 07:19:25.
createBlocks.2.patch (2.2 kB) - added by crschmidt on 04/02/08 09:12:26.
this includes the test file i added. tests fail before patch, pass after.
createBlocks.3.patch (2.3 kB) - added by crschmidt on 04/02/08 09:56:33.
pop.patch (4.9 kB) - added by euzuro on 04/02/08 11:17:22.
tests pass ie7, ff
pop2.patch (5.1 kB) - added by euzuro on 04/06/08 23:34:10.
take two. thanks for the sharp eye, bro

Change History

04/01/08 16:50:35 changed by crschmidt

  • milestone set to 2.6 Release.

Alternatively, to only call setHTML after the popup is added to the map (so you can change the content if it's already on the map).

If we do another RC, I'll take a fix for this into the branch, but I think it's just something we need to document, otherwise.

04/02/08 07:19:25 changed by crschmidt

  • attachment createBlocks.patch added.

04/02/08 07:21:19 changed by crschmidt

  • state set to Review.

Eric:

Could you look at / try out this patch and let me know what you think? Essentially, I've made it so that both createBlocks and updateBlocks bail if things aren't going the way they're supposed to.

04/02/08 09:12:26 changed by crschmidt

  • attachment createBlocks.2.patch added.

this includes the test file i added. tests fail before patch, pass after.

04/02/08 09:51:51 changed by elemoine

Chris, with your patch, the popup window doesn't render complete, there are parts missing. You didn't see that?

04/02/08 09:56:33 changed by crschmidt

  • attachment createBlocks.3.patch added.

04/02/08 09:58:14 changed by crschmidt

Eric:

Good catch. The 'cont' variable was only defined in the loop, so the initial createBlocks would work, but updateBlocks never would. I'm not sure why this worked in the quick test I tossed together, but I've tested this better now.

I think this is complete.

04/02/08 10:06:01 changed by elemoine

  • state changed from Review to Commit.

04/02/08 10:08:01 changed by crschmidt

  • keywords set to pullup.
  • state changed from Commit to Pullup.

(In [6765]) The FramedCloud popup throws an error if you attempt to call setContentHTML before you add the popup to the map. To prevent this, don't call construct or updateBlocks if relativePosition is null, to prevent this error message. r=elemoine, (Pullup #1479)

04/02/08 11:17:22 changed by euzuro

  • attachment pop.patch added.

tests pass ie7, ff

04/02/08 11:19:49 changed by euzuro

  • owner set to euzuro.
  • status changed from new to assigned.
  • state changed from Pullup to Review.

instead of adding conditionals and everything, just pull the first position from the positionBlocks hash to use for building the blocks.

then in the updateBlocks, we can still createBlocks() if there is no relative position... it's the actual updating that we don't want to do if there is no relative position.

I think this makes things a little more intuitive.

Good work on finding this bug, by the way. hopefully more reports like this will come in so we can plug up all the little holes

04/06/08 23:23:30 changed by crschmidt

createBlocks function was given a return value before: this was kept by your patch (return true) but is no longer useful since it will always return true. This can probably be dropped.

Other than that, this looks fine. Upload a new patch and I'll run tests on it.

04/06/08 23:34:10 changed by euzuro

  • attachment pop2.patch added.

take two. thanks for the sharp eye, bro

04/06/08 23:46:26 changed by euzuro

tests pass ff & ie7

04/07/08 00:03:44 changed by crschmidt

Looks good. Please commit.

04/07/08 00:03:56 changed by crschmidt

  • state changed from Review to Commit.

04/07/08 00:09:46 changed by euzuro

  • state changed from Commit to Pullup.

(In [6800]) fix issue where we try to createblocks without a proper this.relativePosition. Robustness++. (Pullup #1479)

04/07/08 21:51:04 changed by crschmidt

  • keywords deleted.
  • status changed from assigned to closed.
  • state changed from Pullup to Complete.
  • resolution set to fixed.

(In [6819]) Pullup commits from trunk to 2.6 branch:

  • virtualStyle typo (Closes #1495)
  • JSON fix for Safari 3.1 (Closes #1493)
  • panzoombar off-by-one (Closes #1486)
  • Handler Hover exception (Closes #1480)
  • Popup.framedcloud exception (Closes #1479)
  • VML Renderer when including namespace in page (Closes #1477)
  • SLD/Rule/Filter changes -- most of this commit (Closes #1492)