Remove the opening of external links in a new browser window

even if we force external links to open in an external window (which generally pisses me off, but anyway...) we shouldn't be doing it by using invalid markup, even if it is at runtime

I know it's tempting, but you might aswel just use the open method of the window object, keeping validity of ur document

The target attribute is only invalid if you're a xhtml validator freak. The target attribute is perfectly fine in both html4.01 and html5, and doesn't require any extra javascript that only leads to more slowdowns.

Don't get me started about xhtml1.0, xhtml1.0 served as text/html is nothing more than html4.01 with some extra strict (unnecessary) rules.

not to be a dick or anything, but core doesn't serve HTML4.01 or HTML5. It serves XHTML+RDFa 1.0 out of the box, but it seems this DTD actually allows target on <a>

and comparing

      $('a').attr('target', '_new');

and

      $('a').bind('click', function() {
        window.open(this.href);
      }

on 1000 links showed that the second method was double as expensive as the first (22ms vs 44ms on FF3.5.5) -> first - second

so I guess it is better to use the target-way

one thing though: target _new instead of _blank causes a second external link to open in the same window as the first one, instead of a new window... is this what a user would expect?

Code is fixed. Question remains whether external links should open in a new window or not.

related follow-up pathc: http://drupal.org/node/668104

current code logic is broken imho.

I would like to see the external links behavior gone entirely; it seems an odd thing to force on all overlay users. But in the meantime, here's a patch to fix a bug in which links without real hrefs -- such as "javascript:void(0)" -- open new blank tabs.

I would check link's protocol.

wouldn't that destroy links like spotify?

No they just won't be opened in the overlay; just what we want.

Looks good to me.

The patch from #7 has been used in DrupalGardens for months, and the updated patch seems to be an improvement.

The patches from #7 and #9 are a totally different approach, so the DG testing doesn't mean anything. #9 seems okay to me though (although I don't even know what spotify links look like so can't speak to that).

I'm sorry, I should have been more clear: I meant any link that doesn't use http:// or https://

#668640: Overlay shouldn't be based on jQuery UI Dialog landed which contains something like in patch of #9.

I think we should only open external links in a new window when the are within the overlay, but we could also remove the whole processing of external links. Attached patch does the first.

I believe the underlying overlay JS code got a revamp since this patch was rolled and might not apply/work anymore. Sent for a re-test.

#15: overlay-externallinks.patch queued for re-testing.

Anyone?

I've got a couple comments, but I'm not marking as needs work because I think it will work as advertised. Let me know if these are off-base:

1). Hostname too restrictive?

if (target.hostname != window.location.hostname) {

A lot of people have secure.mysite.com or perhaps use subdomains for various site sections? I know this isn't a majority case, but perhaps this is too restrictive? At the very least, perhaps a variable_get() with a regex for "onsite vs offsite". If not provided, then this behavior? I dunno, just ruminating that this might be an issue which will require a core hack.

2). Performance hit avoidable?
It appears to me that when I'm in the overlay if I click on a isAdmin() link it will reload the main page, giving it a new overlay fragment and then reload the overlay. Is that correct? I understand we want the fragment to point to where we actually are, but this causes 2 page loads when we really only need one. Can it be avoided somehow? (I don't know how, but just asking).

Performance hit? I don't see the parent page reloading?

How can I reproduce?

Patch still applies.

This is a reroll with some comment edits. We do need to handle external links within the overlay iframe (unless we want Google suddenly appearing in people's overlay iframe), but we shouldn't be hijacking regular links in nodes the way we do in HEAD right now, so I think this is the right approach. It's a much smaller change than the patch makes it look like - it just moves the external link handling into an if clause that checks to make sure we're inside the overlay iframe.

Oh, and I forgot to address Jacob's two points. Re #1, the lovely thing about JS is you don't have to hack core to change it - you just substitute your own method for the one you don't like. In the corner case you mention, where someone has lots of internal subdomain links on admin pages, they can just drupal_add_js their own Drupal.overlay.eventhandlerOverrideLink. Re #2, there's no performance hit because of the way browsers handle fragments. I checked with Jacob, and he and Casey chatted on IRC and resolved that concern. So we're good to go here.

Chasing HEAD (my local workspace was out of date).

Looks good now. Committed to CVS HEAD. Thanks.

That is indeed closer to browser's normal behavior.

Committed to CVS HEAD. Thanks.