Bug 16092

Summary: [GTK] Middle-mouse click should allow opening a URL in a new tab
Product: WebKit Reporter: Nigel Tao <nigel.tao.gnome>
Component: WebKitGTKAssignee: Alp Toker <alp>
Status: RESOLVED FIXED    
Severity: Normal CC: alp, christian, cosimoc, gustavo, jakub.rusinek, jasper, martin.sourada, xan.lopez, xclaesse
Priority: P2 Keywords: Gtk
Version: 528+ (Nightly build)   
Hardware: PC   
OS: Linux   
Bug Depends on: 16562    
Bug Blocks:    
Attachments:
Description Flags
Patch
alp: review-
Patch
none
Patch
none
Patch
alp: review-
IRC log
none
Implement WebKitNavigationAction object in GTK API
none
less verbose duplication of string, return const gchar* through api, correct code style issues
none
0001-Add-mouse-button-and-keyboard-state-modifiers-info-t.patch
none
0001-Add-mouse-button-and-keyboard-state-modifiers-info-t.patch
none
0001-Add-mouse-button-and-keyboard-state-modifiers-info-t.patch
none
0001-Add-mouse-button-and-keyboard-state-modifiers-info-t.patch
none
0001-Add-mouse-button-and-keyboard-state-modifiers-info-t.patch
none
0001-Add-mouse-button-and-keyboard-state-modifiers-info-t.patch none

Nigel Tao
Reported 2007-11-22 03:51:35 PST
Middle-clicking (or Control-clicking) on a link should enable the program that embeds a WebKitPage to open that link in a new tab, or otherwise presented however the program sees appropriate.
Attachments
Patch (3.50 KB, patch)
2007-11-22 03:59 PST, Nigel Tao
alp: review-
Patch (12.61 KB, patch)
2007-12-05 02:41 PST, Nigel Tao
no flags
Patch (12.61 KB, patch)
2007-12-10 23:24 PST, Nigel Tao
no flags
Patch (12.89 KB, patch)
2007-12-11 00:11 PST, Nigel Tao
alp: review-
IRC log (3.41 KB, text/plain)
2007-12-12 21:25 PST, Nigel Tao
no flags
Implement WebKitNavigationAction object in GTK API (20.51 KB, patch)
2008-03-19 19:56 PDT, Jasper Bryant-Greene
no flags
less verbose duplication of string, return const gchar* through api, correct code style issues (20.40 KB, patch)
2008-03-19 20:43 PDT, Jasper Bryant-Greene
no flags
0001-Add-mouse-button-and-keyboard-state-modifiers-info-t.patch (9.89 KB, patch)
2008-12-20 07:36 PST, Xan Lopez
no flags
0001-Add-mouse-button-and-keyboard-state-modifiers-info-t.patch (9.88 KB, patch)
2008-12-20 07:43 PST, Xan Lopez
no flags
0001-Add-mouse-button-and-keyboard-state-modifiers-info-t.patch (9.92 KB, patch)
2008-12-20 08:02 PST, Xan Lopez
no flags
0001-Add-mouse-button-and-keyboard-state-modifiers-info-t.patch (9.92 KB, patch)
2008-12-21 02:25 PST, Xan Lopez
no flags
0001-Add-mouse-button-and-keyboard-state-modifiers-info-t.patch (9.91 KB, patch)
2008-12-21 02:26 PST, Xan Lopez
no flags
0001-Add-mouse-button-and-keyboard-state-modifiers-info-t.patch (10.07 KB, patch)
2008-12-21 05:00 PST, Xan Lopez
no flags
Nigel Tao
Comment 1 2007-11-22 03:59:21 PST
Alp Toker
Comment 2 2007-11-22 16:09:45 PST
Comment on attachment 17441 [details] Patch The concept of tabs is only relevant at the application level. It's true we don't have the necessary way to support new window creation and tabs, but this isn't the way. Need to do something similar to the other public APIs instead.
Nigel Tao
Comment 3 2007-12-05 02:41:21 PST
Created attachment 17717 [details] Patch Second attempt, now passing a dictionary with the navigation-requested signal, which AFAICT is similar to what the Mac and Win ports do.
Nigel Tao
Comment 4 2007-12-09 14:41:21 PST
Any comments from a reviewer?
Xan Lopez
Comment 5 2007-12-10 10:23:08 PST
(In reply to comment #3) > Created an attachment (id=17717) [edit] > Patch > > Second attempt, now passing a dictionary with the navigation-requested signal, > which AFAICT is similar to what the Mac and Win ports do. > Hi, so thinking out loud here. This looks pretty good to me, just one comment: - Having to connect to the signal with _after looks really ugly to me, so I think we should try something else instead. One solution is to decouple the WebKitNavigationResponse from the fact of whether we want to block or not the default handler. We can put the NavigationResponse in the signal arguments, and make it return a boolean value (+ a boolean accumulator). If you want to block the default handler, you simply return TRUE from your callback (and put the response as something different than IGNORE I guess); otherwise, you return FALSE. Would this be feasible? How does it sound?
Xan Lopez
Comment 6 2007-12-10 10:26:46 PST
With being feasible I mean that I guess we'd need to change other code somewhere that depends on the action being passed as return value. The change I described is entirely possible on its own :)
Nigel Tao
Comment 7 2007-12-10 23:24:36 PST
Nigel Tao
Comment 8 2007-12-10 23:25:16 PST
Comment on attachment 17840 [details] Patch Same patch, synced to head
Nigel Tao
Comment 9 2007-12-11 00:11:17 PST
Created attachment 17841 [details] Patch Patch version 4 removes the need to connect *with _after* to the "navigation-requested" signal. This involved removing the default handler, webkit_web_view_real_navigation_requested.
Alp Toker
Comment 10 2007-12-12 10:28:24 PST
Comment on attachment 17841 [details] Patch I asked Anders for his thoughts on this and he suggested a different API with a NavigationAction/NavigationInfo object. The discussion was on IRC Wed 12 Dec 6:20ish, but will summarise here later if you missed it.
Nigel Tao
Comment 11 2007-12-12 21:25:15 PST
Created attachment 17871 [details] IRC log This is the IRC log (as far as I can tell) that Alp referred to in the previous comment.
Nigel Tao
Comment 12 2007-12-12 21:34:16 PST
Was the patch rejected just because the navigation action signal parameter is a GHashTable instead of a separate GObject subclass with a nice, readable API like const char *url = webkit_web_navigation_action_get_original_url(action) or is it deeper (i.e. the "listen to the navigation-action signal and return ACCEPT, IGNORE or DOWNLOAD" design is flawed, perhaps because it doesn't allow popping a dialog box for downloads or something)? I suppose that the answer is simply "look at what the other ports do". :)
Jasper Bryant-Greene
Comment 13 2008-03-19 19:56:49 PDT
Created attachment 19892 [details] Implement WebKitNavigationAction object in GTK API Implements a WebKitNavigationAction object in the GTK API providing the context for the navigation request.
Jasper Bryant-Greene
Comment 14 2008-03-19 20:43:16 PDT
Created attachment 19893 [details] less verbose duplication of string, return const gchar* through api, correct code style issues This corrects all issues raised about the patch with the exception of making the NavigationResponse be provided asynchronously. This patch provides much-needed functionality so perhaps it could be committed and improved to be asynchronous later? I'm happy to continue working on this to make it async.
Anders Carlsson
Comment 15 2008-03-19 21:00:26 PDT
Comment on attachment 19893 [details] less verbose duplication of string, return const gchar* through api, correct code style issues r=me
Mark Rowe (bdash)
Comment 16 2008-03-22 15:19:44 PDT
This was landed in r31183 but rolled out in r31184.
Mark Rowe (bdash)
Comment 17 2008-03-22 15:19:57 PDT
Comment on attachment 19893 [details] less verbose duplication of string, return const gchar* through api, correct code style issues Clearing + flag since the patch was rolled out.
Mark Rowe (bdash)
Comment 18 2008-03-22 15:20:27 PDT
Alp, can you please provide some info on why the patch was rolled out?
Pierre-Luc Beaudoin
Comment 19 2008-07-11 02:06:56 PDT
This bug and its patches look like Bug #16562
Christian Dywan
Comment 20 2008-07-15 07:02:50 PDT
(In reply to comment #19) > This bug and its patches look like Bug #16562 I think this is essentially a less complete implementation of the NavigationAction and could be discussed only as an enhancement. I suggest this proposal should be revisited after #16562 is resolved.
Xan Lopez
Comment 21 2008-12-20 06:58:27 PST
(In reply to comment #20) > (In reply to comment #19) > > This bug and its patches look like Bug #16562 > > I think this is essentially a less complete implementation of the > NavigationAction and could be discussed only as an enhancement. I suggest this > proposal should be revisited after #16562 is resolved. > I think it still makes a lot of sense to get the code in this patch getting the button and modifier state from action.event to add it to our navigation action in #16562. It's needed for browsers to be able to do different things in response to actions (the most common one being the one mentioned in this bug topic :))
Xan Lopez
Comment 22 2008-12-20 07:36:47 PST
Created attachment 26169 [details] 0001-Add-mouse-button-and-keyboard-state-modifiers-info-t.patch Something like this is good enough for Epiphany, for instance.
Xan Lopez
Comment 23 2008-12-20 07:43:20 PST
Created attachment 26170 [details] 0001-Add-mouse-button-and-keyboard-state-modifiers-info-t.patch Fix return values for getters in case of error.
Xan Lopez
Comment 24 2008-12-20 08:02:46 PST
Created attachment 26171 [details] 0001-Add-mouse-button-and-keyboard-state-modifiers-info-t.patch Improve the wording of the modifier keys state property per discussion on IRC.
Holger Freyther
Comment 25 2008-12-20 17:24:24 PST
Comment on attachment 26171 [details] 0001-Add-mouse-button-and-keyboard-state-modifiers-info-t.patch Two minor style nitpicks. > - // TODO: use action.event(). > + gint button = -1, modifierFlags = 0; From what I know we put that on two different lines. > + > + const Event* event = action.event(); > + const MouseEvent* mouseEvent = static_cast<const MouseEvent*>(event); > + const UIEventWithKeyState* keyStateEvent = findEventWithKeyState(const_cast<Event*>(event)); I'm just the messenger here, but we omit this const in WebKit.org. Besides that, did you see the WebActionPropertyBag.cpp of the Windows port? I think it is fine to export this information here and if there is no need for further API discussion I will say r=me.
Xan Lopez
Comment 26 2008-12-21 02:25:18 PST
Created attachment 26180 [details] 0001-Add-mouse-button-and-keyboard-state-modifiers-info-t.patch Address both comments by Holger.
Xan Lopez
Comment 27 2008-12-21 02:26:29 PST
Created attachment 26181 [details] 0001-Add-mouse-button-and-keyboard-state-modifiers-info-t.patch And remove another const.
Xan Lopez
Comment 28 2008-12-21 05:00:55 PST
Created attachment 26183 [details] 0001-Add-mouse-button-and-keyboard-state-modifiers-info-t.patch Re-add const for Events, you need it and it's used in the other ports too. Also add +1 to the mouse button value to use the same values than GTK+ does (DOM starts in 0, GTK+ in 1).
Christian Dywan
Comment 29 2008-12-21 06:51:04 PST
2008-12-21 Xan Lopez <xan@gnome.org> Reviewed by Holger Freyther. https://bugs.webkit.org/show_bug.cgi?id=16092 [GTK] Middle-mouse click should allow opening a URL in a new tab Add mouse button and keyboard state modifiers info to navigation action. * WebCoreSupport/FrameLoaderClientGtk.cpp: (WebKit::FrameLoaderClient::dispatchDecidePolicyForNavigationAction): * webkit/webkitwebnavigationaction.cpp: (_WebKitWebNavigationActionPrivate::): (_WebKitWebNavigationActionPrivate::webkit_web_navigation_action_get_property): (_WebKitWebNavigationActionPrivate::webkit_web_navigation_action_set_property): (_WebKitWebNavigationActionPrivate::webkit_web_navigation_action_class_init): (_WebKitWebNavigationActionPrivate::webkit_web_navigation_action_get_button): (_WebKitWebNavigationActionPrivate::webkit_web_navigation_action_get_modifier_state): * webkit/webkitwebnavigationaction.h:
Note You need to log in before you can comment on or make changes to this bug.