Make overlay respect other click handlers

The idea sounds great, although I haven't tried it out yet. Will try it out soon and report back. Props!

Seems to make a lot of sense at first glance. Works fine on Firefox 3.0 also (if there was any doubt).

Trying this out I noticed that weird things happen when adding a shortcut (via the '+' icon), though. At least the first time you do it, adding a shortcut launches a new window - haven't looked into the problem in detail, but given that behavior I wonder if it is being interpreted as an external link for some reason?

Yup, that fixes it - thanks!

I haven't looked closely at the new patch, but it's probably best to keep it as limited to this issue as possible - will make it easier for people to review it.

I think it's better not to combine multiple patches (which already live in separate issues), makes it only harder to review....

Small patches are easy to re-roll, and the small pain it brings to re-roll doesn't outweigh the potential review issues with big patches like this one, especially if we're talking about click events....

Ok, sorry, what I meant was don't include fixes that already live in separate issues and don't have anything to do with your patch (like #667074: Very slow / delayed fade in with overlay on Firefox)...

I think this makes sense, with the tab behavior of many users now its unrealistic to assume just one click pattern. Other modes should be allowed, however I do think its rare usecase - to be exploited primary by contributed modules.

Wow -- and this is not ironic -- somebody actually took account of an issue I raised. Thanks, casey!

Great patch, big improvements

+++ modules/overlay/overlay-parent.js	31 Dec 2009 15:59:40 -0000
@@ -7,58 +7,23 @@
+    // Instead of binding a click event handler to every link we bind one to the

Some of the new code comments seem to be explaining the rationale for changes. You can just state what it does rather than what it doesn't do anymore.

+++ modules/overlay/overlay-parent.js	31 Dec 2009 15:59:40 -0000
@@ -688,39 +635,127 @@
+    // variable. By setting it to -1, jQuery UI Dialog let the event propagate.

"jQuery UI Dialog LETS the event propagate."

+++ modules/overlay/overlay-parent.js	31 Dec 2009 15:59:40 -0000
@@ -688,39 +635,127 @@
+    $(window).triggerHandler('resize.drupal-overlay');

This resizing when I toggle the toolbar didn't work for me (Chrome and Firefox). Drupal.overlay.outerResize(); did work.

+++ modules/overlay/overlay-parent.js	31 Dec 2009 15:59:40 -0000
@@ -688,39 +635,127 @@
+  if (!$target.is('a') || $target.hasClass('overlay-exclude')) {

The use of css class overlay-exclude should be documented

+++ modules/overlay/overlay.module	31 Dec 2009 15:59:40 -0000
@@ -303,7 +305,7 @@
Index: modules/toolbar/toolbar.js

The clean-ups to toolbar.js look like they could go into a separate issue.

Powered by Dreditor.

I opened a new issue with the toolbar.js cleanup at #673450: Cleanup javascript-files; missing semicolons + whitespace

Love it. Great work. I can do normal tabbed browsing now :)

I really like this patch a lot. Seems to speed up the Overlay a bunch as well! Without Apache's mod_rewrite, however, it breaks. I think we're missing a ?q= somewhere. Better to use the ?q= in the Overlay then to not use it.

Not 100% yet. Ctrl+click on a link in the toolbar to open it in a new tab. In that new tab, many keyboard functions do not work properly. eg. ctrl+t does not open a new tab. ctrl+w does not close the tab.

Firefox 3.5, Ubuntu.

@cassey: See #673810: Make overlay respect key handlers

This is really, really good. It speeds up the performance of the Overlay immensely, and doesn't destroy any other click/ajax handlers that are in tact. I'm setting this to RTBC and I'll ping Katherine on it too.

Did some reviewing of this, and it looks excellent. Great work. A couple things jumped out though.

+      // Allow other scripts to bind their own handlers, but prevent default
+      // action.
+      e.preventDefault();

I think this at a minimum needs more explanation in the code comment - why do we want to do this particular combination?

The answer for the purposes of this patch is that the overlay needs to be able to adjust its position when the toolbar is toggled... But this immediately raises the question of what other scripts out there in Drupal-land need to allow for this this, and how do they decide?

It would be more comforting if we had a general principle to follow here, or even better if we can make the overlay place nice with return false. If the overlay needs to impose the requirement that return false can't be used in certain kinds of other code, then we need to be clear about the extent of that since it's effectively an API change.

+      // Only override default behavior when left-clicking and user is not 
+      // pressing the ALT, CTRL, META (Command key on the Macintosh keyboard)
+      // or SHIFT key.

How do we know (or do we know) that this is the complete list of keys to worry about? If we do know, can the code comment explain more here? And if not, is it worth thinking about including a wider range of keys in this list?

+Drupal.overlayChild.behaviors.parseForms = function (context, settings) {

Maybe I'm missing something, but I don't see where in the code this function ever gets called?

+ * @param ignoreQ
+ *   Boolean whether to ignore path from query string if path appears empty.

This abbreviation is a bit cryptic (yes, Drupal does use $_GET['q'] for this, but it still looks cryptic in this context). I don't think verbose is bad here, so how about something like ignorePathFromQueryString?

I'm also a bit unclear on why this parameter is needed. Perhaps the code which calls this function using it can add a comment explaining why?

+ * Instead of binding a click event handler to every link we bound one to the
+ * document and handle events that bubble up. This allows other scripts to bind
+ * their own handlers to links and also to prevent overlay's handling.
+ *
+ * This handler makes links in displaced regions work correctly, even when the
+ * overlay is open.
+ *
+ * This handler also is being used by the overlay iframe.
+ *
+ * @see Drupal.overlayChild.behaviors.addClickHandler (overlay-child.js)

Some minor comments here:
* In the first sentence, "bound" should be "bind".
* In the last sentence, "This handler also is being used by" should be something like "This handler is also used by". In addition, the reference to the "overlay iframe" seems awkward here, and probably not needed since you have the @see statement below. I think this sentence should be replaced by a comment that explains in general who would want to use this function and why. Then the @see statement becomes an example of one such use case.
* For the @see statement, there is no need to refer to the file in which the function is contained (overlay-child.js) - that is not done elsewhere as far as I know. You have the function name there, which is sufficient for anyone who needs to find it.

+ * Helper function to get the (corrected) drupal path of a link.

(minor) Here and in a variety of other places in the patch, "drupal" should be "Drupal".

***

Also, to clarify #26: This is a separate issue because it actually is reproducible without this patch; otherwise it would not be. (I was confused at first because experiencing that bug following a ctrl-click does require this patch, but then realized that there are other ways to experience it too.)

Sub. Awesome work here, casey.

On a quick test, still seems to break some instances of .dialog(). I still need to test more extensively, but the Media browser's implementation is still broken with this patch:

    var mediaIframe = $.fn.mediaBrowser.getIframe(options.src);
      mediaIframe.bind('load', function (e) {
        if (typeof this.contentWindow.Drupal.mediaBrowser != undefined) {
          this.contentWindow.Drupal.mediaBrowser.start(options);
        }
        debug.debug('browser is loaded');
        debug.debug(options);
      });

    var horizontalPadding = 30;
    var verticalPadding = 30;
    mediaIframe.dialog({
      buttons: { "Ok": function() {
        var selected = this.contentWindow.Drupal.mediaBrowser.selectedMedia;
        onSelect(selected);
        debug.debug(this.contentWindow.Drupal.mediaBrowser);
        $(this).dialog("close");
        }
      },
      modal: true,
      draggable: true,
      resizable: true,
      minWidth: 600,
      width: 800,
      height:500,
      position: 'top',
      overlay: {
        backgroundColor: '#000000',
        opacity: 0.4
      }
      })
      .width(800 - horizontalPadding)
      .height(500 - verticalPadding);

Probably because it's opening an iFrame? I don't know.

commit this thing its working for this part, and lets fix other issues in an other thread

Hm. It's not clear to me whether David Rothstein's feedback has been entirely addressed. However, this bug is creating all sorts of problems for both other patches in the queue and contrib modules, so I've committed what's here to HEAD as a starting point. Thanks, casey, this is a huge improvement.

Marking fixed, tentatively, but feel free to re-open if there is follow-up work to be done.

Yes latest patch works!
Need to get in fast, else webkit users need to press F5 every time they wonna go into the overlay.

Works for me

Confirmed. Works for me.

Ok can we get some actual reviews here please? ;)

Here's #49 but with extra spaces removed from the new line.

+++ modules/overlay/overlay-parent.js	6 Jan 2010 16:48:54 -0000
@@ -210,7 +210,7 @@
       // As the iframe is being removed we need to remove all load handlers, not
       // just the ones namespaced with overlay-event.
-      self.$iframe.unbind('load');
+      self.$iframe.unbind('load load.overlay-event');

Normally, 'load' should be sufficient. If this is working around a bug in jQuery, then we should document so.

+++ modules/overlay/overlay-parent.js	6 Jan 2010 16:48:54 -0000
@@ -716,6 +720,15 @@
+        // When the link has a destination query parameter and that destination
+        // is an admin link we need to fragmentize it.

Why?

+++ modules/overlay/overlay-parent.js	6 Jan 2010 16:48:54 -0000
@@ -716,6 +720,15 @@
+          var fragmentizedDestination = $.param.fragment(self.getPath(window.location), {overlay: params.destination});
+          href = $.param.querystring(href, {destination: fragmentizedDestination});

There should be a space after { and before }.

Powered by Dreditor.

What sun said. Also,

+++ modules/overlay/overlay-parent.js	6 Jan 2010 16:48:54 -0000
@@ -674,7 +678,7 @@
+  if (href != undefined && href != '' && href.charAt(0) != '#') {

It took me a minute to parse this. Let's add to the comment "Only continue if link has an href attribute" something like "and is not just linking to an anchor."

I used dreditor for this patch review and should probably suggest some new signatures for it.

No, seriously, we need comments on "load load.overlay-event", even if it's just a link to the docs. That makes no sense whatsoever, and without comments somebody's going to strip it out as cruft.

Sufficient readable comments now.
It passes the test bot.
It works on my local machine.

If someone can review this once more we can put this to RTBC

FYI it's bad form to RTBC your own patches. Give the rest of us a chance to look at them first. :) I am reviewing this and will report back.

Ok, now it's RTBC. :) Looks great. Tried my darnedest to break it with destination query strings and everything, and it all worked flawlessly.

And aspilicious, just so you know the unfortunate state of our automated JS testing, you could put the Gettysburg Address into overlay-parent.js and the testbot wouldn't know the difference. We have no JS test harness yet. Getting there though: #237566: Automated JavaScript unit testing framework

Committed to HEAD.

I think there is a logical error in the test for the 'overlay-exclude' class:

677 	 // Only continue if clicked target (or one of its parents) is a link and does
678 	// not have class overlay-exclude. The overlay-exclude class allows to prevent
679 	// opening a link in the overlay.
680 	if (!$target.is('a') || $target.hasClass('overlay-exclude')) {
681 	  $target = $target.closest('a');
682 	  if (!$target.length) {
683 	    return;
684 	  }
685 	} 

It would seem that even if I add the class, this is ignored since it only returns if we find an empty link.

this separates out the two checks.

grammar error "allows to prevent"

Thanks for the clarified code comments - looks good.

Committed to HEAD! Thanks.