Overlay currently modifies all admin links and binds an aggressive event handler to it. This potentionally could break other modules. Especially since special click behavior often occurs in admin section (think of all kind of special form elements).
Therefor I added a clickHandler. Instead of binding this to each link it's bound to the document and only handle events that bubble up. This allows other scripts to bind their own handlers to links and also to prevent overlay's handling.
Links will keep their origional href (#663660: Is it a good idea to have two different types of URL for many administration functions in core?).
The new clickHandler also respects [ALT, CTRL, META and SHIFT] + clicks.
The new clickHandler is also being used by the document inside the iframe; so they are handled the same.
I renamed Drupal.overlay.trigger to Drupal.overlay.hashchangeHandler as being more consistent. Besides that it more clearly indicates that it is an event handler.
This patch fixes #667012: Remove the opening of external links in a new browser window.
Lastly this patch improves performance: no more parsing of ALL links on every page.
(I tested it in IE8/IE7/IE6, FF3.5, Chrome, Opera and Safari)
Comment | File | Size | Author |
---|---|---|---|
#70 | overlay-linkexclude.patch | 1.12 KB | casey |
#67 | overlaylinkexclude.patch | 1.12 KB | casey |
#66 | fix-overlay-exclude-668104-66.patch | 803 bytes | pwolanin |
#59 | overlayclickhandler.patch | 2.2 KB | casey |
#57 | overlayclickhandler.patch | 1.93 KB | casey |
Comments
Comment #1
Kiphaas7 CreditAttribution: Kiphaas7 commentedThe idea sounds great, although I haven't tried it out yet. Will try it out soon and report back. Props!
Comment #2
David_Rothstein CreditAttribution: David_Rothstein commentedSeems 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?
Comment #3
casey CreditAttribution: casey commentedBetter now? Also contains some fixes for other issues since they are closely related.
Comment #4
David_Rothstein CreditAttribution: David_Rothstein commentedYup, 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.
Comment #5
casey CreditAttribution: casey commentedThis patch needs some things to be done differently, because of the links no longer being replaced by their fragmentized version. So I think they should be part of this patch.
Comment #6
casey CreditAttribution: casey commentedSome improvements minus something that wasn't (latest patch in #667074: Very slow / delayed fade in with overlay on Firefox) supposed to be in this patch.
Comment #7
casey CreditAttribution: casey commented+patch
Comment #8
Kiphaas7 CreditAttribution: Kiphaas7 commentedI think it's better not to combine multiple patches (which already live in separate issues), makes it only harder to review....
Comment #9
casey CreditAttribution: casey commentedIf I don't include them here they don't work anymore...
Comment #10
Kiphaas7 CreditAttribution: Kiphaas7 commentedSmall 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....
Comment #11
casey CreditAttribution: casey commentedwell... then review initial patch. But you'll get all kind of issues like David_Rothstein in #2.
Comment #12
Kiphaas7 CreditAttribution: Kiphaas7 commentedOk, 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)...
Comment #13
casey CreditAttribution: casey commentedTried to remove anything that could be left out. I'll have to create follow up issues for them.
Plz review this patch....
Comment #14
casey CreditAttribution: casey commentedComment #15
Bojhan CreditAttribution: Bojhan commentedI 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.
Comment #16
casey CreditAttribution: casey commentedTalked to Bojhan:
CTRL-click should open in new tab but in overlay; any alteration of that should be discussed in #666974: Open admin links without overlay using CTRL+Click.
Comment #17
shunting CreditAttribution: shunting commentedWow -- and this is not ironic -- somebody actually took account of an issue I raised. Thanks, casey!
Comment #18
Jody LynnGreat patch, big improvements
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.
"jQuery UI Dialog LETS the event propagate."
This resizing when I toggle the toolbar didn't work for me (Chrome and Firefox). Drupal.overlay.outerResize(); did work.
The use of css class overlay-exclude should be documented
The clean-ups to toolbar.js look like they could go into a separate issue.
Powered by Dreditor.
Comment #19
casey CreditAttribution: casey commentedThis isn't stating what it doesn't do anymore; it's an explanation of why not binding that handler to all links, which is what you'd expect.
Oops that contains code from a followup patch... shoud have been $(window).triggerHandler('resize');
Anyway, new patch incoming...
Comment #20
casey CreditAttribution: casey commented- kept comment on binding click handler to window
- fixed "jQuery UI Dialog LETS the event propagate."
- $(window).triggerHandler('resize.drupal-overlay'); to (window).triggerHandler('resize'); (i'll post a patch with namespaced events in a followup issue)
- added a comment for overlay-exclude class (was in overlay-child.js before and didn't have any documentation)
- removed all cleanup code from toolbar.js
Comment #21
Jody LynnI opened a new issue with the toolbar.js cleanup at #673450: Cleanup javascript-files; missing semicolons + whitespace
Comment #22
casey CreditAttribution: casey commentedComment #23
mrfelton CreditAttribution: mrfelton commentedLove it. Great work. I can do normal tabbed browsing now :)
Comment #24
RobLoachI 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.
Comment #25
casey CreditAttribution: casey commentedMaking it work when clean urls are disabled.
Comment #26
mrfelton CreditAttribution: mrfelton commentedNot 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.
Comment #27
casey CreditAttribution: casey commented#26 Key handlers should be discussed in a seperate issue. Please open a new issue for that and I'll provide a patch there.
Comment #28
mrfelton CreditAttribution: mrfelton commented@cassey: See #673810: Make overlay respect key handlers
Comment #29
RobLoachThis 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.
Comment #30
David_Rothstein CreditAttribution: David_Rothstein commentedDid some reviewing of this, and it looks excellent. Great work. A couple things jumped out though.
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 thatreturn 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.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?
Maybe I'm missing something, but I don't see where in the code this function ever gets called?
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?
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.
(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.)
Comment #31
casey CreditAttribution: casey commentedpreventDefault is clear: we don't wan't to change location. The event is propagated so overlay can catch it. I wanted to say that it won't hurt clickhandlers that change location or something like that because it's href is #. but that's not true. Hmmm.
If you want to let all javascript work together nicely you shouldn't stop propagation of any event you catch. Only the events you are sure of you are the only one that should handle it. But you are right, we do need a general principle here.
This toolbar toggle button is a special case: It has a url (for graceful degradation), but this url is not (and should not) used when javascript is enabled. So normally you'd use return false; here. This link is however inside a overlay displaced region (on which overlay's top position is dependant). overlay needs to know about the toggling. Currently we hardcoded this, which is not very generic.
I did however come up with a nice solution (I wanted to bring this up in a seperate issue): http://docs.jquery.com/Namespaced_Events
Namespaced events are already being used by the key handlers inside bindChild(). "overlay-event" is being used.
I however suggest using "drupal-overlay". drupal to prevent collision with non drupal scripts and overlay to indicate the module. Maybe we should discus this in a seperate issue and document this. Lets use "overlay-event" for now, we can easily alter this later. Alfa-release is upon us!
I found out that this isn't a very smart thing to do (eg when using multiple dialogs). Luckily with same approach as patch in #671276: Overlay blocks my scrollbar this can be fixed. So I incorporated that patch into here.
http://www.w3.org/TR/2003/NOTE-DOM-Level-3-Events-20031107/events.html#E...
There's no easy way to know whether other keys are pressed during click. I don't think this is a problem; CTRL+{otherkey}+click or other combinations are correcty handled. Besides this is at least better than current behaviour.
See Drupal.overlayChild.attachBehaviors(). This approach was already there. We could merge all of this into Drupal.behaviors.overlayChild.attach()
Ok replaced by ignorePathFromQueryString. This is used by Drupal.overlay.fragmentizeLink to prevent transforming non-clean URLs into clean URLs; making it work with clean URLS disabled. I added a comment there.
Updated comments.
Comment #32
casey CreditAttribution: casey commentedComment #33
casey CreditAttribution: casey commentedI opened seperate issues
#674714: We need guidelines for javascript event bubbling/propagation
#674716: We need guidelines on usage of DOM events and jQuery's namespaced events
Comment #34
mcrittenden CreditAttribution: mcrittenden commentedSub. Awesome work here, casey.
Comment #35
aaron CreditAttribution: aaron commentedOn 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:
Probably because it's opening an iFrame? I don't know.
Comment #36
casey CreditAttribution: casey commented#35 Its currenlty not working either. Lets fix this in a followup. This is not directly related to this issue!
Comment #38
casey CreditAttribution: casey commentedUhh... not sure why this is happening.
Comment #40
aspilicious CreditAttribution: aspilicious commentedcommit this thing its working for this part, and lets fix other issues in an other thread
Comment #41
webchickHm. 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.
Comment #42
casey CreditAttribution: casey commentedThnx webchick!
Sorry small issue for webkit browsers, caused by namespaced events that was added in latest patch. Can go right in I guess.
Comment #43
aspilicious CreditAttribution: aspilicious commentedYes latest patch works!
Need to get in fast, else webkit users need to press F5 every time they wonna go into the overlay.
Comment #44
casey CreditAttribution: casey commented+ a small problem with the href attribute only having a anchor (interferes for example with dashboard)
Comment #45
aspilicious CreditAttribution: aspilicious commentedWorks for me
Comment #46
casey CreditAttribution: casey commentedComment #47
casey CreditAttribution: casey commentedThis is the last one. Ready to commit!
Comment #48
mcrittenden CreditAttribution: mcrittenden commentedConfirmed. Works for me.
Comment #49
casey CreditAttribution: casey commentedOk latest one... for real now. Guess this can remain RTBC
Comment #50
webchickOk can we get some actual reviews here please? ;)
Comment #51
casey CreditAttribution: casey commentedThat probably gonna take another week :(
Comment #52
mcrittenden CreditAttribution: mcrittenden commentedHere's #49 but with extra spaces removed from the new line.
Comment #53
sunNormally, 'load' should be sufficient. If this is working around a bug in jQuery, then we should document so.
Why?
There should be a space after { and before }.
Powered by Dreditor.
Comment #54
casey CreditAttribution: casey commentedTo re-open in the overlay. Try editing a user via "People".
Comment #55
casey CreditAttribution: casey commentedNormally, 'load' should be sufficient. If this is working around a bug in jQuery, then we should document so.
You're right. webkit doesn't seem to unbind it.
/Edit hmmm no browser does. Are you sure that is supposed to be enough?
Comment #56
ksenzeeWhat sun said. Also,
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.
Comment #57
casey CreditAttribution: casey commentedUpdated whitespace and comments.
Not adding extra comments to "self.$iframe.unbind('load load.overlay-event');" as this seems to be the way it is supposed to be done.
Comment #58
ksenzeeNo, 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.
Comment #59
casey CreditAttribution: casey commentedOk removed "load" as this was just for other scripts that bind a load handler to the overlay iframe. Also improved the comment on that a bit.
Comment #60
aspilicious CreditAttribution: aspilicious commentedSufficient 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
Comment #61
casey CreditAttribution: casey commentedThis patch makes customizing dashboard in overlay working (any browser) + closing-reopening the overlay in webkit.
aspilicious also tested this. I am currently working on #649982: Improve the drag-drop handling for dashboard customisation but need to have this patch applied to be able to use dashboard in overlay.
I improved comments according to suggestions of ksenzee and sun.
So... back to RTBC!?
Comment #62
ksenzeeFYI 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.
Comment #63
ksenzeeOk, 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
Comment #64
webchickCommitted to HEAD.
Comment #65
pwolanin CreditAttribution: pwolanin commentedI think there is a logical error in the test for the 'overlay-exclude' class:
It would seem that even if I add the class, this is ignored since it only returns if we find an empty link.
Comment #66
pwolanin CreditAttribution: pwolanin commentedthis separates out the two checks.
Comment #67
casey CreditAttribution: casey commentedYup that's a bug. Patch updates comment.
Comment #68
pwolanin CreditAttribution: pwolanin commentedgrammar error "allows to prevent"
Comment #70
casey CreditAttribution: casey commentedSimplified comment.
Comment #71
pwolanin CreditAttribution: pwolanin commentedThanks for the clarified code comments - looks good.
Comment #72
webchickCommitted to HEAD! Thanks.