Google layers won't get resized properly on map.updateSize()

[gregers]

We had the same problems as described in ticket #830 before 2.7, but fixed them on our own. The changes in 2.7 broke our fix.  http://trac.openlayers.org/ticket/830

Steps to cause the bug: - Start with small map. Fixed size. - Increase map size (more than one tile width). - Run map.updateSize(). - Switch to another google layer - this layer only updates the tiles within the maps original size

After many hours digging through obfuscated Google Maps code, I found out that it requires a coordinate before calling checkResize(). If the layer div and mapObject.getSize() are equal, the resizing is skipped. So if checkResize() is called to early, mapObject.getSize() will get updated to the div size, but the rest is not executed.

The patch overrides EventPane.moveTo and calls checkResize() after the layer has a position.

Reproduced the error in a modified example.

[gregers]

Example to reproduce the bug

[gregers]

Patch to fix the bug

[gregers]

Oh, while I remember it. Here is my interpretation for the obfuscated mapObject.checkResize function:

if layerDiv.offsetSize != mapObject.size
	mapObject.size = layerDiv.offsetSize
	mapObject.centerLatLng = calcCenterLatLngFromViewportPx

	for each zoomlevel-div (guess)
		add/remove tile img nodes (probably)

	//some other code I haven't looked at

	trigger map onresize event

[vdeparday]

I had the same issue because I am using Mapfish which creates a small map at first and then resizes it. I applied the patch and it works fine now.

[crschmidt]

Confirmed by watching over vdeparday's shoulder.

[crschmidt]

(In [8990]) "Switching between google layers after map resize", after map size change, as is used by mapfish and others, the google map can get confused as to its size and not resize correctly. This patch changes that so that the check happens on moveTo, so we know if the map has resized. The checkResize is a cheap call in the default case. r=me, patch by gregers, (Closes #1797)

[vdeparday]

After more testing, I realize that now all the tiles are updated when switching layer. However, I have a vector layer on top of the google layers and when I switch from one google layer to another, the vector layer is offset until the next map move. Also, this only happens the first time switching to a new google layer. If you switch to this layer, switch back to another one and then back on this one. It works fine. So should we reopen the ticket or open a new one?

[ahocevar]

brute-force approach to resizing invisible google layers

[ahocevar]

 1797.patch reverts the previous patch. The new patch creates a new GMap object when making a Google layer visible that was resized during being invisible. This fixes the issue. It has no noticeable performance impact.

To try it out, remove the style.css link in the spherical-mercator.html example and do the following:

• draw a vector feature
• change the size of the browser window
• switch to a different google layer

Without the patch, the position of the vector feature will show you that the new google layer is misaligned. With the patch, everything will be fine.

Tests continue to pass in FF3. Please review.

[vdeparday]

This is not a review but the patch works for me now in FF3.

[tschaub]

Try this:

• apply this patch and comment out the style.css link in spherical-mercator.html
• open spherical-mercator.html and expand the layer switcher
• switch from the google streets to the google satellite and back to google streets
• resize your browser
• try viewing the google satellite layer again

(Disregard the yahoo error.)

Seems like resizing while a previously viewed layer is invisible messes things up.

[ahocevar]

I finally found a way to do proper resizing without recreating the GMap object. For some reason, all tiles of the GMap need to be loaded before checkResize will work on previously invisible layers. The new patch also fixes the issue encountered by tschaub.

Please review again.

[ahocevar]

solid workaround after further investigation

[tschaub]

Thanks for the work on this Andreas. The behavior of the google layers look good now when resizing/switching etc.

[ahocevar]

(In [9333]) Fix GMap initialization sequence when resizing previously invisible layers. r=tschaub (pullup #1797)

[crschmidt]

Brought to trunk in r9348.

[pgiraud]

I'm reopening the ticket since the example provided by gregers still doesn't work. This seems to be a regression and it's blocker for 2.8. Sorry about that.

[ahocevar]

new fix which did not work when I looked at the issue last time, but now it does

[ahocevar]

This is indeed strange. My first thought when fixing it last time was to use the load event of the GMap object, but that did not cure the issue. Today it does. Please review (try with the google-resize.html example provided as attachment to this ticket).

[pgiraud]

Andreas, your last patch is OK. You can commit.

[ahocevar]

(In [9418]) Another fix for the problem with resized Google layers that were previously invisible. Has the GMaps API changed in the meantime? At least this fix (listening for the "load" event) feels like it makes more sense than the previous one (listening for the "tilesloaded" event). r=pgiraud (closes #1797)

[ahocevar]

Setting to "Needs Discussion" because there is no consensus yet whether this should go into 2.8 or not.

[crschmidt]

As I understand it, this:

• CHanged something that worked when we fixed it, to something that didn't
• Was a change due to a GMaps API change
• Was something that we put a lot of effort into fixing
• Affects MapFish/GeoExt (and probably other JS Frameworks with flexible map sizes)

As a result, I think it's worth including in 2.8: It was worth it the first time, it's still worth it this time (even though we aren't responsible for the regression, we've still got to deal with it.)

IF this ticket doesn't affect people (I think it does -- i just emailed the geoext list about it, and pierre bumped into it, after all) then we can bump it, but otherwise, it's worth another RC.

[elemoine]

I'l test Andreas' patch with GeoExt.MapPanel on Monday.

[crschmidt]

(In [9423]) Pullups for 2.8 RC5:

Google Layer resizing (Closes #1797) SelectFeature default scope (Closes #2115)

[ahocevar]

The solution to these resize problems would be to use just one GMap object and change its map type. This is a bit quirky and non-api at the moment (i.e. one layer for all types instead of one layer per type), but maybe future attempts to tackle the issue should go in this direction:

myGoogleLayer.type = G_SATELLITE_MAP;
myGoogleLayer.setMapType();
// now we have a satellite map

myGoogleLayer.type = G_HYBRID_MAP;
myGoogleLayer.setMapType();
// now we have a hybrid map

Hope this helps, and if anyone wants to create a patch to make this more consistent with the API, I'd be willing to mentor and review. I have the feeling that the right way to do it would be to store the mapObject as property on the prototype when first requested, and instances only do the mapType handling in setVisibility. This would also be a good step towards memory efficiency.

[ahocevar]

Reasons for re-opening the ticket:

• New related issues were found. See http://openlayers.org/pipermail/dev/2009-June/005006.html
• The above comment suggests a new solution. Hopefully someone wants to take this on.

[ahocevar]

one GMap2 instance per map

[ahocevar]

The above patch implements the one-instance-per-map approach I described earlier. Before adding unit tests, I would like to hear from users if this patch fixes the issue. I tried to reproduce the steps described in http://openlayers.org/pipermail/dev/2009-June/005006.html and it worked, but I would like to hear from others if it works in different configurations.

[ahocevar]

Setting state to Review since no user feedback was given -- looking for develper feedback instead :-)

[vdeparday]

Sorry Andreas, it was on my todo list and I forgot about it. I just tried it with Firefox on Kubuntu with the 2.8 stable and your patch and it fixes the issues for me on the gregers example but also in my application using extJS. Great work. Thanks a lot.

[tschaub]

(In [10016]) Adding manual acceptance test for multiple Google layers (see #1797).

[tschaub]

I added the multiple-google-layers.html example to demonstrate the current state of affairs here.

The first assertion in this test (adding & removing layers multiple times w/o errors) was addressed by r10013. Not specifically related to this ticket, but related to multiple Google layers in a map.

This ticket is about making the fourth assertion pass (resizing then switching doesn't screw up). This problem was a bit confounded by the issue addressed in r10014.

With the latest patch applied, the fourth assertion passes (resizing then switching works). However, the second, third, and fifth assertions fail. That is, when you add or remove a layer, the terms of service and powered by links of a previously visible layer are hidden. And, switching to the dummy layer doesn't hide a previously visible Google layer.

I like the idea of sharing a GMap object and associated elements. I think this patch is close. I'll work on this a bit to address the remaining issues.

[tschaub]

Alternative patch attached. This is still the single GMap approach, but differs in a number of ways:

• GMap elements are cached on OpenLayers.Layer.Google instead of the map
• the need for the moveend listener and setMapType has been removed
• GMap element visibility is only updated when necessary
• nothing special is done in layer.display (this was triggering a reflow with each layer.moveTo call)

All the assertions in the multiple-google-layer.html test pass. Still some subtle issues to work out:

• The terms of service link shows up when a layer is added even if the layer is not visible. It may be that GMap is setting the style display in a timeout. This occurs with and without this patch, so I don't consider it critical.
• When you remove a visible Google layer and a non-Google layer is set as base layer. The extent is wrong when you set another Google layer as base layer. This could be an issue with the "dummy" layer in the example. I'll investigate.

And a more serious issue:

• The spherical mercator example and the google example (and I imagine any example that adds multiple Google layers to the map before setting center) don't work with either of the single GMap patches (mine or Andreas'). This needs work.

[tschaub]

The "remove a visible Google layer" issue is addressed in #2462.

[tschaub]

one GMap alternative

[tschaub]

The latest patch fixes all issues I was able to find (or have energy for today at least). The acceptance tests only pass with the patch for #2462.

This latest patch cleans up a fair bit more, including properly disposing of the GMap container when layers are removed/destroyed. There is some unnecessary complexity and safe-guarding in the code because the destroy sequence for layers is weird. The event pane trashes layer.mapObject before the layer is removed from the map. The remove sequence is where we would naturally have the chance to deal with shared GMap elements when the last layer is removed.

Anyway, tests pass and examples work on FF. Will check IE tomorrow.

[ahocevar]

Hey Tim, thanks for your efforts on this! Here is my first impression (tested with FF3.5):

* The acceptance test page does not display any map in IE. I am trying to fix that now. * With the latest patch applied, switching to the dummy base layer will remove terms of use and powered by, but leave the previously selected google layer on the map. * When the dummy base layer is active and removing all google layer checkboxes, the last visibile google layer still remains on the map. Checking one of the checkboxes again will raise an error (this.pane is null in EventPane.js, L211).

Unit tests pass in IE7 though.

Will talk to you more on IRC.

[ahocevar]

Ok, the acceptance test works in IE. There must have been something wrong with my IE.

[ahocevar]

My first report is with just the latest patch for #1797 and #2462 applied. After also applying #2461, I get the following behavior for map1:

• Unchecking the streets layer of map1 after loading the example changes the zoom level to 15. This does not happen if I switch to a different layer and then back to streets before unchecking it.
• Unchecking the imagery layer works as expected, topography is set as new base layer.
• Unchecking the topography layer raises an error in FF3.5 (map is not defined, Google.js L298). In IE7, there is no error. The layer disappears from the map, but not from the layer switcher.
• In this state, when I switch to the dummy layer, the topography layer disappear from the layer switcher.
• Moving on from here seems to work fine.

In map2 everything works as expected.

[ahocevar]

The issue with the changing zoom level is related to #2462. Not sure about the other issue yet.

[ahocevar]

Stupid /me said that the acceptance tests work in IE, but they don't. Was accidently looking at FF3 in my Windows VM.

[ahocevar]

Same as above, but fixed a typo that caused the reported error in L298.

[ahocevar]

With attachment:1797.5.patch and the latest patch for #2462, all tests on the acceptance test page pass in FF3.5. Still unable to test on IE.

[tschaub]

Ok, acceptance test page works in IE with r10020.

The two remaining, and unassociated, issues that I've found are #2465 and #2466.

Accounting for those, everything (miraculously) appears to work in FF3.5, IE6, and IE8. That is, tests pass, the acceptance example works, and the google/spherical mercator examples work.

[ahocevar]

Great work Tim, please commit.

[tschaub]

Definitely a shared effort. I think this idea of using one GMap object is great. We should see if the same would make sense for other proprietary layers.

Seems to me like this ticket has lived a good enough life. I think any associated issues deserve a new ticket.

[tschaub]

(In [10021]) Reworking the Google layer so that a shared GMap object is used for all Google layers on a single map. This reduces the DOM overhead and gets rid of map resizing issues. Excellent solution by ahocevar. r=me,ahocevar (closes #1797).

[ahocevar]

(In [10036]) manual test showing a regression (see #1797)

[bartvde]

Andreas, Tim, any chance r10021 broke setOpacity, see ticket:2562 ?