Ticket #107 (closed defect: fixed)

Opened 9 months ago

Last modified 8 months ago

Popup tests cause errors in Map.js

Reported by: tschaub Assigned to: sbenthall
Priority: major Milestone: 0.5
Component: GeoExt.Popup Version: 0.5 RC1
Keywords: Cc:
State: Complete

Description

Running the Popup.html tests, I see an unhandled error in Map.js about maxExtent being null. I'm guessing this is because the map is still panning to include the popup when the map is destroyed. This may not require any change in GeoExt. Investigating more now.

Attachments

107.patch (2.3 kB) - added by tschaub on 06/25/09 00:08:36.
don't tween during tests without a timeout
geoext-107.patch (3.0 kB) - added by ahocevar on 06/25/09 12:02:39.
make tests for anchored popups more clear and fixed a fatal typo
107.2.patch (3.3 kB) - added by tschaub on 06/25/09 20:56:44.
don't tween and count moves per action

Change History

06/25/09 00:08:36 changed by tschaub

  • attachment 107.patch added.

don't tween during tests without a timeout

06/25/09 00:11:38 changed by tschaub

  • state changed from None to Review.

Though this is a test only change and could go in without a review, I'd like to get feedback to make sure I'm not missing anything that was being tested in test_anchorPopup. I've set the map so it doesn't do any tween animation while panning. This allows us to destroy the map immediately after a popup is opened without getting errors from Map.js (will link to OL ticket soon).

It looks to me like in test_anchorPopup the popup should expect only one "move" event. Please confirm and I'll commit.

06/25/09 00:27:13 changed by tschaub

See http://trac.openlayers.org/ticket/2156 for the OpenLayers issue.

06/25/09 12:02:39 changed by ahocevar

  • attachment geoext-107.patch added.

make tests for anchored popups more clear and fixed a fatal typo

06/25/09 12:29:38 changed by ahocevar

  • state changed from Review to Commit.

An anchored popup needs to have its anchor centered on the feature. So it needs to move not only when the map moves, but also when it collapses, expands and resizes. I added a test for resize (which was previously missing), and hopefully made clearer what gets tested by proper test messages and additional comments.

Note that the the move handler gets called twice with the move action, but this is unrelated to tweening. It is because pop.isVisible() returns false at the time pop.position() is first called with the move event enabled. This only happens in the test, I made sure it does not happen in the example by setting a conditional breakpoint. I think this has to do with some deferred rendering.

While at it, I fixed a fatal typo that caused panIntoView() to be executed when a feature with a popup re-enters the viewport.

Please commit if you agree with my changes, and set the ticket state to Pullup.

06/25/09 20:56:44 changed by tschaub

  • attachment 107.2.patch added.

don't tween and count moves per action

06/25/09 21:05:16 changed by tschaub

Thanks for the panIntoView fix and the test explanations Andreas.

I've added a patch that I hope changes things in a way that makes it easier for others to also understand what is going on.

The tests confirm that we get one move per action for each of setCenter, collapse, expand, and resize. If we ever get more than one move, tests will fail. If the moves don't occur in the right places, tests will fail.

I added one additional map.setCenter call. This makes it so the popup is actually visible before running the tests. I don't think this has to do with deferred rendering on the part of Ext. We know the order that code gets executed - and a timeout cannot start running code while our test function is still running. I do see that the popup element.style is still set so that the popup is far to the top left (-100000px or whatever) and visibility is "hidden". Didn't look deeper to see if this is a browser issue (like the reflow isn't getting forced with popup.show - doLayout doesn't do the trick either). Anyway, I don't think we are hiding a big issue here by setting the center twice (though I am still curious about what is going on).

Anyway, thanks for the review.

06/25/09 21:11:08 changed by tschaub

  • state changed from Commit to Pullup.

In with r1157.

06/29/09 11:38:27 changed by ahocevar

  • status changed from new to closed.
  • resolution set to fixed.

(In [1159]) Ticket pullups since 0.5 RC1 (closes #103, #106, #107)

07/02/09 10:22:43 changed by ahocevar

  • state changed from Pullup to Complete.