Bug 23932 – Current API does not allow us to open target="_blank" links in new tabs instead of windows

• Component: WebKitGTK
• Status: RESOLVED FIXED

[Gustavo Noronha (kov)]

This is doable, and I believe Midori is able to do it, but our current API doesn't give us an easy/elegant way of doing this.

[Alejandro G. Castro]

Created attachment 28066 [details]
Patch adding the requested feature

This patch adds a signal that allows the browser to handle the new window request, if not handled we do the usual new window opening. I'm going to add also a patch to Epiphany to use this new signal.

[Alejandro G. Castro]

(In reply to comment #1)
> Created an attachment (id=28066) [review]
> Patch adding the requested feature
> 
> This patch adds a signal that allows the browser to handle the new window
> request, if not handled we do the usual new window opening. I'm going to add
> also a patch to Epiphany to use this new signal.
> 

I've uploaded the patch for Epiphany to the bugzilla of gnome, it uses this signal to handle the middle button click:

   http://bugzilla.gnome.org/show_bug.cgi?id=573357

[Gustavo Noronha (kov)]

Comment on attachment 28066 [details]
Patch adding the requested feature

Thanks for working on this; a much needed fix =).

> +static GObject *getNavigationAction(const NavigationAction& action)

The * should be close to the type here. Why not use WebKitNavigationAction here, instead of GObject, by the way? It would look more correct to me, and it's not like you're saving on casts by using GObject, right?

Otherwise looks good to me.

[Xan Lopez]

(In reply to comment #1)

+    if (!isHandled)
+        (core(m_frame)->loader()->*policyFunction)(PolicyUse);

Here you can just do webkit_web_policy_decision_use(policyDecision); (which you created earlier). Also this has a bug that we just found for the other signals, in that you have to call webkit_web_policy_decision_ignore(policyDecision); if the signal is not handled.

[Alejandro G. Castro]

(In reply to comment #3)
> (From update of attachment 28066 [details] [review])
> Thanks for working on this; a much needed fix =).
> 
> > +static GObject *getNavigationAction(const NavigationAction& action)
> 
> The * should be close to the type here. Why not use WebKitNavigationAction
> here, instead of GObject, by the way? It would look more correct to me, and
> it's not like you're saving on casts by using GObject, right?
> 

Yes, saving casts was not my intention (actually I'm seeing an unnecesary G_OBJECT cast :), the only reason to use Gobject it was reuse the old code of the function as it was. I agree with your proposal.

Thanks for the review.

[Alejandro G. Castro]

Created attachment 28240 [details]
Modified patch

I've modified the patch according to the comments: changed the type of the return value (WebKitWebNavigationAction) and modified the statements using the policy decision depending on the result of signal handling.

[Christian Dywan]

(In reply to comment #6)
> Created an attachment (id=28240) [review]
> Modified patch
> 
> I've modified the patch according to the comments: changed the type of the
> return value (WebKitWebNavigationAction) and modified the statements using the
> policy decision depending on the result of signal handling.


After looking at the patch, I'm not 100% what functionality the signal is meant to cover. It looks like it is pretty similar to what navigation-policy-decision or a create-web-view do.

Could you describe exactly what page content and according user action trigger this? I guess it must at least be anchors with a blank target, but then again, that's what create-web-view already does? And does it cover anchors with, say, "_new" or "madeuptargetname"? Does it cover the context menu item "New Window"?

I'm not sure if it's a good idea to introduce several similar signals which all say "handles new window", application developers will have a hard time implementing them.

[Xan Lopez]

(In reply to comment #7)
> Could you describe exactly what page content and according user action trigger
> this? I guess it must at least be anchors with a blank target, but then again,
> that's what create-web-view already does? And does it cover anchors with, say,
> "_new" or "madeuptargetname"? Does it cover the context menu item "New Window"?

Keep in mind that create-web-view is much more generic than this, and does not include neither a 'reason' (like 'link clicked') or state/button information. This signal overlaps a lot with navigation-action, but the problem with that one signal is that it won't include any metadata (reason, state, button, ...) for, at least, target=_blank links. I suggested fixing that, but various Apple developers told me this was by design.

One way to see it would be that this signal covers new-windows created by some navigation action, while create-web-view covers all possible cases of new 'windows' (all covered by this signal + popups, etc). They both cover different use-case cases, and I think if properly documented users will know which one they need.

[Alejandro G. Castro]

Created attachment 28284 [details]
New patch, with Changelog included

I've added the Changelog comment to the patch.

[Alejandro G. Castro]

Created attachment 28850 [details]
New version of the patch

New version of the patch, I've reviewed the behavior when the handler returns TRUE after talking to Xan. We have also added more information about the signals to the documentation trying to clarify how to use this signal:

     * Emitted when @frame requests opening a new window. With this
     * signal the browser can use the context of the request to decide
     * about the new window. If the request is not handled the default
     * behavior is to allow opening the new window to load the url,
     * which will cause a create-web-view signal emission where the
     * browser hanbles the new window action but without information
     * of the context that caused the navigation. The following
     * navigation-policy-decision-requested emissions will load the
     * page after the creation of the new window just with the
     * information of this new navigation context, without any
     * information about the action that made this new window to be
     * opened.

[Emilio Pozuelo Monfort]

(In reply to comment #8)
> Keep in mind that create-web-view is much more generic than this, and does not
> include neither a 'reason' (like 'link clicked') or state/button information.
> This signal overlaps a lot with navigation-action, but the problem with that
> one signal is that it won't include any metadata (reason, state, button, ...)
> for, at least, target=_blank links. I suggested fixing that, but various Apple
> developers told me this was by design.

Actually navigation-policy-decision-requested won't be emitted at all for target=_blank links, and as you point out, create-web-view doesn't have enough information. So we either need a new signal like it's done here, or implement the missing cases in the current signals (I'd prefer the later i.e. having one signal for all the "user clicks on a link" cases, but if that's not wanted due to the design, this is OK).

[Xan Lopez]

(In reply to comment #11)
> (In reply to comment #8)
> > Keep in mind that create-web-view is much more generic than this, and does not
> > include neither a 'reason' (like 'link clicked') or state/button information.
> > This signal overlaps a lot with navigation-action, but the problem with that
> > one signal is that it won't include any metadata (reason, state, button, ...)
> > for, at least, target=_blank links. I suggested fixing that, but various Apple
> > developers told me this was by design.
> 
> Actually navigation-policy-decision-requested won't be emitted at all for
> target=_blank links, and as you point out, create-web-view doesn't have enough
> information. So we either need a new signal like it's done here, or implement
> the missing cases in the current signals (I'd prefer the later i.e. having one
> signal for all the "user clicks on a link" cases, but if that's not wanted due
> to the design, this is OK).
> 

navigation-policy-decision-requested *is* emitted, but it lacks the information to do certain things, like open them in a new tab, as commented in the bug. The signal doc in alex's comment is pretty accurate AFAIK, and I think is the best solution, because "fixing" the navigation-policy action is not an option (ie, there's nothing wrong with it, it's just that webkit gives you a lot of details about what's going on in the browser, not just one-fits-all signal).

[Emilio Pozuelo Monfort]

(In reply to comment #12)
> navigation-policy-decision-requested *is* emitted, but it lacks the information
> to do certain things, like open them in a new tab, as commented in the bug. The
> signal doc in alex's comment is pretty accurate AFAIK, and I think is the best
> solution, because "fixing" the navigation-policy action is not an option (ie,
> there's nothing wrong with it, it's just that webkit gives you a lot of details
> about what's going on in the browser, not just one-fits-all signal).

JFTR, I've debugged this with Xan on IRC and in my case (Liferea) navigation-policy-decision-requested wasn't emitted because the create-web-view callback wasn't implemented, and navigation-policy-decision-requested is emitted after that one is processed and the new webview is ready.

[Holger Freyther]

Comment on attachment 28850 [details]
New version of the patch

Please use the prepare-ChangeLog script in the future and who ever lands this patch please fix the coding style.



> [phone]  Alejandro Garcia Castro  <[e-mail]>
> +
> +        Reviewed by .

tabs?


> +        * WebCoreSupport/FrameLoaderClientGtk.cpp:

tabs? and maybe even in th enext lines...



>      // FIXME: I think Qt version marshals this to another thread so when we
>      // have multi-threaded download, we might need to do the same
> -    (core(m_frame)->loader()->*policyFunction)(PolicyUse);
> +    if (!isHandled)
> +        webkit_web_policy_decision_use(m_policyDecision);

please don't include this hunk in this when landing, IIRC there was another patch making these consistent!



> +    WebKitWebNavigationAction* navigationAction = getNavigationAction (action);

Coding Style, no ' ' between Action and (action) please. The other call to getNavigationAction was wrong too.

[Xan Lopez]

(In reply to comment #14)
> >      // FIXME: I think Qt version marshals this to another thread so when we
> >      // have multi-threaded download, we might need to do the same
> > -    (core(m_frame)->loader()->*policyFunction)(PolicyUse);
> > +    if (!isHandled)
> > +        webkit_web_policy_decision_use(m_policyDecision);
> 
> please don't include this hunk in this when landing, IIRC there was another
> patch making these consistent!
> 

The other bug was about

 if (isHandled) webkit_web_policy_decision_ignore(policy);

I think. We are already using the policy if the signal is not handled in the other cases, and I think that's right.
> 
> 
> > +    WebKitWebNavigationAction* navigationAction = getNavigationAction (action);
> 
> Coding Style, no ' ' between Action and (action) please. The other call to
> getNavigationAction was wrong too.
> 

[Holger Freyther]

(In reply to comment #15)
> (In reply to comment #14)
> > >      // FIXME: I think Qt version marshals this to another thread so when we
> > >      // have multi-threaded download, we might need to do the same
> > > -    (core(m_frame)->loader()->*policyFunction)(PolicyUse);
> > > +    if (!isHandled)
> > > +        webkit_web_policy_decision_use(m_policyDecision);
> > 
> > please don't include this hunk in this when landing, IIRC there was another
> > patch making these consistent!
> > 
> 
> The other bug was about
> 
>  if (isHandled) webkit_web_policy_decision_ignore(policy);
> 
> I think. We are already using the policy if the signal is not handled in the
> other cases, and I think that's right.

Sorry, what I meant is: It is okay to use the WebKit functions but probably should not be done in the same commit.

[Xan Lopez]

(In reply to comment #15)
> (In reply to comment #14)
> > >      // FIXME: I think Qt version marshals this to another thread so when we
> > >      // have multi-threaded download, we might need to do the same
> > > -    (core(m_frame)->loader()->*policyFunction)(PolicyUse);
> > > +    if (!isHandled)
> > > +        webkit_web_policy_decision_use(m_policyDecision);
> > 
> > please don't include this hunk in this when landing, IIRC there was another
> > patch making these consistent!
> > 
> 
> The other bug was about
> 
>  if (isHandled) webkit_web_policy_decision_ignore(policy);
> 
> I think. We are already using the policy if the signal is not handled in the
> other cases, and I think that's right.

Ok, nevermind, he was saying we shouldn't change the style of the function call, no that the call should be removed. Didn't have coffee yet today ;)

[Xan Lopez]

Landed in r41969 with the style changes, closing the bug.