Bug 99202 - [Chromium] shift-click fails to spawn new window via window.open
Summary: [Chromium] shift-click fails to spawn new window via window.open
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Eric U.
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-10-12 13:33 PDT by Eric U.
Modified: 2012-10-24 14:08 PDT (History)
4 users (show)

See Also:


Attachments
First try patch--no tests yet. (3.12 KB, patch)
2012-10-12 15:16 PDT, Eric U.
ericu: commit-queue-
Details | Formatted Diff | Diff
Same code, updated ChangeLog (3.21 KB, patch)
2012-10-22 17:18 PDT, Eric U.
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Eric U. 2012-10-12 13:33:11 PDT
[Chromium] shift-click fails to spawn new window with target=_blank
Comment 1 Eric U. 2012-10-12 13:37:29 PDT
This is easy to test in GMail; any link in an email will always open in a new tab, even with shift held.  The correct behavior would be to spawn a new window if shift is held during the click.
Comment 2 Eric U. 2012-10-12 15:16:03 PDT
Created attachment 168493 [details]
First try patch--no tests yet.

This code path affects <a target="_blank"...> and <a href="javascript:window.open...>, but apparently not ordinary links.

This didn't break any tests, which is a bad sign.  Where should I be putting a new test for this?
Comment 3 Eric U. 2012-10-12 15:29:24 PDT
Digging a bit more, I see that this doesn't make e.g. middle-click on a javascript popup go to a background tab--it goes to a tab, but it's backgrounded.

E.g. "SEND TO A FRIEND" on http://www.yoshis.com/oakland/jazzclub/artist/show/3030

I haven't chased down why yet.
Comment 4 Peter Kasting 2012-10-12 15:40:36 PDT
See also bug 22382, which is normally the cause of this sort of thing for middle-clicks.
Comment 5 Eric U. 2012-10-16 16:55:46 PDT
(In reply to comment #4)
> See also bug 22382, which is normally the cause of this sort of thing for middle-clicks.

AhhhhhOK, I'm not going to chase middle-click right now, then.

I think this patch will just take care of shift-click in the broken case I know about, but I'd feel more comfortable if someone could suggest a test that it should be breaking.
Comment 6 Eric U. 2012-10-17 12:38:14 PDT
OK, it's not target=_blank that does it--that works fine.  GMail is actually grabbing the event before that can hit, and using window.open, which is why we're hitting this code path.

Up until now, we've been ignoring much of the user gesture's information when we're in the window.open path; this patch changes that to allow shift and ctrl to override the default action more.  So now shift-click will work in GMail, but also if you shift-click on a button that spawns a popup, you'll get a new window [i.e. with toolbars, etc.] instead of a popup.

That seems like appropriate behavior to me.
Comment 7 Eric U. 2012-10-17 13:10:39 PDT
(In reply to comment #3)
> Digging a bit more, I see that this doesn't make e.g. middle-click on a javascript popup go to a background tab--it goes to a tab, but it's backgrounded.
> 
> E.g. "SEND TO A FRIEND" on http://www.yoshis.com/oakland/jazzclub/artist/show/3030
> 
> I haven't chased down why yet.

(In reply to comment #3)
> Digging a bit more, I see that this doesn't make e.g. middle-click on a javascript popup go to a background tab--it goes to a tab, but it's backgrounded.
> 
> E.g. "SEND TO A FRIEND" on http://www.yoshis.com/oakland/jazzclub/artist/show/3030
> 
> I haven't chased down why yet.

Correction: that link now does what I expect it to with my change.  It's new-background-tab with middle-click and ctrl-click, new-foreground-tab with shift-middle-click.
Comment 8 Eric U. 2012-10-22 17:18:18 PDT
Created attachment 170033 [details]
Same code, updated ChangeLog

The test for this code will be a Chromium browser_test; see https://codereview.chromium.org/11235048/ for the suite.
I've updated the ChangeLog to indicate this; the code is the same as before.

This should now be fully baked.
Comment 9 Adam Barth 2012-10-24 13:16:21 PDT
Comment on attachment 170033 [details]
Same code, updated ChangeLog

Ok.
Comment 10 WebKit Review Bot 2012-10-24 14:08:18 PDT
Comment on attachment 170033 [details]
Same code, updated ChangeLog

Clearing flags on attachment: 170033

Committed r132399: <http://trac.webkit.org/changeset/132399>
Comment 11 WebKit Review Bot 2012-10-24 14:08:22 PDT
All reviewed patches have been landed.  Closing bug.