Fix Google terms/poweredby/popup #472

Merged
merged 1 commit into from May 17, 2012

Conversation

Projects
None yet
6 participants
@probins
Contributor

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

@ahocevar

This comment has been minimized.

Show comment
Hide comment
@ahocevar

ahocevar May 17, 2012

Member

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.

Member

ahocevar commented May 17, 2012

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.

@probins

This comment has been minimized.

Show comment
Hide comment
@probins

probins May 17, 2012

Contributor

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.

Contributor

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.

ahocevar added a commit that referenced this pull request May 17, 2012

Merge pull request #472 from probins/goopop
Fix Google terms/poweredby/popup. Thanks @probins for the quick fix.

@ahocevar ahocevar merged commit 92f04a7 into openlayers:2.12 May 17, 2012

@elemoine

This comment has been minimized.

Show comment
Hide comment
@elemoine

elemoine May 17, 2012

Member

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

Member

elemoine commented May 17, 2012

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

@probins

This comment has been minimized.

Show comment
Hide comment
@probins

probins May 17, 2012

Contributor

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?

Contributor

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?

@ahocevar

This comment has been minimized.

Show comment
Hide comment
@ahocevar

ahocevar May 17, 2012

Member

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

Member

ahocevar commented May 17, 2012

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

@ManoMarks

This comment has been minimized.

Show comment
Hide comment
@ManoMarks

ManoMarks May 17, 2012

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

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

This comment has been minimized.

Show comment
Hide comment
@garey

garey 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.

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.

@tschaub

This comment has been minimized.

Show comment
Hide comment
@tschaub

tschaub May 18, 2012

Member

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.

Member

tschaub commented May 18, 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.

@probins

This comment has been minimized.

Show comment
Hide comment
@probins

probins May 18, 2012

Contributor

@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)

Contributor

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)

@probins

This comment has been minimized.

Show comment
Hide comment
@probins

probins May 18, 2012

Contributor

@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.

Contributor

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

This comment has been minimized.

Show comment
Hide comment
@ManoMarks

ManoMarks May 18, 2012

I am not an OpenLayers developer, and I am trying to understand what is
happening. Why is it that OpenLayers needs to essentially hack into the
content coming from the Maps API? Is it trying to figure out the position
of the copyright notification? For what purpose? I'm concerned that this
might continue to break as it is not a documented part of the API, and
therefore also potentially terms breaking.

Thanks,

Mano Marks
Maps Developer Advocate
http://manomarks.net/+
http://twitter.com/ManoMarks

On Fri, May 18, 2012 at 3:20 AM, Peter Robins <
reply@reply.github.com

wrote:

@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.


Reply to this email directly or view it on GitHub:
openlayers/openlayers#472 (comment)

I am not an OpenLayers developer, and I am trying to understand what is
happening. Why is it that OpenLayers needs to essentially hack into the
content coming from the Maps API? Is it trying to figure out the position
of the copyright notification? For what purpose? I'm concerned that this
might continue to break as it is not a documented part of the API, and
therefore also potentially terms breaking.

Thanks,

Mano Marks
Maps Developer Advocate
http://manomarks.net/+
http://twitter.com/ManoMarks

On Fri, May 18, 2012 at 3:20 AM, Peter Robins <
reply@reply.github.com

wrote:

@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.


Reply to this email directly or view it on GitHub:
openlayers/openlayers#472 (comment)

@ahocevar

This comment has been minimized.

Show comment
Hide comment
@ahocevar

ahocevar May 18, 2012

Member

@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.

Member

ahocevar commented May 18, 2012

@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

This comment has been minimized.

Show comment
Hide comment
@ManoMarks

ManoMarks May 18, 2012

Ah, interesting! My apologies again for not knowing much about the
OpenLayers code base, but why is it that this isn't clickable
otherwise? Is there an overlay or something on top of it?

Mano Marks
Maps Developer Advocate
http://manomarks.net/+
http://twitter.com/ManoMarks

On Fri, May 18, 2012 at 4:21 AM, ahocevar
reply@reply.github.com
wrote:

@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.


Reply to this email directly or view it on GitHub:
openlayers/openlayers#472 (comment)

Ah, interesting! My apologies again for not knowing much about the
OpenLayers code base, but why is it that this isn't clickable
otherwise? Is there an overlay or something on top of it?

Mano Marks
Maps Developer Advocate
http://manomarks.net/+
http://twitter.com/ManoMarks

On Fri, May 18, 2012 at 4:21 AM, ahocevar
reply@reply.github.com
wrote:

@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.


Reply to this email directly or view it on GitHub:
openlayers/openlayers#472 (comment)

@ahocevar

This comment has been minimized.

Show comment
Hide comment
@ahocevar

ahocevar May 18, 2012

Member

@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.

Member

ahocevar commented May 18, 2012

@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

This comment has been minimized.

Show comment
Hide comment
@garey

garey May 18, 2012

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

garey commented May 18, 2012

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

@elemoine

This comment has been minimized.

Show comment
Hide comment
@elemoine

elemoine May 18, 2012

Member

@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.

Member

elemoine commented May 18, 2012

@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.

@probins

This comment has been minimized.

Show comment
Hide comment
@probins

probins May 19, 2012

Contributor

@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.

Contributor

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.

@probins

This comment has been minimized.

Show comment
Hide comment
@probins

probins May 19, 2012

Contributor

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

Contributor

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

@elemoine

This comment has been minimized.

Show comment
Hide comment
@elemoine

elemoine May 19, 2012

Member

@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!

Member

elemoine commented May 19, 2012

@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!

@elemoine

This comment has been minimized.

Show comment
Hide comment
@elemoine

elemoine May 20, 2012

Member

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.

Member

elemoine commented May 20, 2012

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.

@probins

This comment has been minimized.

Show comment
Hide comment
@probins

probins May 20, 2012

Contributor

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.

Contributor

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.

- poweredBy.style.display = "";
- cache.poweredBy = poweredBy;
+ // depends on value of zIndex, which is not robust
+ for (var i=div.children.length-1; i>=0; --i) {

This comment has been minimized.

@elemoine

elemoine May 20, 2012

Member

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.

@elemoine

elemoine May 20, 2012

Member

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.

This comment has been minimized.

@elemoine

elemoine May 21, 2012

Member

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.

@elemoine

elemoine May 21, 2012

Member

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.

This comment has been minimized.

@probins

probins May 21, 2012

Contributor

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

@probins

probins May 21, 2012

Contributor

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