Overlay locks up the browser and consumes 100% of CPU for certain browsers/graphics cards/operating systems

Subscribe.

With the latest overlay code from today, the performance problem seems to have magically fixed itself (at least for me)! Apparently this wasn't done intentionally - but it certainly doesn't hurt :)

Let's leave this issue open for now, though, because it's not totally clear that the problem is fixed for real.

Unfortunately, this problem is back for me with the latest overlay code from the git repository. I do wonder if it has something to do with caching... it's strange that it comes and goes.

Retested with Firefox, and I do not see the performance problems currently either (Mac OS X, Firefox 3.5). Interesting, seen it before multiple times.

Similar things happen for me on Ubuntu 9.10 with nvidia-glx-new and Firefox 3.5.5 (with current head). It doesn't freeze but "feels" very slow and especially scrolling jumps and the overlay moves every time above the toolbar and then gets rebuilt.

(sort of a subscribe)

This is driving me crazy right now, so I'm gonna tentatively mark this critical :) It seems like a number of people are or have been affected by it, and when the worst of it happens, it renders your site almost completely unusable...

On MacBook with GMA X3100 graphics card and OS X 10.6.2 and Safari 4.0.4, the overlay window pops up but nothing gets loaded no matter how long I wait. On same notebook with Firefox 3.5.5, it works but still slow.

Very slow performance on Firefox 3.5.5 / Ubuntu 9.10 / Nvidia GTS250 / Nvidia binary drivers.

Basically, top shows my CPU 87% utilized by Firefox, and 13% by X, neither of which generally use more than 3% each with overlay disabled.

Just in case another report was needed: (apologies if it wasn't)

My firefox shows 47% avg CPU usage with *just* the dashboard overlay open, while not clicking nor scrolling anything... closing that tab brings it back down to 4%. This is on a Lenovo T61, Windows Vista, Firefox 3.5.5

Some more details:

Ubuntu 9.10
Nvidia Driver version: 185.18.36-0ubuntu9
Graphis card: Quadro NVS 140M

Firefox: 3.5.5+nobinonly-0ubuntu0.9.10.1

Although I assume that just having the same nvidia version might not be enough, binary drivers usually differ quite a bit on different graphic cards..

Hint: It seems that I can't enable visual effects at the moment, that used to work before but is *extreeemely* slow (0.1 frames per second or something :)) at the moment. Even overlay module is fast compared to that ;)

@ksenzee, issue #649692 seems similar to what I reported except I tested it in Safari 4.0.4. I don't see the similar Errror: WRONG_DOCUMENT_ERR: DOM Exception 4 in activity window though.

Ubuntu 9.10
Nvidia Driver version: 185.18.36-0ubuntu9
Graphis card: Geforce GTS250

Firefox: 3.5.5+nobinonly-0ubuntu0.9.10.1

Overlay performance is the same whether or not the Visual effects are enabled, or for that matter, whether I use the nvidia drivers, or the open source Nouveau (2d-only) drivers. In my case at least, it doesn't look tied to the graphics hardware.

It seemed really sluggish on my work machine as well running Firefox 3.5.5, Windows Server 2008, Intel Xeon @ 2.5 GHz, 8 GB RAM, and an NVIDIA Quadro FX 1700 with 512 MB RAM.

Honestly, it was sluggish enough that I turned it off and was much happier. It felt like a nice addition, but maybe not a sensible default.

As per my original report at the top of this issue, here is the patch that "solves" the performance problem for me.

I'm providing this for two reasons:

1. So others can try it out as well, and we can then confirm that the code in this function is indeed the overall culprit.
2. We might seriously consider committing this patch as an interim step... although I believe it creates a bug or two, they are much smaller than the bug it fixes, so we could just leave this function dead until a better-performing solution is found. (The performance problems seem to be affecting more people than originally thought, and they definitely hurt the ability of some developers to even use the default install profile right now.)

@David_Rothstein:

Tried the patch, and it didn't ease the issue for me. CPU still pegged at 100% with the overlay open.

On ubuntu Jaunty with Firefox 3.5 I'm getting similar issues, firefox cpu usage goes from between 2-10% to around 70% on initializing the overlay and hovers between 50-70% just leaving it open. Visually it doesn't seem unacceptably slow yet, firefox doesn't hang etc. but I haven't really done anything with it yet and I don't happen to be doing any other CPU intensive tasks locally right now.

It also takes a lot longer to initialize the overlay than it does to change pages once it's opened (looks like about 5-6 times as long). Patch didn't do anything for me either, I hard refreshed and restarted firefox just to check.

Same issue for me.

Snow Leopard
Firefox 3.5.5

Just opening the overlay puts the processor usage at 100% and stays there even without doing anything.

I assume here are several different bugs involved which lead to the same result.

I was now able to enable visual effects and the overlay does now work perfectly.

So there's two separate bugs here, and no one else can reproduce mine? Great... :)

Patch from #18 didn't work for me either (no change whatsoever), Windows Vista, Firefox 3.5.5.

I get horrible scrolling with the overlay. I assume that is graphics...but all the transparencies and stuff aren't helping. It is sad that the linux drivers are so crapping considering my graphics card is an Radeon HD 4870 which should be MASSIVE overkill.

Since I do not see the benefit of the overally anyway I'll just disable it.

I have same problem with performance in Firefox and Safari on a MacBook Air

With that one, xorg jumps up to ~30%, and firefox jumps to ~40%, so it's different ;). I'll check xorg more closely without the patch later.

If I kill the box shadow, my CPU usage drops from 100% down to 40-50%.

Of course, if overlay is closed, I use 3%.

It's an improvement, but still problematic IMO.

We routinely rollback patches that do this to HEAD. Please rollback and re-commit when this is ready. We are late in the release cycle; we should be more strict than usual, not less.

Tested this performance problem on a 2Ghz Core Duo Mac with Firefox 3.6b4.

The performance issue can really be fixed here in this patch seen in #29:

diff --git modules/overlay/overlay-parent.css modules/overlay/overlay-parent.css
index 582409b..9714aa7 100644
--- modules/overlay/overlay-parent.css
+++ modules/overlay/overlay-parent.css
@@ -69,7 +69,7 @@
   overflow: visible;
   background: #fff url(images/loading.gif) no-repeat 50% 50%;
   -webkit-box-shadow: 8px 8px 8px rgba(0,0,0,.5);
-  -moz-box-shadow: 8px 8px 8px rgba(0,0,0,.5);
+  /*-moz-box-shadow: 8px 8px 8px rgba(0,0,0,.5);*/
   box-shadow: 8px 8px 8px rgba(0,0,0,.5);
 }
 .overlay #overlay-element {

However, the line to comment out is NOT the -moz-box-shadow, but rather the background loading.gif line.

When I keep the shadow and remove the image loading line, Firefox CPU usage drop from 40% to 5%. When I uncomment the image loading line, CPU jumps back to 40%.

background: #fff url(images/loading.gif) no-repeat 50% 50%; is always on!

In the cascading CSS, the image loading should be overridden by a 'none' to disable the image loading file from still being active.

Can someone else test this?

By the way I purge my browser cache to make sure the CSS load with and without the loading image and look at the CPU usage for Firefox between each test.

I'm doing a new patch. Tested my patch at the same time on Firefox 3.6b4 and Chrome for Mac latest dev version.

Firefox takes 5% CPU and Chrome 1% CPU running at the same time both browsers. ;-)

This patch removes the loading image throbber and keep the shadows.

The animated GIF is still running and I think that it's really the critter that steal CPU cycles. If we still need an image loading throbber, we will need to find a way to override it so as not to have it still enabled in the DOM.

Tested only on Mac! See my previous comment.

xmacinfo OMG thank you.

There's no visual change, this is just straight bug. There may well be other issues, so let's stick this back to active once this is in. Screenshot of patch + overlay enabled + top output to show it's normal levels.

Browser caching still had the throbber being displayed, misunderstood xmacinfo's comment.

So, this removes the loading throbber, however it takes about 10% of the time to actually load, so who gives a shit if there's a throbber or not.

Ok, I've committed #36 as a hot-fix. Setting back to active though because we'd like that to show up, just not in a way that it sucks CPU as though it were delicious, marshmallow and whipped cream-laden hot chocolate.

In other news, brb, I need to run to the kitchen. ;)

So now I get an initial 90% spike when the overlay loads, and scrolling is slow/jerky and spikes both xorg and firefox to about 30/40% each - but that's at least when something is happening rather than all the time.

Thanks for the commit.

The problem was that the loading image was still active in the CSS tree and not overriden by a more important declaration. Not a caching issue.

This screen capture have been made before the commit to show that the loading image was still active behind the overlay.

I agree, we should leave this open and find a better way to show the loading images and override it properly.

XD, that was not a patch. :-)

The patch in #18 is still the only one that does anything to solve the problem for me.

Surely someone else can reproduce that???

@David_Rothstein What is your environment? I'd like to duplicate your problem.

To fix this we need to dynamically add a class to the overlay container when the iframe is loaded.

From
<div id="overlay-container" class="ui-dialog-content ui-widget-content" [...]>

To
<div id="overlay-container" class="ui-dialog-content ui-widget-content loaded" [...]>

Once the "loaded" class is added in the "overlay-container", we could then override the loading image properly through CSS.

.overlay #overlay-container.loaded {background-image: none;}

I guess we need a jQuery pro here to add the "loaded" class when the page have finished loading.

We could then reapply the original loading line. ;-)

could any1 check if adding element-invisible does the trick to properly remove it?

@David_Rothstein What is your environment? I'd like to duplicate your problem.

@xmacinfo, thanks. See the very top of this issue for details on the environment (though I've since updated the graphics card driver to the latest one released this weekend, nothing else has changed).

I've played around with this a fair amount, clearing browser caches, etc, and doesn't seem to be related to that. As per the article linked to at the top of this issue, some of the performance issues with this graphics card likely aren't Drupal's fault, and indeed I can now experience some similar slowdowns on other graphics-intensive websites - but the overlay is still by far the worst :)

It would definitely be useful if anyone is able to look into whether the overlay.resize() function can be reduced or eliminated. It's an awful lot of processor-intensive code that doesn't seem to do a lot, and one of the ways in which its called is via toolbar-specific code in the overlay module which in addition to being theoretically out of place seems to be buggy anyway...?

In reply to #46:

I'm by far a jquery pro.... The load event of jquery does exactly what you are describing:

http://docs.jquery.com/Events/load

It's an event that fires as soon as whatever you specified in the jquery selector part. See also the examples in the link above.

@David_Rothstein: Is Firefox 3.6b5 available on your platform? Version 3.5, and to a lot more extent version 3.6, incorporate a new powerful JavaScript engine.

As for duplicating you environment, I'm not sure if I'll be able to do it using VirtualBox running Ubuntu. At the end it would use my Mac CPU and graphic card. But it may be worth a try.

@David_Rothstein: Is Firefox 3.6b5 available on your platform? Version 3.5, and to a lot more extent version 3.6, incorporate a new powerful JavaScript engine.

I'm sure it's possible to install them on Hardy - and at some point I'll move away from Hardy anyway since it's getting old :)

But I'm a little worried if Drupal doesn't perform well on this system because it's not that uncommon of a configuration. For now I think I'll stick on it to help debug...

I think it should be disabled by default unless it performs well on across the board (something I will have to due because of the poor performance, but at least I have the advantage of knowing what is causing it and how to fix it), not just on beta software. No need to drive people away because the admin interface laggs there computer.

By virtue of this being in peoples' faces, we fixed one critical performance issue within 48 hours. I have little faith that we will continue to fix performance problems if we disable it by default, just as we didn't fix performance problems with it when it was sitting in the queue an extra 6 weeks.

My comment was directed toward the release.

the release will hopefully not have performance issues like this.

(other than _really_ weird edge cases like Netscape Navigator 4.7 on Iphone or Safari on Solaris...)

Drupal has crossbrowser compatibility like no system I've seen, so our standard is pretty high.

    var autoResize = function () {
      if (typeof self.iframe.$element == 'undefined') {
        autoResizing = false;
        $(window).unbind('resize', windowResize);
        return;
      }
      var iframeElement = self.iframe.$element.get(0);
      var iframeDocument = (iframeElement.contentWindow || iframeElement.contentDocument);
      if (iframeDocument.document) {
        iframeDocument = iframeDocument.document;
      }
      // Use outerHeight() because otherwise the calculation will be off
      // because of padding and/or border added by the theme.
      var height = $(iframeDocument).find('body').outerHeight() + 25;
      self.iframe.$element.css('height', height);
      self.iframe.$container.css('height', height);
      self.iframe.$container.parent().css('height', height + 45);
      // Don't allow the shadow background to shrink so it's not enough to hide
      // the whole page. Take the existing document height (with overlay) and
      // the body height itself for our base calculation.
      var docHeight = Math.min($(document).find('body').outerHeight(), $(document).height());
      $('.ui-widget-overlay').height(Math.max(docHeight, $(window).height(), height + 145));
      setTimeout(autoResize, 150);
    };

errr, wtf? isn't this a bit heavy to be firing every 150ms? is there a way we can have this sorta event-triggered instead of a timeout that calls a timeout that calls a timeout ...

simply commenting out the setTimeout didn't change the behavior for me (wtf?), so can any1 who's experiencing bad performance try this? attached patch just comments it out for testing purposes

Just verifying this is a major wtf problem on Safari 4.x on a new MacBook Pro - the page scrolling is incredibly slow.

Also verifying this is a Major issue for me. Currently, if I'm doing anything in my D7 environment, I have to disable the overlay - the flickering when scrolling is distracting.

My setup:

Dell XPS M1330
Ubuntu Karmic
NVIDIA 185 driver
Firefoxx 3.5.5

I have tried all the patches on this issue and none of them seemed to make the slightest bit of difference.

• When the overlay first loads I see a large spike in CPU usage (up to 50% or so).
• If I sit there constantly moving the scroll wheel, CPU sits at 50%.
• When I scroll quickly, the top of the overlay appears to 'flicker' - sometimes appearing infront of the toolbar, sometimes half infront of it, and sometimes behind it (looks like it takes a few miliseconds to move itself behind the toolbar for every movement)
• When I scroll very very slowly, I can see what it happening a little better... For a split second, the entire middle section of the toolbar actually moves with the overlay, only to move back to it's proper position after a few miliseconds. As I increase the scroll speed, the 'flicker', or 'jumping' of the toolbar get more pronounced.

With the SeaMonkey web browser, there is no flickering, and no movement of the toolbar, however scrolling is slow - about 1/4 the speed of scrolling a Google search results page.

Take a look at this screencast so you can see what I see:
http://www.screentoaster.com/watch/stVkxcRUFIR19fR1xfXF9RXlZW/d7_overlay...

Re #56: Getting rid of the 150ms setTimeout() was the absolute first thing I tried - see the original post that started this issue :) Just tried the patch in #56 again to make sure, and same result; it unfortunately has no effect.

Re #58: This issue is about extremely bad overlay performance that prevents the overlay from even being used, not rendering issues involving scrolling. See #615204: Overlay (and other iframe) scrolling overlaps the toolbar for that issue - it seems unlikely they are due to the same cause since that issue can be reproduced without the overlay module even being enabled.

@David_Rothstein: thanks for pointing out that other issue. Whilst my overlay is usable, cpu does sit at 50% when scrolling, so perhaps guess that part at least is related to this issue.

Scrolling CPU spike IS a Firefox issue and not related to the overlay.

For example, this issue page here is a long page for a browser to display. When you use this Drupal.org page and scroll quickly, the CPU will spike. I can go from 1% to 25% quickly just by scrolling rapidly.

The overlay is definitely having problems in chrome on my XAMMP/Vista local environment, it loads slowly and I cannot use the pointer to move the screen up and down (I can only scroll using my keyboard).

I really have a hard time understanding how this is being considered anywhere near ready for prime time, nor do I get how this would be considered a gain in any way, shape or form even if it didn't have (apparently) so many performance/cross-browser issues. Is moving between pages really such a problem? Are there any sites on the internet that utilize such a modal strategy at the moment? D7 is amazing, but I really feel it would be a major mistake to include this in D7 core- push it to contrib and see if anyone uses it before we all get it pushed onto us. Can we please not shoot ourselves in the foot on the eve of such a major moment for Drupal?

I also think that the current state of the overlay module is in no way an improvement for regular admin pages.

I _kinda_ found the problem for jerky scrolling in firefox 3.5.x in windows 7, removing the position:fixed from the toolbar makes scrolling much more fluent for me.

EDIT: One could argue that while the modal window is open, the fixed position on the toolbar is not necessary (can't use it while the modal window is open), and could be temporarily replaced by (for example) absolute position untill the modal window is closed. Once it's closed, applying the fixed position should(?) make it jump back into view.

EDIT2: Scratch that, the toolbar is usable while the modal window is open...

I wonder if upgrading to the latest jQuery would help?

@59: so should I make a separate issue about this ugliness, as it doesn't rly seem to affect the performance

@64: I tried this a while back and it didn't seem to make a difference, will try again with the 1.4 alpha 1 release

[edit] 1.4a1 also doesn't seem to make any difference for me on the machine I was having slight performance issues on, but perhaps ppl with bigger performance problems should try it

Removing:

-moz-box-shadow

from the CSS worked for me.

If we want to keep the shadow,

-moz-border-image

can give a similar look, and at least on my windows box performs better than box-shadow.

Really rough files attached below (unzip into the overlay module). It'll have all sorts of positioning problems but should give an idea of border-images performance.

#66, that only fixes performance in firefox....

I pursued my own idea of removing position:fixed, fixes scrolling in opera and firefox for me, while providing alternative position:fixed behavior (yes, it's a dirty workaround...)

Also: since I introduced a cached var for $(window), I changed that twice too.

Sigh, attempt #2...

Not sure whether already mentioned:
slow scrolling in pages with position:fixed elements: https://bugzilla.mozilla.org/show_bug.cgi?id=201307

Maybe we should remove position:fixed from toolbar.css and replace it with a javascript solution.

#70: I didn't experience scrolling issues in firefox (or opera) with the overlay disabled, so no need to remove position:fixed entirely.

True but maybe it's just cleaner to move your solution (which works for me bye the way) into toolbar.js.

http://jqueryfordesigners.com/fixed-floating-elements/

position:fixed as mentioned by Kiphaas7 in #67 except using approach of http://jqueryfordesigners.com/fixed-floating-elements/ and moved to toolbar.js.

Update...

hmm #74 and #75 are not working, my mistake. If I get it working I'll post a new patch.

http://jqueryfordesigners.com/fixed-floating-elements/ isn't usable.

So continue with patch in #69.

OK, we really need to get a handle on this issue, and if we keep discussing four different things in the same issue, none of them will get solved.

I am retitling this issue to reflect the original problem, which is critical. Please move any discussion of other issues to the appropriate place. In particular:

1. Although I see how they can be confused, "jerkiness"/overlap while scrolling the overlay is not part of this issue (I can reproduce the performance problems and browser lock-up without scrolling at all). Please move any patches for that issue to #615204: Overlay (and other iframe) scrolling overlaps the toolbar. We already know from discussion there that the scrolling problem is not at all limited to the overlay, so any patch that changes the overlay module to solve it will not be a complete solution.
2. I've created a new issue at #659486: Add back the "loading" image to the Overlay in a way that does not kill performance for adding back the "loading" image. Although that certainly was a legitimate followup to discuss here, given that there are still critical performance problems to solve, whereas adding back the "loading" image is not critical, it seems like it would be best to move the discussion and patches (e.g. from comments #36-#49 or thereabouts) to that issue instead.
3. Regardless of whether the overlay module is in core or contrib, that has absolutely nothing to do with this issue - the bug we're discussing here still exists. I've created #659488: Properly test the overlay to determine if it belongs in core or contrib for that discussion to take place. Please note that Drupal 7 has over 350 critical issues right now, only a tiny percentage of which have anything to do with the overlay (and a number of those aren't actually bugs caused by the overlay, but rather bugs elsewhere in core which the overlay module revealed), so "the overlay has critical bugs" is not really a compelling argument for anything.

Thanks!

Neither A, B, nor C do anything to improve performance for me.

And no, I can't really explain why A doesn't work when the patch in #18 does... but that's what I'm seeing :(

It darn near drove me crazy, but I think I've tracked it down.

This one-line patch appears to completely fix the major (a.k.a. "locks up the browser and consumes 100% of CPU") overlay performance issue.... at least on a Dell Latitude D830 with an NVIDIA graphics card running Firefox 3.0.15 via Ubuntu Hardy :)

Perhaps a jQuery expert can explain why this works (or propose a better fix)? But luckily, it seems to be occurring in a line of code that isn't particularly important anyway.

Once we confirm and understand this, we should add an appropriate code comment here as well.

@David_Rothstein Aren't you animating the body of the parent window now instead of the iframe's?

I think it works for you because it saves repaints (you removed a repaint; body.overlay does not exist in the parent window). We need to track down all repaints/reflows and try to minimize that number.

Once we confirm and understand this, we should add an appropriate code comment here as well.

The patch doesn't change performance on my windows machine. I get the feeling my sluggishness/slight visual tearing was several degrees less than what David was experiencing with his lockups etc.

That said I still like this patch. I also don't know jQuery's inner workings but the patch has a much simpler/readable selector. I have read that the jQuery 'animate' function runs every 13ms and if it has to compute that complicated selector every 13ms that may be the performance sink on certain machines...

I appreciate my last post was only addressing firefox, but I think a large part of CPU time is taken up in calculating all the variable transparencies of overlays & shadows. The mere fact that the CSS is using -moz- and -webkit- tell us that the standards aren't finalized and that the browser vendors are treating their implementations as experimental. Should this kind of CSS be in core or should it be left to a contrib module/theme?

I'm not a jQuery expert, but @meatsacks comment about having to calculate the selector every 13ms just reminded me of something that I learned when working on the rotor module, which is that you can cache jQuery selectors in local variables to save recalculating them all the time.

http://www.artzstudio.com/2009/04/jquery-performance-rules/#cache-jquery...

Perhaps this is a candidate for such treatment?

Re #85, it indeed seems like the $('body.overlay') selector is doing nothing. I could have sworn I observed it working and actually doing something, but apparently I was wrong :)

Which raises the question of whether we can simply delete this line of code after all. At least for me, this animation doesn't appear to be doing anything particularly noticeable - anyone else?

Also, #86 and #87 lead to a rather simple idea. Looking at the code more closely, there is no reason we need to calculate this selector again when we already have it about 10 lines above that...

So, attached are two patches: One removes the line of code and the other fixes it. Both solve the major performance problems for me; I think the one that removes it is a bit faster, but not by much.

#86:

#615204: Overlay (and other iframe) scrolling overlaps the toolbar is probably more suited for the problem you are describing, which is the same problem I was describing. Also, since I'm experiencing the same issue in opera (which does not support css shadows), your proposed fix won't solve it for everyone. Allthough I do agree that the border shadows don't add anything but a small visual "woah". We can live perfectly without them.

#87:

In my experience, caching variables and other performance optimizations only help making the script execute faster and more smoothly. Locking up the browser, jerky scrolling etc usually indicate more serious performance issues which need to be solved by actually changing the code. The latter is handled here (#78), while the former should live in it's own issue (if it doesn't exist already? couldn't find it.), since I also think quite a few objects could be cached in variables.

In fact, reviewing the entire code for optimization by these pointers should be done in that new issue. Not saying that the authors of overlay did a bad job, on the contrary, but it's never a bad idea to again and again try to squeeze every bit of performance out of huge scripts like these, especially since animations are not smooth, which is (again, in my experience) usually a sign that the script could be further micro-optimized.

Just wanted to throw a quick shout-out to some of the new faces I see in this issue. Thanks very much for helping us get to the bottom of this!

#89 certainly sounds like a good plan - I'm absolutely certain there are other optimizations that could be done :)

Given that, let's consider the minimal fix from #88 for now rather than the one that removes the code completely. I'm reuploading that patch here, and I also made a similar change a few lines above it. As far as I can tell, this would be a good cleanup anyway, even if it didn't have the added bonus of leading to a gigantic performance improvement on my computer...

Just subscribing here but I can confirm that removing the -moz-box-shadow makes scrolling much more fluid for me on Ubuntu Karmic, FF 3.3.5. Hope I'll be able to help when I get some time over.

       // Animate body opacity, so we fade in the page as it loads in.
-      $(self.iframe.$element.get(0)).contents().find('body.overlay').animate({opacity: 0.9999}, 'slow');
+      $(this).animate({opacity: 0.9999}, 'slow');
     }

This is not the same. First selects body element of iframe, second the div in parent window with class "overlay" (which is the jQuery UI Dialog using the dialogClass option).

So....

- Do we really need the animation?
- If yes, why isn't the parent div of the iframe (or the iframe itself for that matter) animated? Why go through all the trouble of animating the body of the iframe?

True, there is a slight difference - hadn't even noticed because they were so close together :)

So something like the attached patch also works for me and comes closer to maintaining the exact behavior. It actually looks the exact same to me, but I think what I'm seeing is definitely not the intended effect... What I see (either with or without this patch) is some crazy thing where the overlay kind of resizes, flashes and fades in all at once. The close button (being outside the iframe) does not participate in any of this, which looks really really weird.

What I think we ideally want to see here (and I assume was the intention?) is that first, a blank overlay of the correct size is drawn on the screen as a white box, with the close button attached to it, and the size doesn't change after that. Then, the contents of the overlay fade in. Is that what anyone else actually sees?

So to answer #94 - yeah, it is hard to imagine why animating the iframe itself would be different than animating the body inside the iframe :) Anyway, I think that's what the above patch does, so hopefully we can test it out.

#93: the flashing does not happen for me. I'm still scratching my head why there is a need for animating this in the first place...

I'd like to know if you are using Firebug with Firefox. Firebug is know to consume a lot of processes.

The article describing this issue is: http://antennasoft.net/robcee/2009/12/15/firebug-and-the-jit/

If you are using Firebug, try to disable it and see if the performance is improving. :-)

@David_Rothstein I've had a look at the code and the body of the iframe's document is not even hidden. So that can be removed. I think it's leftover code as the iframe element itself is already hidden in Drupal.overlay.create() and fadeIn at Drupal.overlay.bindChild().

I did some cleaning;
- added some semicolons where forgotten
- cached jquery calls
- removed some unnecessary code
- contains code of #95 (bit altered)

Nope I broke it... better luck tomorrow

I know, but its not working in safari any more :p

FF3.5.5
Win2k/SP4
ATI Xpert128 (?9 years old)
Installed a few hours ago (I don't know how to interpret what compile or whatever...so...)
overlay.info 12/15/2009 4:05AM ... if that's any help

I'm new to Drupal - few weeks - thought I might benefit from 'learning' with the latest but
I'm experiencing the same CPU problem you're all dealing with - my many thanks.

However, after my 30min of working on the Permissions page to turn off Overlay and then
when that didn't turn off Overlay I turned off Dashboard and though they are both unchecked,
they are both still on.

Any help?

UID1 has persistent permissions for everything

Followup on patch in #100

- working in safari again
- fixed tabs in IE6 and below (is disabled in HEAD)
- only resize in autoResize when height actually is changed.

This cleanup patch gave me an idea, the resizing that happens is overkill. Iframes with a width of 100% resize fine when resizing the browser window. So, with an extra wrapper div around the title and content:

Current situation

<div id="dimmed"></div>

<div class="overlay">
  <div>title</div>
  <div>content</div>
</div>

Proposed situation in simple code:

<div id="dimmed"></div>

<div class="overlay">
  <div id="wrapperoverlay">
    <div>title</div>
    <div>content</div>
  </div>
</div>

The new proposed css would then be:

.overlay {
  position:absolute;
  width:100%;
}

#wrapperoverlay {
  margin:0 auto;
  min-width:300px;
  width:78%;
}

Potential problem is the toolbar, allthough that could be solved by adding a margin-top with jquery (as it does now, kinda).

On a sidenote: If we weren't using an iframe at all, the entire resize code could be thrown out offcourse. Could someone here point me to the reason why an iframe is used in the first place? Couldn't find it myself.

preventing collision of eventhandlers/id's and stuff i guess.

A followup patch is coming in a few minutes... still some things not working.

Followup on #108

Yeah, preventing ID clashes was definitely one of the reasons an iframe was originally chosen, as I recall. For background, see the initial writeup at #517688: Initial D7UX admin overlay, as well as http://hojtsy.hu/blog/2009-jun-17/two-alternatives-d7ux-overlay-implemen...

By the way, the above patch looks really nice on a first glance! (I haven't studied it in detail, though.) Trying to do a lot of things at once, but seems like some necessary cleanup aimed at improving the performance. Thanks for working on it...

@kenzee - I disagree with you in regards to User 1 (from a factual standpoint, there's obviously examples that disprove what you say, for example WYSIWYG editors), but this is not the place to debate this. Here's an issue I created to discuss: #659480: Per-user setting for the Overlay

ksenzee - Thanks for the instructions: no cache clearing was required. As soon as Modules > Overlay was unchecked and the Save Configuration applied, the Overlay disappeared and the page was transformed into a (normal?) page.

Please review #112. There's room for more improvements, but I understand I shouldn't change too much at once.

casey, that's a huge patch (kudos for that!), still busy trying to grasp all the changes...

While we're at it, I found another small performance improvement around lines 133:

Old:

self.iframe.$element = $(Drupal.theme('overlayElement'));
self.iframe.$container = $(Drupal.theme('overlayContainer')).append(self.iframe.$element);

New:

self.iframe.$element = $(Drupal.theme('overlayElement'));
self.iframe.$container = $(Drupal.theme('overlayContainer', Drupal.theme('overlayElement')));

with offcourse a modified theme function:

Drupal.theme.prototype.overlayContainer = function (el) {
  return '<div id="overlay-container">' + el + '</div>';
}

After creating a loop that runs over above code 1000 times, the average times per run for firefox are:
old:0.390ms
new:0.337ms

But in IE8 the difference get's more drastic:
old:0.922ms
new:0.750ms

So even with objects we're creating on the fly we should be carefull with using manipulation functions like append().

EDIT:

quick code review of your patch (just the code, didn't check if everything was still working). You're accessing the same object twice, please use $window = $(window). I know it's accessible really fast since it's window, but it's still a waste.

     var windowResize = function () {
-      var width = $(window).width()
+      var width = $(window).width();

Same here, right after lastFrameWidth.

     if (!autoResizing) {
       autoResizing = true;
       autoResize();
-      var lastFrameWidth = self.iframe.$element.width();
+      var lastFrameWidth = self.$iframe.width();
       var lastWidth = $(window).width();
       $(window).resize(windowResize);
     }

The autoresize change looks awesome, allthough it's still a shame we have to call it every 150ms just because the height of the content inside the iframe could change, and we have no decent way of knowing this by events.

The autoresize change looks awesome, allthough it's still a shame we have to call it every 150ms just because the height of the content inside the iframe could change, and we have no decent way of knowing this by events.

Well we could remove it for modern browsers that support Cross Document Messaging (IE 8+, Firefox 3+, Opera 9+, Webkit ?+). Attached is an example that'll work for collapsing fieldsets. The annoying thing is having to call the overlayChild.resized function every time the page has changed size. If there was a way to hook into every jQuery show/hide animation easily it'd be easier.

What about new elements that are added on runtime to the overlay? for example addmore for fields?

A DOM change event is what we need, but that one doesn't exist in IE (http://www.quirksmode.org/dom/events/index.html#link2)

however, maybe something like this: http://tobiasz123.wordpress.com/2009/01/19/utilizing-mutation-events-for...

But plz review #112 first before that one gets lost.

Yeah thats what I was trying to say, that any function that could potentially change the size of the page needs to trigger the overlayChild.resized function. I only included that code in the fieldsets js file just to prove the concept. We would need to add the call to other js files which seems like it would turn into a big/messy patch.

If we knew for sure that everytime an element is revealed/hidden that it was done using jQuery.show/hide we might be able to put a wrapper around show/hide and get them all at once. Not sure how practical it is though.

A DOM change event is new to me, so one exists in FF/Safari? Has anyone actually reported performance problems in IE? If not, then the problem may be browser specific in which case using that event could work to fix it for FF/Safari. How does it handle something like jQuery animating something on the page?

@Kiphaas7 in #118

self.iframe.$element = $(Drupal.theme('overlayElement'));
self.iframe.$container = $(Drupal.theme('overlayContainer', Drupal.theme('overlayElement')));

Is incorrect as self.iframe.$element as Drupal.theme('overlayElement') is added twice to the DOM (first only in jquery's memory).

--

I'll post a new followup patch in an hour or so; only calling autoResize() when DOM changes.

@casey #123

Hmm, I see, you're right. Waiting for your patch so I can start making a follow up with some of my own ideas :).

#112:

There is some calculation going wrong when toggling fieldsets, resulting in incorrect heights (too small). This also happens when a drupal message is set (error: you don't have... etc), and probably in some other cases too, where the internal height of the iframe is toggled without reloading the iframe.

Attached is a screenshot for the fieldset case.

However, on the plus side, it definately makes things much, MUCH more snappier, finally getting smooth transitions and fast responses, kudos!

err, forgot to attach...

Also, scrolling is broken in safari 4 (windows), but that was something I noticed also before the patch (patch didn't fix it though). You can only scroll by mouse, not by actually picking up the slider and dragging it.

New patch takes at least one more hour (need to test some things), but its going the right direction. I'll also try to fix your notices.

Followup on patch in #112

changes:
- only Drupal.overlay.resize() actually changes size/position of overlay.
- Drupal.overlay.resize() only called when something actually changed (window.resize or iframe content DOM change)
- IE doesn't support MutationEvents so for IE still same approach as before.

--- modules/overlay/overlay-parent.css	15 Dec 2009 05:28:59 -0000	1.3
+++ modules/overlay/overlay-parent.css	18 Dec 2009 16:03:43 -0000

+++ modules/overlay/overlay-parent.css	18 Dec 2009 16:03:43 -0000
@@ -68,9 +68,6 @@

@@ -68,9 +68,6 @@
   padding: 0;
   overflow: visible;
   background: #fff url(images/loading.gif) no-repeat 50% 50%;
-  -webkit-box-shadow: 8px 8px 8px rgba(0,0,0,.5);
-  -moz-box-shadow: 8px 8px 8px rgba(0,0,0,.5);
-  box-shadow: 8px 8px 8px rgba(0,0,0,.5);
 }

Do not remove box shadows in this patch. We need to do only jQuery optimization in this patch.

Shadows changes should be discussed in a parallel patch or in a follow up.

I'm on crack. Are you, too?

+++ modules/overlay/overlay-parent.js	18 Dec 2009 16:03:44 -0000
@@ -130,23 +140,25 @@
 
   // Open callback for jQuery UI Dialog.
   var dialogOpen = function () {
+    // Adjust overlay size when window is resized.
+    $(window).bind('resize', self.autoResize);

@@ -157,32 +169,21 @@
+    // Hide the iframe hidden until the child document is loaded.

-hidden?

There are a number of added empty lines with indents too. A search/replace regexp could take care of those. ;)

This review is powered by Dreditor.

Ok lots of changes. I am not going to explain them all, just the ones you ask about.

It works pretty snappy now so give it a try.

Looks great casey, works snappy indeed. Only thing I did to your patch was remove spaces from empty lines, and modify the min-width to 650px, since otherwise tables (like on the modules page) get cut off. But all kudos to you for writing such a monster patch! I was planning on optimising the number of width/height calls for your #2 patch, but you beat me to the punch with #3, and probably did a better job at it as well :). Also, you rewriting the resize function solved a big WTF for me when I first saw it (resize code in 2 different locations). So again, kudos.

@129: box-shadow is a performance hog in firefox, so yes this should be adressed in a follow up, but it wasn't considered a critical feature in the first place since only webkit and mozilla browsers support box-shadow. IE and opera users didn't see the box-shadow at all. So IMHO it's legit to remove it here.

-----

My issues with the current patch:

1. My own reported issue of scrollbars (#126) not working properly in webkit browsers seems to be an issue with jquery ui Dialog in general. Couldn't find a way around it, but did notice that the normal dialog (so not the modal one) on the jquery ui page does appear to have working scrollbars. The modal dialog message on that same page does have our issue. Still a WTF for me.
2. Also, the issue I had in #125 (screenshot in #126) is still not solved for me, I'll look into it later today if I have the time.
3. In the patch you use $.browser.opera. We shouldn't rely on user agent testing, but rather on method testing. I'll also look into that later today.

-----

Moving on: One thing I'm not really comfortable with in the current implementation is the toolbar specific code: the click event and the resize call with timeout. It confuses/distracts me visually, since the body and onderlying content instantly jump to the new height, while the overlay waits 150ms (otherwise it can't get the new height), then animates to the new position...I'd ditch the general approach where the top value is recalculated everytime in px, in favor of:

• Ditch the toolbar click event.
• Keep top at 0px, always.
• Set .toolbar .overlay { margin-top: 2.2em; padding-top: 15px; } and .toolbar-drawer .overlay { margin-top: 6.6em; } in the css.
• Other implementations that might overlap the toolbar should provide their own CSS that positions the overlay correctly.

Thoughts?

Patch had a bug in IE; when you close immediately after opening the overlay you get an error

1. is indeed a jquery issue (also in chrome).
2. I can't reproduce that; In which browser does that happen?
3. I think there is no method to test whether a certain DOM event exists, or is there?

Here are some tests for DOM events: http://perfectionkills.com/detecting-event-support-without-browser-sniff...

#133 number 2: it happened to me both in firefox 3.5.x and chromium recently (ubuntu) after toggling a fieldset. Chromium also showed a scrollbar and a height which is too small. If memory serves me right it also happened at least in firefox 3.5.x on windows.

#135 The post you're referring to didn't have support for mutation events, this one does. (link in that post)

Test to see if browser supports the DOMSubtreeModified event is pretty simple.

$('<div>')
  .appendTo(document).bind('DOMSubtreeModified', function() {
    Drupal.overlay.useDOMSubtreeModified = true;
  }).attr('class', 'test').remove();

I have a test tomorrow so I'll continue after that.

Gah, changing self.$iframeDocument.bind('DOMSubtreeModified', delayedResize); to self.$iframeDocument.bind('DOMSubtreeModified', self.autoResize); fixes my issue in #125 beatifully in firefox, but totally borks in webkit (incorrect height). After some googling it turns out that webkit does not support DOMAttrModified, but does support DOMSubtreeModified, but DOMSubtreeModified fires before (!!!) the actual attribute modification resulting in a calculation of the height before (with self.autoResize) or during (with delayedResize -> 150ms) an animation... Cranking the timeout way up for self.$iframeDocument.bind('DOMSubtreeModified', delayedResize); obviously fixes webkit for me, but firefox remains broken. So as it stands, I have no idea how to fix this properly in webkit, other than falling back to the timeout solution, which actually starts looking like a good idea with this crossbrowser mess.

Interestingly though, the height is set correctly in firefox with delayedResize, but the document inside has shifted upwards somehow. Changing/setting/removing one of the following attributes on the iframe (might be more properties or values, these are just the ones I tried) after the resize with firebug makes the content jump in sight again, fixing the issue! It probably has something to do with firefox doing a redraw or repaint when changing these settings. So, if we pursue the current approach, we might have to force a redraw as a callback once autoResize is done.

// Changing/setting/removing one of the following fixes iframe content position after toggling a fieldset for me.
display:block;
position:relative;
overflow:hidden;

While looking for yet another alternative, I stumbled upon a possible alternative for IE. Apparently microsoft has a proprietary solution called onpropertychange, which might be interesting as an alternative for the timeout.

On a sidenote, while debugging, I found out that resize is called four times during initial load, and twice for every change of page while the overlay is open. The latter might have some special meaning I currently don't see (around rule 439):

  self.autoResize();

  // Adjust overlay to fit the iframe content?
  if (self.options.autoFit) {
    // autoresize bound to events
  }

Could be changed to the code below, lowering the calls to autoResize to 2 initially, and 1 for every page change while open.

  // Adjust overlay to fit the iframe content?
  if (self.options.autoFit) {
    // autoresize bound to events
  }
  // Else autoResize() should be fired just once.
  else {
    self.autoResize();
  }

Couldn't find where the 2 initial autoResize calls come from and why there should be 2 instead of 1, also I'm out of time and ideas for today :).

Just tried out the latest patch - and overall everything is looking much, much smoother. I am seeing an issue I didn't see before, though. The pages are taking a long time to "fade out" and "fade in" while loading. Especially on complex page, I'm seeing it sit for a few seconds with the contents of the page fully rendered but sort of looking like they're whited out - see attached screenshot. Then eventually the full page appears with the correct colors. (This is different from the performance issue I saw before, which is fixed by this patch as well as with the earlier simple one.) Anyone else see this?

Also, to (belatedly) answer a question from @xmacinfo above, I am using Firebug, but disabling it doesn't have any effect on any of the performance issues I've been noticing with the overlay.

@David in #138 I see it too, but that is expected behavior, aside from that it takes a few seconds for you. However, I might have an idea why this is: the spinner is shown again between page loads.

New page load- > fade to 0.2 and show spinner. It probably goes wrong there for you, since you had problems with the spinner before. I made some modifications to the code so that the opacity change happens first, THEN show spinner. Once page is loaded, immediately remove spinner. I did a similiar thing for the close statement, animate first, then do the actual callback.

See if that works for you. Otherwise, you should try commenting out line 297 with this patch applied.

I'm thinking about adding a callback to the autoResize function, so that only after resizing, actual content gets loaded. This should make resizing more fluent while pages are loading.

so this patch contains the following changes to overlaycleanup4.patch:

• Tries to optimize animation in between page loads.
• Tries to optimize the close effect.
• Reduces the number of calls to resize() from 4 initially and 2 every page load to 2 initially and 1 for every page load while open.
• Sets the min-width from 300px to 650px to prevent content being chopped off (like tables on the modules page).

Fixed whitespace at end of patch

The patch in #140 does not apply - here is a straight reroll that does.

This kind of solves my problem with the opacity animation, but not in a good way :) This one is now back to freezing up the browser and hitting 100% CPU... I'll try to compare patches later and see if I can pin down what in the latest patches is causing it.

nice, removed that 0.9999 weird ass opacity

and what's this? a sensible min-width? /me cheers

#141 Did you try commenting out the line I gave? That's the line where the spinner is added again in between page loads. Did you ever try replacing it with a non-animated gif, a different animated gif, or a picture in a different format? Just curious exactly why this gif is causing a browser freeze...

Enjoyed reading up this issue. Great work all -- especially good to see newer core people like Kiphaas7 and casey jump in. Keep up the great work as this is an important issue.

By the way, I'm comfortable removing certain animations if that provides a faster experience. Animations are nice to have, but should not get in the way of actually using Drupal. So if we want to remove certain overlay animations for now (and re-introduce them once we figured out how to make them fast), I'd be OK with that.

Ok new patch.

Kiphaas7 and I did some testing on the DOMSubtreeModified event. Especially Safari doesn't seem to handle it very well. I think we should drop that for now and maybe implement that when that event is better supported by all browsers (minus IE).

So I reversed that to the timeout approach.

I also removed the animations on content switching/resizing.

Further I tried to let css handle as much as possible. Could use some more cleanup, but I'd like you to test and review it first.

I just did some quick testing of #145 and am happy to report that it seems to be extremely fast, plus the transitions are overall much smoother too.

Re #143, yeah, I did try commenting out the line (forgot to mention it) and tried now changing the image - overall, it didn't seem to help much (maybe a little). But anyway, it seems like #145 is probably the way to go for now, and some of the flashier features can be added back later if possible.

I think we should definitely work on getting #145 tested, reviewed, and committed - it appears to be a huge improvement. The two of you have been doing absolutely amazing work with this, but there's no reason it has to all happen in one big patch :) There can definitely be further cleanup patches later, after the initial one is committed.

I tested the patch and it is notably faster. I agree that we should streamline #145 and get it in.

I skimmed the code so this is not a complete review:

+++ modules/overlay/overlay-parent.css	22 Dec 2009 18:35:59 -0000
@@ -96,6 +98,20 @@
+/**
+ * CSS Hack for IE 7 and below.
+ * http://www.webdevout.net/css-hacks#in_css-selectors
+ */
+*:first-child+html .overlay .ui-dialog-titlebar ul li {
+  display: inline;
+}
+* html .overlay .ui-dialog-titlebar ul li {
+  display: inline;
+}
+/**
+ * End of CSS Hack for IE 7 and below.
+ */

Personally, I'd prefer to avoid browser specific hacks as much as possible. Is this a big performance win?

+++ modules/overlay/overlay-parent.js	22 Dec 2009 18:35:59 -0000
@@ -344,203 +373,159 @@
+    // instead of moving the DOM element, because Webkit and IE browsers will
+    // not move DOM elements between two DOM documents.

Should probably start with 'Instead' of 'instead'.

#146 No that's actually another issue. Moving tabs from the iframe content to the top of the overlay is currently skipped in IE7 and below.

HEAD:

    // This breaks in anything less than IE 7. Prevent it from running.
    if (typeof tabs != 'undefined' && (!$.browser.msie || parseInt($.browser.version) >= 7)) {

IE7 and below doesn't support inline-block so it needed that hack. Fortunatly the used hack is totally valid CSS and probably never going to be supported by newer browsers.

But we could replace inline-block by block and use floats. I'll change that in the followup patch.

"inline-block" is supported by IE 5.5 +, Opera 7+, webkit and has been introduced in Firefox 3.

https://developer.mozilla.org/en/CSS_improvements_in_Firefox_3
http://www.brunildo.org/test/InlineBlockLayout.html#d42

Ehm: http://www.quirksmode.org/css/display.html#t03

But I understand I can do this:

.overlay .ui-dialog-titlebar ul li {
  display: inline-block;
  list-style: none;
  margin: 0;
  padding: 0;
}
.overlay .ui-dialog-titlebar ul li {
  display: inline;
}

But that is a hack too right?

Besides I get different results per browser (with and without both hacks).

It's mostley casey's hard work on the last slew of patches, I've just been blabbering a lot :). I'll try to review the current patch tomorrow, and just nitpick on the small things. Further improvements/issues should indeed go into a follow up.

@147-150: A li element has a natural block display state. CSS can only override the natural display state, so inline-block does NOT work for li items in IE7. (More on natural states here.) Also, with these kind of complex layouts and the requirement to keep supporting IE6 (and to a lesser degree, IE7), you can't escape some css hacks. Unless there is some way to add a conditional commented css file via the module, which would be a good thing for third party modules anyway. But again, not something for this issue...

(2 extra arguments for add CSS, one boolean for determining if it needs conditional comment or not, second one stating the target, for instance "lte IE 7".)

For this case, it's not an issue probably. I'd prefer to solve it with setting the li to display:inline, making them sit next to each other, while setting display: inline-block to the <a> elements, who do have a natural inline state. That should eliminate any need for a hack, although I'm not really aware of what purpose this hack serves, until now never really bothered with it. :)

#151 Perfect! display:inline on li and display:inline-block + some padding/margin changes makes it work everywhere the same!

subscribe
btw, #145 fixes #656190: tab 'configuration' and 'modules' disappear in the overlay.

Followup on #145

- removed CSS hack
- some documentation
- A new wrapper div so CSS can handle positioning and dynamic width of overlay (instead of JS; much faster).

+ patch

Holy crap, thanks to casey this is en entirely different beast now :)
I'd say http://drupal.org/node/664450 and http://drupal.org/node/574164 are about solved by this.

The overlay still feels kind of nervous using it, and I'm tracking down reasons.
One is: we should add a force-scrollbar CCS property (must be to garland or any other frontend theme), maybe this one http://doctype.com/force-vertical-scrollbar-show-ff-ie8 maybe overflow-y :scroll or html { min-height: 101%; } so it stops FF jumping horizontally.

Another is that I more and more come to the thought, that the background may be too dark and thus the contrast is too big.

Adressed both issues here #666298: Make Overlay easier on the eye, Patch provided.

Added force-scrollbar when autoFit is enabled and overlay is open.

Subscribing.

Followup on 7c:

• made a new theme function for the wrapper.
• used parent() instead of parents('#overlay-wrapper').
• introduced some caching variables in Drupal.overlay.create().
• moved min-height and min-width from js to css.
• increased the min-width to 700, there were still some minor clipping issues with tables if min-width was set to 650.
• moved the overlay-open class from the html element to the body element, and made it more generic (not just when autoFit is enabled).
• introduced a new class (overlay-autofit) for when autofit is enabled, to be added to the body element.
• Fixed some minor typos in comments in Drupal.overlay.outerResize().
• Changed the commenting in Drupal.overlay.load().
• Incorporated a fix by casey for a regression discovered by me, where after closing the overlay webkit would throw errors and refuse to open a new overlay.

What this needs after the patch has landed is some extensive testing in IE6, 7 and 8, since those are known for breaking stuff. It also needs a good accessibility review, with all the changes we made there might be some stuff broken in that department.

- IE6/IE7 need a delayed resize on window resize; added a function for that.
- removed comment about the IE bug on scrollbars in Drupal.theme.prototype.overlayElement, scrolling="yes" for IE is no longer necessary.

Looks good to me, let's have a few reviews before someone else besides me and casey can set this to RTBC?

It's soooo much better with the patch. Especially scrolling! It's actually smooth now, while it is very very sluggish without the patch (Win XP, AMD 64 X2 6000+, GeForce GTX260, 4GB RAM, Firefox 3.5.6, Nginx 0.8.3.1, PHP-CGI 5.3.1).

Things I noticed:
- with the patch applied, there is no loading symbol displayed. The "Loading..." is displayed, but with a white Overlay.
- after closing the Overlay on the frontpage, the house symbol on the upper left is not marked (probably because I'm now on URL /#?)
- with todays HEAD checkout I _always_ get two toolbars (one out of and one inside the overlay).

already found solution for first problem. looking into the second one.

Please continue reviewing the rest of the patch.

The second issue in #162 is also not caused by this patch - it exists without it as well and is closely related to #663126: Active items in top level IA don't have the light gray background when overlay is enabled (essentially the same bug, actually).

So no need to fix it here - but if you come up with a solution, please post it in the other issue :)

ksenzee: You're apparently right. As stated in that thread, I disabled the Update module in the Setup.

Ok new patch:
- loading symbol is back in all browsers.
- Some improvements in Drupal.overlayChild.behaviors.parseLinks in overlay-child.js (is related to this issue. HEAD used try/catch blocks to dodge errors, which I think was the case here).

I made a mistake.

one minute plz...

I need to hide/show the iframe to make the loading symbol work in FF and Opera (webkit + IE work), but that makes the iframe content flikker when its loaded (this is also how its done in HEAD). Still figuring out how to fix this.

Follow up for 7f:

The loading animation is fixed for when the dialog is opened for the first time. No loading animation while changing pages with the overlay already open, since that made David Rothstein's browser go upside down. Opacity is changed while loading though, so together with the title changing to "Loading...", it should be enough....

  // When a new overlay is opened and loaded, we add a loaded class to
  // the dialog. The loaded class is not removed and added back again
  // while switching between pages with the overlay already open,
  // due to performance issues.

  //self.$dialog.removeClass('overlay-loaded');
  self.$iframe
    .css('opacity', 0.2)
    .load(function () {
      self.isLoading = false;

      // Only continue when overlay is still open and not closing.
      if (self.isOpen && !self.isClosing) {
        self.$iframe.css('opacity', 1);
        self.$dialog.addClass('overlay-loaded');
      }
      else {
        self.destroy();
      }
  });

Thnx Kiphaas7!

Lets get this in...

Excellent.

I scanned through this and didn't see anything too horrendous. Granted, my front-end skillz are not really up to par with the folks in this thread. I think committing this sooner than later is important so we can evaluate the Overlay on its actual merits, and not become side-tracked by buggy behaviour.

Therefore, committed to HEAD. Marking needs work for the couple of small follow-up issues ksenzee noted.

Also, this is no longer critical, IMO, unless we figure out a place where we've broken something.

Thanks so much - this is great! For the first time in weeks, I am able to use the default install profile without having to hack core first to prevent my machine from locking up :)

I think we should do followups in separate issues. I reviewed this patch and found a couple... although about 80% of what I found there wasn't even caused by this patch, rather just pre-existing issues in code this patch touched for other reasons:
#667004: Minor code comment cleanups in overlay-parent.js
#667012: Remove the opening of external links in a new browser window

Overall the code looks really, really good.

I guess we can safely add back the overlay shadows now that the performance issue have been fixed.

.overlay #overlay-container {
  -webkit-box-shadow: 8px 8px 8px rgba(0,0,0,.5);
  -moz-box-shadow: 8px 8px 8px rgba(0,0,0,.5);
  box-shadow: 8px 8px 8px rgba(0,0,0,.5);

Can someone confirm that adding these three lines keeps their browser performing well?

Is ok in webkit (always was) but still not in FF. Shadow + position:fixed of toolbar are not working very well together in FF.

I just cvs upped, seems much better in terms of CPU, but I can reproduce exactly the same issue as David Rothstein in #138. I've opened #667074: Very slow / delayed fade in with overlay on Firefox

I have the slow/delayed fade in FF as well, but will participate in #667074.

Can it be, that the spinner itself is slowing down page loads?
When switching between admin pages, it always turns up for about the same amount of time.

I believe to remember pages were loading much faster in the interim phase when we had no spinner.

When there was no spinner, the frame content was blank for the same amount as we now display the spinner.