Bug 13618 – HTMLAnchorElement should not force target to _blank for middle clicks

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Peter Kasting]

In HTMLAnchorElement.cpp, defaultEventHandler() sets the target of a link to "_blank" if the user middle-clicks the link.  This sort of thing should happen at the policy delegate level, not in the anchor element's event handler.

Patch coming shortly.

[Peter Kasting]

Created attachment 14406 [details]
patch v1

Simple patch to fix.

[Peter Kasting]

Created attachment 14480 [details]
patch v2

As discussed with Maciej on IRC, this patch adds a WebPreference to toggle the behavior.  On Mac OS X, the preference is set to do the old behavior (force target="_blank" on middle-click); otherwise, the preference defaults to the new behavior (don't change target, let embedders handle middle-clicks as they wish via the policy delegate functions).

[Maciej Stachowiak]

Comment on attachment 14480 [details]
patch v2

r=me

Is there any way to make a test case for this? (Probably not).

[Mark Rowe (bdash)]

Hrm, I think your API additions need to be added to WebPreferencesPrivate.h rather than WebPreferences.h until they go through an Apple API review and are approved for public API.  The middleClickShouldForceTargetToBlank/setMiddleClickShouldForceTargetToBlank: methods also have extraneous whitespace before arguments.

[Peter Kasting]

Created attachment 14500 [details]
patch v3

This moves the functions to the Private section and fixes the whitespace issues.

Unlike the last patch, this one is untested (my access to a Mac is limited), so please make sure it compiles :)

I don't know of any way to make a testcase for this, but with the last patch I did at least use a debugger to verify the preference had been set correctly when stepping through the HTMLAnchorElement code.

[Timothy Hatcher]

Comment on attachment 14500 [details]
patch v3

I think setMiddleClickShouldForceTargetToBlank should be named differently.

I think setMiddleClickShouldOpneNewWindow makes more sense to API users. An API user who isn't familiar with Web terms wont know what target blank is.

[Timothy Hatcher]

That should read: setMiddleClickShouldOpenNewWindow

[Peter Kasting]

(In reply to comment #6)
> (From update of attachment 14500 [details] [edit])
> I think setMiddleClickShouldForceTargetToBlank should be named differently.
> 
> I think setMiddleClickShouldOpneNewWindow makes more sense to API users. An API
> user who isn't familiar with Web terms wont know what target blank is.

I don't think that's a good idea.  It's misleading in multiple ways.

First, it doesn't force the link to open in a new window.  It's perfectly possible for links with target=_blank to _not_ open in new windows, based on what you do in DecidePolicyForNewWindowSomething() (can't remember the exact name, not looking at the source ATM).  And it's also possible to open a link in a new window _without_ setting its target to blank, by proper implementation inside DecidePolicyForNavigationAction().

Second, it implies that if you want middle clicks to open in new windows, you should turn it on.  But that isn't true.  If you want middle clicks to open in new windows, you _should_ do that in your policy delegates.  What _this_ flag does is maintain a broken behavior for backwards-compat reasons.  So if I weren't naming it middleClickShouldForceTargetToBlank, I'd name it hackAttributesOnLinksToMaintainCompatWithOldVersionsOfWebCore.

The name says exactly what it does.  If you don't know what it does, you shouldn't be toggling it back and forth.  This is not a pref that most people should be changing.  If that's not clear enough to embedders, I suggest that we change the pref to a linktime version check, which was one option when this patch got discussed in IRC.

Of course, backing off of that a bit, we could always just add voluminous comments about the effects of this flag somewhere.  I just don't happen to know where that place should be :)

(Incidentally, I'm not sure I buy your original assertion.  I haven't made a web page since 1998, can't write JS, and didn't even know what "DOM" stood for until a year ago, and I know what a blank target is.  Are people less knowledgable than me likely to be embedding WebKit _and_ flipping its prefs?)

[Dave Hyatt]

My own opinion is that this blank-setting code could just be yanked with no compatibility fears.  We're talking about middle clicking here, which isn't exactly something to worry about on the Mac platform.

[Peter Kasting]

Comment on attachment 14406 [details]
patch v1

I can't pass up endorsement like that, so I'll unobsolete v1 in case hyatt wants to r+ or land it :)

[Dave Hyatt]

Comment on attachment 14500 [details]
patch v3

Someone who can run internal Safari should test/land this though, to verify that middle click still works as expected.

[Dave Hyatt]

Comment on attachment 14406 [details]
patch v1

r=me

[Sam Weinig]

Finally landed in r24401.