Bug 16092 – [GTK] Middle-mouse click should allow opening a URL in a new tab

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKitGTK

[Nigel Tao]

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.

[Nigel Tao]

Created attachment 17441 [details]
Patch

[Alp Toker]

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]

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]

Any comments from a reviewer?

[Xan Lopez]

(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]

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]

Created attachment 17840 [details]
Patch

[Nigel Tao]

Comment on attachment 17840 [details]
Patch

Same patch, synced to head

[Nigel Tao]

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 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]

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]

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]

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]

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 on attachment 19893 [details]
less verbose duplication of string, return const gchar* through api, correct code style issues

r=me

[Mark Rowe (bdash)]

This was landed in r31183 but rolled out in r31184.

[Mark Rowe (bdash)]

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)]

Alp, can you please provide some info on why the patch was rolled out?

[Pierre-Luc Beaudoin]

This bug and its patches look like Bug #16562

[Christian Dywan]

(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]

(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]

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]

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]

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 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]

Created attachment 26180 [details]
0001-Add-mouse-button-and-keyboard-state-modifiers-info-t.patch

Address both comments by Holger.

[Xan Lopez]

Created attachment 26181 [details]
0001-Add-mouse-button-and-keyboard-state-modifiers-info-t.patch

And remove another const.

[Xan Lopez]

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]

2008-12-21  Xan Lopez  <[e-mail]>

        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: