Google Style Popups

Complete. See http://svn.openlayers.org/sandbox/CloudAmber/.

I'll reopen this in the event that it is something that can be integrated into the trunk. Thanks for the contribution!

I really appreciate the work that CloudAmber did to make the google style popups. I have a few issues with them though: 1. they fail if you instanstiate the popup with no content 2. they shouldn't really overwrite the base popup class. it is inherently an anchored bubble and should inherit/overwrite it. If it is to be used with just a plain popup, then it shouldn't have the stick.

from Matt Priour m priour A kestrelcomputer D com

I agree with Matt on issue 2. For an internal project I rewrote the CloudAmber code a bit to make it work in OL 2.5, but it still overwrites the default Popup.js (and AnchoredBubble etc). I'd love to see this Google style bubble make it into OL 2.6. If someone with more time is interested to continue where we left off, please drop me a line and I'll make the code available.

6 days would be paid by a customer of ours for that. I should be able to spend time on that late december or early january. I would be interested in your code.

Replying to pgiraud:

6 days would be paid by a customer of ours for that. I should be able to spend time on that late december or early january. I would be interested in your code.

Ask funky_c for his rewrite and enjoy 6 days of skiing.

Google style popups for 2.5. No guarantees. Need img-corners.zip as well.

Use with patch for 2.5

OK, in the patch you find the code we used. Again: it's not clean, since it overwrites the standard Popup.js Popup/* files, but it works.

Hack: you can predefine the innner size of the popup as a global variable (I know, it sux): var POPUP_WIDTH = 240; var POPUP_HEIGHT = 200;

Disclaimer: not intended to go into the trunk. Just for educational purposes. All credit goes to CloudAmber and all complaints go into the bitbucket. ;-)

Replying to rdewit: Is it possible to have my email address be either: - made invisible/obfuscated - removed from the change history please?

I am thinking about doing a reimplementation of the CloudAmber popup without overwriting the base classes. If I really should do that, I would like to contribute it. In general, are there still plans to include this popup in a future release?

The main problem I see is that GUI initializing takes place in the initialize function of Popup.js, so the only possibility would be not to call the initialize functions of the parent classes or to overwrite the settings made there in the inherited class. Is there a chance to change the code of Popup.js for a future release? My idea would be, that the initialize functions of Popup.js and inherited classes call a function named for example "initGUI", which I would override in my own class.

This feature is intended to be included in the future 2.6 release coming soon. We are currently work on that hard, though nothing really stable at the moment.

The code will perhaps be completely separated to the existing classes. I'm supposed to work on that tomorrow morning and I know that other core developers want to have a look soon so that it has a chance to get into trunk. Please also note that this feature has been marked as blocker for 2.6.

Here's a new patch for this ticket. As proposed, the FramedBubble doesn't inherit from an existing class. Though, it behaves like an AnchoredBubble and offers the same APIMethod and APIProperties. I kept the same namespace. I'm not sure this is a good idea though.

What this code does :

• resize the bubble based on its content,
• allow user to define his own bubble decoration by setting a different image and dimensions (see framedPopups.html and click on "change decoration"),
• shift the map if bubble isn't entirely viewable

What should be easy to add to the code :

• the animated panning when map is shifted,
• the animation when the popup is resized

What the code is still missing :

• a way to handle overflow 'auto' when the content size is too large,
• the close button creation should rely on a property of the bubble,
• a well documented procedure on how to create the decoration bubble properties (an image with dimensions is available on the framedPopups.html example page though)

Replying to pgiraud:

Here's a new patch for this ticket. As proposed, the FramedBubble doesn't inherit from an existing class. Though, it behaves like an AnchoredBubble and offers the same APIMethod and APIProperties. I kept the same namespace. I'm not sure this is a good idea though. What this code does : - resize the bubble based on its content, - allow user to define his own bubble decoration by setting a different image and dimensions (see framedPopups.html and click on "change decoration"), - shift the map if bubble isn't entirely viewable What should be easy to add to the code : - the animated panning when map is shifted, - the animation when the popup is resized What the code is still missing : - a way to handle overflow 'auto' when the content size is too large, - the close button creation should rely on a property of the bubble, - a well documented procedure on how to create the decoration bubble properties (an image with dimensions is available on the framedPopups.html example page though)

Pierre!!!! This is great work. I've got a window to do some OL work, so I'm going to take this excellent patch and I'm going to poke it a bit to see if I can't get it to fit inside the classical (but yes, admittedly, flawed) popup architecture.

Give me a day or so and let's see how it advances. Big Big Big thanks for your work on this... you have singlehandedly reeled in the diaspora of the original CloudAmber code. Bravo!

note that the patch that cr5 has added to this ticket is generated from my 'pop' sandbox, which can be found at /sandbox/euzuro/pop

The following is my personal list of things I would like to get done before this patch goes in:

1) add better examples 2) improve nd comments 3) add support for 'maxSize' to complement 'minSize' 4) overflow: auto when content size is too large 5) better close button support

I'd be willing to cut numbers 4 and 5 if we are in a rush to get it out, but I think they are important.

2 and 3 are done.

working now on "better examples"

ok. a better example is now in there as FramedPopup.js

basically, i wanted something that would show how to get this working with a fully relative popup. it is done.

the only thing that remains is a suitable image. if someone could take this: http://trac.openlayers.org/browser/sandbox/euzuro/pop/img/cloud-popup-relative.PNG

and make it transparent again, this example would look a whole lot nicer.

i'll still take a look at fixing (4) the overflow bit

but otherwise, i think this is pretty damn close to ready for official review

ok. i have fixed 1-4. the process for creating one of these beasts is still far from "easy", but it is at least *doable*. We should probably re-do the cool helper image that ?idontknowwho? created, being more explicit about which values correspond to what in the actual wierd json blocks code. I would be happy to do that.

as soon as someone can make a prettified version of this beast:

http://trac.openlayers.org/browser/sandbox/euzuro/pop/img/cloud-popup-relative.PNG

then I can take it and make the helper image, and then i think this baby is ready to go.

nope. i lied. we still need to do a thorough memory cleanup, especially since this business is creating a *lot* of divs. but we're close!

might i repeat that hopefully someone can help me with the alpha image??

Erik, I just sent a new image with transparency. I first sent one with shadow but there are other issues to fix if we want this. The last one commited in : http://trac.openlayers.org/browser/sandbox/euzuro/pop/img/cloud-popup-relative.png should match what you are looking for.

pierre to the rescue!

thanks for this quick help. i am unfortunately too exhausted now to finish up the job, but we are very very very close now. the memory management is done and in.

the example running with your new png looks good, just some small tweeking to fix up the br and bl ones. minor stuff.

i'll do that and then redo the "how to build me" image and then this should be good to go and up for review.

thanks again pierre!

ps. what do you mean by "other issues to fix if we want this?"

gnight

I'd add to the list something like #1339. Do others think we should have the ultra slick popups that zoom the map when you try to wheel scroll the content?

Latest patch file for new framed popups.

I found some issues I want to list here before I forget about them :

• once the popup is closed, if you try to zoom in/out the map continues to pan (with tween effect),
• Erik, you had the good idea to keep l, b, r, t order consistent. But why didn't you choose the css order instead (t, r, b, l) ?

• with the overflow set to auto we have weird display effects on FF if we try to pan the map and the opened popup gets outside the viewport, I think we should consider temporarily setting the overflow to hidden, as google maps does.

Erik, this code assumes there is always a this.closeDiv, but this is optional when you create the popups, so it needs checks for the existance of the closeDiv or not?

OpenLayers.Popup.FramedCloud crashes for me on IE6 (it makes the whole browser crash and close).

comment from b. grünefeld on dev list:

Hi all,
recently somebody (i think it was Erik) posted a link to a new popup class implementation. http://dev.openlayers.org/sandbox/camptocamp/FramedPopups/examples/framedPopups.html

Then i found FramedCloud.js and AmberCloud.js and some other Ol changes at the web and intergrate this stuff in my own OL build and everything looks fine - in Firefox.
In IE 6 the popup (AmberCloud) could opened but it has a black frame in background. Firebug told me that xxx_FrameDecorationDiv_0 and xxx_FrameDecorationDiv_1 are black  painted :-(
is there a possiblity to control the style of the FrameDecorationDiv
or is there another workaround?

adding comment from pierre from the mailing list:

Hello Erik, Tim and all,

I would first like to congratulate Erik for his great and useful job
on the new popups. It sounds like it's almost ready to go in.
However, there are still some issues to fix.

The most important for me (and a customer I work on this for) is the
ability to scroll the popup content without any interaction with the
map (avoid zoom in/out).

I first wondered I we were able to prevent map from zooming when
wheeling on controls UI's (LayerSwitcher, panZoomBar) and proposed a
simple patch to fix this. [1]

Back to the popup specific issue, my first thought was to observe
mousewheel events and simply stop event propagation. But, the
MouseWheel has already observers for those events and as they are
registered before, they are informed first and the map is zoomed
in/out. We could imagine dealing with some sort of priority but I
didn't find a way to do it.
Should we consider having a proper registry for mousewheel events and
not simply calling observe() ?

We could also imagine temporarily disabling the MouseWheel handler.
Tim, are you confident with the feature proposed by sbenthall [2] ?
How could we implement this ? Do we have to detect that the mouse
cursor entered and leaved the popup to call enable/disable the wheel
handler ?

Any help on that would be appreciated.

Regards,
Pierre


[1] http://trac.openlayers.org/ticket/1382
[2] http://trac.openlayers.org/ticket/1339

Replying to pgiraud:

I found some issues I want to list here before I forget about them : - once the popup is closed, if you try to zoom in/out the map continues to pan (with tween effect), - Erik, you had the good idea to keep l, b, r, t order consistent. But why didn't you choose the css order instead (t, r, b, l) ?

good eye pierre. I've fixed the tweening popup on zoom issue. that is a great one.

regarding the l,b,r,t bit... i hadnt thought about css ordering... i was basing it on the OpenLayers.Bounds class, which is the one I remember. Which is better? I am unsure. If you think I should change them, I'll def change them. Let me know.

so i got sidetracked halfway through my OL day and didnt get nearly what i'd wanted to done. i'll be on it again tomorrow morning. i think i have a solution to the overflow problem, btw.

....ok, so i have a reasonable fix for the overflow problem, but it doesn't work because of #1403. There are, of course, other ways to do it, but the most elegant relies on the map events working correctly.

another day and still these damn popups are not finished.

Replying to pgiraud:

- with the overflow set to auto we have weird display effects on FF if we try to pan the map and the opened popup gets outside the viewport, I think we should consider temporarily setting the overflow to hidden, as google maps does.

ok. r6403, r6404, r6405 have solved this one. on to the next (there are many)

Replying to bartvde:

Erik, this code assumes there is always a this.closeDiv, but this is optional when you create the popups, so it needs checks for the existance of the closeDiv or not?

ok. this is fixed in r6406. marching on...

Replying to bartvde:

OpenLayers.Popup.FramedCloud crashes for me on IE6 (it makes the whole browser crash and close).

can someone besides me do a checkout of the latest from sandbox/euzuro/pop and see if this is still true? i have ie6 here and it is more or less working. for some reason, the scroll bar seems to be showing up as a hole in the popup... but i'm working on it. at any rate, it certainly doesn't kill my browser

good new issue brought up by cr5 today which is that these popups do not respect that passed-in anchor.size or anchor.offset properties. this needs to be fixed.

Another issue is that we don't take into account the scrollbar size when determining the autosize of the popup, so if a popup is wide enough to overflow, and has a non-breakable wide feature like an image or a long line of text, we'll get a scrollbar on the bottom, and a scrollbar on the right hand side even if the popup could have been taller to accomodate for it.

And, Erik says he knows about this, so I'll just document it: when the popup is in the lower left or lower right, the image ends up looking wrong, due to incorrect settings for those positions.

Replying to crschmidt:

And, Erik says he knows about this, so I'll just document it: when the popup is in the lower left or lower right, the image ends up looking wrong, due to incorrect settings for those positions.

For the record, this is fixed in r6445. I didn't update the patch though.

Yay, thanks.

So, outstanding issues:

1. AmberCloud should be an addin
2. anchor size/offset is ignored, can be checked with 'sundials.html' in the pop/examples/ directory to see
3. Scrollbar not taken into account when autosizing
4. Doesn't work in IE? Or at least, might not?

Are there other issues? I think that's it.

r6292 claims to add an 'extra bit of width for scrolling', but I guess it's not working?

r5569 might be helpful for the scrollbar issue, from tcoulter's sandbox

For the record, rdewit claimed on IRC last night that when opening the popups such that the image is displayed to the lower right instead of the top, there is still a 'white border' on the sundials.html example.

Replying to crschmidt:

Yay, thanks. So, outstanding issues: 1. AmberCloud should be an addin

i will pull that code out as a last step. for now until the end of the development of this thinG, it is useful to have both of them in there for testinG.

1. anchor size/offset is ignored, can be checked with 'sundials.html' in the pop/examples/ directory to see

ok, i have spent all afternoon workinG on this, and finally have reached a solution. i was just about to check it in when it occurred to me another way to do this that will work even better and even more eleGantly. i will implement that over the weekend and check it in.

1. Scrollbar not taken into account when autosizing

thanks for the link to the vespuccio sandbox, cr5. i followed it and sent mail to the freeGix or whatever its called dude to see if we can Get an ICLA from him to use that patch of code.

1. Doesn't work in IE? Or at least, might not?

God save us, it really doesnt work on ie6. somethinG funky with the alpha transparency. black backGround for *some* (but not all) of the imaGe bits. very wierd. have not beGun to debuG or even think about debuGGinG. any help here Greatly appreciated.

Are there other issues? I think that's it.

1. anchor size/offset is ignored, can be checked with 'sundials.html' in the pop/examples/ directory to see

ok, i have spent all afternoon workinG on this, and finally have reached a solution. i was just about to check it in when it occurred to me another way to do this that will work even better and even more eleGantly. i will implement that over the weekend and check it in.

and the sun shines down upon us. my moment of inspiration at commit time turned out to be a Good one. implemented and workinG in just about the time it takes for my wife to Get ready to Go out on a date. anchor size and offset now taken into account with framed popups. see r6475

i lament to say that after much hackinG today, the "scrollbar" issue has only worsened itself. there is a Great confusion in the loGic of the popups in the pop sandbox riGht now that will need to rectify itself. unfortunately, this is takinG way lonGer than i thouGht it would and than i have time to dedicate to tryinG to fix it. alas, it will have to wait until next week. sorry, Guys.

1. Doesn't work in IE? Or at least, might not?

God save us, it really doesnt work on ie6. somethinG funky with the alpha transparency. black backGround for *some* (but not all) of the imaGe bits. very wierd. have not beGun to debuG or even think about debuGGinG. any help here Greatly appreciated.

Fixed with r6522. We now suppose that we don't set opacity for framed popups. This is a bit harsh but fine enough.

There is certainly a better workaround, but I'm too lazy tonight.

is this patch ready to be review and committed? anyone got any last words?

Replying to sderle:

is this patch ready to be review and committed? anyone got any last words?

No, it's not. There is still significant bad behavior in the way that scrollbars work when content is too big for the popup.

as cr5 says above, the only thing left to do here is fix the overflow issues with the scrollbar. im going to try to fix this first thing next week (mon-tues)

as can be denoted from the timeline, I have been at work on this for the past 14 hours, pretty much straight on.

i have not yet solved this definitively, but i am well on my way. instead of poking and pulling and praying for a miracle, i just decided to hell with it and i've gone back to the very drawing board.

today i spent almost entirely getting the Anchored and AnchoredBubble popups working correctly. I made a new example that clearly demonstrates them, which you can see here:

http://dev.openlayers.org/sandbox/euzuro/pop/examples/popupMatrix.html

It demonstrates clean functionality for the Anchored and AnchoredBubble popups in varying combinations of closeBox, overflow, autoSize, minSize, maxSize. Currently there are a few tests which challenge the autoSize to DO THE RIGHT THING given wierd conditions like wide and short (dynamic and static) and there will be more coming. At this point, I think all the hard bits are worked out (at least for those two popup types) and it remains only to further elaborate the examples in the matrix to prove that all is well.

Once that is done, then I can go back and make another row for the FramedCloud popup. This will take some tweeking, for sure, but I am hopeful that it will be only minor, as all the complicated scrollbars and multiple paddings and closebox calculations should in theory now be all working just automagically.

At any rate, I have budgeted myself all of tomorrow's work day to tackle these remaining bits, and to hopefully put a lid on this seemingly interminable ticket. My deep apologies to everyone that I have allowed this to drag out so long. I should have done what I did today one month ago, it would have saved me a boatload of time. Ah, but such are the ways. Note to self: Any time you plan to do any serious dev work, give yourself at least two full days.

ok! so, as predicted, the re-engineering work did pay off and getting the framed popup to behave well was not a difficult task.

The full three rows of: http://dev.openlayers.org/sandbox/euzuro/pop/examples/popupMatrix.html

are working as expected now, which is good.

i would even say this beast is ready for review, were it not for a few things:

• the sundials example is unfortunately still not working. luckily, this doesnt seem to be my fault entirely -- the getRenderedDimensions() function is not returning big enough size to fit the content :-(
• we might want to consider an option that does some sort of resizing on image load? i dont really want to do this, but...

and this one, which is unfortunately the showstopper:

• in ie6, the framedcloud popups are leaking around 20Mb of memory each time you open or close them. it's too late and i'm too burned out to look into it. ff behaves correctly.

i have done a:

$ svn diff http://svn.openlayers.org/trunk/openlayers http://svn.openlayers.org
/sandbox/euzuro/pop > popup.patch

...and pleasantly enough, the changes aren't too bad. most of the patch file ends up being the sundials kml.

note that a simple patch file, however, will not work, as new binary files need to be added.

There are really a few options here. I can split this ticket out into many different tickets, one for the framed popups and then others for things like autoSize, panIntoView, etc.

I would love to *not* do that, because it will take lots of time. As the popupMatrix.html clearly shows, all combinations of the thing are still working, so there should be no issue.

one thing I do have to do before getting into trunk land is to pull the amber cloud stuff out and get that ready for a addins/customPopups/amber

Replying to euzuro:

ok! so, as predicted, the re-engineering work did pay off and getting the framed popup to behave well was not a difficult task. The full three rows of: http://dev.openlayers.org/sandbox/euzuro/pop/examples/popupMatrix.html are working as expected now, which is good.

Cool.

i would even say this beast is ready for review, were it not for a few things: * the sundials example is unfortunately still not working. luckily, this doesnt seem to be my fault entirely -- the getRenderedDimensions() function is not returning big enough size to fit the content :-(

Yes. The problem is that the <p> tag adds a margin, which getRenderedDImensions doesn't account for. I've confirmed that there the scrollWidth/scrollHeight are incorrect for this content. I can not see an easy way to change this: there is no property in the DOM which has the height/width of the element including this margin. (What the hell causes that? who knows!)

So, I've committed a change to the sundials.html example which sets the margin on .olPopup p {} to 0px, and that's fixed the issue. I think this is a fine workaround for an unworkable issue.

* we might want to consider an option that does some sort of resizing on image load? i dont really want to do this, but...

No need, in my opinion: If you want your images in popups, give them sizes.

and this one, which is unfortunately the showstopper: * in ie6, the framedcloud popups are leaking around 20Mb of memory each time you open or close them. it's too late and i'm too burned out to look into it. ff behaves correctly.

Oof.

Replying to euzuro:

i have done a: {{{ $ svn diff http://svn.openlayers.org/trunk/openlayers http://svn.openlayers.org /sandbox/euzuro/pop > popup.patch }}} ...and pleasantly enough, the changes aren't too bad. most of the patch file ends up being the sundials kml.

If this bothers anyone, we can pull this out and I can add it in seperately.

note that a simple patch file, however, will not work, as new binary files need to be added. There are really a few options here. I can split this ticket out into many different tickets, one for the framed popups and then others for things like autoSize, panIntoView, etc. I would love to *not* do that, because it will take lots of time. As the popupMatrix.html clearly shows, all combinations of the thing are still working, so there should be no issue.

Agreed. I don't see the need.

one thing I do have to do before getting into trunk land is to pull the amber cloud stuff out and get that ready for a addins/customPopups/amber

Agreed. One thing to possibly do would be to just dump the current patch (as you've done) and put the attachments somewhere, then remove them from the sandbox totally in preperation for the patch, and deal with the addin after we get 2.6RC1 out.

Replying to crschmidt:

Replying to euzuro:

ok! so, as predicted, the re-engineering work did pay off and getting the framed popup to behave well was not a difficult task. The full three rows of: http://dev.openlayers.org/sandbox/euzuro/pop/examples/popupMatrix.html are working as expected now, which is good.

Cool.

i would even say this beast is ready for review, were it not for a few things: * the sundials example is unfortunately still not working. luckily, this doesnt seem to be my fault entirely -- the getRenderedDimensions() function is not returning big enough size to fit the content :-(

Yes. The problem is that the <p> tag adds a margin, which getRenderedDImensions doesn't account for. I've confirmed that there the scrollWidth/scrollHeight are incorrect for this content. I can not see an easy way to change this: there is no property in the DOM which has the height/width of the element including this margin. (What the hell causes that? who knows!)

excellent work, señor schmidt. thank you for finding and (sort of) fixing this

So, I've committed a change to the sundials.html example which sets the margin on .olPopup p {} to 0px, and that's fixed the issue. I think this is a fine workaround for an unworkable issue.

* we might want to consider an option that does some sort of resizing on image load? i dont really want to do this, but...

No need, in my opinion: If you want your images in popups, give them sizes.

good. we can always look at putting in an intelligent patch to the autosizing code at a later date.

and this one, which is unfortunately the showstopper: * in ie6, the framedcloud popups are leaking around 20Mb of memory each time you open or close them. it's too late and i'm too burned out to look into it. ff behaves correctly.

Oof.

oof is right.

Replying to euzuro:

Replying to crschmidt:

Replying to euzuro:

ok! so, as predicted, the re-engineering work did pay off and getting the framed popup to behave well was not a difficult task. The full three rows of: http://dev.openlayers.org/sandbox/euzuro/pop/examples/popupMatrix.html are working as expected now, which is good.

Cool.

i would even say this beast is ready for review, were it not for a few things: * the sundials example is unfortunately still not working. luckily, this doesnt seem to be my fault entirely -- the getRenderedDimensions() function is not returning big enough size to fit the content :-(

Yes. The problem is that the <p> tag adds a margin, which getRenderedDImensions doesn't account for. I've confirmed that there the scrollWidth/scrollHeight are incorrect for this content. I can not see an easy way to change this: there is no property in the DOM which has the height/width of the element including this margin. (What the hell causes that? who knows!)

excellent work, señor schmidt. thank you for finding and (sort of) fixing this

So, I've committed a change to the sundials.html example which sets the margin on .olPopup p {} to 0px, and that's fixed the issue. I think this is a fine workaround for an unworkable issue.

* we might want to consider an option that does some sort of resizing on image load? i dont really want to do this, but...

No need, in my opinion: If you want your images in popups, give them sizes.

good. we can always look at putting in an intelligent patch to the autosizing code at a later date.

and this one, which is unfortunately the showstopper: * in ie6, the framedcloud popups are leaking around 20Mb of memory each time you open or close them. it's too late and i'm too burned out to look into it. ff behaves correctly.

Oof.

oof is right.

An oof update just in: this huge memory blorp does *not* occur in ie7.

well, a month after the initial push and I think we have something workable here.

See the newly attached popups.patch

The IE memory bug was fixed by tim schaub's genius, and the hide/show issue I solved by doing a small rewrite to reuse the divs (entonces no hiding necessary)

There are two possible improvements that remain in my mind at this point:

1) Autosizing to images as they load 2) Special property on Framed specifying that there will only ever be *one* popup on the map at a time, therefore use a common pool of divs for the all the popups, instead of generating five new ones with each popup we open. This would be an easy thing and I might tackle it this weekend if I get the energy.

Neither of the above do I consider necessary before the review of this patch can happen. In fact, I think the review can happen RIGHT NOW.

One thing that does need to be done is a wiki page showing how the hell to build your own one of these bastards, because it is unfortunately not *that* easy.

To review, I recommend just checking out my sandbox. I have been keeping it *strictly* up to date with trunk, so there is no need to apply the patch to trunk, etc etc (though you can if you wish -- see instructions with the addition of the patch to the ticket for which other files you need)

This is the enormous patch file (takes between 5 and 10 minutes to generate) for the new popups deal. If you are trying to apply this to your checkout of trunk, you will also have to include the examples/popupMatrix.jpg and img/cloud-popup-relative.png

note that changing the overflow in the css for the default popup to 'auto' may be a violation of API. We can always change that back, it is easy.

put this in examples/

put this in img/

Replying to euzuro:

note that changing the overflow in the css for the default popup to 'auto' may be a violation of API. We can always change that back, it is easy.

I think it's the right decision since we have decent support for scrolling popups now, which we never did before. I'm in favor of it.

Replying to euzuro:

There are two possible improvements that remain in my mind at this point: * Autosizing to images as they load

Do you have a good idea of how to do this? Not that I think we need to do it now, but if you feel like you have an idea of how to do it, I'd like to open a new ticket to remind us to look into it in 2.7 if possible.

2) Special property on Framed specifying that there will only ever be *one* popup on the map at a time, therefore use a common pool of divs for the all the popups, instead of generating five new ones with each popup we open. This would be an easy thing and I might tackle it this weekend if I get the energy.

I'm somewhat in favor of putting this *after* 2.6, just for reduction of complexity. But I'll bow to you on getting it in.

I'm going to start a review now.

not really done yet, got distracted by a lot of other things, but one thing I'd like to do is just change the closebox to use CSS instead of hardcoding the image. It seems pretty simple. I'll look into it tomorrow.

lib/OpenLayers.js references AmberCloud still

fixed lib/OpenLayers.js.

We don't currently have a way to determine the size of the closebox image from CSS, so it's hardcoded to 17x17 in the code for the time being. I think that the right fix for this is to add some code like tim's 'styleValue' code in the scalebar addin

http://trac.openlayers.org/browser/addins/scalebar/trunk/lib/OpenLayers/Control/ScaleBar.js#L546

in trunk, but I'm happy going with what we've got and leaving that for post 2.6: in the meantime, one can replace the image with another image, and I've put a comment in indicating that using anything other than 17px x 17px will not behave as expected. Essentially, if you do that, you have to override the size in the CSS with "width: 22px!important;", and then your closebox size calculation will be inaccurate.

The current code is more forward thinking, and I don't think it iwll break anything: I've kept the old image as the default, so people who have manually changed the image file will still get their changed image.

latest from svn

Alright, I've now completed my review. I can confirm that the current patch does nothing to break backwards compatibility of existing apps so far as I can tell, and the end result is a much nicer set of code that will do well for us going forward.

Erik, and all others who made this happen, thank you for all your help.

It is my opinion that the patch that I've attached, current from SVN, is ready for trunk. Please commit.

latest version -- try 2

(In [6718]) Adding support for new generation 'framed' popups. This patch includes numerous improvements to the main popups, including the addition of autoSizing, panIntoView, and full support for overflow:auto of the contents div. Thanks go out to the CloudAmber folks, to Pierre in la belle France, to the guys at TOPP and of course, to senior cr5 for his patience and help in the last and final stretch. this is a huge improvement in the popup arena and couldn't have been done without the broad help of everyone out there in the community. Thank you everyone for making this possible. Big step for OpenLayers. (Closes #926)