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

Description Nigel Tao 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.
Comment 1 Nigel Tao 2007-11-22 03:59:21 PST
Created attachment 17441 [details]
Patch
Comment 2 Alp Toker 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.
Comment 3 Nigel Tao 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.
Comment 4 Nigel Tao 2007-12-09 14:41:21 PST
Any comments from a reviewer?
Comment 5 Xan Lopez 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?
Comment 6 Xan Lopez 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 :)
Comment 7 Nigel Tao 2007-12-10 23:24:36 PST
Created attachment 17840 [details]
Patch
Comment 8 Nigel Tao 2007-12-10 23:25:16 PST
Comment on attachment 17840 [details]
Patch

Same patch, synced to head
Comment 9 Nigel Tao 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.
Comment 10 Alp Toker 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.
Comment 11 Nigel Tao 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.
Comment 12 Nigel Tao 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".  :)
Comment 13 Jasper Bryant-Greene 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.
Comment 14 Jasper Bryant-Greene 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.
Comment 15 Anders Carlsson 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
Comment 16 Mark Rowe (bdash) 2008-03-22 15:19:44 PDT
This was landed in r31183 but rolled out in r31184.
Comment 17 Mark Rowe (bdash) 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.
Comment 18 Mark Rowe (bdash) 2008-03-22 15:20:27 PDT
Alp, can you please provide some info on why the patch was rolled out?
Comment 19 Pierre-Luc Beaudoin 2008-07-11 02:06:56 PDT
This bug and its patches look like Bug #16562
Comment 20 Christian Dywan 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.
Comment 21 Xan Lopez 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 :))
Comment 22 Xan Lopez 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.
Comment 23 Xan Lopez 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.
Comment 24 Xan Lopez 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.
Comment 25 Holger Freyther 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.
Comment 26 Xan Lopez 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.
Comment 27 Xan Lopez 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.
Comment 28 Xan Lopez 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).
Comment 29 Christian Dywan 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: