Skip to content
This repository

Fix Google terms/poweredby/popup #472

Merged
merged 1 commit into from almost 2 years ago

6 participants

Peter Robins Andreas Hocevar Éric Lemoine ManoMarks Garey Mills Tim Schaub
Peter Robins
probins commented May 17, 2012

Fix to bring Google/v3/repositionMapElements() in line with current Google setup.
This changes the logic so it no longer depends on the order of the child divs in Google's mapObject, but uses zIndex instead. This is no more robust, but I couldn't think of any other way to distinguish the divs. AFAICS the only reliable way to fix this is to ask Google nicely if they could add an id to their divs, so OL can fish them out correctly.

examples/google-v3 with this patch shows all working - terms, logo, plus the popup when you click on 'Map.Data'.

@elemoine I assume you want this in 2.12

Andreas Hocevar
Owner

Thanks for addressing this @probins. Great work, only a minor remark: It would be interesting to see if this also works with more recent versions of the GMaps API (i.e. >3.7), and I think the google-v3.html file should also be updated to use a more recent version than the no longer available 3.6. Using something newer gives us more leeway until the next fix is in order.

Peter Robins
probins commented May 17, 2012

yes, forgot to say I also tested with my own setup using the latest GMaps, i.e. without specifying a version no.

Curiously, when I used a for loop starting from 0, the terms weren't included, so presumably they get loaded a bit later. The whole process is very flaky.

Andreas Hocevar ahocevar merged commit 92f04a7 into from May 17, 2012
Andreas Hocevar ahocevar closed this May 17, 2012
Éric Lemoine
Owner

What about the examples? Do we need to update them so they explicitly use Google Maps API 3.7?

Peter Robins
probins commented May 17, 2012

As this now works with the latest version (3.9), I would suggest removing the v= param from the examples. If and when Google change their setup again, then a new fix will have to be found. Specifying the version doesn't really solve the problem, it's just a temporary workaround until a fix is implemented.

I'm wondering whether it would be worth a brief explanation of the issue in the release notes?

Andreas Hocevar
Owner

@probins: If you can prepare a pull request with your suggested example changes and release notes, it would be much appreciated.

ManoMarks

Requests for older versions of the Google Maps API roll up to the current released version, so 3.6 will give you 3.7.

Garey Mills
garey commented May 17, 2012

I have

/**

  • Constant: VERSION_NUMBER */ OpenLayers.VERSION_NUMBER="OpenLayers 2.10 -- $Revision: 10721 $";

and the four lines 140 - 143

  • // move the Map Data popup to the container, if any 140
  • while (div.lastChild.style.display == "none") { 141
  • container.appendChild(div.lastChild); 142
  • } 143

are not in the v3.js file.

I made the changes in the diff to OpenLayers-2.10/lib/OpenLayers/Layer/Google/v3.js and they did not hide the popup.

Tim Schaub
Owner
tschaub commented May 17, 2012

Including a version parameter means the current hack will work for longer. We should be testing with each new version, but I don't think we should encourage users to always go with the latest.

Peter Robins
probins commented May 18, 2012

@ManoMarks see https://developers.google.com/maps/documentation/javascript/basics#Versioning

@garey you should use the patch in this pull request. The previous lastChild logic no longer works (because it is no longer the last child)

Peter Robins
probins commented May 18, 2012

@tschaub If we do use a version, then ISTM the only one we can rely on is the 'frozen' one, currently 3.7. The hack here depends on something which is not part of Google's api, so there's no reason why this can't be changed in one of their updates to the 'release' version.

ManoMarks
Andreas Hocevar
Owner

@ManoMarks It is to comply with the Maps API Terms Of Use: the "Powered by" and "Map Data" links need to be clickable, which they are not if we do not apply this hack. Having said that, there may be better ways to accomplish this, and any contributions to unhack this would be greatly appreciated.

ManoMarks
Andreas Hocevar
Owner

@ManoMarks We have a dom element on top of the Google Map to catch all events, because we control the dragging etc. and don't want this to go through to the GMaps API.

Garey Mills
garey commented May 18, 2012

@probins I tried using the patch and it did not work.

Éric Lemoine
Owner

@probins, what version do we get today if v isn't set in the URL? 3.7? More generally are we assured we get the “release version” when v isn't set. I've found no documentation on this.

If 3.7 is the frozen version and we know OpenLayers 2.12 works with 3.7 I think we should mention it in the Layer.Google.v3 doc, and use v=3.7 in the examples. Doing otherwise would mean loosing information. I want to know what GMaps API version OpenLayers 2.12 supports. Google not giving me 3.7 when I request it (with v=3.7) in the future is another story, for which we can't do anything.

Peter Robins
probins commented May 19, 2012

@elemoine Yes, I too have come to the conclusion this whole issue should be described in Google.v3 rather than in the release notes.

If you don't specify a version you get the latest 'nightly' release, currently 3.9. See the link I gave before - "specified with v=3 or by omitting the v parameter". Google themselves recommend "Production applications should always specify a minor version", so, yes, I think the recommendation should be to use the current frozen version (though you will automatically get this by leaving it set to v=3.6 (a 'retired' version), so there's no real need to change this in the examples). Perhaps a good idea would be to encourage developers to use the frozen version on their production server, but to omit the v param on their localhost/development to provide early warning of problems; there would then be 6 months to find a fix before the frozen version is affected.

I can put together something for the docs, if you like.

Peter Robins
probins commented May 19, 2012

correction to my previous post: "there would then be 6 months" - that's probably not true, as Google might well change the 'release' as well as the 'nightly' version, so there would be 3 months max.

As I said before, this pull request works with both frozen and current 3.9

Éric Lemoine
Owner

@probins, thanks for the detailed explanation. Yes, it would be great if you could add some notes in the docs. The docs should also mention what “release” version the current code supports (3.7 for 2.12 and current master). I now understand why you're suggesting not specifying any version in the examples – to catch problems as early as possible. It makes sense. But we should also encourage people to use frozen versions, so a quick note about that in the examples would also make sense. Thank you!

Éric Lemoine
Owner

I was confused with the terms “release” et “frozen”. So as I understand it version 3.7 is supported by the current OpenLayers.Layer.Google.v3 code. Version 3.8 and 3.9 are currently supported, but these versions are moving so they may be incompatible with OpenLayers 2.12 and master in the future. And when version 3.8 changes from “release” to “frozen” version 3.7 won't be available anymore. And if 2.12 does not work with 3.8 at that time, which might happen, we're screwed – a 2.12.1 or something will be necessary.

I hope my understanding is more correct this time.

Peter Robins
probins commented May 20, 2012

yup, that's what happened before: using 3.6 was only a temporary workaround and stopped working once 3.6 was 'retired'.

Just updating docs at the moment. I plan to put a general description of the issue in Google.v3, and put info specific to which api version is supported in the release notes. And yes if Google do make a change that's not compatible with current OL, there will need to be some sort of amended OL function release.

Éric Lemoine elemoine commented on the diff May 20, 2012
lib/OpenLayers/Layer/Google/v3.js
((11 lines not shown))
147  
-        container.appendChild(termsOfUse);
148  
-        termsOfUse.style.zIndex = "1100";
149  
-        termsOfUse.style.bottom = "";
150  
-        termsOfUse.className = "olLayerGoogleCopyright olLayerGoogleV3";
151  
-        termsOfUse.style.display = "";
152  
-        cache.termsOfUse = termsOfUse;
153  
-
154  
-        var poweredBy = div.lastChild;
155  
-        container.appendChild(poweredBy);
156  
-        poweredBy.style.zIndex = "1100";
157  
-        poweredBy.style.bottom = "";
158  
-        poweredBy.className = "olLayerGooglePoweredBy olLayerGoogleV3 gmnoprint";
159  
-        poweredBy.style.display = "";
160  
-        cache.poweredBy = poweredBy;
  141
+        // depends on value of zIndex, which is not robust
  142
+        for (var i=div.children.length-1; i>=0; --i) {
3
Éric Lemoine Owner
elemoine added a note May 20, 2012

Using childNodes instead of children is certainly safer. Apparently IE < 9 does not (correctly) support children, nor does FireFox < 3.5. See https://developer.mozilla.org/en/DOM/Element.children.

Éric Lemoine Owner
elemoine added a note May 20, 2012

I have tested the google-v3 example in several browsers (IE 7 and 8, and FF 3 included) and I haven't actually noticed any issue.

Peter Robins
probins added a note May 21, 2012

according to http://www.quirksmode.org/dom/w3c_core.html IE problems are only with comment nodes, which doesn't apply here. However, can easily change to childNodes if you like

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Showing 1 unique commit by 1 author.

May 17, 2012
Peter Robins Fix Google terms/poweredby/popup f435a98
This page is out of date. Refresh to see the latest.

Showing 1 changed file with 24 additions and 20 deletions. Show diff stats Hide diff stats

  1. 44  lib/OpenLayers/Layer/Google/v3.js
44  lib/OpenLayers/Layer/Google/v3.js
@@ -137,27 +137,31 @@ OpenLayers.Layer.Google.v3 = {
137 137
         var cache = OpenLayers.Layer.Google.cache[this.map.id];
138 138
         var container = this.map.viewPortDiv;
139 139
         
140  
-        // move the Map Data popup to the container, if any
141  
-        while (div.lastChild.style.display == "none") {
142  
-            container.appendChild(div.lastChild);
143  
-        }
144  
-
145 140
         // move the ToS and branding stuff up to the container div
146  
-        var termsOfUse = div.lastChild;
147  
-        container.appendChild(termsOfUse);
148  
-        termsOfUse.style.zIndex = "1100";
149  
-        termsOfUse.style.bottom = "";
150  
-        termsOfUse.className = "olLayerGoogleCopyright olLayerGoogleV3";
151  
-        termsOfUse.style.display = "";
152  
-        cache.termsOfUse = termsOfUse;
153  
-
154  
-        var poweredBy = div.lastChild;
155  
-        container.appendChild(poweredBy);
156  
-        poweredBy.style.zIndex = "1100";
157  
-        poweredBy.style.bottom = "";
158  
-        poweredBy.className = "olLayerGooglePoweredBy olLayerGoogleV3 gmnoprint";
159  
-        poweredBy.style.display = "";
160  
-        cache.poweredBy = poweredBy;
  141
+        // depends on value of zIndex, which is not robust
  142
+        for (var i=div.children.length-1; i>=0; --i) {
  143
+            if (div.children[i].style.zIndex == 1000001) {
  144
+                var termsOfUse = div.children[i];
  145
+                container.appendChild(termsOfUse);
  146
+                termsOfUse.style.zIndex = "1100";
  147
+                termsOfUse.style.bottom = "";
  148
+                termsOfUse.className = "olLayerGoogleCopyright olLayerGoogleV3";
  149
+                termsOfUse.style.display = "";
  150
+                cache.termsOfUse = termsOfUse;
  151
+            }
  152
+            if (div.children[i].style.zIndex == 1000000) {
  153
+                var poweredBy = div.children[i];
  154
+                container.appendChild(poweredBy);
  155
+                poweredBy.style.zIndex = "1100";
  156
+                poweredBy.style.bottom = "";
  157
+                poweredBy.className = "olLayerGooglePoweredBy olLayerGoogleV3 gmnoprint";
  158
+                poweredBy.style.display = "";
  159
+                cache.poweredBy = poweredBy;
  160
+            }
  161
+            if (div.children[i].style.zIndex == 10000002) {
  162
+                container.appendChild(div.children[i]);
  163
+            }
  164
+        }
161 165
 
162 166
         this.setGMapVisibility(this.visibility);
163 167
 
Commit_comment_tip

Tip: You can add notes to lines in a file. Hover to the left of a line to make a note

Something went wrong with that request. Please try again.